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: 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: Sat,  3 Apr 2021 10:09:02 +0800	[thread overview]
Message-ID: <20210403020902.1384-1-hdanton@sina.com> (raw)
In-Reply-To: <20210402191638.3249835-2-schatzberg.dan@gmail.com>

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.

> +		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.

> +		worker->last_ran_at = jiffies;
> +		list_add_tail(&worker->idle_list, &lo->idle_worker_list);
> +		loop_set_timer(lo);
> +	}
> +	spin_unlock_irq(&lo->lo_work_lock);
> +	current->flags = orig_flags;
> +}