Date:	Tue, 19 Feb 2013 23:04:31 +0800
From:	Lai Jiangshan <eag0628@...il.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Lai Jiangshan <laijs@...fujitsu.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 13/15] workqueue: also record worker in work->data if running&&queued

Hi, TJ

Thank you for reviews and comments.
First, the patchset can't be merged to 3.9 since there are under
discussion and are not shown they benefit.
(But are patch 1, 5-8 possible merged after rebased and revised?)
Second, the removal of the hash table is very possible in future with
different implementation.

On Tue, Feb 19, 2013 at 3:50 AM, Tejun Heo <tj@...nel.org> wrote:
> Hello, Lai.
>
> On Tue, Feb 19, 2013 at 12:12:14AM +0800, Lai Jiangshan wrote:
>> +/**
>> + * get_work_cwq - get cwq of the work
>> + * @work: the work item of interest
>> + *
>> + * CONTEXT:
>> + * spin_lock_irq(&pool->lock), the work must be queued on this pool
>> + */
>> +static struct cpu_workqueue_struct *get_work_cwq(struct work_struct *work)
>> +{
>> +     unsigned long data = atomic_long_read(&work->data);
>> +     struct worker *worker;
>> +
>> +     if (data & WORK_STRUCT_CWQ) {
>> +             return (void *)(data & WORK_STRUCT_WQ_DATA_MASK);
>> +     } else if (data & WORK_OFFQ_REQUEUED) {
>> +             worker = worker_by_id(data >> WORK_OFFQ_WORKER_SHIFT);
>> +             BUG_ON(!worker || !worker->requeue);
>> +             return worker->current_cwq;
>> +     } else {
>> +             BUG();
>> +             return NULL;
>> +     }
>> +}
>
> So, work->data points to the last worker ID if off-queue or on-queue
> with another worker executing it and points to cwq if on-queue w/o
> another worker executing.  If on-queue w/ concurrent execution, the
> excuting worker updates work->data when it finishes execution, right?

right.

>
> Why no documentation about it at all?  The mechanism is convoluted
> with interlocking from both work and worker sides.  Lack of
> documentation makes things difficult for reviewers and later readers
> of the code.

sorry.

>
>> @@ -1296,8 +1283,16 @@ static void __queue_work(unsigned int cpu, struct workqueue_struct *wq,
>>               worklist = &cwq->delayed_works;
>>       }
>>
>> -     color_flags = work_color_to_flags(cwq->work_color);
>> -     insert_work(cwq, work, worklist, color_flags | delayed_flags);
>> +     if (worker) {
>> +             worker->requeue = true;
>> +             worker->requeue_color = cwq->work_color;
>> +             set_work_worker_and_keep_pending(work, worker->id,
>> +                             delayed_flags | WORK_OFFQ_REQUEUED);
>> +             list_add_tail(&work->entry, worklist);
>> +     } else {
>> +             color_flags = work_color_to_flags(cwq->work_color);
>> +             insert_work(cwq, work, worklist, color_flags | delayed_flags);
>> +     }
>
> I can't say I like this.  In interlocks the work being queued and the
> worker so that both have to watch out for each other.  It's kinda
> nasty.
>
>> @@ -2236,6 +2241,16 @@ __acquires(&pool->lock)
>>       worker->current_func = NULL;
>>       worker->current_cwq = NULL;
>>       cwq_dec_nr_in_flight(cwq, work_color);
>> +
>> +     if (unlikely(worker->requeue)) {
>> +             unsigned long color_flags, keep_flags;
>> +
>> +             worker->requeue = false;
>> +             keep_flags = atomic_long_read(&work->data);
>> +             keep_flags &= WORK_STRUCT_LINKED | WORK_STRUCT_DELAYED;
>> +             color_flags = work_color_to_flags(worker->requeue_color);
>> +             set_work_cwq(work, cwq, color_flags | keep_flags);
>> +     }
>
> So, what was before mostly one way "is it still executing?" query
> becomes three party handshake among the queuer, executing worker and
> try_to_grab_pending(), and we end up shifting information from the
> queuer through the executing worker because work->data can't hold both
> workqueue and worker information.
>
> I don't know, Lai.  While removal of busy_hash is nice, I'm not really
> sure whether we're ending up with better or worse code by doing this.
> It's more convoluted for sure.  Performance-wise, now that idr_find()
> for pool costs almost nothing (because we're very unlikely to have
> more than 256 pools), we're comparing one idr lookup (which can easily
> blow through 256 single layer optimization limit) against two simple
> hash table lookup.  I don't really think either would be noticeably
> better than the other in any measureable way.
>
> The trade-off, while doesn't seem too bad, doesn't seem much
> beneficial either.  It's different from what we're currently doing but
> I'm not sure we're making it better by doing this.
>

I agree your comments.

Thanks,
Lai

PS: Some small benefits of this patchset
1) work_busy() can be reimplement lockless.
2) the user can issue *reentrance" works by re-init the work.
    (w/o this patchset, the users must defer the free and alloc new
work item if they want reentrancable)
3) natural immunity of such
bugs(https://bugzilla.kernel.org/show_bug.cgi?id=51701)

but small.
need to find a better way in future.

>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists