Re: 6.0-BETA2: taskqueue_drain for if_xl.c:2796

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