[PATCH] io_uring: consider cgroup setting when binding sqpoll cpu

[PATCH] 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>
---
 fs/io_uring.c | 41 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 51c4166f68b5..35d0e7bc283b 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>
@@ -6903,15 +6904,35 @@ static int io_sq_thread(void *data)
 	struct io_ring_ctx *ctx;
 	unsigned long timeout = 0;
 	char buf[TASK_COMM_LEN];
+	cpumask_var_t cpus_allowed;
 	DEFINE_WAIT(wait);
 
 	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);
+	if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
+		mutex_lock(&sqd->lock);
+		sqd->thread = NULL;
+		list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
+			io_ring_set_wakeup_flag(ctx);
+		io_run_task_work();
+		mutex_unlock(&sqd->lock);
+
+		complete(&sqd->exited);
+		do_exit(1);
+	}
+	cpuset_cpus_allowed(current, cpus_allowed);
+	if (sqd->sq_cpu != -1) {
+		if (!cpumask_test_cpu(sqd->sq_cpu, cpus_allowed)) {
+			sqd->sq_cpu = -1;
+			pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
+		} else {
+			set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
+		}
+	}
+	if (sqd->sq_cpu == -1)
+		set_cpus_allowed_ptr(current, cpus_allowed);
+	free_cpumask_var(cpus_allowed);
 	current->flags |= PF_NO_SETAFFINITY;
 
 	mutex_lock(&sqd->lock);
@@ -8060,11 +8081,23 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
 
 		if (p->flags & IORING_SETUP_SQ_AFF) {
 			int cpu = p->sq_thread_cpu;
+			cpumask_var_t cpus_allowed;
 
 			ret = -EINVAL;
 			if (cpu >= nr_cpu_ids || !cpu_online(cpu))
 				goto err_sqpoll;
+			if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
+				ret = -ENOMEM;
+				goto err_sqpoll;
+			}
+			cpuset_cpus_allowed(current, cpus_allowed);
+			if (!cpumask_test_cpu(cpu, cpus_allowed)) {
+				free_cpumask_var(cpus_allowed);
+				goto err_sqpoll;
+			}
+
 			sqd->sq_cpu = cpu;
+			free_cpumask_var(cpus_allowed);
 		} else {
 			sqd->sq_cpu = -1;
 		}
-- 
2.24.4

Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu

[Jens Axboe]

On 8/15/21 9:10 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.

This seems a bit convoluted for what it needs to do. Surely we can just
test sqd->sq_cpu directly in the task_cs()?

-- 
Jens Axboe

Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/15 下午11:10, 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>
> ---
>   fs/io_uring.c | 41 +++++++++++++++++++++++++++++++++++++----
>   1 file changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 51c4166f68b5..35d0e7bc283b 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>
> @@ -6903,15 +6904,35 @@ static int io_sq_thread(void *data)
>   	struct io_ring_ctx *ctx;
>   	unsigned long timeout = 0;
>   	char buf[TASK_COMM_LEN];
> +	cpumask_var_t cpus_allowed;
>   	DEFINE_WAIT(wait);
>   
>   	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);
> +	if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
> +		mutex_lock(&sqd->lock);
> +		sqd->thread = NULL;
> +		list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
> +			io_ring_set_wakeup_flag(ctx);
> +		io_run_task_work();
> +		mutex_unlock(&sqd->lock);
> +
> +		complete(&sqd->exited);
> +		do_exit(1);
> +	}
> +	cpuset_cpus_allowed(current, cpus_allowed);
> +	if (sqd->sq_cpu != -1) {
> +		if (!cpumask_test_cpu(sqd->sq_cpu, cpus_allowed)) {
Here comes a small race with cpus_allowed updating. If this is the case,
it means the cpus_allowed are changing frequently, in this situation
sqthread's cpumask will be setup to cpus_allowed anyway, so it's still
in the correct range.
(considering the aim of this patch is to limit sqthread to the cpuset
where its cgroup allows. If cpuset of its cgroup is changing frequently
we cannot bind sqthread to some cpu anyway.)
> +			sqd->sq_cpu = -1;
> +			pr_warn("sqthread %d: bound cpu not allowed\n", current->pid);
> +		} else {
> +			set_cpus_allowed_ptr(current, cpumask_of(sqd->sq_cpu));
> +		}
> +	}
> +	if (sqd->sq_cpu == -1)
> +		set_cpus_allowed_ptr(current, cpus_allowed);
> +	free_cpumask_var(cpus_allowed);
>   	current->flags |= PF_NO_SETAFFINITY;
>   
>   	mutex_lock(&sqd->lock);
> @@ -8060,11 +8081,23 @@ static int io_sq_offload_create(struct io_ring_ctx *ctx,
>   
>   		if (p->flags & IORING_SETUP_SQ_AFF) {
>   			int cpu = p->sq_thread_cpu;
> +			cpumask_var_t cpus_allowed;
>   
>   			ret = -EINVAL;
>   			if (cpu >= nr_cpu_ids || !cpu_online(cpu))
>   				goto err_sqpoll;
> +			if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
> +				ret = -ENOMEM;
> +				goto err_sqpoll;
> +			}
> +			cpuset_cpus_allowed(current, cpus_allowed);
> +			if (!cpumask_test_cpu(cpu, cpus_allowed)) {
> +				free_cpumask_var(cpus_allowed);
> +				goto err_sqpoll;
> +			}
> +
>   			sqd->sq_cpu = cpu;
> +			free_cpumask_var(cpus_allowed);
>   		} else {
>   			sqd->sq_cpu = -1;
>   		}
> 

Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/15 下午11:19, Jens Axboe 写道:
> On 8/15/21 9:10 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.
> 
> This seems a bit convoluted for what it needs to do. Surely we can just
> test sqd->sq_cpu directly in the task_cs()?
I didn't know task_cs() before, it seems to be a static function, which
is called by cpuset_cpus_allowed(), and this one is exposed.
> 

Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu

[Jens Axboe]

On 8/16/21 12:04 AM, Hao Xu wrote:
> 在 2021/8/15 下午11:19, Jens Axboe 写道:
>> On 8/15/21 9:10 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.
>>
>> This seems a bit convoluted for what it needs to do. Surely we can just
>> test sqd->sq_cpu directly in the task_cs()?
> I didn't know task_cs() before, it seems to be a static function, which
> is called by cpuset_cpus_allowed(), and this one is exposed.

But it'd be a much saner to add a helper for this rather than add all
of that boiler plate code to io_uring just to check for whether or not
a CPU is set in a mask.

-- 
Jens Axboe

Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu

[Hao Xu]

在 2021/8/17 上午12:58, Jens Axboe 写道:
> On 8/16/21 12:04 AM, Hao Xu wrote:
>> 在 2021/8/15 下午11:19, Jens Axboe 写道:
>>> On 8/15/21 9:10 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.
>>>
>>> This seems a bit convoluted for what it needs to do. Surely we can just
>>> test sqd->sq_cpu directly in the task_cs()?
>> I didn't know task_cs() before, it seems to be a static function, which
>> is called by cpuset_cpus_allowed(), and this one is exposed.
> 
> But it'd be a much saner to add a helper for this rather than add all
> of that boiler plate code to io_uring just to check for whether or not
> a CPU is set in a mask.
I looked into cpuset_cpus_allowed(), it seems the code logic
guarantee_online_cpus() is neccessary? It guarantees the cpumask
returned are all online cpus.


>