On Fri, May 14, 2021 at 11:20 AM Suren Baghdasaryan [off-list ref] wrote:

On Fri, May 14, 2021 at 10:52 AM Peter Zijlstra [off-list ref] wrote:

quoted

On Fri, May 14, 2021 at 08:54:47AM -0700, Suren Baghdasaryan wrote:

quoted

Correct, for this function CONFIG_CGROUPS=n and
cgroup_disable=pressure are treated the same. True, from the code it's
not very obvious. Do you have some refactoring in mind that would make
it more explicit?

Does this make sense?

--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c

@@ -744,24 +744,26 @@ static void psi_group_change(struct psi_

 static struct psi_group *iterate_groups(struct task_struct *task, void **iter)
 {
+       if (cgroup_psi_enabled()) {
 #ifdef CONFIG_CGROUPS
-       struct cgroup *cgroup = NULL;
+               struct cgroup *cgroup = NULL;

-       if (!*iter)
-               cgroup = task->cgroups->dfl_cgrp;
-       else if (*iter == &psi_system)
-               return NULL;
-       else
-               cgroup = cgroup_parent(*iter);
+               if (!*iter)
+                       cgroup = task->cgroups->dfl_cgrp;
+               else if (*iter == &psi_system)
+                       return NULL;
+               else
+                       cgroup = cgroup_parent(*iter);

-       if (cgroup && cgroup_parent(cgroup)) {
-               *iter = cgroup;
-               return cgroup_psi(cgroup);
-       }
-#else
-       if (*iter)
-               return NULL;
+               if (cgroup && cgroup_parent(cgroup)) {
+                       *iter = cgroup;
+                       return cgroup_psi(cgroup);
+               }
 #endif
+       } else {
+               if (*iter)
+                       return NULL;
+       }
        *iter = &psi_system;
        return &psi_system;
 }

Hmm. Looks like the case when cgroup_psi_enabled()==true and
CONFIG_CGROUPS=n would miss the "if (*iter) return NULL;" condition.
Effectively with CONFIG_CGROUPS=n this becomes:

       if (cgroup_psi_enabled()) {           <== assume this is true
#ifdef CONFIG_CGROUPS                <== compiled out
#endif
       } else {
               if (*iter)                                  <== this
statement will never execute
                       return NULL;
       }
       *iter = &psi_system;
        return &psi_system;

Ah, sorry. I forgot that CONFIG_CGROUPS=n would force
cgroup_psi_enabled()==false (the way function is defined in cgroup.h),
so (CONFIG_CGROUPS=n && cgroup_psi_enabled()==true) is an invalid
configuration. I think adding a comment to your suggestion would make
it more clear.
So your suggestion seems to work. I'll test it and include it in the
next revision. Thanks!

quoted

--
To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.