Re: [PATCH V4 1/2] cgroup/rstat: Helper functions for locking expose trylock

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

 



On 6/27/24 18:22, Waiman Long wrote:

On 6/27/24 17:18, Jesper Dangaard Brouer wrote:
Signed-off-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
---
  kernel/cgroup/rstat.c |   40 ++++++++++++++++++++++++++++++----------
  1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index fb8b49437573..2a42be3a9bb3 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -279,17 +279,30 @@ __bpf_hook_end();
   * value -1 is used when obtaining the main lock else this is the CPU
   * number processed last.
   */
-static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int cpu_in_loop) +static inline bool __cgroup_rstat_trylock(struct cgroup *cgrp, int cpu_in_loop)
+{
+    bool locked;
+
+    locked = spin_trylock_irq(&cgroup_rstat_lock);
+    if (!locked)
+        trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, !locked);
+
+    return locked;
+}
+
+static inline void __cgroup_rstat_lock(struct cgroup *cgrp, int cpu_in_loop,
+                       bool check_contention)
      __acquires(&cgroup_rstat_lock)
  {
-    bool contended;
+    bool locked = false;
  -    contended = !spin_trylock_irq(&cgroup_rstat_lock);
-    if (contended) {
-        trace_cgroup_rstat_lock_contended(cgrp, cpu_in_loop, contended);
+    if (check_contention)
+        locked = __cgroup_rstat_trylock(cgrp, cpu_in_loop);
+
+    if (!locked)
          spin_lock_irq(&cgroup_rstat_lock);
-    }
-    trace_cgroup_rstat_locked(cgrp, cpu_in_loop, contended);
+
+    trace_cgroup_rstat_locked(cgrp, cpu_in_loop, !locked);
  }
    static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop) @@ -328,7 +341,7 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
              __cgroup_rstat_unlock(cgrp, cpu);
              if (!cond_resched())
                  cpu_relax();
-            __cgroup_rstat_lock(cgrp, cpu);
+            __cgroup_rstat_lock(cgrp, cpu, true);
          }
      }
  }
@@ -348,9 +361,16 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
   */
  __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
  {
+    bool locked;
+
      might_sleep();
  -    __cgroup_rstat_lock(cgrp, -1);
+    locked = __cgroup_rstat_trylock(cgrp, -1);
+    if (!locked) {
+        /* Opportunity to ongoing flush detection */
+        __cgroup_rstat_lock(cgrp, -1, false);
+    }
+
      cgroup_rstat_flush_locked(cgrp);
      __cgroup_rstat_unlock(cgrp, -1);
  }
@@ -368,7 +388,7 @@ void cgroup_rstat_flush_hold(struct cgroup *cgrp)
      __acquires(&cgroup_rstat_lock)
  {
      might_sleep();
-    __cgroup_rstat_lock(cgrp, -1);
+    __cgroup_rstat_lock(cgrp, -1, true);
      cgroup_rstat_flush_locked(cgrp);
  }


Will it be cleaner to add a "bool *flushed" output parameter to __cgroup_rstat_lock() so that the caller can respond differently whether the flushed flag is set or not? In that way, you don't need to expose a separate trylock() API. Also your commit log is empty.

Looking at the use case in patch 2, I would suggest the following APIs.

- bool cgroup_rstat_lock(struct cgroup *cgrp)
- bool cgroup_rstat_lock_or_flushed(struct cgroup *cgrp)

Both will return a bool indicating a flush has already been done if true. The 2nd function will not take the lock if a flush is ongoing. Both will wait if a flush is ongoing.

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