Re: [PATCH 1/3] loop: Use worker per cgroup instead of kworker

From: Hillf Danton <hdanton@sina.com>
To: Dan Schatzberg <schatzberg.dan@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>, Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [PATCH 1/3] loop: Use worker per cgroup instead of kworker
Date: Wed,  7 Apr 2021 14:53:00 +0800	[thread overview]
Message-ID: <20210407065300.1478-1-hdanton@sina.com> (raw)
In-Reply-To: <YGyvra69F/DIa7KI@dschatzberg-fedora-PC0Y6AEN.dhcp.thefacebook.com>

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.