On Tue, 6 Apr 2021 Dan Schatzberg wrote:
>On Sat, Apr 03, 2021 at 10:09:02AM +0800, Hillf Danton wrote:
>> On Fri, 2 Apr 2021 12:16:32 Dan Schatzberg wrote:
>> > +queue_work:
>> > + if (worker) {
>> > + /*
>> > + * We need to remove from the idle list here while
>> > + * holding the lock so that the idle timer doesn't
>> > + * free the worker
>> > + */
>> > + if (!list_empty(&worker->idle_list))
>> > + list_del_init(&worker->idle_list);
>>
>> Nit, only queue work if the worker is inactive - otherwise it is taking
>> care of the cmd_list.
>
>By worker is inactive, you mean worker is on the idle_list? Yes, I
>think you're right that queue_work() is unnecessary in that case since
>each worker checks empty cmd_list then adds itself to idle_list under
>the lock.
>
>>
>> > + work = &worker->work;
>> > + cmd_list = &worker->cmd_list;
>> > + } else {
>> > + work = &lo->rootcg_work;
>> > + cmd_list = &lo->rootcg_cmd_list;
>> > + }
>> > + list_add_tail(&cmd->list_entry, cmd_list);
>> > + queue_work(lo->workqueue, work);
>> > + spin_unlock_irq(&lo->lo_work_lock);
>> > }
>> [...]
>> > + /*
>> > + * We only add to the idle list if there are no pending cmds
>> > + * *and* the worker will not run again which ensures that it
>> > + * is safe to free any worker on the idle list
>> > + */
>> > + if (worker && !work_pending(&worker->work)) {
>>
>> The empty cmd_list is a good enough reason for worker to become idle.
>
>This is only true with the above change to avoid a gratuitous
>queue_work(), right?
It is always true because of the empty cmd_list - the idle_list is the only
place for the worker to go at this point.
>Otherwise we run the risk of freeing a worker
>concurrently with loop_process_work() being invoked.
My suggestion is a minor optimization at most without any change to removing
worker off the idle_list on queuing work - that cuts the risk for you.
