From owner-freebsd-current@FreeBSD.ORG Mon Feb 18 17:34:10 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id DCA11A04 for ; Mon, 18 Feb 2013 17:34:10 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: from mail-qe0-f51.google.com (mail-qe0-f51.google.com [209.85.128.51]) by mx1.freebsd.org (Postfix) with ESMTP id A3C68B23 for ; Mon, 18 Feb 2013 17:34:10 +0000 (UTC) Received: by mail-qe0-f51.google.com with SMTP id 6so2557950qea.24 for ; Mon, 18 Feb 2013 09:34:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type; bh=n/dbKeHdMBK5TVM6p5WLOl5jxC2RNdNIxoHvUZsiO6g=; b=pqCr+H7EyQqKDw0lF7EtZplfYyUyN2sTXi3uWzUSu3stoHDJkE0nU77utZYs/fIZ12 ejhtP5wCeoyM+oM5Ks5OeE4N6UBvQYN6YSZNOxAyqLSyTjWe1QlJiL2lX+XtedlzJPwc VC4qDT8vAMU/USa1zfDCO7Hrmza7N2NXx1+3cG1nGJwmoY9F5uCBZaBpiNVvV6yJWbRw Iye1ErGBV/GYIhg6YVPXrBEzvdh/7huOiErIAWPn8QumEOSJx5RATInlNt92SMmNIf5p B8eo0wvFQS6SL1bS+Vs5z2VDY6ROqShoMy55vzmuw5R3FptdvAYiWX+h2f7zRLeoSzae 6tkA== MIME-Version: 1.0 X-Received: by 10.49.105.73 with SMTP id gk9mr5547956qeb.40.1361207202501; Mon, 18 Feb 2013 09:06:42 -0800 (PST) Received: by 10.49.121.198 with HTTP; Mon, 18 Feb 2013 09:06:42 -0800 (PST) In-Reply-To: <20130218150809.GG2598@kib.kiev.ua> References: <20130218150809.GG2598@kib.kiev.ua> Date: Mon, 18 Feb 2013 18:06:42 +0100 Message-ID: Subject: Re: [patch] i386 pmap sysmaps_pcpu[] atomic access From: Svatopluk Kraus To: Konstantin Belousov Content-Type: text/plain; charset=ISO-8859-1 Cc: freebsd-current@freebsd.org X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2013 17:34:10 -0000 On Mon, Feb 18, 2013 at 4:08 PM, Konstantin Belousov wrote: > On Mon, Feb 18, 2013 at 01:44:35PM +0100, Svatopluk Kraus wrote: >> Hi, >> >> the access to sysmaps_pcpu[] should be atomic with respect to >> thread migration. Otherwise, a sysmaps for one CPU can be stolen by >> another CPU and the purpose of per CPU sysmaps is broken. A patch is >> enclosed. > And, what are the problem caused by the 'otherwise' ? > I do not see any. The 'otherwise' issue is the following: 1. A thread is running on CPU0. sysmaps = &sysmaps_pcpu[PCPU_GET(cpuid)]; 2. A sysmaps variable contains a pointer to 'CPU0' sysmaps. 3. Now, the thread migrates into CPU1. 4. However, the sysmaps variable still contains a pointers to 'CPU0' sysmaps. mtx_lock(&sysmaps->lock); 4. The thread running on CPU1 locked 'CPU0' sysmaps mutex, so the thread uselessly can block another thread running on CPU0. Maybe, it's not a problem. However, it definitely goes against the reason why the submaps (one for each CPU) exist. > Really, taking the mutex while bind to a CPU could be deadlock-prone > under some situations. > > This was discussed at least one more time. Might be, a comment saying that > there is no issue should be added. I missed the discussion. Can you point me to it, please? A deadlock is not problem here, however, I can be wrong, as I can't imagine now how a simple pinning could lead into a deadlock at all. >> >> Svata >> >> Index: sys/i386/i386/pmap.c >> =================================================================== >> --- sys/i386/i386/pmap.c (revision 246831) >> +++ sys/i386/i386/pmap.c (working copy) >> @@ -4146,11 +4146,11 @@ >> { >> struct sysmaps *sysmaps; >> >> + sched_pin(); >> sysmaps = &sysmaps_pcpu[PCPU_GET(cpuid)]; >> mtx_lock(&sysmaps->lock); >> if (*sysmaps->CMAP2) >> panic("pmap_zero_page: CMAP2 busy"); >> - sched_pin(); >> *sysmaps->CMAP2 = PG_V | PG_RW | VM_PAGE_TO_PHYS(m) | PG_A | PG_M | >> pmap_cache_bits(m->md.pat_mode, 0); >> invlcaddr(sysmaps->CADDR2); >> @@ -4171,11 +4171,11 @@ >> { >> struct sysmaps *sysmaps; >> >> + sched_pin(); >> sysmaps = &sysmaps_pcpu[PCPU_GET(cpuid)]; >> mtx_lock(&sysmaps->lock); >> if (*sysmaps->CMAP2) >> panic("pmap_zero_page_area: CMAP2 busy"); >> - sched_pin(); >> *sysmaps->CMAP2 = PG_V | PG_RW | VM_PAGE_TO_PHYS(m) | PG_A | PG_M | >> pmap_cache_bits(m->md.pat_mode, 0); >> invlcaddr(sysmaps->CADDR2); >> @@ -4220,13 +4220,13 @@ >> { >> struct sysmaps *sysmaps; >> >> + sched_pin(); >> sysmaps = &sysmaps_pcpu[PCPU_GET(cpuid)]; >> mtx_lock(&sysmaps->lock); >> if (*sysmaps->CMAP1) >> panic("pmap_copy_page: CMAP1 busy"); >> if (*sysmaps->CMAP2) >> panic("pmap_copy_page: CMAP2 busy"); >> - sched_pin(); >> invlpg((u_int)sysmaps->CADDR1); >> invlpg((u_int)sysmaps->CADDR2); >> *sysmaps->CMAP1 = PG_V | VM_PAGE_TO_PHYS(src) | PG_A | >> @@ -5072,11 +5072,11 @@ >> vm_offset_t sva, eva; >> >> if ((cpu_feature & CPUID_CLFSH) != 0) { >> + sched_pin(); >> sysmaps = &sysmaps_pcpu[PCPU_GET(cpuid)]; >> mtx_lock(&sysmaps->lock); >> if (*sysmaps->CMAP2) >> panic("pmap_flush_page: CMAP2 busy"); >> - sched_pin(); >> *sysmaps->CMAP2 = PG_V | PG_RW | VM_PAGE_TO_PHYS(m) | >> PG_A | PG_M | pmap_cache_bits(m->md.pat_mode, 0); >> invlcaddr(sysmaps->CADDR2); >> _______________________________________________ >> freebsd-current@freebsd.org mailing list >> http://lists.freebsd.org/mailman/listinfo/freebsd-current >> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"