Re: [RFC] Outline of USB process integration in the kernel taskqueue system

On Mon, Nov 8, 2010 at 8:42 AM, John Baldwin <jhb_at_freebsd.org> wrote:
> On Monday, November 08, 2010 10:34:33 am Matthew Fleming wrote:
>> On Mon, Nov 8, 2010 at 6:47 AM, John Baldwin <jhb_at_freebsd.org> wrote:
>> > On Saturday, November 06, 2010 4:33:17 pm Matthew Fleming wrote:
>> >> On Sat, Nov 6, 2010 at 7:22 AM, Hans Petter Selasky <hselasky_at_c2i.net> wrote:
>> >> > Hi,
>> >> >
>> >> > On Saturday 06 November 2010 14:57:50 Matthew Fleming wrote:
>> >> >>
>> >> >> 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
>> >> >> > +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() ?
>> >>
>> >> Ah!  I see what you mean.
>> >>
>> >> I'm not quite sure what the best thing to do here is; I agree it would
>> >> be nice if taskqueue_cancel(9) dequeued the task, but I believe it
>> >> also needs to indicate that the task is currently running.  I guess
>> >> the best thing would be to return the old pending count by reference
>> >> parameter, and 0 or EBUSY to also indicate if there is a task
>> >> currently running.
>> >>
>> >> Adding jhb_at_ to this mail since he has good thoughts on interfacing.
>> >
>> > I agree we should always dequeue when possible.  I think it should return
>> > -EBUSY in that case.  That way code that uses 'cancel' followed by a
>> > conditional 'drain' to implement a blocking 'cancel' will DTRT.
>>
>> Do we not also want the old ta_pending to be returned?  In the case
>> where a task is pending and is also currently running (admittedly a
>> narrow window), how would we do this?  This is why I suggested
>> returning the old ta_pending by reference.  This also allows callers
>> who don't care about the old pending to pass NULL and ignore it.
>
> I would be fine with that then.  I wonder if taskqueue_cancel() could then
> return a simple true/false.  False if the task is running, and true
> otherwise?

Sure, though since we don't really have a bool type in the kernel I'd
still prefer to return an int with EBUSY meaning a task was running.

Thanks,
matthew