Received-SPF: pass (google.com: domain of linux-kernel+bounces-128121-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 2 Apr 2024 16:13:41 +0200
From: Michal =?utf-8?Q?Koutn=C3=BD?= <mkoutny@suse.com>
To: Waiman Long <longman@redhat.com>
Cc: Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>, 
	Johannes Weiner <hannes@cmpxchg.org>, Thomas Gleixner <tglx@linutronix.de>, 
	Peter Zijlstra <peterz@infradead.org>, "Rafael J. Wysocki" <rafael@kernel.org>, 
	Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>, Shuah Khan <shuah@kernel.org>, 
	linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-pm@vger.kernel.org, 
	linux-kselftest@vger.kernel.org, Frederic Weisbecker <frederic@kernel.org>, 
	"Paul E. McKenney" <paulmck@kernel.org>, Ingo Molnar <mingo@kernel.org>, 
	Valentin Schneider <vschneid@redhat.com>, Anna-Maria Behnsen <anna-maria@linutronix.de>, 
	Alex Shi <alexs@kernel.org>, Vincent Guittot <vincent.guittot@linaro.org>, 
	Barry Song <song.bao.hua@hisilicon.com>
Subject: Re: [PATCH 1/2] cgroup/cpuset: Make cpuset hotplug processing
 synchronous
Message-ID: <kce74bx6aafxfuw5yovaschym4ze4kommfk74eq5totojytest@mdxnfvl2kdol>
References: <20240401145858.2656598-1-longman@redhat.com>
 <20240401145858.2656598-2-longman@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="m3jtt4lb3pupq4nw"
Content-Disposition: inline
In-Reply-To: <20240401145858.2656598-2-longman@redhat.com>


--m3jtt4lb3pupq4nw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hello Waiman.

(I have no opinion on the overall locking reworks, only the bits about
v1 migrations caught my attention.)

On Mon, Apr 01, 2024 at 10:58:57AM -0400, Waiman Long <longman@redhat.com> wrote:
..
> @@ -4383,12 +4377,20 @@ hotplug_update_tasks_legacy(struct cpuset *cs,
>  	/*
>  	 * Move tasks to the nearest ancestor with execution resources,
>  	 * This is full cgroup operation which will also call back into
> -	 * cpuset. Should be done outside any lock.
> +	 * cpuset. Execute it asynchronously using workqueue.

                   ...to avoid deadlock on cpus_read_lock

Is this the reason?
Also, what happens with the tasks in the window till the migration
happens?
Is it handled gracefully that their cpu is gone?


> -	if (is_empty) {
> -		mutex_unlock(&cpuset_mutex);
> -		remove_tasks_in_empty_cpuset(cs);
> -		mutex_lock(&cpuset_mutex);
> +	if (is_empty && css_tryget_online(&cs->css)) {
> +		struct cpuset_remove_tasks_struct *s;
> +
> +		s = kzalloc(sizeof(*s), GFP_KERNEL);

Is there a benefit of having a work for each cpuset?
Instead of traversing whole top_cpuset once in the async work.


Thanks,
Michal


--m3jtt4lb3pupq4nw
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQQpEWyjXuwGT2dDBqAGvrMr/1gcjgUCZgwSkwAKCRAGvrMr/1gc
jku7AP4wunT3qTbn0gNfnNlgS+aZURi7eScVHpxboPBoGEeorgEA0EEpGH79hJUs
C5xcOZLm/5ZRgE5MXKIx0kiWKBGt9QY=
=8lGP
-----END PGP SIGNATURE-----

--m3jtt4lb3pupq4nw--