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>
index | next in thread | previous in thread | raw e-mail
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
help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070723211055.GC6575>