public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu
@ 2021-08-15 15:10 Hao Xu
  2021-08-15 15:19 ` Jens Axboe
  2021-08-15 15:22 ` Hao Xu
  0 siblings, 2 replies; 6+ messages in thread
From: Hao Xu @ 2021-08-15 15:10 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

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 <[email protected]>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu
  2021-08-15 15:10 [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu Hao Xu
@ 2021-08-15 15:19 ` Jens Axboe
  2021-08-16  6:04   ` Hao Xu
  2021-08-15 15:22 ` Hao Xu
  1 sibling, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2021-08-15 15:19 UTC (permalink / raw)
  To: Hao Xu; +Cc: io-uring, Pavel Begunkov, Joseph Qi

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu
  2021-08-15 15:10 [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu Hao Xu
  2021-08-15 15:19 ` Jens Axboe
@ 2021-08-15 15:22 ` Hao Xu
  1 sibling, 0 replies; 6+ messages in thread
From: Hao Xu @ 2021-08-15 15:22 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

在 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 <[email protected]>
> ---
>   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;
>   		}
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu
  2021-08-15 15:19 ` Jens Axboe
@ 2021-08-16  6:04   ` Hao Xu
  2021-08-16 16:58     ` Jens Axboe
  0 siblings, 1 reply; 6+ messages in thread
From: Hao Xu @ 2021-08-16  6:04 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu
  2021-08-16  6:04   ` Hao Xu
@ 2021-08-16 16:58     ` Jens Axboe
  2021-08-17 12:08       ` Hao Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2021-08-16 16:58 UTC (permalink / raw)
  To: Hao Xu; +Cc: io-uring, Pavel Begunkov, Joseph Qi

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu
  2021-08-16 16:58     ` Jens Axboe
@ 2021-08-17 12:08       ` Hao Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Hao Xu @ 2021-08-17 12:08 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi

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


> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-17 12:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-15 15:10 [PATCH] io_uring: consider cgroup setting when binding sqpoll cpu Hao Xu
2021-08-15 15:19 ` Jens Axboe
2021-08-16  6:04   ` Hao Xu
2021-08-16 16:58     ` Jens Axboe
2021-08-17 12:08       ` Hao Xu
2021-08-15 15:22 ` Hao Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox