From owner-cvs-all@FreeBSD.ORG  Mon Jul 23 21:10:56 2007
Return-Path: <owner-cvs-all@FreeBSD.ORG>
Delivered-To: cvs-all@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id 8F8DC16A418;
	Mon, 23 Jul 2007 21:10:56 +0000 (UTC)
	(envelope-from thompsa@FreeBSD.org)
Received: from heff.fud.org.nz (203-109-251-39.static.bliink.ihug.co.nz
	[203.109.251.39])
	by mx1.freebsd.org (Postfix) with ESMTP id 2B87813C428;
	Mon, 23 Jul 2007 21:10:56 +0000 (UTC)
	(envelope-from thompsa@FreeBSD.org)
Received: by heff.fud.org.nz (Postfix, from userid 1001)
	id 20E7F1CC58; Tue, 24 Jul 2007 09:10:55 +1200 (NZST)
Date: Tue, 24 Jul 2007 09:10:55 +1200
From: Andrew Thompson <thompsa@FreeBSD.org>
To: John Baldwin <jhb@freebsd.org>
Message-ID: <20070723211055.GC6575@heff.fud.org.nz>
References: <200707222053.l6MKrS6v040649@repoman.freebsd.org>
	<200707231050.51004.jhb@freebsd.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <200707231050.51004.jhb@freebsd.org>
User-Agent: Mutt/1.5.13 (2006-08-11)
Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org
Subject: Re: cvs commit: src/sys/compat/ndis subr_ntoskrnl.c
X-BeenThere: cvs-all@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-all>
List-Post: <mailto:cvs-all@freebsd.org>
List-Help: <mailto:cvs-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Mon, 23 Jul 2007 21:10:56 -0000

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