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

On Thursday 11 August 2005 07:07 pm, Scott Long wrote:
> John Baldwin wrote:
> > On Thursday 11 August 2005 05:41 pm, John Baldwin wrote:
> >>On Thursday 11 August 2005 04:09 pm, Joerg Pulz wrote:
> >>>Hi,
> >>>
> >>>with a fresh installed 6.0-BETA2 i get this when xl(4) gets configured
> >>> at the system startup.
> >>>System is P3-800MHz SMP. dmesg is attached.
> >>
> >>I'm working on fixes for this.  Ping me in a day or so for a patch.
> >
> > Ok, I've got a patch.  I added a taskqueue_stop() function to bring
> > taskqueue's a bit closer inline with the callout*() API and use
> > taskqueue_stop() in xl_stop() as it is ok to be called with locks held
> > and doesn't block.  The xl task handler function now bails if
> > IFF_DRV_RUNNING is clear, and I added a taskqueue_drain() in detach to
> > make sure we were finished with the mutex and function before detach
> > finishes.  Unfortunately, the patch is to HEAD, but you can probably get
> > it to work on 6.x by changing if_drv_flags to if_flags and
> > IFF_DRV_RUNNING to IFF_RUNNING on 6.x.
> >
> > http://www.FreeBSD.org/~jhb/patches/xl_locking
>
> It looks like taskqueue_stop merely removes a pending task from the
> queue, it doesn't protect against there being a task already running
> and/or sleeping.  I know that you're looking for the convenience of
> being able to cancel a taskqueue without having to worry about locks,
> but ignoring the possibility of an in-progress task is dangerous.  It's
> incovenient, but it's the price of concurrency in the kernel.  I've
> objected to callout_stop for the same reason.  Never the less, if you're
> looking to have a similar API as callout_*, why not follow their model
> and have _taskqueue_stop_safe() ?

Note that I modified xl_rxeof_task to not call the task handler if 
IFF_DRV_RUNNING is clear to handle this race.  I also just added locally 
another check after it relocks the driver lock after if_input() in xl_rxeof()  
to bail if the interface has been stopped as well which should handle that 
race.  Both of these races are present even in the taskqueue_drain() case as 
you need the task to die if taskqueue_drain() is ever going to unblock.

Also, I specifically call taskqueue_drain() after xl_stop() in xl_detach() 
(same as I do for callouts) to make sure it really has stopped in the one 
case where I need more assurance than just taskqueue_stop().  (The drains 
here for both callout and taskqueue are to make sure that if the other thread 
is blocked on our lock when we call xl_stop(), it will have a chance to get 
it and release it so it's not contested when we go to destroy it later and to 
ensure that the threads won't be in the text of our module.)

I didn't try to merge stop and drain for taskqueue as the current 
taskqueue_drain() is different from callout_drain() in that callout_drain() 
still removes the callout if possible and then waits, whereas 
taskqueue_drain() makes no attempt currently to dequeue the task but just 
waits on it to finish if it is either in the queue or running.  One nicer 
thing about callouts is that you can use callout_init_mtx() which lets you 
push the initial equivalent to the IFF_DRV_RUNNING check out into softclock() 
since it can check to see if you are cancelled after grabbing your lock.  
Given the fact that tasks can sleep, I think this would be less useful for 
taskqueue as task handlers need to handle stop races around sleeps so they 
might as well handle it during startup as well so that it is more consistent 
that they handle it in all cases instead of just some of them.

Being able to stop separately from drain is not "bad", it's just a different 
model, it lets you separate "fire off an event now" from "wait for ack from 
the event" instead of forcing you to do it all at once.

-- 
John Baldwin <jhb_at_FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org