[LTP] [PATCH] lib: rewrite cgroup_find_ctrl with using for_each_ctrl

[LTP] [PATCH] lib: rewrite cgroup_find_ctrl with using for_each_ctrl

[Li Wang]

It is safe to start from controllers[0] to traverse each of
the controller whatever V2 or V1, then we can make use of it
in the cgroup_find_ctrl() function.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 lib/tst_cgroup.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
index 2ef599d9e..10b65364b 100644
--- a/lib/tst_cgroup.c
+++ b/lib/tst_cgroup.c
@@ -214,7 +214,7 @@ static const char *cgroup_v2_ltp_mount = "unified";
 #define for_each_v1_root(r)			\
 	for ((r) = roots + 1; (r)->ver; (r)++)
 #define for_each_ctrl(ctrl)			\
-	for ((ctrl) = controllers + 1; (ctrl)->ctrl_name; (ctrl)++)
+	for ((ctrl) = controllers; (ctrl)->ctrl_name; (ctrl)++)
 
 /* In all cases except one, this only loops once.
  *
@@ -325,15 +325,14 @@ void tst_cgroup_print_config(void)
 __attribute__ ((nonnull, warn_unused_result))
 static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name)
 {
-	struct cgroup_ctrl *ctrl = controllers;
-
-	while (ctrl->ctrl_name && strcmp(ctrl_name, ctrl->ctrl_name))
-		ctrl++;
+	struct cgroup_ctrl *ctrl;
 
-	if (!ctrl->ctrl_name)
-		ctrl = NULL;
+	for_each_ctrl(ctrl) {
+		if (!strcmp(ctrl_name, ctrl->ctrl_name))
+			return ctrl;
+	}
 
-	return ctrl;
+	return NULL;
 }
 
 /* Determine if a mounted cgroup hierarchy is unique and record it if so.
-- 
2.31.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: rewrite cgroup_find_ctrl with using for_each_ctrl

[Richard Palethorpe]

Hello Li,

Li Wang <liwang@redhat.com> writes:

> It is safe to start from controllers[0] to traverse each of
> the controller whatever V2 or V1, then we can make use of it
> in the cgroup_find_ctrl() function.

Right, it seems we never set ctrl_root on "cgroup" nor is it added to
ctrl_field. So it will be skipped in other loops. This might not be what
people expect, but I'm not sure what to do about that.

>
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  lib/tst_cgroup.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c
> index 2ef599d9e..10b65364b 100644
> --- a/lib/tst_cgroup.c
> +++ b/lib/tst_cgroup.c
> @@ -214,7 +214,7 @@ static const char *cgroup_v2_ltp_mount = "unified";
>  #define for_each_v1_root(r)			\
>  	for ((r) = roots + 1; (r)->ver; (r)++)
>  #define for_each_ctrl(ctrl)			\
> -	for ((ctrl) = controllers + 1; (ctrl)->ctrl_name; (ctrl)++)
> +	for ((ctrl) = controllers; (ctrl)->ctrl_name; (ctrl)++)
>  
>  /* In all cases except one, this only loops once.
>   *
> @@ -325,15 +325,14 @@ void tst_cgroup_print_config(void)
>  __attribute__ ((nonnull, warn_unused_result))
>  static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name)
>  {
> -	struct cgroup_ctrl *ctrl = controllers;
> -
> -	while (ctrl->ctrl_name && strcmp(ctrl_name, ctrl->ctrl_name))
> -		ctrl++;
> +	struct cgroup_ctrl *ctrl;
>  
> -	if (!ctrl->ctrl_name)
> -		ctrl = NULL;
> +	for_each_ctrl(ctrl) {
> +		if (!strcmp(ctrl_name, ctrl->ctrl_name))
> +			return ctrl;
> +	}
>  
> -	return ctrl;
> +	return NULL;
>  }
>  
>  /* Determine if a mounted cgroup hierarchy is unique and record it if so.

Nice simplification!

Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: rewrite cgroup_find_ctrl with using for_each_ctrl

[Li Wang]

Richard Palethorpe <rpalethorpe@suse.de> wrote:

> > It is safe to start from controllers[0] to traverse each of
> > the controller whatever V2 or V1, then we can make use of it
> > in the cgroup_find_ctrl() function.
>
> Right, it seems we never set ctrl_root on "cgroup" nor is it added to
> ctrl_field. So it will be skipped in other loops. This might not be what
> people expect, but I'm not sure what to do about that.

Yes, but that's fine. It's because of the difference between V1 and V2.


> >
> >  /* Determine if a mounted cgroup hierarchy is unique and record it if so.
>
> Nice simplification!
>
> Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com>

Thanks for the review, I will merge this after the new release.

-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: rewrite cgroup_find_ctrl with using for_each_ctrl

[Petr Vorel]

Hi Li,

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

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] lib: rewrite cgroup_find_ctrl with using for_each_ctrl

[Li Wang]

Hi all,

Patch merged, thanks!

On Fri, Jan 21, 2022 at 1:23 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Li,
>
> LGTM.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> Kind regards,
> Petr
>


-- 
Regards,
Li Wang


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp