[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Richard Palethorpe]

It is not possible to use a controller in V2 cgroups if it has been
mounted as a V1 controller. So if V1 is mounted we use it regardless
of if V2 is available.

We have to include a space in tst_is_mounted so that we do not match
cgroup2.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 lib/tst_cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index ba413d874..73ddd4b82 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -44,7 +44,7 @@ enum tst_cgroup_ver tst_cgroup_version(void)
         enum tst_cgroup_ver cg_ver;
 
         if (tst_cgroup_check("cgroup2")) {
-                if (!tst_is_mounted("cgroup2") && tst_is_mounted("cgroup"))
+                if (tst_is_mounted("cgroup "))
                         cg_ver = TST_CGROUP_V1;
                 else
                         cg_ver = TST_CGROUP_V2;
-- 
2.28.0

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Petr Vorel]

Hi Richie,

> It is not possible to use a controller in V2 cgroups if it has been
> mounted as a V1 controller. So if V1 is mounted we use it regardless
> of if V2 is available.

> We have to include a space in tst_is_mounted so that we do not match
> cgroup2.

Acked-by: Petr Vorel <pvorel@suse.cz>

Thanks for your fix.

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  lib/tst_cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index ba413d874..73ddd4b82 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -44,7 +44,7 @@ enum tst_cgroup_ver tst_cgroup_version(void)
>          enum tst_cgroup_ver cg_ver;

>          if (tst_cgroup_check("cgroup2")) {
> -                if (!tst_is_mounted("cgroup2") && tst_is_mounted("cgroup"))
> +                if (tst_is_mounted("cgroup "))
On first look the space looks like typo. But people can use git blame.

>                          cg_ver = TST_CGROUP_V1;
>                  else
>                          cg_ver = TST_CGROUP_V2;

Kind regards,
Petr

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Li Wang]

Hi Richard,

On Thu, Sep 24, 2020 at 7:11 PM Richard Palethorpe <rpalethorpe@suse.com>
wrote:

> It is not possible to use a controller in V2 cgroups if it has been
> mounted as a V1 controller. So if V1 is mounted we use it regardless
> of if V2 is available.
>
> We have to include a space in tst_is_mounted so that we do not match
> cgroup2.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>  lib/tst_cgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index ba413d874..73ddd4b82 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -44,7 +44,7 @@ enum tst_cgroup_ver tst_cgroup_version(void)
>          enum tst_cgroup_ver cg_ver;
>
>          if (tst_cgroup_check("cgroup2")) {
> -                if (!tst_is_mounted("cgroup2") &&
> tst_is_mounted("cgroup"))
> +                if (tst_is_mounted("cgroup "))
>

Add a space in the suffix still not work as expected.

The reason is that tst_is_mounted("cgroup ") also get non-zero return if
system only mount cgroup_v2, which lead to choose cgroup_v1 in LTP test.

# cat /proc/mounts |grep cgroup
cgroup2 /sys/fs/cgroup cgroup2 rw,seclabel,nosuid,nodev,noexec,relatime 0 0

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200924/04c3bfa6/attachment.htm>

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
> On Thu, Sep 24, 2020 at 7:11 PM Richard Palethorpe <rpalethorpe@suse.com>
> wrote:
>
>> It is not possible to use a controller in V2 cgroups if it has been
>> mounted as a V1 controller. So if V1 is mounted we use it regardless
>> of if V2 is available.
>>
>> We have to include a space in tst_is_mounted so that we do not match
>> cgroup2.
>>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>>  lib/tst_cgroup.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>> index ba413d874..73ddd4b82 100644
>> --- a/lib/tst_cgroup.c
>> +++ b/lib/tst_cgroup.c
>> @@ -44,7 +44,7 @@ enum tst_cgroup_ver tst_cgroup_version(void)
>>          enum tst_cgroup_ver cg_ver;
>>
>>          if (tst_cgroup_check("cgroup2")) {
>> -                if (!tst_is_mounted("cgroup2") &&
>> tst_is_mounted("cgroup"))
>> +                if (tst_is_mounted("cgroup "))
>>
>
> Add a space in the suffix still not work as expected.
>
> The reason is that tst_is_mounted("cgroup ") also get non-zero return if
> system only mount cgroup_v2, which lead to choose cgroup_v1 in LTP test.
>
> # cat /proc/mounts |grep cgroup
> cgroup2 /sys/fs/cgroup cgroup2 rw,seclabel,nosuid,nodev,noexec,relatime 0 0

I wonder if it would be better to simply try mounting/using V2 and if
that fails try V1?

-- 
Thank you,
Richard.

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Li Wang]

On Thu, Sep 24, 2020 at 10:06 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello Li,
>
> Li Wang <liwang@redhat.com> writes:
>
> > Hi Richard,
> >
> > On Thu, Sep 24, 2020 at 7:11 PM Richard Palethorpe <rpalethorpe@suse.com
> >
> > wrote:
> >
> >> It is not possible to use a controller in V2 cgroups if it has been
> >> mounted as a V1 controller. So if V1 is mounted we use it regardless
> >> of if V2 is available.
> >>
> >> We have to include a space in tst_is_mounted so that we do not match
> >> cgroup2.
> >>
> >> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> >> ---
> >>  lib/tst_cgroup.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> >> index ba413d874..73ddd4b82 100644
> >> --- a/lib/tst_cgroup.c
> >> +++ b/lib/tst_cgroup.c
> >> @@ -44,7 +44,7 @@ enum tst_cgroup_ver tst_cgroup_version(void)
> >>          enum tst_cgroup_ver cg_ver;
> >>
> >>          if (tst_cgroup_check("cgroup2")) {
> >> -                if (!tst_is_mounted("cgroup2") &&
> >> tst_is_mounted("cgroup"))
> >> +                if (tst_is_mounted("cgroup "))
> >>
> >
> > Add a space in the suffix still not work as expected.
> >
> > The reason is that tst_is_mounted("cgroup ") also get non-zero return if
> > system only mount cgroup_v2, which lead to choose cgroup_v1 in LTP test.
> >
> > # cat /proc/mounts |grep cgroup
> > cgroup2 /sys/fs/cgroup cgroup2 rw,seclabel,nosuid,nodev,noexec,relatime
> 0 0
>
> I wonder if it would be better to simply try mounting/using V2 and if
> that fails try V1?
>

That will be work but not good, because if cgroup mount fails, how do we
know it is a bug or not support?

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200925/67cf14da/attachment-0001.htm>

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> On Thu, Sep 24, 2020 at 10:06 PM Richard Palethorpe <rpalethorpe@suse.de>
> wrote:
>
>> Hello Li,
>>
>> Li Wang <liwang@redhat.com> writes:
>>
>> > Hi Richard,
>> >
>> > On Thu, Sep 24, 2020 at 7:11 PM Richard Palethorpe <rpalethorpe@suse.com
>> >
>> > wrote:
>> >
>> >> It is not possible to use a controller in V2 cgroups if it has been
>> >> mounted as a V1 controller. So if V1 is mounted we use it regardless
>> >> of if V2 is available.
>> >>
>> >> We have to include a space in tst_is_mounted so that we do not match
>> >> cgroup2.
>> >>
>> >> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> >> ---
>> >>  lib/tst_cgroup.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
>> >> index ba413d874..73ddd4b82 100644
>> >> --- a/lib/tst_cgroup.c
>> >> +++ b/lib/tst_cgroup.c
>> >> @@ -44,7 +44,7 @@ enum tst_cgroup_ver tst_cgroup_version(void)
>> >>          enum tst_cgroup_ver cg_ver;
>> >>
>> >>          if (tst_cgroup_check("cgroup2")) {
>> >> -                if (!tst_is_mounted("cgroup2") &&
>> >> tst_is_mounted("cgroup"))
>> >> +                if (tst_is_mounted("cgroup "))
>> >>
>> >
>> > Add a space in the suffix still not work as expected.
>> >
>> > The reason is that tst_is_mounted("cgroup ") also get non-zero return if
>> > system only mount cgroup_v2, which lead to choose cgroup_v1 in LTP test.
>> >
>> > # cat /proc/mounts |grep cgroup
>> > cgroup2 /sys/fs/cgroup cgroup2 rw,seclabel,nosuid,nodev,noexec,relatime
>> 0 0
>>
>> I wonder if it would be better to simply try mounting/using V2 and if
>> that fails try V1?
>>
>
> That will be work but not good, because if cgroup mount fails, how do we
> know it is a bug or not support?

I think this is a valid point if you are writing a test for mounting
cgroups. However if we are testing something else then trying to guess
if cgroups should be available before using them, makes the test
fragile. I suppose we could check *after* trying to use the cgroups so
we can report some diagnostic info.

-- 
Thank you,
Richard.

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Li Wang]

Richard Palethorpe <rpalethorpe@suse.de> wrote:

...
> >> I wonder if it would be better to simply try mounting/using V2 and if
> >> that fails try V1?
> >>
> >
> > That will be work but not good, because if cgroup mount fails, how do we
> > know it is a bug or not support?
>
> I think this is a valid point if you are writing a test for mounting
> cgroups. However if we are testing something else then trying to guess
> if cgroups should be available before using them, makes the test
> fragile. I suppose we could check *after* trying to use the cgroups so
> we can report some diagnostic info.
>

This sounds practicable, please feel free to work out the patch.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200925/aa8c31f1/attachment.htm>

[LTP] [PATCH] tst_cgroup: Don't try to use V2 if V1 controllers are mounted

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> ...
>> >> I wonder if it would be better to simply try mounting/using V2 and if
>> >> that fails try V1?
>> >>
>> >
>> > That will be work but not good, because if cgroup mount fails, how do we
>> > know it is a bug or not support?
>>
>> I think this is a valid point if you are writing a test for mounting
>> cgroups. However if we are testing something else then trying to guess
>> if cgroups should be available before using them, makes the test
>> fragile. I suppose we could check *after* trying to use the cgroups so
>> we can report some diagnostic info.
>>
>
> This sounds practicable, please feel free to work out the patch.

OK, I just sent a new patch. However there is no diagnostic info yet,
I'm not sure if actually it would be better to make a test specifically
checking if cgroups can be mounted and checking what is in filesystems
and mounts.

-- 
Thank you,
Richard.