Hi,

On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
quoted text
> 
> I think you're misunderstanding the existing taskqueue(9) implementation.
> 
> As long as TQ_LOCK is held, the state of ta->ta_pending cannot change,
> nor can the set of running tasks.  So the order of checks is
> irrelevant.

I agree that the order of checks is not important. That is not the problem.

Cut & paste from suggested taskqueue patch from Fleming:

 > +int
quoted text
> > +taskqueue_cancel(struct taskqueue *queue, struct task *task)
> > +{
> > +       int rc;
> > +
> > +       TQ_LOCK(queue);
> > +       if (!task_is_running(queue, task)) {
> > +               if ((rc = task->ta_pending) > 0)
> > +                       STAILQ_REMOVE(&queue->tq_queue, task, task,
> > ta_link); +               task->ta_pending = 0;
> > +       } else {
> > +               rc = -EBUSY;

What happens in this case if ta_pending > 0. Are you saying this is not 
possible? If ta_pending > 0, shouldn't we also do a STAILQ_REMOVE() ?

quoted text
> > +       }
> > +       TQ_UNLOCK(queue);
> > +       return (rc);
> > +}
>

quoted text
> 
> As John said, the taskqueue(9) implementation cannot protect consumers
> of it from re-queueing a task; that kind of serialization needs to
> happen at a higher level.

Agreed, but that is not what I'm pointing at. I'm pointing at what happens if 
you re-queue a task and then cancel while it is actually running. Will the 
task still be queued for execution after taskqueue_cancel()?

quoted text
> taskqueue(9) is not quite like callout(9); the taskqueue(9)
> implementation drops all locks before calling the task's callback
> function.  So once the task is running, taskqueue(9) can do nothing
> about it until the task stops running. 

This is not the problem.

quoted text
>
> This is why Jeff's
> implementation of taskqueue_cancel(9) slept if the task was running,
> and why mine returns an error code.  The only way to know for sure
> that a task is "about" to run is to ask taskqueue(9), because there's
> a window where the TQ_LOCK is dropped before the callback is entered.

And if you re-queue and cancel in this window, shouldn't this also be handled 
like in the other cases?

Cut & paste from kern/subr_taskqueue.c:

                task->ta_pending = 0;
                tb.tb_running = task;
                TQ_UNLOCK(queue);

If a concurrent thread at exactly this point in time calls taskqueue_enqueue() 
on this task, then we re-add the task to the "queue->tq_queue". So far we 
agree. Imagine now that for some reason a following call to taskqueue_cancel() 
on this task at same point in time. Now, shouldn't taskqueue_cancel() also 
remove the task from the "queue->tq_queue" in this case aswell? Because in 
your implementation you only remove the task if we are not running, and that 
is not true when we are at exactly this point in time.

                task->ta_func(task->ta_context, pending);

                TQ_LOCK(queue);
                tb.tb_running = NULL;
                wakeup(task);


Another issue I noticed is that the ta_pending counter should have a wrap 
protector.

--HPS
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"