[PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[Felix Moessbauer]

The submit queue polling threads are "kernel" threads that are started
from the userland. In case the userland task is part of a cgroup with
the cpuset controller enabled, the poller should also stay within that
cpuset. This also holds, as the poller belongs to the same cgroup as
the task that started it.

With the current implementation, a process can "break out" of the
defined cpuset by creating sq pollers consuming CPU time on other CPUs,
which is especially problematic for realtime applications.

Part of this problem was fixed in a5fc1441 by dropping the
PF_NO_SETAFFINITY flag, but this only becomes effective after the first
modification of the cpuset (i.e. the pollers cpuset is correct after the
first update of the enclosing cgroups cpuset).

By inheriting the cpuset of the creating tasks, we ensure that the
poller is created with a cpumask that is a subset of the cgroups mask.
Inheriting the creators cpumask is reasonable, as other userland tasks
also inherit the mask.

Fixes: 37d1e2e3642e ("io_uring: move SQPOLL thread io-wq forked worker")
Cc: stable@vger.kernel.org # 6.1+
Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 io_uring/sqpoll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index 3b50dc9586d1..4681b2c41a96 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -289,7 +289,7 @@ static int io_sq_thread(void *data)
 	if (sqd->sq_cpu != -1) {
 		set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
 	} else {
-		set_cpus_allowed_ptr(current, cpu_online_mask);
+		set_cpus_allowed_ptr(current, sqd->thread->cpus_ptr);
 		sqd->sq_cpu = raw_smp_processor_id();
 	}
 
-- 
2.39.2

Re: [PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[Jens Axboe]

On 9/6/24 7:44 AM, Felix Moessbauer wrote:
> The submit queue polling threads are "kernel" threads that are started

It's not a kernel thread, it's a normal userland thread that just never
exits to userspace.

> from the userland. In case the userland task is part of a cgroup with
> the cpuset controller enabled, the poller should also stay within that
> cpuset. This also holds, as the poller belongs to the same cgroup as
> the task that started it.
> 
> With the current implementation, a process can "break out" of the
> defined cpuset by creating sq pollers consuming CPU time on other CPUs,
> which is especially problematic for realtime applications.
> 
> Part of this problem was fixed in a5fc1441 by dropping the
> PF_NO_SETAFFINITY flag, but this only becomes effective after the first
> modification of the cpuset (i.e. the pollers cpuset is correct after the
> first update of the enclosing cgroups cpuset).
> 
> By inheriting the cpuset of the creating tasks, we ensure that the
> poller is created with a cpumask that is a subset of the cgroups mask.
> Inheriting the creators cpumask is reasonable, as other userland tasks
> also inherit the mask.

Looks fine to me.

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[MOESSBAUER, Felix]

On Fri, 2024-09-06 at 07:47 -0600, Jens Axboe wrote:
> On 9/6/24 7:44 AM, Felix Moessbauer wrote:
> > The submit queue polling threads are "kernel" threads that are
> > started
> 
> It's not a kernel thread, it's a normal userland thread that just
> never
> exits to userspace.

One more reason to behave like a userland thread. I used the term
"kernel" thread as it was used in commit a5fc1441af as well, referring
to the same thing.

Shall I update the commit message?

Best regards,
Felix

> 
> > from the userland. In case the userland task is part of a cgroup
> > with
> > the cpuset controller enabled, the poller should also stay within
> > that
> > cpuset. This also holds, as the poller belongs to the same cgroup
> > as
> > the task that started it.
> > 
> > With the current implementation, a process can "break out" of the
> > defined cpuset by creating sq pollers consuming CPU time on other
> > CPUs,
> > which is especially problematic for realtime applications.
> > 
> > Part of this problem was fixed in a5fc1441 by dropping the
> > PF_NO_SETAFFINITY flag, but this only becomes effective after the
> > first
> > modification of the cpuset (i.e. the pollers cpuset is correct
> > after the
> > first update of the enclosing cgroups cpuset).
> > 
> > By inheriting the cpuset of the creating tasks, we ensure that the
> > poller is created with a cpumask that is a subset of the cgroups
> > mask.
> > Inheriting the creators cpumask is reasonable, as other userland
> > tasks
> > also inherit the mask.
> 
> Looks fine to me.
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[Jens Axboe]

On 9/6/24 7:52 AM, MOESSBAUER, Felix wrote:
> On Fri, 2024-09-06 at 07:47 -0600, Jens Axboe wrote:
>> On 9/6/24 7:44 AM, Felix Moessbauer wrote:
>>> The submit queue polling threads are "kernel" threads that are
>>> started
>>
>> It's not a kernel thread, it's a normal userland thread that just
>> never
>> exits to userspace.
> 
> One more reason to behave like a userland thread. I used the term
> "kernel" thread as it was used in commit a5fc1441af as well, referring
> to the same thing.
> 
> Shall I update the commit message?

Not needed, I just amended it while committing.

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[Jens Axboe]

On Fri, 06 Sep 2024 15:44:33 +0200, Felix Moessbauer wrote:
> The submit queue polling threads are "kernel" threads that are started
> from the userland. In case the userland task is part of a cgroup with
> the cpuset controller enabled, the poller should also stay within that
> cpuset. This also holds, as the poller belongs to the same cgroup as
> the task that started it.
> 
> With the current implementation, a process can "break out" of the
> defined cpuset by creating sq pollers consuming CPU time on other CPUs,
> which is especially problematic for realtime applications.
> 
> [...]

Applied, thanks!

[1/1] io_uring/sqpoll: inherit cpumask of creating process
      commit: d369fdf0908a8a026a8a4b8729d2a193b75fd2d6

Best regards,
-- 
Jens Axboe

Re: [PATCH 1/1] io_uring/sqpoll: inherit cpumask of creating process

[Jens Axboe]

On 9/6/24 7:44 AM, Felix Moessbauer wrote:
> diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
> index 3b50dc9586d1..4681b2c41a96 100644
> --- a/io_uring/sqpoll.c
> +++ b/io_uring/sqpoll.c
> @@ -289,7 +289,7 @@ static int io_sq_thread(void *data)
>  	if (sqd->sq_cpu != -1) {
>  		set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
>  	} else {
> -		set_cpus_allowed_ptr(current, cpu_online_mask);
> +		set_cpus_allowed_ptr(current, sqd->thread->cpus_ptr);
>  		sqd->sq_cpu = raw_smp_processor_id();
>  	}

On second thought, why are we even setting this in the first place?
sqd->thread == current here, it should be a no-op to do:

	set_cpus_allowed_ptr(current, current->cpus_ptr);

IOW, the line should just get deleted. Can you send out a v2?

-- 
Jens Axboe