[PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

Since sqthread is userspace like thread now, it should respect cgroup
setting, thus we should consider current allowed cpuset when doing
cpu binding for sqthread.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---

v1-->v2
 - add a helper in cpuset.c so that we can directly leverage task_cs()
 - remove v1 code which we don't need now

 fs/io_uring.c          | 21 ++++++++++++++++-----
 include/linux/cpuset.h |  7 +++++++
 kernel/cgroup/cpuset.c | 11 +++++++++++
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index be3c3aea6398..a0f54c545158 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -79,6 +79,7 @@
 #include <linux/pagemap.h>
 #include <linux/io_uring.h>
 #include <linux/tracehook.h>
+#include <linux/cpuset.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/io_uring.h>
@@ -7000,6 +7001,16 @@ static bool io_sqd_handle_event(struct io_sq_data *sqd)
 	return did_sig || test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
 }
 
+static int io_sq_bind_cpu(int cpu)
+{
+	if (!test_cpu_in_current_cpuset(cpu))
+		pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
+	else
+		set_cpus_allowed_ptr(current, cpumask_of(cpu));
+
+	return 0;
+}
+
 static int io_sq_thread(void *data)
 {
 	struct io_sq_data *sqd = data;
@@ -7010,11 +7021,9 @@ static int io_sq_thread(void *data)
 
 	snprintf(buf, sizeof(buf), "iou-sqp-%d", sqd->task_pid);
 	set_task_comm(current, buf);
-
 	if (sqd->sq_cpu != -1)
-		set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
-	else
-		set_cpus_allowed_ptr(current, cpu_online_mask);
+		io_sq_bind_cpu(sqd->sq_cpu);
+
 	current->flags |= PF_NO_SETAFFINITY;
 
 	mutex_lock(&sqd->lock);
@@ -8208,8 +8217,10 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 			int cpu = p->sq_thread_cpu;
 
 			ret = -EINVAL;
-			if (cpu >= nr_cpu_ids || !cpu_online(cpu))
+			if (cpu >= nr_cpu_ids || !cpu_online(cpu) ||
+			    !test_cpu_in_current_cpuset(cpu))
 				goto err_sqpoll;
+
 			sqd->sq_cpu = cpu;
 		} else {
 			sqd->sq_cpu = -1;
diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
index 04c20de66afc..fad77c91bc1f 100644
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -116,6 +116,8 @@ static inline int cpuset_do_slab_mem_spread(void)
 
 extern bool current_cpuset_is_being_rebound(void);
 
+extern bool test_cpu_in_current_cpuset(int cpu);
+
 extern void rebuild_sched_domains(void);
 
 extern void cpuset_print_current_mems_allowed(void);
@@ -257,6 +259,11 @@ static inline bool current_cpuset_is_being_rebound(void)
 	return false;
 }
 
+static inline bool test_cpu_in_current_cpuset(int cpu)
+{
+	return false;
+}
+
 static inline void rebuild_sched_domains(void)
 {
 	partition_sched_domains(1, NULL, NULL);
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index adb5190c4429..a63c27e9430e 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1849,6 +1849,17 @@ bool current_cpuset_is_being_rebound(void)
 	return ret;
 }
 
+bool test_cpu_in_current_cpuset(int cpu)
+{
+	bool ret;
+
+	rcu_read_lock();
+	ret = cpumask_test_cpu(cpu, task_cs(current)->effective_cpus);
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static int update_relax_domain_level(struct cpuset *cs, s64 val)
 {
 #ifdef CONFIG_SMP
-- 
2.24.4

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Jens Axboe]

On 8/27/21 8:13 AM, Hao Xu wrote:
> Since sqthread is userspace like thread now, it should respect cgroup
> setting, thus we should consider current allowed cpuset when doing
> cpu binding for sqthread.

In general, this looks way better than v1. Just a few minor comments
below.

> @@ -7000,6 +7001,16 @@ static bool io_sqd_handle_event(struct io_sq_data *sqd)
>  	return did_sig || test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
>  }
>  
> +static int io_sq_bind_cpu(int cpu)
> +{
> +	if (!test_cpu_in_current_cpuset(cpu))
> +		pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
> +	else
> +		set_cpus_allowed_ptr(current, cpumask_of(cpu));
> +
> +	return 0;
> +}

This should not be triggerable, unless the set changes between creation
and the thread being created. Hence maybe the warn is fine. I'd probably
prefer terminating the thread at that point, which would result in an
-EOWNERDEAD return when someone attempts to wake the thread.

Which is probably OK, as we really should not hit this path.

> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
> index 04c20de66afc..fad77c91bc1f 100644
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -116,6 +116,8 @@ static inline int cpuset_do_slab_mem_spread(void)
>  
>  extern bool current_cpuset_is_being_rebound(void);
>  
> +extern bool test_cpu_in_current_cpuset(int cpu);
> +
>  extern void rebuild_sched_domains(void);
>  
>  extern void cpuset_print_current_mems_allowed(void);
> @@ -257,6 +259,11 @@ static inline bool current_cpuset_is_being_rebound(void)
>  	return false;
>  }
>  
> +static inline bool test_cpu_in_current_cpuset(int cpu)
> +{
> +	return false;
> +}
> +
>  static inline void rebuild_sched_domains(void)
>  {
>  	partition_sched_domains(1, NULL, NULL);
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index adb5190c4429..a63c27e9430e 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -1849,6 +1849,17 @@ bool current_cpuset_is_being_rebound(void)
>  	return ret;
>  }
>  
> +bool test_cpu_in_current_cpuset(int cpu)
> +{
> +	bool ret;
> +
> +	rcu_read_lock();
> +	ret = cpumask_test_cpu(cpu, task_cs(current)->effective_cpus);
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> +
>  static int update_relax_domain_level(struct cpuset *cs, s64 val)
>  {
>  #ifdef CONFIG_SMP

In terms of review and so forth, I'd split this into a prep patch. Then
patch 2 just becomes the io_uring consumer of it.

-- 
Jens Axboe

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/27 下午10:18, Jens Axboe 写道:
> On 8/27/21 8:13 AM, Hao Xu wrote:
>> Since sqthread is userspace like thread now, it should respect cgroup
>> setting, thus we should consider current allowed cpuset when doing
>> cpu binding for sqthread.
> 
> In general, this looks way better than v1. Just a few minor comments
> below.
> 
>> @@ -7000,6 +7001,16 @@ static bool io_sqd_handle_event(struct io_sq_data *sqd)
>>   	return did_sig || test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
>>   }
>>   
>> +static int io_sq_bind_cpu(int cpu)
>> +{
>> +	if (!test_cpu_in_current_cpuset(cpu))
>> +		pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
>> +	else
>> +		set_cpus_allowed_ptr(current, cpumask_of(cpu));
>> +
>> +	return 0;
>> +}
> 
> This should not be triggerable, unless the set changes between creation
> and the thread being created. Hence maybe the warn is fine. I'd probably
> prefer terminating the thread at that point, which would result in an
> -EOWNERDEAD return when someone attempts to wake the thread.
> 
> Which is probably OK, as we really should not hit this path.
Actually I think cpuset change offen happen in container environment(
at leaset in my practice), eg. by resource monitor and balancer. So I
did this check to make sure we are still maintain sq_cpu logic at that
time as possible as we can. Though the problem is still there during
sqthread running time(the cpuset can change at anytime, which changes
the cpumask of sqthread)

Regards,
Hao
> 
>> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
>> index 04c20de66afc..fad77c91bc1f 100644
>> --- a/include/linux/cpuset.h
>> +++ b/include/linux/cpuset.h
>> @@ -116,6 +116,8 @@ static inline int cpuset_do_slab_mem_spread(void)
>>   
>>   extern bool current_cpuset_is_being_rebound(void);
>>   
>> +extern bool test_cpu_in_current_cpuset(int cpu);
>> +
>>   extern void rebuild_sched_domains(void);
>>   
>>   extern void cpuset_print_current_mems_allowed(void);
>> @@ -257,6 +259,11 @@ static inline bool current_cpuset_is_being_rebound(void)
>>   	return false;
>>   }
>>   
>> +static inline bool test_cpu_in_current_cpuset(int cpu)
>> +{
>> +	return false;
>> +}
>> +
>>   static inline void rebuild_sched_domains(void)
>>   {
>>   	partition_sched_domains(1, NULL, NULL);
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index adb5190c4429..a63c27e9430e 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -1849,6 +1849,17 @@ bool current_cpuset_is_being_rebound(void)
>>   	return ret;
>>   }
>>   
>> +bool test_cpu_in_current_cpuset(int cpu)
>> +{
>> +	bool ret;
>> +
>> +	rcu_read_lock();
>> +	ret = cpumask_test_cpu(cpu, task_cs(current)->effective_cpus);
>> +	rcu_read_unlock();
>> +
>> +	return ret;
>> +}
>> +
>>   static int update_relax_domain_level(struct cpuset *cs, s64 val)
>>   {
>>   #ifdef CONFIG_SMP
> 
> In terms of review and so forth, I'd split this into a prep patch. Then
> patch 2 just becomes the io_uring consumer of it.
> 

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/28 上午12:57, Hao Xu 写道:
> 在 2021/8/27 下午10:18, Jens Axboe 写道:
>> On 8/27/21 8:13 AM, Hao Xu wrote:
>>> Since sqthread is userspace like thread now, it should respect cgroup
>>> setting, thus we should consider current allowed cpuset when doing
>>> cpu binding for sqthread.
>>
>> In general, this looks way better than v1. Just a few minor comments
>> below.
>>
>>> @@ -7000,6 +7001,16 @@ static bool io_sqd_handle_event(struct 
>>> io_sq_data *sqd)
>>>       return did_sig || test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
>>>   }
>>> +static int io_sq_bind_cpu(int cpu)
>>> +{
>>> +    if (!test_cpu_in_current_cpuset(cpu))
>>> +        pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
>>> +    else
>>> +        set_cpus_allowed_ptr(current, cpumask_of(cpu));
>>> +
>>> +    return 0;
>>> +}
>>
>> This should not be triggerable, unless the set changes between creation
>> and the thread being created. Hence maybe the warn is fine. I'd probably
>> prefer terminating the thread at that point, which would result in an
>> -EOWNERDEAD return when someone attempts to wake the thread.
>>
>> Which is probably OK, as we really should not hit this path.
> Actually I think cpuset change offen happen in container environment(
> at leaset in my practice), eg. by resource monitor and balancer. So I
> did this check to make sure we are still maintain sq_cpu logic at that
> time as possible as we can. Though the problem is still there during
> sqthread running time(the cpuset can change at anytime, which changes
> the cpumask of sqthread)
And because the cpumask of sqthread may be changed by the cgroup cpuset
change at any time, so here I just print a warnning rather than
terminating sqthread due to this 'normal thing'..
> 
> Regards,
> Hao
>>
>>> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
>>> index 04c20de66afc..fad77c91bc1f 100644
>>> --- a/include/linux/cpuset.h
>>> +++ b/include/linux/cpuset.h
>>> @@ -116,6 +116,8 @@ static inline int cpuset_do_slab_mem_spread(void)
>>>   extern bool current_cpuset_is_being_rebound(void);
>>> +extern bool test_cpu_in_current_cpuset(int cpu);
>>> +
>>>   extern void rebuild_sched_domains(void);
>>>   extern void cpuset_print_current_mems_allowed(void);
>>> @@ -257,6 +259,11 @@ static inline bool 
>>> current_cpuset_is_being_rebound(void)
>>>       return false;
>>>   }
>>> +static inline bool test_cpu_in_current_cpuset(int cpu)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>>   static inline void rebuild_sched_domains(void)
>>>   {
>>>       partition_sched_domains(1, NULL, NULL);
>>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>>> index adb5190c4429..a63c27e9430e 100644
>>> --- a/kernel/cgroup/cpuset.c
>>> +++ b/kernel/cgroup/cpuset.c
>>> @@ -1849,6 +1849,17 @@ bool current_cpuset_is_being_rebound(void)
>>>       return ret;
>>>   }
>>> +bool test_cpu_in_current_cpuset(int cpu)
>>> +{
>>> +    bool ret;
>>> +
>>> +    rcu_read_lock();
>>> +    ret = cpumask_test_cpu(cpu, task_cs(current)->effective_cpus);
>>> +    rcu_read_unlock();
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static int update_relax_domain_level(struct cpuset *cs, s64 val)
>>>   {
>>>   #ifdef CONFIG_SMP
>>
>> In terms of review and so forth, I'd split this into a prep patch. Then
>> patch 2 just becomes the io_uring consumer of it.
>>

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Jens Axboe]

On 8/27/21 11:03 AM, Hao Xu wrote:
> 在 2021/8/28 上午12:57, Hao Xu 写道:
>> 在 2021/8/27 下午10:18, Jens Axboe 写道:
>>> On 8/27/21 8:13 AM, Hao Xu wrote:
>>>> Since sqthread is userspace like thread now, it should respect cgroup
>>>> setting, thus we should consider current allowed cpuset when doing
>>>> cpu binding for sqthread.
>>>
>>> In general, this looks way better than v1. Just a few minor comments
>>> below.
>>>
>>>> @@ -7000,6 +7001,16 @@ static bool io_sqd_handle_event(struct 
>>>> io_sq_data *sqd)
>>>>       return did_sig || test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
>>>>   }
>>>> +static int io_sq_bind_cpu(int cpu)
>>>> +{
>>>> +    if (!test_cpu_in_current_cpuset(cpu))
>>>> +        pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
>>>> +    else
>>>> +        set_cpus_allowed_ptr(current, cpumask_of(cpu));
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> This should not be triggerable, unless the set changes between creation
>>> and the thread being created. Hence maybe the warn is fine. I'd probably
>>> prefer terminating the thread at that point, which would result in an
>>> -EOWNERDEAD return when someone attempts to wake the thread.
>>>
>>> Which is probably OK, as we really should not hit this path.
>> Actually I think cpuset change offen happen in container environment(
>> at leaset in my practice), eg. by resource monitor and balancer. So I
>> did this check to make sure we are still maintain sq_cpu logic at that
>> time as possible as we can. Though the problem is still there during
>> sqthread running time(the cpuset can change at anytime, which changes
>> the cpumask of sqthread)
> And because the cpumask of sqthread may be changed by the cgroup cpuset
> change at any time, so here I just print a warnning rather than
> terminating sqthread due to this 'normal thing'..

Do we even want the warning then? If it's an expected thing, seems very
annoying to warn about it.

-- 
Jens Axboe

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Tejun Heo]

Hello,

On Fri, Aug 27, 2021 at 10:13:15PM +0800, Hao Xu wrote:
> +static int io_sq_bind_cpu(int cpu)
> +{
> +	if (!test_cpu_in_current_cpuset(cpu))
> +		pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
> +	else
> +		set_cpus_allowed_ptr(current, cpumask_of(cpu));
> +
> +	return 0;
> +}
...
> @@ -8208,8 +8217,10 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
>  			int cpu = p->sq_thread_cpu;
>  
>  			ret = -EINVAL;
> -			if (cpu >= nr_cpu_ids || !cpu_online(cpu))
> +			if (cpu >= nr_cpu_ids || !cpu_online(cpu) ||
> +			    !test_cpu_in_current_cpuset(cpu))
>  				goto err_sqpoll;

Given that sq_thread is user-like thread and belongs to the right cgroup,
I'm not quite sure what the above achieves - the affinities can break
anytime, so one-time check doesn't really solve the problem. All it seems to
add is warning messages. What's the expected behavior when an io thread
can't run on the target cpu anymore?

Thanks.

-- 
tejun

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/28 上午1:09, Jens Axboe 写道:
> On 8/27/21 11:03 AM, Hao Xu wrote:
>> 在 2021/8/28 上午12:57, Hao Xu 写道:
>>> 在 2021/8/27 下午10:18, Jens Axboe 写道:
>>>> On 8/27/21 8:13 AM, Hao Xu wrote:
>>>>> Since sqthread is userspace like thread now, it should respect cgroup
>>>>> setting, thus we should consider current allowed cpuset when doing
>>>>> cpu binding for sqthread.
>>>>
>>>> In general, this looks way better than v1. Just a few minor comments
>>>> below.
>>>>
>>>>> @@ -7000,6 +7001,16 @@ static bool io_sqd_handle_event(struct
>>>>> io_sq_data *sqd)
>>>>>        return did_sig || test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
>>>>>    }
>>>>> +static int io_sq_bind_cpu(int cpu)
>>>>> +{
>>>>> +    if (!test_cpu_in_current_cpuset(cpu))
>>>>> +        pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
>>>>> +    else
>>>>> +        set_cpus_allowed_ptr(current, cpumask_of(cpu));
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> This should not be triggerable, unless the set changes between creation
>>>> and the thread being created. Hence maybe the warn is fine. I'd probably
>>>> prefer terminating the thread at that point, which would result in an
>>>> -EOWNERDEAD return when someone attempts to wake the thread.
>>>>
>>>> Which is probably OK, as we really should not hit this path.
>>> Actually I think cpuset change offen happen in container environment(
>>> at leaset in my practice), eg. by resource monitor and balancer. So I
>>> did this check to make sure we are still maintain sq_cpu logic at that
>>> time as possible as we can. Though the problem is still there during
>>> sqthread running time(the cpuset can change at anytime, which changes
>>> the cpumask of sqthread)
>> And because the cpumask of sqthread may be changed by the cgroup cpuset
>> change at any time, so here I just print a warnning rather than
>> terminating sqthread due to this 'normal thing'..
> 
> Do we even want the warning then? If it's an expected thing, seems very
> annoying to warn about it.Hmm, there are several things:
1) cpuset change may happen several times a day on some environment, so
it doesn't make sense to exit
2) there won't be big chance that the cpuset change happen between
sqthread creation and waken up. So there probably won't be many
warnning.
3) Though we can warn about cpuset change in case 2), but we cannot warn
at the sqthread running time (when it's in while loop) when cpuset
change. And I don't think we should do cpu check and re-binding from
time to time in sqthread. Good thing is users can get this info in
userspace on their own.
So maybe you're right, we should remove this warnning since it doesn't
raise up just for 2), not for all cases, and users can get to know the
situation by taskset command. Jens, What do you think?

> 

Re: [PATCH for-5.15 v2] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/28 上午1:26, Tejun Heo 写道:
> Hello,
> 
> On Fri, Aug 27, 2021 at 10:13:15PM +0800, Hao Xu wrote:
>> +static int io_sq_bind_cpu(int cpu)
>> +{
>> +	if (!test_cpu_in_current_cpuset(cpu))
>> +		pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
>> +	else
>> +		set_cpus_allowed_ptr(current, cpumask_of(cpu));
>> +
>> +	return 0;
>> +}
> ...
>> @@ -8208,8 +8217,10 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
>>   			int cpu = p->sq_thread_cpu;
>>   
>>   			ret = -EINVAL;
>> -			if (cpu >= nr_cpu_ids || !cpu_online(cpu))
>> +			if (cpu >= nr_cpu_ids || !cpu_online(cpu) ||
>> +			    !test_cpu_in_current_cpuset(cpu))
>>   				goto err_sqpoll;
> 
> Given that sq_thread is user-like thread and belongs to the right cgroup,
> I'm not quite sure what the above achieves - the affinities can break
A user of io_uring can pass a cpu id to the kernel to indicate which cpu
the sq_thread should be bound to.
> anytime, so one-time check doesn't really solve the problem. All it seems to
Yes, this a problem.
> add is warning messages. What's the expected behavior when an io thread
> can't run on the target cpu anymore?
A user binds sqthread to some cpu due to some reason which we may not
know, so if the target cpu isn't available anymore, I think cpuset of
sqthread should be as same as the task group's it sits, since we don't
know which cpu we should re-bind it to. And this is the current
behavior. I think Jens knows this question better than I do, Jens?

> 
> Thanks.
>