Re: [PATCH 5/6] cgroup/cpuset: Free DL BW in case can_attach() fails

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/30/23 11:14, Dietmar Eggemann wrote:
On 29/03/2023 20:09, Waiman Long wrote:
On 3/29/23 12:39, Dietmar Eggemann wrote:
On 29/03/2023 16:31, Waiman Long wrote:
On 3/29/23 10:25, Waiman Long wrote:
On 3/29/23 08:55, Juri Lelli wrote:
From: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
[...]

@@ -2518,11 +2547,21 @@ static int cpuset_can_attach(struct
cgroup_taskset *tset)
    static void cpuset_cancel_attach(struct cgroup_taskset *tset)
    {
        struct cgroup_subsys_state *css;
+    struct cpuset *cs;
          cgroup_taskset_first(tset, &css);
+    cs = css_cs(css);
          mutex_lock(&cpuset_mutex);
-    css_cs(css)->attach_in_progress--;
+    cs->attach_in_progress--;
+
+    if (cs->nr_migrate_dl_tasks) {
+        int cpu = cpumask_any(cs->effective_cpus);
+
+        dl_bw_free(cpu, cs->sum_migrate_dl_bw);
+        reset_migrate_dl_data(cs);
+    }
+
Another nit that I have is that you may have to record also the cpu
where the DL bandwidth is allocated in cpuset_can_attach() and free the
bandwidth back into that cpu or there can be an underflow if another cpu
is chosen.
Many thanks for the review!

But isn't the DL BW control `struct dl_bw` per `struct root_domain`
which is per exclusive cpuset. So as long cpu is from
`cs->effective_cpus` shouldn't this be fine?
Sorry for my ignorance on how the deadline bandwidth operation work. I
check the bandwidth code and find that we are storing the bandwidth
information in the root domain, not on the cpu. That shouldn't be a
concern then.

However, I still have some question on how that works when dealing with
cpuset. First of all, not all the CPUs in a given root domains are in
the cpuset. So there may be enough bandwidth on the root domain, but it
doesn't mean there will be enough bandwidth in the set of CPUs in a
particular cpuset. Secondly, how do you deal with isolated CPUs that do
not have a corresponding root domain? It is now possible to create a
cpuset with isolated CPUs.
Sorry, I overlooked this email somehow.

IMHO, this is only done for exclusive cpusets:

   cpuset_can_attach()

     if (!cpumask_intersects(oldcs->effective_cpus, cs->effective_cpus))

So they should have their own root_domain congruent to their cpumask.

I am sorry that I missed that check.

Parallel attach is actually an existing problem in cpuset as there is a shared cpuset_attach_old_cs variable being used by cpuset between cpuset_can_attach() and cpuset_attach(). So any parallel attach can lead to corruption of this common data causing incorrect result. So this problem is not specific to this patch series. So please ignore this patch for now. It has to be addressed separately.

Cheers,
Longman




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux