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

[Hao Xu <haoxu@linux.alibaba.com>]

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

>