From owner-freebsd-current@FreeBSD.ORG Fri Jan 27 19:14:53 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id DC5541065670; Fri, 27 Jan 2012 19:14:53 +0000 (UTC) (envelope-from ambrisko@ambrisko.com) Received: from mail.ambrisko.com (mail.ambrisko.com [70.91.206.90]) by mx1.freebsd.org (Postfix) with ESMTP id B1D5C8FC12; Fri, 27 Jan 2012 19:14:53 +0000 (UTC) X-Ambrisko-Me: Yes Received: from server2.ambrisko.com (HELO internal.ambrisko.com) ([192.168.1.2]) by ironport.ambrisko.com with ESMTP; 27 Jan 2012 11:14:56 -0800 Received: from ambrisko.com (localhost [127.0.0.1]) by internal.ambrisko.com (8.14.4/8.14.4) with ESMTP id q0RJEt1k005546; Fri, 27 Jan 2012 11:14:55 -0800 (PST) (envelope-from ambrisko@ambrisko.com) Received: (from ambrisko@localhost) by ambrisko.com (8.14.4/8.14.4/Submit) id q0RJEt1C005544; Fri, 27 Jan 2012 11:14:55 -0800 (PST) (envelope-from ambrisko) From: Doug Ambrisko Message-Id: <201201271914.q0RJEt1C005544@ambrisko.com> In-Reply-To: <201201271302.59011.jhb@freebsd.org> To: John Baldwin Date: Fri, 27 Jan 2012 11:14:55 -0800 (PST) X-Mailer: ELM [version 2.4ME+ PL124d (25)] MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="US-ASCII" Cc: Kostik Belousov , freebsd-current@freebsd.org, Andrew Boyer Subject: Re: knlist_empty locking fix X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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: Fri, 27 Jan 2012 19:14:53 -0000 John Baldwin writes: | On Friday, January 27, 2012 12:52:18 pm Kostik Belousov wrote: | > On Fri, Jan 27, 2012 at 09:42:58AM -0800, Doug Ambrisko wrote: | > > Andrew Boyer writes: | > > | On Jan 27, 2012, at 12:15 PM, Doug Ambrisko wrote: | > > | | > > | > John Baldwin writes: | > > | > | Agreed, I think the missing locking should just be added to the aio | code. | > > | > | > > | > Okay so then just: | > > | > | > > | > Index: vfs_aio.c | > > | > =================================================================== | > > | > RCS file: /usr/local/cvsroot/freebsd/src/sys/kern/vfs_aio.c,v | > > | > retrieving revision 1.243.2.3.4.1 | > > | > diff -u -p -r1.243.2.3.4.1 vfs_aio.c | > > | > --- vfs_aio.c 21 Dec 2010 17:09:25 -0000 1.243.2.3.4.1 | > > | > +++ vfs_aio.c 27 Jan 2012 17:07:11 -0000 | > > | > @@ -2509,9 +2509,12 @@ static void | > > | > filt_aiodetach(struct knote *kn) | > > | > { | > > | > struct aiocblist *aiocbe = kn->kn_ptr.p_aio; | > > | > + struct knlist *knl = &aiocbe->klist; | > > | > | > > | > - if (!knlist_empty(&aiocbe->klist)) | > > | > - knlist_remove(&aiocbe->klist, kn, 0); | > > | > + knl->kl_lock(knl->kl_lockarg); | > > | > + if (!knlist_empty(knl)) | > > | > + knlist_remove(knl, kn, 1); | > > | > + knl->kl_unlock(knl->kl_lockarg); | > > | > } | > > | > | > > | > /* kqueue filter function */ | > > | > | > > | > I was trying to be consistant with knlist_remove but this is a much | > > | > smaller change that can be merge to older branches. | > > | | > > | Does filt_liodetach() need the same treatment? | > > | > > Yes, I had that in the original. I updated that and optimized | > > the knl to just get the structure needed. | > > | > > Index: vfs_aio.c | > > =================================================================== | > > RCS file: /usr/local/cvsroot/freebsd/src/sys/kern/vfs_aio.c,v | > > retrieving revision 1.243.2.3.4.1 | > > diff -u -p -r1.243.2.3.4.1 vfs_aio.c | > > --- vfs_aio.c 21 Dec 2010 17:09:25 -0000 1.243.2.3.4.1 | > > +++ vfs_aio.c 27 Jan 2012 17:35:47 -0000 | > > @@ -2508,10 +2508,12 @@ filt_aioattach(struct knote *kn) | > > static void | > > filt_aiodetach(struct knote *kn) | > > { | > > - struct aiocblist *aiocbe = kn->kn_ptr.p_aio; | > > + struct knlist *knl = &kn->kn_ptr.p_aio->klist; | > > | > > - if (!knlist_empty(&aiocbe->klist)) | > > - knlist_remove(&aiocbe->klist, kn, 0); | > > + knl->kl_lock(knl->kl_lockarg); | > > + if (!knlist_empty(knl)) | > > + knlist_remove(knl, kn, 1); | > > + knl->kl_unlock(knl->kl_lockarg); | > > } | > > | > > /* kqueue filter function */ | > > @@ -2553,10 +2555,12 @@ filt_lioattach(struct knote *kn) | > > static void | > > filt_liodetach(struct knote *kn) | > > { | > > - struct aioliojob * lj = kn->kn_ptr.p_lio; | > > + struct knlist *knl = &kn->kn_ptr.p_lio->klist; | > It is easy to be style-compiant there and move initialization of knl | > after the blank line. | > | > Do you need two different functions now ? I think you can leave just one. | | Hmm, I think p_lio != p_aio, so two functions are required. Correct, even thought they are together in a union, the structures are different so klist would have a different offset for each function. | I think the patch looks fine. The style fix to not assign 'knl' in its | declaration would be nice to fix as you suggested, but that's minor. Here is the updated version that seems to work fine. Index: vfs_aio.c =================================================================== RCS file: /usr/local/cvsroot/freebsd/src/sys/kern/vfs_aio.c,v retrieving revision 1.243.2.3.4.1 diff -u -p -r1.243.2.3.4.1 vfs_aio.c --- vfs_aio.c 21 Dec 2010 17:09:25 -0000 1.243.2.3.4.1 +++ vfs_aio.c 27 Jan 2012 18:22:10 -0000 @@ -2508,10 +2508,13 @@ filt_aioattach(struct knote *kn) static void filt_aiodetach(struct knote *kn) { - struct aiocblist *aiocbe = kn->kn_ptr.p_aio; + struct knlist *knl; - if (!knlist_empty(&aiocbe->klist)) - knlist_remove(&aiocbe->klist, kn, 0); + knl = &kn->kn_ptr.p_aio->klist; + knl->kl_lock(knl->kl_lockarg); + if (!knlist_empty(knl)) + knlist_remove(knl, kn, 1); + knl->kl_unlock(knl->kl_lockarg); } /* kqueue filter function */ @@ -2553,10 +2556,13 @@ filt_lioattach(struct knote *kn) static void filt_liodetach(struct knote *kn) { - struct aioliojob * lj = kn->kn_ptr.p_lio; + struct knlist *knl; - if (!knlist_empty(&lj->klist)) - knlist_remove(&lj->klist, kn, 0); + knl = &kn->kn_ptr.p_lio->klist; + knl->kl_lock(knl->kl_lockarg); + if (!knlist_empty(knl)) + knlist_remove(knl, kn, 1); + knl->kl_unlock(knl->kl_lockarg); } /* kqueue filter function */ Thanks, Doug A.