6.0-BETA2: taskqueue_drain for if_xl.c:2796
Scott Long
scottl at samsco.org
Wed Aug 17 20:05:04 GMT 2005
John Baldwin wrote:
> On Wednesday 17 August 2005 01:38 pm, Scott Long wrote:
>
>>John Baldwin wrote:
>>
>>>On Tuesday 16 August 2005 01:21 pm, John Baldwin wrote:
>>>
>>>>On Monday 15 August 2005 02:22 am, Scott Long wrote:
>>>>
>>>>>John Baldwin wrote:
>>>>>
>>>>>>On Thursday 11 August 2005 07:07 pm, Scott Long wrote:
>>>>>
>>>>>I still don't see what the point is of having a function that only
>>>>>removes pending tasks from the queue if it doesn't also drain in
>>>>>progress tasks. Simple example:
>>>>>
>>>>>CPU1 calls xl_stop. The XL_LOCK gets taken. At the same time, CPU2
>>>>>calls taskqueue_swi, which pops the queue and calls xl_rxeof_task. It
>>>>>tries to take the XL_LOCK but instead sleeps (or adaptively spins)
>>>>>because CPU1 already has it. CPU1 calls taskqueue_stop, which doesn't
>>>>>do anything because the task was alrady popped off.
>>>>>
>>>>>Now, at some point, CPU1 **MUST** drop the XL_LOCK and let CPU2 continue
>>>>>that task to completion. There simply is no way around that, right? So
>>>>>why muddle the API with a function that really doesn't solve any
>>>>>problems. The argument that being able to call taskqueue_stop with
>>>>>locks held is a convenience doesn't hold water. You'll still need to
>>>>>drop locks in order to be sure that a mess isn't created.
>>>>
>>>>Well, you need to think through this longer. During a normal stop, we
>>>>don't care if the task runs later if it is just going to stop anyway,
>>>>which it will with the IFF_DRV_RUNNING checks I added (which it needs
>>>>anyway even if you don't add a taskqueue_stop()). See, when CPU2
>>>>eventually runs the xl_rxeof_task, it will drop the lock and return
>>>>instantly because the xl_stop() function clears IFF_DRV_RUNNING in
>>>>addition to calling taskqueue_stop().
>>>
>>>Actually, I thought through this longer some myself on the way home and
>>>because of the RUNNING checks, the taskqueue_stop() in xl_stop() isn't
>>>actually needed for correct operation. It's merely an optimization for
>>>the case that the task is sitting in the queue, but the code will work
>>>fine without it with the taskqueue_drain() moved to xl_detach() and the
>>>IFF_DRV_RUNNING checks added. Thus, if you just really hate
>>>taskqueue_stop() or don't think the optimization is worth it, I can leave
>>>it out. I kind of would like to call it in xl_detach() at least so that
>>>detaching xl0 won't be dependent on another driver's task deciding to
>>>yield. However, if I moved it there I could add blocking to it. I could
>>>also just use the existing taskqueue_drain() and not worry about having
>>>to wait for other tasks to finish. I can't call taskqueue_drain() in
>>>xl_stop() though since xl_stop() is called indirectly from xl_intr().
>>
>>You can't force detach while data is in flight, at least not right now.
>>That is a motivation behind the recent ifnet changes, but it still has a
>>long way to go. And regardless of the rest of the stack being able to
>>handle forced detach, the driver needs to be written in a way so that
>>it's own resources aren't orphaned, which again means making sure that
>>blocked threads are allowed to run to completion, which again means not
>>having the luxury of being fast and loose with locks when trying to
>>drain those tasks.
>
>
> I'm not running "fast and loose" with locks per se. Mostly I'm just
> optimizing the case where we stop the interface (i.e. detach or ifconfig
> down) and the task hasn't even started running yet so that it can just
> discard the task from the queue without running it rather than waiting until
> it gets a chance to run so it can immediately abort.
>
>
>>I do think that taskqueue_stop() is a muddying of the API. If anyone
>>else has a comment on this (BDE?) I would be very interested to hear it.
>
>
> *shrug* It's already confusing in that callout_drain() does both a stop and
> then wait kind of like pthread_cancel() whereas taskqueue_drain() just waits
> until the task has run and finished without trying to cancel it like
> pthread_join(). I'd actually probably be inclined to rename callout_drain()
> to something else if we wanted to resolve that specific instance of confusion
> personally.
>
True, consistency would be nice =-) Would you rather change
taskqueue_drain() to be more like callout_drain()?
Scott
More information about the freebsd-current
mailing list