Date: Tue, 24 Jul 2007 09:10:55 +1200 From: Andrew Thompson <thompsa@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/compat/ndis subr_ntoskrnl.c Message-ID: <20070723211055.GC6575@heff.fud.org.nz> In-Reply-To: <200707231050.51004.jhb@freebsd.org> References: <200707222053.l6MKrS6v040649@repoman.freebsd.org> <200707231050.51004.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jul 23, 2007 at 10:50:50AM -0400, John Baldwin wrote:
> On Sunday 22 July 2007 04:53:28 pm Andrew Thompson wrote:
> > thompsa 2007-07-22 20:53:28 UTC
> >
> > FreeBSD src repository
> >
> > Modified files:
> > sys/compat/ndis subr_ntoskrnl.c
> > Log:
> > ndis will signal the kthread to exit and then sleep on the proc pointer to
> > be woken up by kthread_exit. This is racey and in some cases the kthread
> will
> > exit before ndis gets around to sleep so it will be stuck indefinitely.
> This
> > change reuses the kq_exit variable to indicate that the thread has gone
> and
> > will loop on tsleep with a timeout waiting for it. If the kthread has
> already
> > exited then it will not sleep at all.
>
> As long as you use a lock you are ok. That is:
>
> foo_detach()
> {
>
> mtx_lock(&lock);
> please_die = 1;
> msleep(&proc, &lock, ..., 0);
> mtx_unlock(&lock);
> }
>
> foo_main()
> {
>
> mtx_lock(&lock);
> while (!please_die) {
> do_stuff();
> }
> mtx_unlock(&lock);
> kthread_exit(0);
> }
>
> works fine. If you try to do this:
>
> foo_detach()
> {
>
> mtx_lock(&lock);
> please_die = 1;
> while (!dead_yet)
> msleep(&proc, &lock, ... , hz/10);
> mtx_unlock(&lock);
> }
>
> foo_main()
> {
>
> mtx_lock(&lock);
> while (!please_die) {
> do_stuff();
> }
> dead_yet = 1;
> mtx_unlock(&lock);
> kthread_exit(0);
> }
>
> and foo_main() can be unloaded (it's part of a module) then you are still
> racey and can panic on kldunload if you foo_main() is preempted after the
> mtx_unlock() but before the kthread_exit() and foo_detach() completes and
> returns to kldunload() which unmaps the module. I think you didn't make the
> race worse though, as the old code was missing the lock and only used
> tsleep() before.
Thanks. I really do not want to delve into the ndis locking so at least
its not worse as you say.
cheers,
Andrew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070723211055.GC6575>