diff mbox series

[v2,1/4] riscv: defconfig: drop RT_GROUP_SCHED=y

Message ID 20240823-fix-riscv-rt_group_sched-v2-1-e4dbae24f7e1@gmail.com
State Handled Elsewhere
Headers show
Series defconfig: drop RT_GROUP_SCHED=y | expand

Commit Message

Celeste Liu Aug. 22, 2024, 5:43 p.m. UTC
Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
introduced it because of Docker, but Docker has removed this requirement
since [1] (2023-04-19).

For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
needs an RT budget assigned, otherwise the processes in it will not be able to
get RT at all. The problem with RT group scheduling is that it requires the
budget assigned but there's no way we could assign a default budget, since the
values to assign are both upper and lower time limits, are absolute, and need to
be sum up to < 1 for each individal cgroup. That means we cannot really come up
with values that would work by default in the general case.[2]

For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
can only be enabled when all RT processes are in the root cgroup. But it will
lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.

Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
support it.[3]

[1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
[3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383

Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com>
---
 arch/riscv/configs/defconfig | 1 -
 1 file changed, 1 deletion(-)

Comments

Conor Dooley Sept. 9, 2024, 11:53 a.m. UTC | #1
On Fri, Aug 23, 2024 at 01:43:26AM +0800, Celeste Liu wrote:
> Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
> introduced it because of Docker, but Docker has removed this requirement
> since [1] (2023-04-19).
> 
> For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
> needs an RT budget assigned, otherwise the processes in it will not be able to
> get RT at all. The problem with RT group scheduling is that it requires the
> budget assigned but there's no way we could assign a default budget, since the
> values to assign are both upper and lower time limits, are absolute, and need to
> be sum up to < 1 for each individal cgroup. That means we cannot really come up
> with values that would work by default in the general case.[2]
> 
> For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
> can only be enabled when all RT processes are in the root cgroup. But it will
> lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.
> 
> Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
> support it.[3]
> 
> [1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
> [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
> [3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383
> 
> Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")

I don't think this fixes tag is suitable, the commit you cite in
moby/docker is a year younger than the one in the fixes tag, so it was
correct at the time it was written. I think the fixes tag should just be
removed, since that commit was not wrong. Or am I missing something?

> Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com>
> ---
>  arch/riscv/configs/defconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> index 12dc8c73a8ac..de85c3ab261e 100644
> --- a/arch/riscv/configs/defconfig
> +++ b/arch/riscv/configs/defconfig
> @@ -9,7 +9,6 @@ CONFIG_CGROUPS=y
>  CONFIG_MEMCG=y
>  CONFIG_CGROUP_SCHED=y
>  CONFIG_CFS_BANDWIDTH=y
> -CONFIG_RT_GROUP_SCHED=y
>  CONFIG_CGROUP_PIDS=y
>  CONFIG_CGROUP_FREEZER=y
>  CONFIG_CGROUP_HUGETLB=y
> 
> -- 
> 2.46.0
>
Celeste Liu Sept. 9, 2024, 12:14 p.m. UTC | #2
On 2024-09-09 19:53, Conor Dooley wrote:

> On Fri, Aug 23, 2024 at 01:43:26AM +0800, Celeste Liu wrote:
>> Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
>> introduced it because of Docker, but Docker has removed this requirement
>> since [1] (2023-04-19).
>>
>> For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
>> needs an RT budget assigned, otherwise the processes in it will not be able to
>> get RT at all. The problem with RT group scheduling is that it requires the
>> budget assigned but there's no way we could assign a default budget, since the
>> values to assign are both upper and lower time limits, are absolute, and need to
>> be sum up to < 1 for each individal cgroup. That means we cannot really come up
>> with values that would work by default in the general case.[2]
>>
>> For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
>> can only be enabled when all RT processes are in the root cgroup. But it will
>> lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.
>>
>> Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
>> support it.[3]
>>
>> [1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
>> [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
>> [3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383
>>
>> Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
> 
> I don't think this fixes tag is suitable, the commit you cite in
> moby/docker is a year younger than the one in the fixes tag, so it was
> correct at the time it was written. I think the fixes tag should just be
> removed, since that commit was not wrong. Or am I missing something?

The Docker commit I cited introduce the fix, not the bug. So it should be later
than commit in fixes tag, otherwise Heinrich Schuchardt won't pick the wrong
config from upstream.

Timeline is:
1) 2015-06-19 Qiang Huang introduce RT_GROUP_SCHED check in Docker.[1]
2) 2022-06-08 Heinrich Schuchardt pick this to defconfig in commit
              in fixes tag.
3) 2023-04-19 Florian Schmaus remove this requirement from Docker in
              commit I cited.

[1]: https://github.com/moby/moby/commit/fb85a99f5899df7475ba6b1ac4b2e1d32974eca2

> 
>> Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com>
>> ---
>>  arch/riscv/configs/defconfig | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
>> index 12dc8c73a8ac..de85c3ab261e 100644
>> --- a/arch/riscv/configs/defconfig
>> +++ b/arch/riscv/configs/defconfig
>> @@ -9,7 +9,6 @@ CONFIG_CGROUPS=y
>>  CONFIG_MEMCG=y
>>  CONFIG_CGROUP_SCHED=y
>>  CONFIG_CFS_BANDWIDTH=y
>> -CONFIG_RT_GROUP_SCHED=y
>>  CONFIG_CGROUP_PIDS=y
>>  CONFIG_CGROUP_FREEZER=y
>>  CONFIG_CGROUP_HUGETLB=y
>>
>> -- 
>> 2.46.0
>>
Conor Dooley Sept. 9, 2024, 1:06 p.m. UTC | #3
On Mon, Sep 09, 2024 at 08:14:16PM +0800, Celeste Liu wrote:
> On 2024-09-09 19:53, Conor Dooley wrote:
> 
> > On Fri, Aug 23, 2024 at 01:43:26AM +0800, Celeste Liu wrote:
> >> Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
> >> introduced it because of Docker, but Docker has removed this requirement
> >> since [1] (2023-04-19).
> >>
> >> For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
> >> needs an RT budget assigned, otherwise the processes in it will not be able to
> >> get RT at all. The problem with RT group scheduling is that it requires the
> >> budget assigned but there's no way we could assign a default budget, since the
> >> values to assign are both upper and lower time limits, are absolute, and need to
> >> be sum up to < 1 for each individal cgroup. That means we cannot really come up
> >> with values that would work by default in the general case.[2]
> >>
> >> For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
> >> can only be enabled when all RT processes are in the root cgroup. But it will
> >> lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.
> >>
> >> Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
> >> support it.[3]
> >>
> >> [1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
> >> [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
> >> [3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383
> >>
> >> Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
> > 
> > I don't think this fixes tag is suitable, the commit you cite in
> > moby/docker is a year younger than the one in the fixes tag, so it was
> > correct at the time it was written. I think the fixes tag should just be
> > removed, since that commit was not wrong. Or am I missing something?
> 
> The Docker commit I cited introduce the fix, not the bug. So it should be later
> than commit in fixes tag, otherwise Heinrich Schuchardt won't pick the wrong
> config from upstream.
> 
> Timeline is:
> 1) 2015-06-19 Qiang Huang introduce RT_GROUP_SCHED check in Docker.[1]
> 2) 2022-06-08 Heinrich Schuchardt pick this to defconfig in commit
>               in fixes tag.
> 3) 2023-04-19 Florian Schmaus remove this requirement from Docker in
>               commit I cited.

Yes, this is the way I understood things to be. IOW, when Heinrich wrote
ba6cfef057e1 ("riscv: enable Docker requirements in defconfig"), it was
a requirement for docker. The requirement later being removed doesn't
make his patch incorrect, which is why I don't think this is a fix.
Celeste Liu Sept. 9, 2024, 1:14 p.m. UTC | #4
On 2024-09-09 21:06, Conor Dooley wrote:
> On Mon, Sep 09, 2024 at 08:14:16PM +0800, Celeste Liu wrote:
>> On 2024-09-09 19:53, Conor Dooley wrote:
>>
>>> On Fri, Aug 23, 2024 at 01:43:26AM +0800, Celeste Liu wrote:
>>>> Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
>>>> introduced it because of Docker, but Docker has removed this requirement
>>>> since [1] (2023-04-19).
>>>>
>>>> For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
>>>> needs an RT budget assigned, otherwise the processes in it will not be able to
>>>> get RT at all. The problem with RT group scheduling is that it requires the
>>>> budget assigned but there's no way we could assign a default budget, since the
>>>> values to assign are both upper and lower time limits, are absolute, and need to
>>>> be sum up to < 1 for each individal cgroup. That means we cannot really come up
>>>> with values that would work by default in the general case.[2]
>>>>
>>>> For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
>>>> can only be enabled when all RT processes are in the root cgroup. But it will
>>>> lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.
>>>>
>>>> Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
>>>> support it.[3]
>>>>
>>>> [1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
>>>> [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
>>>> [3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383
>>>>
>>>> Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
>>>
>>> I don't think this fixes tag is suitable, the commit you cite in
>>> moby/docker is a year younger than the one in the fixes tag, so it was
>>> correct at the time it was written. I think the fixes tag should just be
>>> removed, since that commit was not wrong. Or am I missing something?
>>
>> The Docker commit I cited introduce the fix, not the bug. So it should be later
>> than commit in fixes tag, otherwise Heinrich Schuchardt won't pick the wrong
>> config from upstream.
>>
>> Timeline is:
>> 1) 2015-06-19 Qiang Huang introduce RT_GROUP_SCHED check in Docker.[1]
>> 2) 2022-06-08 Heinrich Schuchardt pick this to defconfig in commit
>>               in fixes tag.
>> 3) 2023-04-19 Florian Schmaus remove this requirement from Docker in
>>               commit I cited.
> 
> Yes, this is the way I understood things to be. IOW, when Heinrich wrote
> ba6cfef057e1 ("riscv: enable Docker requirements in defconfig"), it was
> a requirement for docker. The requirement later being removed doesn't
> make his patch incorrect, which is why I don't think this is a fix.

Oh, you are right. I will send v3 later.
Heinrich Schuchardt Sept. 9, 2024, 1:14 p.m. UTC | #5
On 09.09.24 14:14, Celeste Liu wrote:
> On 2024-09-09 19:53, Conor Dooley wrote:
> 
>> On Fri, Aug 23, 2024 at 01:43:26AM +0800, Celeste Liu wrote:
>>> Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
>>> introduced it because of Docker, but Docker has removed this requirement
>>> since [1] (2023-04-19).
>>>
>>> For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
>>> needs an RT budget assigned, otherwise the processes in it will not be able to
>>> get RT at all. The problem with RT group scheduling is that it requires the
>>> budget assigned but there's no way we could assign a default budget, since the
>>> values to assign are both upper and lower time limits, are absolute, and need to
>>> be sum up to < 1 for each individal cgroup. That means we cannot really come up
>>> with values that would work by default in the general case.[2]
>>>
>>> For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
>>> can only be enabled when all RT processes are in the root cgroup. But it will
>>> lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.
>>>
>>> Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
>>> support it.[3]
>>>
>>> [1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
>>> [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
>>> [3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383
>>>
>>> Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
>>
>> I don't think this fixes tag is suitable, the commit you cite in
>> moby/docker is a year younger than the one in the fixes tag, so it was
>> correct at the time it was written. I think the fixes tag should just be
>> removed, since that commit was not wrong. Or am I missing something?
> 
> The Docker commit I cited introduce the fix, not the bug. So it should be later
> than commit in fixes tag, otherwise Heinrich Schuchardt won't pick the wrong
> config from upstream.
> 
> Timeline is:
> 1) 2015-06-19 Qiang Huang introduce RT_GROUP_SCHED check in Docker.[1]
> 2) 2022-06-08 Heinrich Schuchardt pick this to defconfig in commit
>                in fixes tag.
> 3) 2023-04-19 Florian Schmaus remove this requirement from Docker in
>                commit I cited.
> 
> [1]: https://github.com/moby/moby/commit/fb85a99f5899df7475ba6b1ac4b2e1d32974eca2

I experience no problems running Docker without CONFIG_RT_GROUP_SCHED on 
Ubuntu 24.04.

The Docker patch is only for contrib/check-config.sh. Probably Docker at 
the time of my patch would have worked without CONFIG_RT_GROUP_SCHED, 
too. I have no issue with the Fixes: tag in this patch.

Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

> 
>>
>>> Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com>
>>> ---
>>>   arch/riscv/configs/defconfig | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
>>> index 12dc8c73a8ac..de85c3ab261e 100644
>>> --- a/arch/riscv/configs/defconfig
>>> +++ b/arch/riscv/configs/defconfig
>>> @@ -9,7 +9,6 @@ CONFIG_CGROUPS=y
>>>   CONFIG_MEMCG=y
>>>   CONFIG_CGROUP_SCHED=y
>>>   CONFIG_CFS_BANDWIDTH=y
>>> -CONFIG_RT_GROUP_SCHED=y
>>>   CONFIG_CGROUP_PIDS=y
>>>   CONFIG_CGROUP_FREEZER=y
>>>   CONFIG_CGROUP_HUGETLB=y
>>>
>>> -- 
>>> 2.46.0
>>>
>
Celeste Liu Sept. 10, 2024, 12:57 p.m. UTC | #6
On 2024-09-09 20:16, Conor Dooley wrote:
> On Mon, Sep 09, 2024 at 08:14:16PM +0800, Celeste Liu wrote:
> > On 2024-09-09 19:53, Conor Dooley wrote:
> > 
> > > On Fri, Aug 23, 2024 at 01:43:26AM +0800, Celeste Liu wrote:
> > >> Commit ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
> > >> introduced it because of Docker, but Docker has removed this requirement
> > >> since [1] (2023-04-19).
> > >>
> > >> For cgroup v1, if turned on, and there's any cgroup in the "cpu" hierarchy it
> > >> needs an RT budget assigned, otherwise the processes in it will not be able to
> > >> get RT at all. The problem with RT group scheduling is that it requires the
> > >> budget assigned but there's no way we could assign a default budget, since the
> > >> values to assign are both upper and lower time limits, are absolute, and need to
> > >> be sum up to < 1 for each individal cgroup. That means we cannot really come up
> > >> with values that would work by default in the general case.[2]
> > >>
> > >> For cgroup v2, it's almost unusable as well. If it turned on, the cpu controller
> > >> can only be enabled when all RT processes are in the root cgroup. But it will
> > >> lose the benefits of cgroup v2 if all RT process were placed in the same cgroup.
> > >>
> > >> Red Hat, Gentoo, Arch Linux and Debian all disable it. systemd also doesn't
> > >> support it.[3]
> > >>
> > >> [1]: https://github.com/moby/moby/commit/005150ed69c540fb0b5323e0f2208608c1204536
> > >> [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1229700
> > >> [3]: https://github.com/systemd/systemd/issues/13781#issuecomment-549164383
> > >>
> > >> Fixes: ba6cfef057e1 ("riscv: enable Docker requirements in defconfig")
> > > 
> > > I don't think this fixes tag is suitable, the commit you cite in
> > > moby/docker is a year younger than the one in the fixes tag, so it was
> > > correct at the time it was written. I think the fixes tag should just be
> > > removed, since that commit was not wrong. Or am I missing something?
> > 
> > The Docker commit I cited introduce the fix, not the bug. So it should be later
> > than commit in fixes tag, otherwise Heinrich Schuchardt won't pick the wrong
> > config from upstream.
> > 
> > Timeline is:
> > 1) 2015-06-19 Qiang Huang introduce RT_GROUP_SCHED check in Docker.[1]
> > 2) 2022-06-08 Heinrich Schuchardt pick this to defconfig in commit
> >               in fixes tag.
> > 3) 2023-04-19 Florian Schmaus remove this requirement from Docker in
> >               commit I cited.
> 
> Yes, this is the way I understood things to be. IOW, when Heinrich wrote
> ba6cfef057e1 ("riscv: enable Docker requirements in defconfig"), it was
> a requirement for docker. The requirement later being removed doesn't
> make his patch incorrect, which is why I don't think this is a fix.

v3 has been sent.
diff mbox series

Patch

diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index 12dc8c73a8ac..de85c3ab261e 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -9,7 +9,6 @@  CONFIG_CGROUPS=y
 CONFIG_MEMCG=y
 CONFIG_CGROUP_SCHED=y
 CONFIG_CFS_BANDWIDTH=y
-CONFIG_RT_GROUP_SCHED=y
 CONFIG_CGROUP_PIDS=y
 CONFIG_CGROUP_FREEZER=y
 CONFIG_CGROUP_HUGETLB=y