Date: Fri, 27 Jan 2012 19:52:18 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: Doug Ambrisko <ambrisko@ambrisko.com> Cc: freebsd-current@freebsd.org, Andrew Boyer <aboyer@averesystems.com> Subject: Re: knlist_empty locking fix Message-ID: <20120127175218.GC2726@deviant.kiev.zoral.com.ua> In-Reply-To: <201201271742.q0RHgwGS091009@ambrisko.com> References: <5EDB646F-513F-411A-ABBC-6DA555D2EAC6@averesystems.com> <201201271742.q0RHgwGS091009@ambrisko.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
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.
Otherwise looks fine.
>
> - if (!knlist_empty(&lj->klist))
> - knlist_remove(&lj->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 */
>
> Thanks,
>
> Doug A.
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk8i5FEACgkQC3+MBN1Mb4hdCgCgo+14TtYF4K7VCahvfsZhavRQ
040AoNU2pTmCGC5g69EtH1IdoS4fwDv1
=yD6O
-----END PGP SIGNATURE-----
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120127175218.GC2726>