diff mbox series

[RFC] tst_cgroup: Attempt to use CGroups V2 then V1 instead of guessing

Message ID 20200925121941.10475-1-rpalethorpe@suse.com
State Superseded
Headers show
Series [RFC] tst_cgroup: Attempt to use CGroups V2 then V1 instead of guessing | expand

Commit Message

Richard Palethorpe Sept. 25, 2020, 12:19 p.m. UTC
The best way to find out if we can do something is to try it, we don't
check if the system has enough RAM and PIDs available before calling
fork() in safe_fork(). Currently we try to guess what cgroups are
currently in use then try to use the same version. We guess by
grepping the mounts and filesystem files, these files need to be
parsed in a structured way and even then, it is not the job of all
tests which *use* cgroups to test that if cgroup(2) is present in
mounts or filesystem that it can then be used.

The cpuset group is only available on V1 and we can mount and use V1
even if V2 is active. Just because the system has V2 active does not
mean we cannot execute tests which require V1. This is one example
where trying to figure out ahead of time what can or can't be used
results in less testing.

Furthermore removing these checks results in a ~50% reduction in code
and this is without correct parsing and checking of mounts and
filesystems. We also have to consider that just because one V1
controller is mounted, this does not prevent all V2 controllers from
being used. So someone may mount V1 cpuset for legacy reasons, while
using V2 for other controllers. To account for this we would need to
check each controller individually.

Note that the above may be a valid thing to do in a test checking
specific cgroup behavior, but this library code is meant for use by
all tests which need cgroups for some reason.
---
 include/tst_cgroup.h |   2 -
 lib/tst_cgroup.c     | 118 ++++++++++++++-----------------------------
 2 files changed, 39 insertions(+), 81 deletions(-)

Comments

Li Wang Sept. 28, 2020, 3:36 a.m. UTC | #1
Hi Richard,

On Fri, Sep 25, 2020 at 8:20 PM Richard Palethorpe <rpalethorpe@suse.com>
wrote:

> The best way to find out if we can do something is to try it, we don't
> check if the system has enough RAM and PIDs available before calling
> fork() in safe_fork(). Currently we try to guess what cgroups are
> currently in use then try to use the same version. We guess by
> grepping the mounts and filesystem files, these files need to be
> parsed in a structured way and even then, it is not the job of all
> tests which *use* cgroups to test that if cgroup(2) is present in
> mounts or filesystem that it can then be used.
>
> The cpuset group is only available on V1 and we can mount and use V1
> even if V2 is active. Just because the system has V2 active does not
> mean we cannot execute tests which require V1. This is one example
> where trying to figure out ahead of time what can or can't be used
> results in less testing.
>

Good point.

My previous thought on the Cgroup test-lib design was only to respect one
version,
because the upstream developer hopes to use V2 replaces the V1 step by step.
So the key point of work mainly focused on the supported&mounted version,
which sounds like Cgroup itself is the leading actor, and to easily extend
Cgroup
itself testing according to different versions.

But I didn't realize that there will be a mixed using situation we have to
take care of.
At that moment, the Cgroup test-lib actor as an assistant in the general
test case.

Anyway, this patch looks practicable and fitted for the period of
transition of Cgroup.


>
> Furthermore removing these checks results in a ~50% reduction in code
> and this is without correct parsing and checking of mounts and
> filesystems. We also have to consider that just because one V1
> controller is mounted, this does not prevent all V2 controllers from
> being used. So someone may mount V1 cpuset for legacy reasons, while
> using V2 for other controllers. To account for this we would need to
> check each controller individually.
>
> Note that the above may be a valid thing to do in a test checking
> specific cgroup behavior, but this library code is meant for use by
> all tests which need cgroups for some reason.
> ---
>  include/tst_cgroup.h |   2 -
>  lib/tst_cgroup.c     | 118 ++++++++++++++-----------------------------
>  2 files changed, 39 insertions(+), 81 deletions(-)
>
> diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
> index 77780e0d6..62d381ce9 100644
> --- a/include/tst_cgroup.h
> +++ b/include/tst_cgroup.h
> @@ -21,8 +21,6 @@ enum tst_cgroup_ctrl {
>         /* add cgroup controller */
>  };
>
> -enum tst_cgroup_ver tst_cgroup_version(void);
> -
>  /* To mount/umount specified cgroup controller on 'cgroup_dir' path */
>  void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir);
>  void tst_cgroup_umount(const char *cgroup_dir);
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index ba413d874..887423bc6 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -21,47 +21,6 @@
>  static enum tst_cgroup_ver tst_cg_ver;
>  static int clone_children;
>
> -static int tst_cgroup_check(const char *cgroup)
> -{
> -       char line[PATH_MAX];
> -       FILE *file;
> -       int cg_check = 0;
> -
> -       file = SAFE_FOPEN("/proc/filesystems", "r");
> -       while (fgets(line, sizeof(line), file)) {
> -               if (strstr(line, cgroup) != NULL) {
> -                       cg_check = 1;
> -                       break;
> -               }
> -       }
> -       SAFE_FCLOSE(file);
> -
> -       return cg_check;
> -}
> -
> -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"))
> -                        cg_ver = TST_CGROUP_V1;
> -                else
> -                        cg_ver = TST_CGROUP_V2;
> -
> -                goto out;
> -        }
> -
> -        if (tst_cgroup_check("cgroup"))
> -                cg_ver = TST_CGROUP_V1;
> -
> -        if (!cg_ver)
> -                tst_brk(TCONF, "Cgroup is not configured");
> -
> -out:
> -        return cg_ver;
> -}
> -
>  static void tst_cgroup1_mount(const char *name, const char *option,
>                         const char *mnt_path, const char *new_path)
>  {
> @@ -100,26 +59,18 @@ out:
>         tst_res(TINFO, "Cgroup(%s) v1 mount at %s success", option,
> mnt_path);
>  }
>
> -static void tst_cgroup2_mount(const char *mnt_path, const char *new_path)
> +static int cgroup2_mount(const char *mnt_path, const char *new_path)
>

We'd like to make the series function name starts with tst_*.


>  {
> -       if (tst_is_mounted(mnt_path))
> -               goto out;
> +       if (!tst_is_mounted(mnt_path)) {
> +               SAFE_MKDIR(mnt_path, 0777);
>
> -       SAFE_MKDIR(mnt_path, 0777);
> -       if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1) {
> -               if (errno == ENODEV) {
> -                       if (rmdir(mnt_path) == -1)
> -                               tst_res(TWARN | TERRNO, "rmdir %s failed",
> mnt_path);
> -                       tst_brk(TCONF,
> -                                "Cgroup v2 is not configured in kernel");
> -               }
> -               tst_brk(TBROK | TERRNO, "mount %s", mnt_path);
> +               if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1)
>

Here we have to remove mnt_path if the mount fails since it also tries to
create
the same path in V1 then.

+                       if (rmdir(mnt_path) == -1)
+                               tst_res(TWARN | TERRNO, "rmdir %s",
mnt_path);


> +                       return -1;
>         }
>
> -out:
>         SAFE_MKDIR(new_path, 0777);
>
> -       tst_res(TINFO, "Cgroup v2 mount at %s success", mnt_path);
> +       return 0;
>  }
>
>  static void tst_cgroupN_umount(const char *mnt_path, const char *new_path)
> @@ -274,39 +225,48 @@ void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl,
> const char *cgroup_dir)
>         char *cgroup_new_dir;
>         char knob_path[PATH_MAX];
>
> -       tst_cg_ver = tst_cgroup_version();
> -
>         tst_cgroup_set_path(cgroup_dir);
>         cgroup_new_dir = tst_cgroup_get_path(cgroup_dir);
>
> -       if (tst_cg_ver & TST_CGROUP_V1) {
> -               switch(ctrl) {
> -               case TST_CGROUP_MEMCG:
> -                       tst_cgroup1_mount("memcg", "memory", cgroup_dir,
> cgroup_new_dir);
> -               break;
> -               case TST_CGROUP_CPUSET:
> -                       tst_cgroup1_mount("cpusetcg", "cpuset",
> cgroup_dir, cgroup_new_dir);
> -               break;
> -               default:
> -                       tst_brk(TBROK, "Invalid cgroup controller: %d",
> ctrl);
> -               }
> +       if (ctrl == TST_CGROUP_CPUSET) {
> +               tst_res(TINFO, "CGroup V2 lacks cpuset subsystem, trying
> V1");
> +               goto cgroup_v1;
>         }
>
> -       if (tst_cg_ver & TST_CGROUP_V2) {
> -               tst_cgroup2_mount(cgroup_dir, cgroup_new_dir);
> +       if (cgroup2_mount(cgroup_dir, cgroup_new_dir)) {
> +               tst_res(TINFO | TERRNO, "Mounting CGroup V2 failed, trying
> V1");
>

Can we add the diagnostic check when mounting Cgroup V2 failed?
(i.e reserve the tst_cgroup_check() function and used in
tst_cgroup2_mount())

    if (tst_cgroup_check("cgroup2"))
       warning for mounting failed, else ignore this failure

+               goto cgroup_v1;
> +       }
> +
> +       tst_res(TINFO, "Mounted CGroup V2");
> +
> +       switch(ctrl) {
> +       case TST_CGROUP_MEMCG:
> +               sprintf(knob_path, "%s/cgroup.subtree_control",
> cgroup_dir);
> +               if (FILE_PRINTF(knob_path, "%s", "+memory")) {
> +                       tst_res(TINFO,
> +                               "Can't add V2 memory controller, this
> might be because it is mounted as V1");
>

Seems we have to umount Cgroup_V2 and do the cleanup for the shift to
Cgroup_V1.

if (rmdir(cgroup_new_dir) == -1)
    tst_res(TWARN | TERRNO, "rmdir %s", cgroup_new_dir);
if (umount(cgroup_dir) == -1)
    tst_res(TWARN | TERRNO, "umount %s", cgroup_dir);
if (rmdir(cgroup_dir) == -1)
    tst_res(TWARN | TERRNO, "rmdir %s", cgroup_dir);


> +                       break;
> +               }
> +               tst_cg_ver = TST_CGROUP_V2;
> +               return;
> +       default:
> +               tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
> +       }
>
> -               switch(ctrl) {
> -               case TST_CGROUP_MEMCG:
> -                       sprintf(knob_path, "%s/cgroup.subtree_control",
> cgroup_dir);
> -                       SAFE_FILE_PRINTF(knob_path, "%s", "+memory");
> +cgroup_v1:
> +       switch(ctrl) {
> +       case TST_CGROUP_MEMCG:
> +               tst_cgroup1_mount("memcg", "memory", cgroup_dir,
> cgroup_new_dir);
>                 break;
> -               case TST_CGROUP_CPUSET:
> -                       tst_brk(TCONF, "Cgroup v2 hasn't achieve cpuset
> subsystem");
> +       case TST_CGROUP_CPUSET:
> +               tst_cgroup1_mount("cpusetcg", "cpuset", cgroup_dir,
> cgroup_new_dir);
>                 break;
> -               default:
> -                       tst_brk(TBROK, "Invalid cgroup controller: %d",
> ctrl);
> -               }
> +       default:
> +               tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
>         }
> +
> +       tst_cg_ver = TST_CGROUP_V1;
>  }
>
>  void tst_cgroup_umount(const char *cgroup_dir)
> --
> 2.28.0
>
>
Richard Palethorpe Sept. 28, 2020, 9 a.m. UTC | #2
Hello Li,

Li Wang <liwang@redhat.com> writes:

>>
>> -static void tst_cgroup2_mount(const char *mnt_path, const char *new_path)
>> +static int cgroup2_mount(const char *mnt_path, const char *new_path)
>>
>
> We'd like to make the series function name starts with tst_*.
>

The idea is this will be an internal/static function and
tst_cgroup2_mount will be a public function if it is needed. I guess
that eventually there will be features only available in cgroup2, in
which case the test author will want to call tst_cgroup2_mount not
tst_cgroup_mount and they will just want it to fail with tst_brk if
cgroup2 can't be mounted.

Infact, if the user wants cpuset or some other V1 only controller, then
they should probably call tst_cgroup1_mount. AFAICT some of these
controllers will not be moved to V2. OTOH a functionally similar feature
may be available in V2, but with a different interface. There is a
difference between requiring a specific controller to test it and
needing some functionality without caring how it is provided.

So I suggest providing an API for mounting specific cgroup versions and
controllers and an API to mount specific controllers of either version
(i.e. tst_cgroup_mount). Then we can create helper functions to provide
functionality without caring how it is achieved, if we need to do that.

Other comments sound good! I will try creating another patch with
diagnostics.
Li Wang Sept. 29, 2020, 11:29 a.m. UTC | #3
On Mon, Sep 28, 2020 at 5:00 PM Richard Palethorpe <rpalethorpe@suse.de>
wrote:

> Hello Li,
>
> Li Wang <liwang@redhat.com> writes:
>
> >>
> >> -static void tst_cgroup2_mount(const char *mnt_path, const char
> *new_path)
> >> +static int cgroup2_mount(const char *mnt_path, const char *new_path)
> >>
> >
> > We'd like to make the series function name starts with tst_*.
> >
>
> The idea is this will be an internal/static function and
> tst_cgroup2_mount will be a public function if it is needed. I guess
> that eventually there will be features only available in cgroup2, in
> which case the test author will want to call tst_cgroup2_mount not
> tst_cgroup_mount and they will just want it to fail with tst_brk if
> cgroup2 can't be mounted.
>

Sounds good.


>
> Infact, if the user wants cpuset or some other V1 only controller, then
> they should probably call tst_cgroup1_mount. AFAICT some of these
> controllers will not be moved to V2. OTOH a functionally similar feature
> may be available in V2, but with a different interface. There is a
> difference between requiring a specific controller to test it and
> needing some functionality without caring how it is provided.
>
> So I suggest providing an API for mounting specific cgroup versions and
> controllers and an API to mount specific controllers of either version
> (i.e. tst_cgroup_mount). Then we can create helper functions to provide
> functionality without caring how it is achieved, if we need to do that.
>

This is a really good suggestion.


> Other comments sound good! I will try creating another patch with
> diagnostics.
>

Thanks!
Richard Palethorpe Nov. 3, 2020, 11:39 a.m. UTC | #4
Hello,

Li Wang <liwang@redhat.com> writes:

> On Mon, Sep 28, 2020 at 5:00 PM Richard Palethorpe <rpalethorpe@suse.de>
> wrote:
>
>> Hello Li,
>>
>> Li Wang <liwang@redhat.com> writes:
>>
>> >>
>> >> -static void tst_cgroup2_mount(const char *mnt_path, const char
>> *new_path)
>> >> +static int cgroup2_mount(const char *mnt_path, const char *new_path)
>> >>
>> >
>> > We'd like to make the series function name starts with tst_*.
>> >
>>
>> The idea is this will be an internal/static function and
>> tst_cgroup2_mount will be a public function if it is needed. I guess
>> that eventually there will be features only available in cgroup2, in
>> which case the test author will want to call tst_cgroup2_mount not
>> tst_cgroup_mount and they will just want it to fail with tst_brk if
>> cgroup2 can't be mounted.
>>
>
> Sounds good.
>
>
>>
>> Infact, if the user wants cpuset or some other V1 only controller, then
>> they should probably call tst_cgroup1_mount. AFAICT some of these
>> controllers will not be moved to V2. OTOH a functionally similar feature
>> may be available in V2, but with a different interface. There is a
>> difference between requiring a specific controller to test it and
>> needing some functionality without caring how it is provided.
>>
>> So I suggest providing an API for mounting specific cgroup versions and
>> controllers and an API to mount specific controllers of either version
>> (i.e. tst_cgroup_mount). Then we can create helper functions to provide
>> functionality without caring how it is achieved, if we need to do that.
>>
>
> This is a really good suggestion.
>
>
>> Other comments sound good! I will try creating another patch with
>> diagnostics.
>>
>
> Thanks!

Well, I have learnt some more about CGroups and reviewed some of our
tests which use them and am now considering the following. This is so
complicated that the below will probably turn out to be wrong as I try
to implement it.

1) Scan the system for mounted Cgroup controllers and create a data
   structure describing what controllers are mounted and where.

There is only one cgroup root, it is possible to mount it multiple
times, but it simplifies matters if we try to reuse whatever is already
mounted. Especially in the case of V1 where remounting with different
controller combinations will fail. Possibly there is some advantage to
remounting, but I can't see what because changes to one mount are likely
to be reflected in others, plus remounting is likely to fail if you
don't use the same mount options.

2) The user requests some controller values to be set in a unified
   hierarchy. The LTP library then tries to translate this to whatever
   CGroup setup the system is actually using.

If no cgroups are mounted, then we try to mount a simple V2 setup
falling back to the standard V1 setup with the (required) controllers in
separate hierarchies. For some tests this will result in a hybrid setup
because they first request a V2 compatible controller then a V1 only
controller (or the inverse if there are any V2 only controllers). At
least SUSE and Ubuntu are using hybrid setups so this is a valid thing
to test (unfortunately).

If we find mounted controllers then try to create a new LTP hierarchy in
the root of each controller (on V2 all the controllers are mounted to
the same place, but V1 allows all kinds of stuff).

3) The user requests some process is moved to a node of the unified
   hierarchy for one or more controllers.

Do the same setup as 2) if necessary. For V2 setups or V1 setups where
all the controllers are mounted to the same place the controller
argument is ignored. It is only relevant for V1 setups with separate
hierarchies for some of the controllers. Of course a version of the
interface can be provided without the controller argument.

4) The user requests processes are removed from our hierarchy (back to
root) and/or we destroy our hierarchy.

If we mounted any controllers unmount them, otherwise we just drain our
hierarchy and remove it. Some tests currently just move their process
into a cgroup (on each iteration instead of in setup) and never out of
it. I don't think this makes sense, so that is another thing to
investigate.

I think the above will work for tests which are simply trying to use
CGroup features even on systems which have an unusual V1 setup (but not
all V1 setups). For tests which are trying to test CGroups themselves,
then we will have to look at each test case and figure out if any code
can be shared.

For some tests (e.g. madvise06) we can provide a declarative interface
like:

tst_test.cgroup = {
                {"memory", "max", 256MB},
                {"memory", "swappiness", 60},
                { NULL },
};

Then the library will create a cgroup, set the memory controllers limit
and put the test process in the cgroup. However a more thorough review
of our cgroup usage is needed before deciding on a declarative
interface.

Note that so far I have not seen a need to create complex hierarchies
for our tests or use threaded V2 controllers, but we will need to do
this to test cgroups themselves. However most tests just need some basic
cgroup features and we can use test variants to enable random cgroup
features on any test or implement cgroups in the test runner.
Li Wang Nov. 4, 2020, 9:12 a.m. UTC | #5
Hi Richard,

Richard Palethorpe <rpalethorpe@suse.de> wrote:

...
>
> Well, I have learnt some more about CGroups and reviewed some of our
> tests which use them and am now considering the following. This is so
> complicated that the below will probably turn out to be wrong as I try
> to implement it.
>

Glad to see your improvement plan.

It's okay, we can try it in practice. Sometimes the perfect solution
will be born in the code iteration :).


>
> 1) Scan the system for mounted Cgroup controllers and create a data
>    structure describing what controllers are mounted and where.
>
> There is only one cgroup root, it is possible to mount it multiple
> times, but it simplifies matters if we try to reuse whatever is already
> mounted. Especially in the case of V1 where remounting with different
> controller combinations will fail. Possibly there is some advantage to
> remounting, but I can't see what because changes to one mount are likely
> to be reflected in others, plus remounting is likely to fail if you
> don't use the same mount options.
>

I doubt that will be simplified to reuse the existing CGroup, It
sounds a bit idealization. Since there might be different paths
mounted on their Linux distributions or self-customized system,
we have to cover/scan all unsure situations if go this way, isn't it?


>
> 2) The user requests some controller values to be set in a unified
>    hierarchy. The LTP library then tries to translate this to whatever
>    CGroup setup the system is actually using.
>
> If no cgroups are mounted, then we try to mount a simple V2 setup
> falling back to the standard V1 setup with the (required) controllers in
> separate hierarchies. For some tests this will result in a hybrid setup
> because they first request a V2 compatible controller then a V1 only
> controller (or the inverse if there are any V2 only controllers). At
> least SUSE and Ubuntu are using hybrid setups so this is a valid thing
> to test (unfortunately).
>
+1
Yes, I totally agree to handle this kind of issue, which currently
LTP-CGroup-Lib has not done it. This is the new progress for us.


>
> If we find mounted controllers then try to create a new LTP hierarchy in
> the root of each controller (on V2 all the controllers are mounted to
> the same place, but V1 allows all kinds of stuff).
>

That sounds too complicated, there would be a situation that partly
used V2 in the root CGroup and also to use the V1-only controller
in a newly mounted V1 hierarchy.

Another concern is about the cleanup work, I'm not sure this will
be more simple or needs unify way to remove all created dir in the
hierarchy for both V2 and V1 mixed.

And, it is probably much difficult for debugging if hitting some problems.


>
> 3) The user requests some process is moved to a node of the unified
>    hierarchy for one or more controllers.
>
> Do the same setup as 2) if necessary. For V2 setups or V1 setups where
> all the controllers are mounted to the same place the controller
> argument is ignored. It is only relevant for V1 setups with separate
> hierarchies for some of the controllers. Of course a version of the
> interface can be provided without the controller argument.
>
> 4) The user requests processes are removed from our hierarchy (back to
> root) and/or we destroy our hierarchy.
>
> If we mounted any controllers unmount them, otherwise we just drain our
> hierarchy and remove it. Some tests currently just move their process
> into a cgroup (on each iteration instead of in setup) and never out of
>

Right, but at least, all of the processes will be kick-out from CGroup
hierarchy before unmounting in the cleanup phase.


> it. I don't think this makes sense, so that is another thing to
> investigate.
>
> I think the above will work for tests which are simply trying to use
> CGroup features even on systems which have an unusual V1 setup (but not
> all V1 setups). For tests which are trying to test CGroups themselves,
> then we will have to look at each test case and figure out if any code
> can be shared.
>
> For some tests (e.g. madvise06) we can provide a declarative interface
> like:
>
> tst_test.cgroup = {
>                 {"memory", "max", 256MB},
>                 {"memory", "swappiness", 60},
>                 { NULL },
> };
>
> Then the library will create a cgroup, set the memory controllers limit
> and put the test process in the cgroup. However a more thorough review
> of our cgroup usage is needed before deciding on a declarative
> interface.
>
> Note that so far I have not seen a need to create complex hierarchies
> for our tests or use threaded V2 controllers, but we will need to do
> this to test cgroups themselves. However most tests just need some basic
>

+1
That's also a long term goal for LTP and I had left open interfaces for the
purpose of extending and further development work.


> cgroup features and we can use test variants to enable random cgroup
> features on any test or implement cgroups in the test runner.
>

Good point.

Btw, I think you can try the thoughts first, as you mentioned it is
too complicated so we can't finish everything just in one time.
But the better solution will be fine out in practice I believe.

Here is the previous discussion about cover different CGroup scenarios
supporting(the end part of email), FYI:
http://lists.linux.it/pipermail/ltp/2020-June/017442.html
Richard Palethorpe Nov. 4, 2020, 11:44 a.m. UTC | #6
Hello Li,

Li Wang <liwang@redhat.com> writes:

> Hi Richard,
>
> Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> ...
>>
>> Well, I have learnt some more about CGroups and reviewed some of our
>> tests which use them and am now considering the following. This is so
>> complicated that the below will probably turn out to be wrong as I try
>> to implement it.
>>
>
> Glad to see your improvement plan.
>
> It's okay, we can try it in practice. Sometimes the perfect solution
> will be born in the code iteration :).

+1

>
>
>>
>> 1) Scan the system for mounted Cgroup controllers and create a data
>>    structure describing what controllers are mounted and where.
>>
>> There is only one cgroup root, it is possible to mount it multiple
>> times, but it simplifies matters if we try to reuse whatever is already
>> mounted. Especially in the case of V1 where remounting with different
>> controller combinations will fail. Possibly there is some advantage to
>> remounting, but I can't see what because changes to one mount are likely
>> to be reflected in others, plus remounting is likely to fail if you
>> don't use the same mount options.
>>
>
> I doubt that will be simplified to reuse the existing CGroup, It
> sounds a bit idealization. Since there might be different paths
> mounted on their Linux distributions or self-customized system,
> we have to cover/scan all unsure situations if go this way, isn't it?

To clarify; I think the only option is to reuse existing CGroups if the
system has already mounted them. We can rebind/mount them to a new path,
but it is the same structure underneath.

If you try to remount the CGroup with different options from the
original mount then it will fail. This is especially true for V1
controllers which may be mounted together or separately. So we must
parse the mounts file and look at the options anyway.

The alternative would be to not run the test if cgroups are already
mounted or refuse to run if they are mounted in a way we can't handle.

Note that the kernel self-tests scan the system for an existing V2
hierarchy and don't try to mount anything. Because they only test V2
this is pretty simple.

>
>
>>
>> 2) The user requests some controller values to be set in a unified
>>    hierarchy. The LTP library then tries to translate this to whatever
>>    CGroup setup the system is actually using.
>>
>> If no cgroups are mounted, then we try to mount a simple V2 setup
>> falling back to the standard V1 setup with the (required) controllers in
>> separate hierarchies. For some tests this will result in a hybrid setup
>> because they first request a V2 compatible controller then a V1 only
>> controller (or the inverse if there are any V2 only controllers). At
>> least SUSE and Ubuntu are using hybrid setups so this is a valid thing
>> to test (unfortunately).
>>
> +1
> Yes, I totally agree to handle this kind of issue, which currently
> LTP-CGroup-Lib has not done it. This is the new progress for us.
>
>
>>
>> If we find mounted controllers then try to create a new LTP hierarchy in
>> the root of each controller (on V2 all the controllers are mounted to
>> the same place, but V1 allows all kinds of stuff).
>>
>
> That sounds too complicated, there would be a situation that partly
> used V2 in the root CGroup and also to use the V1-only controller
> in a newly mounted V1 hierarchy.
>
> Another concern is about the cleanup work, I'm not sure this will
> be more simple or needs unify way to remove all created dir in the
> hierarchy for both V2 and V1 mixed.
>
> And, it is probably much difficult for debugging if hitting some
> problems.

This is why CGroups V1 is deprecated and V2 only has a unified hierarchy
:-p.

Again if the system has already mounted some unusual V1 setup then you
can not change that. We either have to scan it and create our hierarchy
multiple times for each set of mounted controllers or refuse to run.

For example try something like the following (with no cgroups mounted):

$ mount -t cgroup -o cpu,cpuacct nodev /tmp/a
$ mount -t cgroup -o cpu         nodev /tmp/b

The second mount should fail, however the following should work:

$ mount -t cgroup -o cpuacct nodev /tmp/a
$ mount -t cgroup -o cpu     nodev /tmp/b

This is because there is only one instance of each controller and the
hierarchy it is attached to. The controllers and cgroup hierarchies are
created during the first mount, but are then reused by any further
mounts.
diff mbox series

Patch

diff --git a/include/tst_cgroup.h b/include/tst_cgroup.h
index 77780e0d6..62d381ce9 100644
--- a/include/tst_cgroup.h
+++ b/include/tst_cgroup.h
@@ -21,8 +21,6 @@  enum tst_cgroup_ctrl {
 	/* add cgroup controller */
 };
 
-enum tst_cgroup_ver tst_cgroup_version(void);
-
 /* To mount/umount specified cgroup controller on 'cgroup_dir' path */
 void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir);
 void tst_cgroup_umount(const char *cgroup_dir);
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index ba413d874..887423bc6 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -21,47 +21,6 @@ 
 static enum tst_cgroup_ver tst_cg_ver;
 static int clone_children;
 
-static int tst_cgroup_check(const char *cgroup)
-{
-	char line[PATH_MAX];
-	FILE *file;
-	int cg_check = 0;
-
-	file = SAFE_FOPEN("/proc/filesystems", "r");
-	while (fgets(line, sizeof(line), file)) {
-		if (strstr(line, cgroup) != NULL) {
-			cg_check = 1;
-			break;
-		}
-	}
-	SAFE_FCLOSE(file);
-
-	return cg_check;
-}
-
-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"))
-                        cg_ver = TST_CGROUP_V1;
-                else
-                        cg_ver = TST_CGROUP_V2;
-
-                goto out;
-        }
-
-        if (tst_cgroup_check("cgroup"))
-                cg_ver = TST_CGROUP_V1;
-
-        if (!cg_ver)
-                tst_brk(TCONF, "Cgroup is not configured");
-
-out:
-        return cg_ver;
-}
-
 static void tst_cgroup1_mount(const char *name, const char *option,
 			const char *mnt_path, const char *new_path)
 {
@@ -100,26 +59,18 @@  out:
 	tst_res(TINFO, "Cgroup(%s) v1 mount at %s success", option, mnt_path);
 }
 
-static void tst_cgroup2_mount(const char *mnt_path, const char *new_path)
+static int cgroup2_mount(const char *mnt_path, const char *new_path)
 {
-	if (tst_is_mounted(mnt_path))
-		goto out;
+	if (!tst_is_mounted(mnt_path)) {
+		SAFE_MKDIR(mnt_path, 0777);
 
-	SAFE_MKDIR(mnt_path, 0777);
-	if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1) {
-		if (errno == ENODEV) {
-			if (rmdir(mnt_path) == -1)
-				tst_res(TWARN | TERRNO, "rmdir %s failed", mnt_path);
-			tst_brk(TCONF,
-				 "Cgroup v2 is not configured in kernel");
-		}
-		tst_brk(TBROK | TERRNO, "mount %s", mnt_path);
+		if (mount("cgroup2", mnt_path, "cgroup2", 0, NULL) == -1)
+			return -1;
 	}
 
-out:
 	SAFE_MKDIR(new_path, 0777);
 
-	tst_res(TINFO, "Cgroup v2 mount at %s success", mnt_path);
+	return 0;
 }
 
 static void tst_cgroupN_umount(const char *mnt_path, const char *new_path)
@@ -274,39 +225,48 @@  void tst_cgroup_mount(enum tst_cgroup_ctrl ctrl, const char *cgroup_dir)
 	char *cgroup_new_dir;
 	char knob_path[PATH_MAX];
 
-	tst_cg_ver = tst_cgroup_version();
-
 	tst_cgroup_set_path(cgroup_dir);
 	cgroup_new_dir = tst_cgroup_get_path(cgroup_dir);
 
-	if (tst_cg_ver & TST_CGROUP_V1) {
-		switch(ctrl) {
-		case TST_CGROUP_MEMCG:
-			tst_cgroup1_mount("memcg", "memory", cgroup_dir, cgroup_new_dir);
-		break;
-		case TST_CGROUP_CPUSET:
-			tst_cgroup1_mount("cpusetcg", "cpuset", cgroup_dir, cgroup_new_dir);
-		break;
-		default:
-			tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
-		}
+	if (ctrl == TST_CGROUP_CPUSET) {
+		tst_res(TINFO, "CGroup V2 lacks cpuset subsystem, trying V1");
+		goto cgroup_v1;
 	}
 
-	if (tst_cg_ver & TST_CGROUP_V2) {
-		tst_cgroup2_mount(cgroup_dir, cgroup_new_dir);
+	if (cgroup2_mount(cgroup_dir, cgroup_new_dir)) {
+		tst_res(TINFO | TERRNO, "Mounting CGroup V2 failed, trying V1");
+		goto cgroup_v1;
+	}
+
+	tst_res(TINFO, "Mounted CGroup V2");
+
+	switch(ctrl) {
+	case TST_CGROUP_MEMCG:
+		sprintf(knob_path, "%s/cgroup.subtree_control", cgroup_dir);
+		if (FILE_PRINTF(knob_path, "%s", "+memory")) {
+			tst_res(TINFO,
+				"Can't add V2 memory controller, this might be because it is mounted as V1");
+			break;
+		}
+		tst_cg_ver = TST_CGROUP_V2;
+		return;
+	default:
+		tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
+	}
 
-		switch(ctrl) {
-		case TST_CGROUP_MEMCG:
-			sprintf(knob_path, "%s/cgroup.subtree_control", cgroup_dir);
-			SAFE_FILE_PRINTF(knob_path, "%s", "+memory");
+cgroup_v1:
+	switch(ctrl) {
+	case TST_CGROUP_MEMCG:
+		tst_cgroup1_mount("memcg", "memory", cgroup_dir, cgroup_new_dir);
 		break;
-		case TST_CGROUP_CPUSET:
-			tst_brk(TCONF, "Cgroup v2 hasn't achieve cpuset subsystem");
+	case TST_CGROUP_CPUSET:
+		tst_cgroup1_mount("cpusetcg", "cpuset", cgroup_dir, cgroup_new_dir);
 		break;
-		default:
-			tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
-		}
+	default:
+		tst_brk(TBROK, "Invalid cgroup controller: %d", ctrl);
 	}
+
+	tst_cg_ver = TST_CGROUP_V1;
 }
 
 void tst_cgroup_umount(const char *cgroup_dir)