From owner-freebsd-arch@FreeBSD.ORG Wed Jul 9 20:40:56 2014 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id EEBA687D; Wed, 9 Jul 2014 20:40:56 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 99BCE27BB; Wed, 9 Jul 2014 20:40:56 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 2050AB953; Wed, 9 Jul 2014 16:40:55 -0400 (EDT) From: John Baldwin To: freebsd-arch@freebsd.org Subject: Re: callout(9) really this complicated? Date: Wed, 9 Jul 2014 12:11:47 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20140415; KDE/4.5.5; amd64; ; ) References: <20140704041521.GW45513@funkthat.com> <53B64694.9030100@selasky.org> <20140704155752.GB45513@funkthat.com> In-Reply-To: <20140704155752.GB45513@funkthat.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201407091211.47642.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 09 Jul 2014 16:40:55 -0400 (EDT) Cc: Hans Petter Selasky , arch@freebsd.org, John-Mark Gurney X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jul 2014 20:40:57 -0000 On Friday, July 04, 2014 11:57:52 am John-Mark Gurney wrote: > Hans Petter Selasky wrote this message on Fri, Jul 04, 2014 at 08:15 +0200: > > On 07/04/14 06:15, John-Mark Gurney wrote: > > >So, I was going to look at using callout(9) for some time delayed > > >actions... But upon reading the docs, a) the docs are inconsistent, > > >and b) the docs only talk about requirements in other section... > > > > > >Is there a better interface? If so, can we mark callout(9) deprecated? > > >If not, I have some questions... > > > > > >If you want callout_drain to work properly, you have to add extra code > > >to both your callout, and around the usage of it... > > > > > >callout_drain does not drain the callout: > > > However, the callout subsystem does guarantee that the callout will > > > be > > > fully stopped before callout_drain() returns. > > > > > >Yet other parse of the docs say that you can depend upon the callout > > >being fully stopped.. I've sent email to Ian (iedowse) about why he > > >added this statement... > > > > > >Second, the amount of work you have to do to make sure you drain > > >seems pretty crazy as documented in Avoiding Race Conditions... > > > > > >It seems like if I have created a callout w/ callout_init_mtx, > > >that I shouldn't have to do extra work to make it work correctly... > > > > > >When calling _callout_stop_safe as callout_drain, there is no assert > > >that the lock is held, though it is documented as requiring it by: > > > The function callout_drain() is identical to callout_stop() except > > > that > > > it will wait for the callout to be completed if it is already in > > > progress. > > > > > >Maybe we need to add an additional statement here? and assert that it > > >isn't locked? > > > > > >Also, I have tried to follow the code, but it's complicated, so I'm > > >hoping that I can get some help here. > > > > Probably the documentation needs an update. The implementation is fine. > > Probably... But w/ bad docs, it makes you wonder about the > implementation... If you use callout_init_mtx(), then you can use callout_reset() and callout_stop() while holding the mutex and everything will work as expected. You typically only need to use callout_drain() during a device detach routine to "destroy" the callout(). (A typical "detach" routine looks something like this: - detach external connections (ifnet, cdev, etc.) - stop the hardware itself (foo_stop in various NIC drivers) - this step typically does a callout_stop() with the relevant lock held - drain any pending async events - bus_teardown_intr() will block until the interrupt handler is idle - callout_drain() will block if the callout is pending because softclock had already dequeued it and it was waiting for the driver lock when you called callout_stop() earlier - taskqueue_drain() (note that tasks do not have implicit locking ala callout_init_mtx(), so you need to set some sort of flag that your task function checks and breaks out of any loops for in the "stop the hardware" step) - free resources and destroy locks, etc. > > drain is always called unlocked. > > Then why the whole long section about avoiding races? Point #3 is > the main one that I'm talking about... It seems like it'd be easier > for callout to properly maintain the active flag (maybe set a flag to > tell callout to do so), or not even maintain it since it really isn't > used for anything important if you can munge it all you want... There > aren't any statements about bad things happening if you call _deactivate > before the callout is called... The language is unclear, but you only need to use one of the 3 options to work around the races. Note that if you use callout_init_mtx() you fall into case #1 and don't need to worry about the others. If you want to use callout_init(.., CALLOUT_MPSAFE) and manage all the locking in your callout handler directly, then you need to handle the assorted races. However, you can generally get by with something far simpler than it suggests. You can just do what I described above for taskqueue_drain(), i.e. have your timer function do: foo(void *arg) { struct foo_softc *sc; sc = arg; FOO_LOCK(sc); if (sc->is_dead) { FOO_UNLOCK(sc); return; } .... callout_reset(...); FOO_UNLOCK(sc); } In this case, simply setting 'is_dead' in the "stop the hardware" step and using callout_drain() will work fine. -- John Baldwin