[PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Greg Thelen]

From: Eric Dumazet <edumazet@google.com>

cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
iterating all possible cpus. It only drops the lock if there is
scheduler or spin lock contention. If neither, then interrupts can be
disabled for a long time. On large machines this can disable interrupts
for a long enough time to drop network packets. On 400+ CPU machines
I've seen interrupt disabled for over 40 msec.

Prevent rstat from disabling interrupts while processing all possible
cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
approach was previously discussed in
https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
though this was in the context of an non-irq rstat spin lock.

Benchmark this change with:
1) a single stat_reader process with 400 threads, each reading a test
   memcg's memory.stat repeatedly for 10 seconds.
2) 400 memory hog processes running in the test memcg and repeatedly
   charging memory until oom killed. Then they repeat charging and oom
   killing.

v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
interrupts are disabled by rstat for 45341 usec:
  #  => started at: _raw_spin_lock_irq
  #  => ended at:   cgroup_rstat_flush
  #
  #
  #                    _------=> CPU#
  #                   / _-----=> irqs-off/BH-disabled
  #                  | / _----=> need-resched
  #                  || / _---=> hardirq/softirq
  #                  ||| / _--=> preempt-depth
  #                  |||| / _-=> migrate-disable
  #                  ||||| /     delay
  #  cmd     pid     |||||| time  |   caller
  #     \   /        ||||||  \    |    /
  stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
  stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
  stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
  stat_rea-96532    52d.... 45343us : <stack trace>
   => memcg1_stat_format
   => memory_stat_format
   => memory_stat_show
   => seq_read_iter
   => vfs_read
   => ksys_read
   => do_syscall_64
   => entry_SYSCALL_64_after_hwframe

With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
longest holder. The longest irqs-off holder has irqs disabled for
4142 usec, a huge reduction from previous 45341 usec rstat finding.

Running stat_reader memory.stat reader for 10 seconds:
- without memory hogs: 9.84M accesses => 12.7M accesses
-    with memory hogs: 9.46M accesses => 11.1M accesses
The throughput of memory.stat access improves.

The mode of memory.stat access latency after grouping by of 2 buckets:
- without memory hogs: 64 usec => 16 usec
-    with memory hogs: 64 usec =>  8 usec
The memory.stat latency improves.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Greg Thelen <gthelen@google.com>
Tested-by: Greg Thelen <gthelen@google.com>
---
 kernel/cgroup/rstat.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index aac91466279f..976c24b3671a 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -323,13 +323,11 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
 			rcu_read_unlock();
 		}
 
-		/* play nice and yield if necessary */
-		if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
-			__cgroup_rstat_unlock(cgrp, cpu);
-			if (!cond_resched())
-				cpu_relax();
-			__cgroup_rstat_lock(cgrp, cpu);
-		}
+		/* play nice and avoid disabling interrupts for a long time */
+		__cgroup_rstat_unlock(cgrp, cpu);
+		if (!cond_resched())
+			cpu_relax();
+		__cgroup_rstat_lock(cgrp, cpu);
 	}
 }
 
-- 
2.49.0.rc1.451.g8f38331e32-goog

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Greg Thelen]

(fix mistyped CC address to Eric)

On Wed, Mar 19, 2025 at 12:13 AM Greg Thelen <gthelen@google.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
>
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.
>
> Prevent rstat from disabling interrupts while processing all possible
> cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
> approach was previously discussed in
> https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> though this was in the context of an non-irq rstat spin lock.
>
> Benchmark this change with:
> 1) a single stat_reader process with 400 threads, each reading a test
>    memcg's memory.stat repeatedly for 10 seconds.
> 2) 400 memory hog processes running in the test memcg and repeatedly
>    charging memory until oom killed. Then they repeat charging and oom
>    killing.
>
> v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> interrupts are disabled by rstat for 45341 usec:
>   #  => started at: _raw_spin_lock_irq
>   #  => ended at:   cgroup_rstat_flush
>   #
>   #
>   #                    _------=> CPU#
>   #                   / _-----=> irqs-off/BH-disabled
>   #                  | / _----=> need-resched
>   #                  || / _---=> hardirq/softirq
>   #                  ||| / _--=> preempt-depth
>   #                  |||| / _-=> migrate-disable
>   #                  ||||| /     delay
>   #  cmd     pid     |||||| time  |   caller
>   #     \   /        ||||||  \    |    /
>   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
>   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
>   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
>   stat_rea-96532    52d.... 45343us : <stack trace>
>    => memcg1_stat_format
>    => memory_stat_format
>    => memory_stat_show
>    => seq_read_iter
>    => vfs_read
>    => ksys_read
>    => do_syscall_64
>    => entry_SYSCALL_64_after_hwframe
>
> With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> longest holder. The longest irqs-off holder has irqs disabled for
> 4142 usec, a huge reduction from previous 45341 usec rstat finding.
>
> Running stat_reader memory.stat reader for 10 seconds:
> - without memory hogs: 9.84M accesses => 12.7M accesses
> -    with memory hogs: 9.46M accesses => 11.1M accesses
> The throughput of memory.stat access improves.
>
> The mode of memory.stat access latency after grouping by of 2 buckets:
> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>
> ---
>  kernel/cgroup/rstat.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index aac91466279f..976c24b3671a 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c
> @@ -323,13 +323,11 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
>                         rcu_read_unlock();
>                 }
>
> -               /* play nice and yield if necessary */
> -               if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
> -                       __cgroup_rstat_unlock(cgrp, cpu);
> -                       if (!cond_resched())
> -                               cpu_relax();
> -                       __cgroup_rstat_lock(cgrp, cpu);
> -               }
> +               /* play nice and avoid disabling interrupts for a long time */
> +               __cgroup_rstat_unlock(cgrp, cpu);
> +               if (!cond_resched())
> +                       cpu_relax();
> +               __cgroup_rstat_lock(cgrp, cpu);
>         }
>  }
>
> --
> 2.49.0.rc1.451.g8f38331e32-goog
>

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

Hello.

On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen <gthelen@google.com> wrote:
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.

This is peanuts, watchdog_thresh defaults to 10000 msec.
(Tongue-in-cheek, to put that threshold into relation but I see the
problem.)


> The mode of memory.stat access latency after grouping by of 2 buckets:
                                                        power

> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>
> ---
>  kernel/cgroup/rstat.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

FTR, the lock may end up split per-subsys [1] but this would still make
sense for memcg's one. (I wonder if Tejun would consider it small enough
then to avoid interrupt disabling. Then this could be converted to more
widely used cond_resched_lock().)

[1] https://lore.kernel.org/r/20250227215543.49928-4-inwardvessel@gmail.com/

But all in all, thanks for this and

Acked-by: Michal Koutný <mkoutny@suse.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Mateusz Guzik]

On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.
> 
> Prevent rstat from disabling interrupts while processing all possible
> cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
> approach was previously discussed in
> https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> though this was in the context of an non-irq rstat spin lock.
> 
> Benchmark this change with:
> 1) a single stat_reader process with 400 threads, each reading a test
>    memcg's memory.stat repeatedly for 10 seconds.
> 2) 400 memory hog processes running in the test memcg and repeatedly
>    charging memory until oom killed. Then they repeat charging and oom
>    killing.
> 
> v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> interrupts are disabled by rstat for 45341 usec:
>   #  => started at: _raw_spin_lock_irq
>   #  => ended at:   cgroup_rstat_flush
>   #
>   #
>   #                    _------=> CPU#
>   #                   / _-----=> irqs-off/BH-disabled
>   #                  | / _----=> need-resched
>   #                  || / _---=> hardirq/softirq
>   #                  ||| / _--=> preempt-depth
>   #                  |||| / _-=> migrate-disable
>   #                  ||||| /     delay
>   #  cmd     pid     |||||| time  |   caller
>   #     \   /        ||||||  \    |    /
>   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
>   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
>   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
>   stat_rea-96532    52d.... 45343us : <stack trace>
>    => memcg1_stat_format
>    => memory_stat_format
>    => memory_stat_show
>    => seq_read_iter
>    => vfs_read
>    => ksys_read
>    => do_syscall_64
>    => entry_SYSCALL_64_after_hwframe
> 
> With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> longest holder. The longest irqs-off holder has irqs disabled for
> 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> 
> Running stat_reader memory.stat reader for 10 seconds:
> - without memory hogs: 9.84M accesses => 12.7M accesses
> -    with memory hogs: 9.46M accesses => 11.1M accesses
> The throughput of memory.stat access improves.
> 
> The mode of memory.stat access latency after grouping by of 2 buckets:
> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>
> ---
>  kernel/cgroup/rstat.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index aac91466279f..976c24b3671a 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c
> @@ -323,13 +323,11 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
>  			rcu_read_unlock();
>  		}
>  
> -		/* play nice and yield if necessary */
> -		if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
> -			__cgroup_rstat_unlock(cgrp, cpu);
> -			if (!cond_resched())
> -				cpu_relax();
> -			__cgroup_rstat_lock(cgrp, cpu);
> -		}
> +		/* play nice and avoid disabling interrupts for a long time */
> +		__cgroup_rstat_unlock(cgrp, cpu);
> +		if (!cond_resched())
> +			cpu_relax();
> +		__cgroup_rstat_lock(cgrp, cpu);
>  	}
>  }

Is not this going a little too far?

the lock + irq trip is quite expensive in its own right and now is
going to be paid for each cpu, as in the total time spent executing
cgroup_rstat_flush_locked is going to go up.

Would your problem go away toggling this every -- say -- 8 cpus?

Just a suggestion.

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Yosry Ahmed]

On Wed, Mar 19, 2025 at 11:47:32AM +0100, Mateusz Guzik wrote:
> On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> > iterating all possible cpus. It only drops the lock if there is
> > scheduler or spin lock contention. If neither, then interrupts can be
> > disabled for a long time. On large machines this can disable interrupts
> > for a long enough time to drop network packets. On 400+ CPU machines
> > I've seen interrupt disabled for over 40 msec.
> > 
> > Prevent rstat from disabling interrupts while processing all possible
> > cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
> > approach was previously discussed in
> > https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> > though this was in the context of an non-irq rstat spin lock.
> > 
> > Benchmark this change with:
> > 1) a single stat_reader process with 400 threads, each reading a test
> >    memcg's memory.stat repeatedly for 10 seconds.
> > 2) 400 memory hog processes running in the test memcg and repeatedly
> >    charging memory until oom killed. Then they repeat charging and oom
> >    killing.
> > 
> > v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> > interrupts are disabled by rstat for 45341 usec:
> >   #  => started at: _raw_spin_lock_irq
> >   #  => ended at:   cgroup_rstat_flush
> >   #
> >   #
> >   #                    _------=> CPU#
> >   #                   / _-----=> irqs-off/BH-disabled
> >   #                  | / _----=> need-resched
> >   #                  || / _---=> hardirq/softirq
> >   #                  ||| / _--=> preempt-depth
> >   #                  |||| / _-=> migrate-disable
> >   #                  ||||| /     delay
> >   #  cmd     pid     |||||| time  |   caller
> >   #     \   /        ||||||  \    |    /
> >   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
> >   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
> >   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
> >   stat_rea-96532    52d.... 45343us : <stack trace>
> >    => memcg1_stat_format
> >    => memory_stat_format
> >    => memory_stat_show
> >    => seq_read_iter
> >    => vfs_read
> >    => ksys_read
> >    => do_syscall_64
> >    => entry_SYSCALL_64_after_hwframe
> > 
> > With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> > longest holder. The longest irqs-off holder has irqs disabled for
> > 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> > 
> > Running stat_reader memory.stat reader for 10 seconds:
> > - without memory hogs: 9.84M accesses => 12.7M accesses
> > -    with memory hogs: 9.46M accesses => 11.1M accesses
> > The throughput of memory.stat access improves.
> > 
> > The mode of memory.stat access latency after grouping by of 2 buckets:
> > - without memory hogs: 64 usec => 16 usec
> > -    with memory hogs: 64 usec =>  8 usec
> > The memory.stat latency improves.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Greg Thelen <gthelen@google.com>
> > Tested-by: Greg Thelen <gthelen@google.com>
> > ---
> >  kernel/cgroup/rstat.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> > index aac91466279f..976c24b3671a 100644
> > --- a/kernel/cgroup/rstat.c
> > +++ b/kernel/cgroup/rstat.c
> > @@ -323,13 +323,11 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
> >  			rcu_read_unlock();
> >  		}
> >  
> > -		/* play nice and yield if necessary */
> > -		if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
> > -			__cgroup_rstat_unlock(cgrp, cpu);
> > -			if (!cond_resched())
> > -				cpu_relax();
> > -			__cgroup_rstat_lock(cgrp, cpu);
> > -		}
> > +		/* play nice and avoid disabling interrupts for a long time */
> > +		__cgroup_rstat_unlock(cgrp, cpu);
> > +		if (!cond_resched())
> > +			cpu_relax();
> > +		__cgroup_rstat_lock(cgrp, cpu);
> >  	}
> >  }
> 
> Is not this going a little too far?
> 
> the lock + irq trip is quite expensive in its own right and now is
> going to be paid for each cpu, as in the total time spent executing
> cgroup_rstat_flush_locked is going to go up.
> 
> Would your problem go away toggling this every -- say -- 8 cpus?

I was concerned about this too, and about more lock bouncing, but the
testing suggests that this actually overall improves the latency of
cgroup_rstat_flush_locked() (at least on tested HW).

So I don't think we need to do something like this unless a regression
is observed.

> 
> Just a suggestion.
> 

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Mateusz Guzik]

[-- Attachment #1: Type: text/plain, Size: 4037 bytes --]

On Wed, Mar 19, 2025 at 05:18:05PM +0000, Yosry Ahmed wrote:
> On Wed, Mar 19, 2025 at 11:47:32AM +0100, Mateusz Guzik wrote:
> > Is not this going a little too far?
> > 
> > the lock + irq trip is quite expensive in its own right and now is
> > going to be paid for each cpu, as in the total time spent executing
> > cgroup_rstat_flush_locked is going to go up.
> > 
> > Would your problem go away toggling this every -- say -- 8 cpus?
> 
> I was concerned about this too, and about more lock bouncing, but the
> testing suggests that this actually overall improves the latency of
> cgroup_rstat_flush_locked() (at least on tested HW).
> 
> So I don't think we need to do something like this unless a regression
> is observed.
> 

To my reading it reduces max time spent with irq disabled, which of
course it does -- after all it toggles it for every CPU.

Per my other e-mail in the thread the irq + lock trips remain not cheap
at least on Sapphire Rapids.

In my testing outlined below I see 11% increase in total execution time
with the irq + lock trip for every CPU in a 24-way vm.

So I stand by instead doing this every n CPUs, call it 8 or whatever.

How to repro:

I employed a poor-man's profiler like so:

bpftrace -e 'kprobe:cgroup_rstat_flush_locked { @start[tid] = nsecs; } kretprobe:cgroup_rstat_flush_locked /@start[tid]/ { print(nsecs - @start[tid]); delete(@start[tid]); } interval:s:60 { exit(); }'

This patch or not, execution time varies wildly even while the box is idle.

The above runs for a minute, collecting 23 samples (you may get
"lucky" and get one extra, in that case remove it for comparison).

A sysctl was added to toggle the new behavior vs old one. Patch at the
end.

"enabled"(1) means new behavior, "disabled"(0) means the old one.

Sum of nsecs (results piped to: awk '{ sum += $1 } END { print sum }'):
disabled:	903610
enabled:	1006833 (+11.4%)

Toggle at runtime with:
sysctl fs.magic_tunable=0 # disabled, no mandatory relocks
sysctl fs.magic_tunable=1 # enabled, relock for every CPU

I attached the stats I got for reference.

I patched v6.14 with the following:
diff --git a/fs/file_table.c b/fs/file_table.c
index c04ed94cdc4b..441f89421413 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -106,6 +106,8 @@ static int proc_nr_files(const struct ctl_table *table, int write, void *buffer,
 	return proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
 }
 
+unsigned long magic_tunable;
+
 static const struct ctl_table fs_stat_sysctls[] = {
 	{
 		.procname	= "file-nr",
@@ -123,6 +125,16 @@ static const struct ctl_table fs_stat_sysctls[] = {
 		.extra1		= SYSCTL_LONG_ZERO,
 		.extra2		= SYSCTL_LONG_MAX,
 	},
+	{
+		.procname	= "magic_tunable",
+		.data		= &magic_tunable,
+		.maxlen		= sizeof(magic_tunable),
+		.mode		= 0644,
+		.proc_handler	= proc_doulongvec_minmax,
+		.extra1		= SYSCTL_LONG_ZERO,
+		.extra2		= SYSCTL_LONG_MAX,
+	},
+
 	{
 		.procname	= "nr_open",
 		.data		= &sysctl_nr_open,
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index 3e01781aeb7b..f6444bf25b2f 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -299,6 +299,8 @@ static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop)
 	spin_unlock_irq(&cgroup_rstat_lock);
 }
 
+extern unsigned long magic_tunable;
+
 /* see cgroup_rstat_flush() */
 static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
 	__releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock)
@@ -323,12 +325,18 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
 			rcu_read_unlock();
 		}
 
-		/* play nice and yield if necessary */
-		if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
+		if (READ_ONCE(magic_tunable)) {
 			__cgroup_rstat_unlock(cgrp, cpu);
 			if (!cond_resched())
 				cpu_relax();
 			__cgroup_rstat_lock(cgrp, cpu);
+		} else {
+			if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
+				__cgroup_rstat_unlock(cgrp, cpu);
+				if (!cond_resched())
+					cpu_relax();
+				__cgroup_rstat_lock(cgrp, cpu);
+			}
 		}
 	}
 }


[-- Attachment #2: disabled --]
[-- Type: text/plain, Size: 138 bytes --]

69869
30473
64670
30544
30950
36445
36235
29920
51179
35760
33424
42426
30177
31211
44974
34450
37871
72642
33016
29518
31800
35730
30326

[-- Attachment #3: enabled --]
[-- Type: text/plain, Size: 138 bytes --]

63507
50113
36280
35148
63329
41232
51265
41341
41418
42824
35200
35550
54684
41597
55325
36120
48675
41179
39339
35794
38826
37411
40676

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Yosry Ahmed]

On Thu, Mar 27, 2025 at 03:38:50PM +0100, Mateusz Guzik wrote:
> On Wed, Mar 19, 2025 at 05:18:05PM +0000, Yosry Ahmed wrote:
> > On Wed, Mar 19, 2025 at 11:47:32AM +0100, Mateusz Guzik wrote:
> > > Is not this going a little too far?
> > > 
> > > the lock + irq trip is quite expensive in its own right and now is
> > > going to be paid for each cpu, as in the total time spent executing
> > > cgroup_rstat_flush_locked is going to go up.
> > > 
> > > Would your problem go away toggling this every -- say -- 8 cpus?
> > 
> > I was concerned about this too, and about more lock bouncing, but the
> > testing suggests that this actually overall improves the latency of
> > cgroup_rstat_flush_locked() (at least on tested HW).
> > 
> > So I don't think we need to do something like this unless a regression
> > is observed.
> > 
> 
> To my reading it reduces max time spent with irq disabled, which of
> course it does -- after all it toggles it for every CPU.
> 
> Per my other e-mail in the thread the irq + lock trips remain not cheap
> at least on Sapphire Rapids.
> 
> In my testing outlined below I see 11% increase in total execution time
> with the irq + lock trip for every CPU in a 24-way vm.
> 
> So I stand by instead doing this every n CPUs, call it 8 or whatever.
> 
> How to repro:
> 
> I employed a poor-man's profiler like so:
> 
> bpftrace -e 'kprobe:cgroup_rstat_flush_locked { @start[tid] = nsecs; } kretprobe:cgroup_rstat_flush_locked /@start[tid]/ { print(nsecs - @start[tid]); delete(@start[tid]); } interval:s:60 { exit(); }'
> 
> This patch or not, execution time varies wildly even while the box is idle.
> 
> The above runs for a minute, collecting 23 samples (you may get
> "lucky" and get one extra, in that case remove it for comparison).
> 
> A sysctl was added to toggle the new behavior vs old one. Patch at the
> end.
> 
> "enabled"(1) means new behavior, "disabled"(0) means the old one.
> 
> Sum of nsecs (results piped to: awk '{ sum += $1 } END { print sum }'):
> disabled:	903610
> enabled:	1006833 (+11.4%)

IIUC this calculates the amount of elapsed time between start and
finish, not necessarily the function's own execution time. Is it
possible that the increase in time is due to more interrupts arriving
during the function execution (which is what we want), rather than more
time being spent on disabling/enabling IRQs?

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Mateusz Guzik]

On Thu, Mar 27, 2025 at 6:17 PM Yosry Ahmed <yosry.ahmed@linux.dev> wrote:
>
> On Thu, Mar 27, 2025 at 03:38:50PM +0100, Mateusz Guzik wrote:
> > On Wed, Mar 19, 2025 at 05:18:05PM +0000, Yosry Ahmed wrote:
> > > On Wed, Mar 19, 2025 at 11:47:32AM +0100, Mateusz Guzik wrote:
> > > > Is not this going a little too far?
> > > >
> > > > the lock + irq trip is quite expensive in its own right and now is
> > > > going to be paid for each cpu, as in the total time spent executing
> > > > cgroup_rstat_flush_locked is going to go up.
> > > >
> > > > Would your problem go away toggling this every -- say -- 8 cpus?
> > >
> > > I was concerned about this too, and about more lock bouncing, but the
> > > testing suggests that this actually overall improves the latency of
> > > cgroup_rstat_flush_locked() (at least on tested HW).
> > >
> > > So I don't think we need to do something like this unless a regression
> > > is observed.
> > >
> >
> > To my reading it reduces max time spent with irq disabled, which of
> > course it does -- after all it toggles it for every CPU.
> >
> > Per my other e-mail in the thread the irq + lock trips remain not cheap
> > at least on Sapphire Rapids.
> >
> > In my testing outlined below I see 11% increase in total execution time
> > with the irq + lock trip for every CPU in a 24-way vm.
> >
> > So I stand by instead doing this every n CPUs, call it 8 or whatever.
> >
> > How to repro:
> >
> > I employed a poor-man's profiler like so:
> >
> > bpftrace -e 'kprobe:cgroup_rstat_flush_locked { @start[tid] = nsecs; } kretprobe:cgroup_rstat_flush_locked /@start[tid]/ { print(nsecs - @start[tid]); delete(@start[tid]); } interval:s:60 { exit(); }'
> >
> > This patch or not, execution time varies wildly even while the box is idle.
> >
> > The above runs for a minute, collecting 23 samples (you may get
> > "lucky" and get one extra, in that case remove it for comparison).
> >
> > A sysctl was added to toggle the new behavior vs old one. Patch at the
> > end.
> >
> > "enabled"(1) means new behavior, "disabled"(0) means the old one.
> >
> > Sum of nsecs (results piped to: awk '{ sum += $1 } END { print sum }'):
> > disabled:     903610
> > enabled:      1006833 (+11.4%)
>
> IIUC this calculates the amount of elapsed time between start and
> finish, not necessarily the function's own execution time. Is it
> possible that the increase in time is due to more interrupts arriving
> during the function execution (which is what we want), rather than more
> time being spent on disabling/enabling IRQs?

I can agree irq handlers have more opportunities to execute in the
toggling case and that the time accounted in the way above will
include them. I don't think explains it, but why not, let's test
without this problem.

I feel compelled to note atomics on x86-64 were expensive for as long
as the architecture was around so I'm confused what's up with the
resistance to the notion that they remain costly even with modern
uarchs. If anything, imo claims that they are cheap require strong
evidence.

That said, I modified the patch to add a section which issues
conditional relock if needed and smp_mb otherwise -- irqs remain
disabled, but we are still paying for a full fence. smp_mb is a lock
add $0 on the stack pointer. Note this has less work to do than what
was added in your patch.

It looks like this:
switch (READ_ONCE(magic_tunable)) {
case 1:
        __cgroup_rstat_unlock(cgrp, cpu);
        if (!cond_resched())
                cpu_relax();
        __cgroup_rstat_lock(cgrp, cpu);
        break;
case 2:
        if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
                __cgroup_rstat_unlock(cgrp, cpu);
                if (!cond_resched())
                        cpu_relax();
                __cgroup_rstat_lock(cgrp, cpu);
        } else {
                smp_mb();
        }
        break;
default:
        if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
                __cgroup_rstat_unlock(cgrp, cpu);
                if (!cond_resched())
                        cpu_relax();
                __cgroup_rstat_lock(cgrp, cpu);
        }
        break;
}

With this in place I'm seeing about 4% increase in execution time
measured the same way, so irq handlers sneaking in don't explain it.
Note smp_mb() alone is a smaller cost than the locked instruction +
func calls + irq trips. I also state I'm running this in a VM
(24-way), where paravirt spinlocks also issue a lock-prefixed
instruction to release the lock. I would say this very much justifies
the original claim of 11% with the patch as proposed.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

Hello Mateusz.

On Thu, Mar 27, 2025 at 06:47:56PM +0100, Mateusz Guzik <mjguzik@gmail.com> wrote:
> I feel compelled to note atomics on x86-64 were expensive for as long
> as the architecture was around so I'm confused what's up with the
> resistance to the notion that they remain costly even with modern
> uarchs. If anything, imo claims that they are cheap require strong
> evidence.

I don't there's strong resistance, your measurements show that it's not
negligible under given conditions.

The question is -- how much benefit would flushers have in practice with
coalesced unlock-locks. There is the approach now with releasing for
each CPU that is simple and benefits latency of irq dependants.

If you see practical issues with the limited throughputs of stat readers
(or flushers in general) because of this, please send a patch for
discussion that resolves it while preserving (some of) the irq freedom.

Also there is ongoing work of splitting up flushing per controller --
I'd like to see whether the given locks become "small" enough to require
no _irq exclusion at all during flushing.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Mateusz Guzik]

On Tue, Apr 1, 2025 at 5:00 PM Michal Koutný <mkoutny@suse.com> wrote:
> On Thu, Mar 27, 2025 at 06:47:56PM +0100, Mateusz Guzik <mjguzik@gmail.com> wrote:
> > I feel compelled to note atomics on x86-64 were expensive for as long
> > as the architecture was around so I'm confused what's up with the
> > resistance to the notion that they remain costly even with modern
> > uarchs. If anything, imo claims that they are cheap require strong
> > evidence.
>
> I don't there's strong resistance, your measurements show that it's not
> negligible under given conditions.
>
> The question is -- how much benefit would flushers have in practice with
> coalesced unlock-locks. There is the approach now with releasing for
> each CPU that is simple and benefits latency of irq dependants.
>

Toggling every n cpus instead of every single time is trivial to do.

I'm trying to avoid sending a patch in hopes of not getting CC'ed for
unrelated stuff later.

> If you see practical issues with the limited throughputs of stat readers
> (or flushers in general) because of this, please send a patch for
> discussion that resolves it while preserving (some of) the irq freedom.
>

This is some background maintenance work and it should do what's
feasible to not eat CPU.

The stock loop was behaving poorly in face of a high CPU count and it
makes excellent sense make it toggle the lock in *some* capacity.

I just don't believe going from 400+ CPUs straight to literally 1
every time is warranted. It seems the author felt justified with the
notion that it does not add overhead on contemporary hardware, but per
your own e-mail I demonstrated this does not hold.

Is this really going to suffer for toggling every 8 CPUs? that's a 50x
factor reduction

I would not be mailing here if the change was hard to do, but it
really is not. it's literally a counter in a loop getting checked.

> Also there is ongoing work of splitting up flushing per controller --
> I'd like to see whether the given locks become "small" enough to require
> no _irq exclusion at all during flushing.

the temp changes like the to stay for a long time.

tl;dr I don't believe going straight from 400 to 1 was properly
justified and I demonstrated how it hurts on a rather modest box. at
the same time a (likely) more than enough improvement over the stock
state can be trivially achieved while adding only a small fraction of
the overhead.

that said, there is bigger fish to fry elsewhere and I have no stake
in this code, so I'm not going to mail any further about this.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Tue, Apr 01, 2025 at 05:46:41PM +0200, Mateusz Guzik <mjguzik@gmail.com> wrote:
> Is this really going to suffer for toggling every 8 CPUs? that's a 50x
> factor reduction

As per the original patch, there's ~10x saving in max holding irqs-off,
naïevely thinking aggregating it flushing by 8 CPUs could reduce it to
(10/8) ~1.25x saving only.
(OTOH, it's not 400x effect, so it's not explained completely, not all
CPUs are same.) I can imagine the balanced value with this information
would be around 20 CPUs (sqrt(400)).
But the issue is it could as well be 4 or 32 or 8. Starting with 1 is
the simplest approach without introducing magic constants or heuristics.


> the temp changes like the to stay for a long time.

That'd mean that no one notices the performance impact there :-)
It can be easily changed later too.

> that said, there is bigger fish to fry elsewhere and I have no stake
> in this code, so I'm not going to mail any further about this.

Thank you for spending your effort on this, it's useful reference for
the future!

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Tejun Heo]

On Wed, Mar 19, 2025 at 11:47:32AM +0100, Mateusz Guzik wrote:
...
> Is not this going a little too far?
> 
> the lock + irq trip is quite expensive in its own right and now is
> going to be paid for each cpu, as in the total time spent executing
> cgroup_rstat_flush_locked is going to go up.

Lock/unlock when the cacheline is already on the cpu is pretty cheap and on
modern x86 cpus at least irq on/off are also only a few cycles, so you
probably wouldn't be able to tell the difference.

Thanks.

-- 
tejun

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Mateusz Guzik]

On Wed, Mar 19, 2025 at 6:26 PM Tejun Heo <tj@kernel.org> wrote:
>
> On Wed, Mar 19, 2025 at 11:47:32AM +0100, Mateusz Guzik wrote:
> ...
> > Is not this going a little too far?
> >
> > the lock + irq trip is quite expensive in its own right and now is
> > going to be paid for each cpu, as in the total time spent executing
> > cgroup_rstat_flush_locked is going to go up.
>
> Lock/unlock when the cacheline is already on the cpu is pretty cheap and on
> modern x86 cpus at least irq on/off are also only a few cycles, so you
> probably wouldn't be able to tell the difference.
>

This does not line up with what I had seen on x86-64 uarchs up to this
point, including Sapphire Rapids, which I think still counts as
reasonably new.

Most notably I see smp_mb() as a factor on my profiles, which compiles
to lock add $0 to a stack pointer -- a very much cache hot variable.

However, the cost of an atomic op has a lot of to do with state
accumulated around it -- the more atomics in short succession, the
lesser the impact. That is to say it may be given code is slow enough
that adding a lock-prefixed instruction does not add notable overhead.

In order to not just handwave, here is overhead of __legitimize_mnt()
while performing a path lookup for access() on my test box. smp_mb()
is the stock variant. irq() merely toggles interrupts, it does not
provide any sanity for the routine, but lets me see the cost if it got
planted there for some magic reason. Finally the last bit is what I
expect the final routine to have -- merely a branch to account for bad
mounts (taking advantage of synchronize_rcu() on unmount).
smp_mb:         3.31%
irq:            1.44%
nothing:        0.63%

These would be higher if it was not for  the fact that memory
allocation (for path buffer) is dog slow.

And indeed I get over a 3% speed up in access() rate by not suffering
that smp_mb() [I note I don't have a viable patch for it, rather I
whacked it for benchmarking purposes to see if pursuing proper removal
is worth it]

I'll note though that profiling open()+close() shows virtually no
difference (less than 0.5%) -- but that's not an argument for atomics
being cheap, instead it is an argument for open() in particular being
slow (which it is, I'm working on it though).

If you want I can ship you the test case, diffs and kernel config to
reproduce on your own.









--
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Yosry Ahmed]

On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.
> 
> Prevent rstat from disabling interrupts while processing all possible
> cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
> approach was previously discussed in
> https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> though this was in the context of an non-irq rstat spin lock.
> 
> Benchmark this change with:
> 1) a single stat_reader process with 400 threads, each reading a test
>    memcg's memory.stat repeatedly for 10 seconds.
> 2) 400 memory hog processes running in the test memcg and repeatedly
>    charging memory until oom killed. Then they repeat charging and oom
>    killing.
> 
> v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> interrupts are disabled by rstat for 45341 usec:
>   #  => started at: _raw_spin_lock_irq
>   #  => ended at:   cgroup_rstat_flush
>   #
>   #
>   #                    _------=> CPU#
>   #                   / _-----=> irqs-off/BH-disabled
>   #                  | / _----=> need-resched
>   #                  || / _---=> hardirq/softirq
>   #                  ||| / _--=> preempt-depth
>   #                  |||| / _-=> migrate-disable
>   #                  ||||| /     delay
>   #  cmd     pid     |||||| time  |   caller
>   #     \   /        ||||||  \    |    /
>   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
>   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
>   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
>   stat_rea-96532    52d.... 45343us : <stack trace>
>    => memcg1_stat_format
>    => memory_stat_format
>    => memory_stat_show
>    => seq_read_iter
>    => vfs_read
>    => ksys_read
>    => do_syscall_64
>    => entry_SYSCALL_64_after_hwframe
> 
> With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> longest holder. The longest irqs-off holder has irqs disabled for
> 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> 
> Running stat_reader memory.stat reader for 10 seconds:
> - without memory hogs: 9.84M accesses => 12.7M accesses
> -    with memory hogs: 9.46M accesses => 11.1M accesses
> The throughput of memory.stat access improves.
> 
> The mode of memory.stat access latency after grouping by of 2 buckets:
> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>
> ---
>  kernel/cgroup/rstat.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index aac91466279f..976c24b3671a 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c
> @@ -323,13 +323,11 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
>  			rcu_read_unlock();
>  		}
>  
> -		/* play nice and yield if necessary */
> -		if (need_resched() || spin_needbreak(&cgroup_rstat_lock)) {
> -			__cgroup_rstat_unlock(cgrp, cpu);
> -			if (!cond_resched())
> -				cpu_relax();
> -			__cgroup_rstat_lock(cgrp, cpu);
> -		}
> +		/* play nice and avoid disabling interrupts for a long time */
> +		__cgroup_rstat_unlock(cgrp, cpu);
> +		if (!cond_resched())
> +			cpu_relax();
> +		__cgroup_rstat_lock(cgrp, cpu);

This patch looks good as-is, so feel free to add:

Reviewed-by: Yosry Ahmed <yosry.ahmed@linux.dev>

That being said I think we can do further cleanups here now. We should
probably inline cgroup_rstat_flush_locked() into cgroup_rstat_flush(),
and move the lock hold and release into the loop.
cgroup_rstat_flush_hold() can simply call cgroup_rstat_flush() then hold
the lock.

Something like this on top:

diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index 976c24b3671a7..4f4b8d22555d7 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -299,17 +299,29 @@ static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop)
 	spin_unlock_irq(&cgroup_rstat_lock);
 }
 
-/* see cgroup_rstat_flush() */
-static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
-	__releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock)
+/**
+ * cgroup_rstat_flush - flush stats in @cgrp's subtree
+ * @cgrp: target cgroup
+ *
+ * Collect all per-cpu stats in @cgrp's subtree into the global counters
+ * and propagate them upwards.  After this function returns, all cgroups in
+ * the subtree have up-to-date ->stat.
+ *
+ * This also gets all cgroups in the subtree including @cgrp off the
+ * ->updated_children lists.
+ *
+ * This function may block.
+ */
+__bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
 {
 	int cpu;
 
-	lockdep_assert_held(&cgroup_rstat_lock);
-
+	might_sleep();
 	for_each_possible_cpu(cpu) {
 		struct cgroup *pos = cgroup_rstat_updated_list(cgrp, cpu);
 
+		/* Reacquire for every CPU to avoiding disabing IRQs too long */
+		__cgroup_rstat_lock(cgrp, cpu);
 		for (; pos; pos = pos->rstat_flush_next) {
 			struct cgroup_subsys_state *css;
 
@@ -322,37 +334,12 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp)
 				css->ss->css_rstat_flush(css, cpu);
 			rcu_read_unlock();
 		}
-
-		/* play nice and avoid disabling interrupts for a long time */
 		__cgroup_rstat_unlock(cgrp, cpu);
 		if (!cond_resched())
 			cpu_relax();
-		__cgroup_rstat_lock(cgrp, cpu);
 	}
 }
 
-/**
- * cgroup_rstat_flush - flush stats in @cgrp's subtree
- * @cgrp: target cgroup
- *
- * Collect all per-cpu stats in @cgrp's subtree into the global counters
- * and propagate them upwards.  After this function returns, all cgroups in
- * the subtree have up-to-date ->stat.
- *
- * This also gets all cgroups in the subtree including @cgrp off the
- * ->updated_children lists.
- *
- * This function may block.
- */
-__bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
-{
-	might_sleep();
-
-	__cgroup_rstat_lock(cgrp, -1);
-	cgroup_rstat_flush_locked(cgrp);
-	__cgroup_rstat_unlock(cgrp, -1);
-}
-
 /**
  * cgroup_rstat_flush_hold - flush stats in @cgrp's subtree and hold
  * @cgrp: target cgroup
@@ -365,9 +352,8 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
 void cgroup_rstat_flush_hold(struct cgroup *cgrp)
 	__acquires(&cgroup_rstat_lock)
 {
-	might_sleep();
+	cgroup_rstat_flush(cgrp);
 	__cgroup_rstat_lock(cgrp, -1);
-	cgroup_rstat_flush_locked(cgrp);
 }
 
 /**

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Johannes Weiner]

On Wed, Mar 19, 2025 at 05:16:02PM +0000, Yosry Ahmed wrote:
> @@ -365,9 +352,8 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
>  void cgroup_rstat_flush_hold(struct cgroup *cgrp)
>  	__acquires(&cgroup_rstat_lock)
>  {
> -	might_sleep();
> +	cgroup_rstat_flush(cgrp);
>  	__cgroup_rstat_lock(cgrp, -1);
> -	cgroup_rstat_flush_locked(cgrp);
>  }

Might as well remove cgroup_rstat_flush_hold/release entirely? There
are no external users, and the concept seems moot when the lock is
dropped per default. cgroup_base_stat_cputime_show() can open-code the
lock/unlock to stabilize the counts while reading.

(btw, why do we not have any locking around the root stats in
cgroup_base_stat_cputime_show()? There isn't anything preventing a
reader from seeing all zeroes if another reader runs the memset() on
cgrp->bstat, is there? Or double times...)

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Yosry Ahmed]

On Wed, Mar 19, 2025 at 02:06:43PM -0400, Johannes Weiner wrote:
> On Wed, Mar 19, 2025 at 05:16:02PM +0000, Yosry Ahmed wrote:
> > @@ -365,9 +352,8 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
> >  void cgroup_rstat_flush_hold(struct cgroup *cgrp)
> >  	__acquires(&cgroup_rstat_lock)
> >  {
> > -	might_sleep();
> > +	cgroup_rstat_flush(cgrp);
> >  	__cgroup_rstat_lock(cgrp, -1);
> > -	cgroup_rstat_flush_locked(cgrp);
> >  }
> 
> Might as well remove cgroup_rstat_flush_hold/release entirely? There
> are no external users, and the concept seems moot when the lock is
> dropped per default. cgroup_base_stat_cputime_show() can open-code the
> lock/unlock to stabilize the counts while reading.
 
Yeah I missed the fact that the users are internal because the functions
are not static. I also don't see the point of keeping them.

Tejun/Greg, should I send a patch on top of this one or do you prefer
sending a new version?


> (btw, why do we not have any locking around the root stats in
> cgroup_base_stat_cputime_show()? There isn't anything preventing a
> reader from seeing all zeroes if another reader runs the memset() on
> cgrp->bstat, is there? Or double times...)


(I think root_cgroup_cputime() operates on a stack allocated bstat, not
cgrp->bstat)

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Tejun Heo]

On Wed, Mar 19, 2025 at 06:35:02PM +0000, Yosry Ahmed wrote:
> On Wed, Mar 19, 2025 at 02:06:43PM -0400, Johannes Weiner wrote:
> > On Wed, Mar 19, 2025 at 05:16:02PM +0000, Yosry Ahmed wrote:
> > > @@ -365,9 +352,8 @@ __bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp)
> > >  void cgroup_rstat_flush_hold(struct cgroup *cgrp)
> > >  	__acquires(&cgroup_rstat_lock)
> > >  {
> > > -	might_sleep();
> > > +	cgroup_rstat_flush(cgrp);
> > >  	__cgroup_rstat_lock(cgrp, -1);
> > > -	cgroup_rstat_flush_locked(cgrp);
> > >  }
> > 
> > Might as well remove cgroup_rstat_flush_hold/release entirely? There
> > are no external users, and the concept seems moot when the lock is
> > dropped per default. cgroup_base_stat_cputime_show() can open-code the
> > lock/unlock to stabilize the counts while reading.
>  
> Yeah I missed the fact that the users are internal because the functions
> are not static. I also don't see the point of keeping them.
> 
> Tejun/Greg, should I send a patch on top of this one or do you prefer
> sending a new version?

Please send a patch on top.

Thanks.

-- 
tejun

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Johannes Weiner]

On Wed, Mar 19, 2025 at 06:35:02PM +0000, Yosry Ahmed wrote:
> On Wed, Mar 19, 2025 at 02:06:43PM -0400, Johannes Weiner wrote:
> > (btw, why do we not have any locking around the root stats in
> > cgroup_base_stat_cputime_show()? There isn't anything preventing a
> > reader from seeing all zeroes if another reader runs the memset() on
> > cgrp->bstat, is there? Or double times...)
> 
> (I think root_cgroup_cputime() operates on a stack allocated bstat, not
> cgrp->bstat)

That was the case until:

commit b824766504e49f3fdcbb8c722e70996a78c3636e
Author: Chen Ridong <chenridong@huawei.com>
Date:   Thu Jul 4 14:01:19 2024 +0000

    cgroup/rstat: add force idle show helper

Now it's doing this:

void cgroup_base_stat_cputime_show(struct seq_file *seq)
{
	struct cgroup *cgrp = seq_css(seq)->cgroup;

	if (cgroup_parent(cgrp)) {
		...
	} else {
		/* cgrp->bstat of root is not actually used, reuse it */
		root_cgroup_cputime(&cgrp->bstat);
		usage = cgrp->bstat.cputime.sum_exec_runtime;
		utime = cgrp->bstat.cputime.utime;
		stime = cgrp->bstat.cputime.stime;
		ntime = cgrp->bstat.ntime;
	}
}

and then:

static void root_cgroup_cputime(struct cgroup_base_stat *bstat)
{
	struct task_cputime *cputime = &bstat->cputime;
	int i;

	memset(bstat, 0, sizeof(*bstat));

	/* various += on bstat and cputime members */
}

So all readers are mucking with the root cgroup's bstat.

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Johannes Weiner]

On Wed, Mar 19, 2025 at 03:16:42PM -0400, Johannes Weiner wrote:
> On Wed, Mar 19, 2025 at 06:35:02PM +0000, Yosry Ahmed wrote:
> > On Wed, Mar 19, 2025 at 02:06:43PM -0400, Johannes Weiner wrote:
> > > (btw, why do we not have any locking around the root stats in
> > > cgroup_base_stat_cputime_show()? There isn't anything preventing a
> > > reader from seeing all zeroes if another reader runs the memset() on
> > > cgrp->bstat, is there? Or double times...)
> > 
> > (I think root_cgroup_cputime() operates on a stack allocated bstat, not
> > cgrp->bstat)
> 
> That was the case until:
> 
> commit b824766504e49f3fdcbb8c722e70996a78c3636e
> Author: Chen Ridong <chenridong@huawei.com>
> Date:   Thu Jul 4 14:01:19 2024 +0000

Nevermind, Tejun pointed me to the follow-up fix he's got already
queued up:

https://web.git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/commit/?id=c4af66a95aa3bc1d4f607ebd4eea524fb58946e3

That brings it all back on the stack.

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Tejun Heo]

On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.
> 
> Prevent rstat from disabling interrupts while processing all possible
> cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
> approach was previously discussed in
> https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> though this was in the context of an non-irq rstat spin lock.
> 
> Benchmark this change with:
> 1) a single stat_reader process with 400 threads, each reading a test
>    memcg's memory.stat repeatedly for 10 seconds.
> 2) 400 memory hog processes running in the test memcg and repeatedly
>    charging memory until oom killed. Then they repeat charging and oom
>    killing.
> 
> v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> interrupts are disabled by rstat for 45341 usec:
>   #  => started at: _raw_spin_lock_irq
>   #  => ended at:   cgroup_rstat_flush
>   #
>   #
>   #                    _------=> CPU#
>   #                   / _-----=> irqs-off/BH-disabled
>   #                  | / _----=> need-resched
>   #                  || / _---=> hardirq/softirq
>   #                  ||| / _--=> preempt-depth
>   #                  |||| / _-=> migrate-disable
>   #                  ||||| /     delay
>   #  cmd     pid     |||||| time  |   caller
>   #     \   /        ||||||  \    |    /
>   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
>   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
>   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
>   stat_rea-96532    52d.... 45343us : <stack trace>
>    => memcg1_stat_format
>    => memory_stat_format
>    => memory_stat_show
>    => seq_read_iter
>    => vfs_read
>    => ksys_read
>    => do_syscall_64
>    => entry_SYSCALL_64_after_hwframe
> 
> With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> longest holder. The longest irqs-off holder has irqs disabled for
> 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> 
> Running stat_reader memory.stat reader for 10 seconds:
> - without memory hogs: 9.84M accesses => 12.7M accesses
> -    with memory hogs: 9.46M accesses => 11.1M accesses
> The throughput of memory.stat access improves.
> 
> The mode of memory.stat access latency after grouping by of 2 buckets:
> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>

Applied to cgroup/for-6.15.

Thanks.

-- 
tejun

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Johannes Weiner]

On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.
> 
> Prevent rstat from disabling interrupts while processing all possible
> cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu. This
> approach was previously discussed in
> https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> though this was in the context of an non-irq rstat spin lock.
> 
> Benchmark this change with:
> 1) a single stat_reader process with 400 threads, each reading a test
>    memcg's memory.stat repeatedly for 10 seconds.
> 2) 400 memory hog processes running in the test memcg and repeatedly
>    charging memory until oom killed. Then they repeat charging and oom
>    killing.
> 
> v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> interrupts are disabled by rstat for 45341 usec:
>   #  => started at: _raw_spin_lock_irq
>   #  => ended at:   cgroup_rstat_flush
>   #
>   #
>   #                    _------=> CPU#
>   #                   / _-----=> irqs-off/BH-disabled
>   #                  | / _----=> need-resched
>   #                  || / _---=> hardirq/softirq
>   #                  ||| / _--=> preempt-depth
>   #                  |||| / _-=> migrate-disable
>   #                  ||||| /     delay
>   #  cmd     pid     |||||| time  |   caller
>   #     \   /        ||||||  \    |    /
>   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
>   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
>   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
>   stat_rea-96532    52d.... 45343us : <stack trace>
>    => memcg1_stat_format
>    => memory_stat_format
>    => memory_stat_show
>    => seq_read_iter
>    => vfs_read
>    => ksys_read
>    => do_syscall_64
>    => entry_SYSCALL_64_after_hwframe
> 
> With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> longest holder. The longest irqs-off holder has irqs disabled for
> 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> 
> Running stat_reader memory.stat reader for 10 seconds:
> - without memory hogs: 9.84M accesses => 12.7M accesses
> -    with memory hogs: 9.46M accesses => 11.1M accesses
> The throughput of memory.stat access improves.
> 
> The mode of memory.stat access latency after grouping by of 2 buckets:
> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Shakeel Butt]

On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> iterating all possible cpus. It only drops the lock if there is
> scheduler or spin lock contention. If neither, then interrupts can be
> disabled for a long time. On large machines this can disable interrupts
> for a long enough time to drop network packets. On 400+ CPU machines
> I've seen interrupt disabled for over 40 msec.

Which kernel was this observed on in production?

> 
> Prevent rstat from disabling interrupts while processing all possible
> cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu.

Doing for each cpu might be too extreme. Have you tried with some
batching?

> This
> approach was previously discussed in
> https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> though this was in the context of an non-irq rstat spin lock.
> 
> Benchmark this change with:
> 1) a single stat_reader process with 400 threads, each reading a test
>    memcg's memory.stat repeatedly for 10 seconds.
> 2) 400 memory hog processes running in the test memcg and repeatedly
>    charging memory until oom killed. Then they repeat charging and oom
>    killing.
> 

Though this benchmark seems too extreme but userspace holding off irqs
for that long time is bad. BTW are these memory hoggers, creating anon
memory or file memory? Is [z]swap enabled?

For the long term, I think we can use make this work without disabling
irqs, similar to how networking manages sock lock.

> v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> interrupts are disabled by rstat for 45341 usec:
>   #  => started at: _raw_spin_lock_irq
>   #  => ended at:   cgroup_rstat_flush
>   #
>   #
>   #                    _------=> CPU#
>   #                   / _-----=> irqs-off/BH-disabled
>   #                  | / _----=> need-resched
>   #                  || / _---=> hardirq/softirq
>   #                  ||| / _--=> preempt-depth
>   #                  |||| / _-=> migrate-disable
>   #                  ||||| /     delay
>   #  cmd     pid     |||||| time  |   caller
>   #     \   /        ||||||  \    |    /
>   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
>   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
>   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
>   stat_rea-96532    52d.... 45343us : <stack trace>
>    => memcg1_stat_format
>    => memory_stat_format
>    => memory_stat_show
>    => seq_read_iter
>    => vfs_read
>    => ksys_read
>    => do_syscall_64
>    => entry_SYSCALL_64_after_hwframe
> 
> With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> longest holder. The longest irqs-off holder has irqs disabled for
> 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> 
> Running stat_reader memory.stat reader for 10 seconds:
> - without memory hogs: 9.84M accesses => 12.7M accesses
> -    with memory hogs: 9.46M accesses => 11.1M accesses
> The throughput of memory.stat access improves.
> 
> The mode of memory.stat access latency after grouping by of 2 buckets:
> - without memory hogs: 64 usec => 16 usec
> -    with memory hogs: 64 usec =>  8 usec
> The memory.stat latency improves.

So, things are improving even without batching. I wonder if there are
less readers then how will this look like. Can you try with single
reader as well?

> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Greg Thelen <gthelen@google.com>
> Tested-by: Greg Thelen <gthelen@google.com>

Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Eric Dumazet]

[-- Attachment #1: Type: text/plain, Size: 4905 bytes --]

On Wed, Mar 19, 2025 at 6:53 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:

> On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> > iterating all possible cpus. It only drops the lock if there is
> > scheduler or spin lock contention. If neither, then interrupts can be
> > disabled for a long time. On large machines this can disable interrupts
> > for a long enough time to drop network packets. On 400+ CPU machines
> > I've seen interrupt disabled for over 40 msec.
>
> Which kernel was this observed on in production?
>

I had these issues with the latest upstream kernels, and have seen a 80 ms
delay.

./trace-cgroup_rstat_flush_locked.sh
Attaching 3 probes...
^C

@cgroup_rstat_flush_locked_us:
[64, 128)              2 |@
  |
[128, 256)             3 |@
  |
[256, 512)             2 |@
  |
[512, 1K)              6 |@@@
  |
[1K, 2K)               8 |@@@@@
  |
[2K, 4K)               3 |@
  |
[4K, 8K)               0 |
   |
[8K, 16K)              0 |
   |
[16K, 32K)             4 |@@
   |
[32K, 64K)            83
|@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[64K, 128K)           74 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
   |

@max_us: 79214




Greg in his tests was able to get 45 ms "only"



>
> >
> > Prevent rstat from disabling interrupts while processing all possible
> > cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu.
>
> Doing for each cpu might be too extreme. Have you tried with some
> batching?
>

Not really, modern platforms are doing the unlock/lock pair faster than
all the cache line misses done in the per-cpu loop.



>
> > This
> > approach was previously discussed in
> > https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> > though this was in the context of an non-irq rstat spin lock.
> >
> > Benchmark this change with:
> > 1) a single stat_reader process with 400 threads, each reading a test
> >    memcg's memory.stat repeatedly for 10 seconds.
> > 2) 400 memory hog processes running in the test memcg and repeatedly
> >    charging memory until oom killed. Then they repeat charging and oom
> >    killing.
> >
>
> Though this benchmark seems too extreme but userspace holding off irqs
> for that long time is bad. BTW are these memory hoggers, creating anon
> memory or file memory? Is [z]swap enabled?
>
> For the long term, I think we can use make this work without disabling
> irqs, similar to how networking manages sock lock.
>
> > v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> > interrupts are disabled by rstat for 45341 usec:
> >   #  => started at: _raw_spin_lock_irq
> >   #  => ended at:   cgroup_rstat_flush
> >   #
> >   #
> >   #                    _------=> CPU#
> >   #                   / _-----=> irqs-off/BH-disabled
> >   #                  | / _----=> need-resched
> >   #                  || / _---=> hardirq/softirq
> >   #                  ||| / _--=> preempt-depth
> >   #                  |||| / _-=> migrate-disable
> >   #                  ||||| /     delay
> >   #  cmd     pid     |||||| time  |   caller
> >   #     \   /        ||||||  \    |    /
> >   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
> >   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
> >   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on
> <-cgroup_rstat_flush
> >   stat_rea-96532    52d.... 45343us : <stack trace>
> >    => memcg1_stat_format
> >    => memory_stat_format
> >    => memory_stat_show
> >    => seq_read_iter
> >    => vfs_read
> >    => ksys_read
> >    => do_syscall_64
> >    => entry_SYSCALL_64_after_hwframe
> >
> > With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> > longest holder. The longest irqs-off holder has irqs disabled for
> > 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> >
> > Running stat_reader memory.stat reader for 10 seconds:
> > - without memory hogs: 9.84M accesses => 12.7M accesses
> > -    with memory hogs: 9.46M accesses => 11.1M accesses
> > The throughput of memory.stat access improves.
> >
> > The mode of memory.stat access latency after grouping by of 2 buckets:
> > - without memory hogs: 64 usec => 16 usec
> > -    with memory hogs: 64 usec =>  8 usec
> > The memory.stat latency improves.
>
> So, things are improving even without batching. I wonder if there are
> less readers then how will this look like. Can you try with single
> reader as well?
>
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Greg Thelen <gthelen@google.com>
> > Tested-by: Greg Thelen <gthelen@google.com>
>
> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
>

[-- Attachment #2: Type: text/html, Size: 7514 bytes --]

Re: [PATCH] cgroup/rstat: avoid disabling irqs for O(num_cpu)

[Greg Thelen]

On Wed, Mar 19, 2025 at 10:53 AM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> On Wed, Mar 19, 2025 at 12:13:30AM -0700, Greg Thelen wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > cgroup_rstat_flush_locked() grabs the irq safe cgroup_rstat_lock while
> > iterating all possible cpus. It only drops the lock if there is
> > scheduler or spin lock contention. If neither, then interrupts can be
> > disabled for a long time. On large machines this can disable interrupts
> > for a long enough time to drop network packets. On 400+ CPU machines
> > I've seen interrupt disabled for over 40 msec.
>
> Which kernel was this observed on in production?
>
> >
> > Prevent rstat from disabling interrupts while processing all possible
> > cpus. Instead drop and reacquire cgroup_rstat_lock for each cpu.
>
> Doing for each cpu might be too extreme. Have you tried with some
> batching?
>
> > This
> > approach was previously discussed in
> > https://lore.kernel.org/lkml/ZBz%2FV5a7%2F6PZeM7S@slm.duckdns.org/,
> > though this was in the context of an non-irq rstat spin lock.
> >
> > Benchmark this change with:
> > 1) a single stat_reader process with 400 threads, each reading a test
> >    memcg's memory.stat repeatedly for 10 seconds.
> > 2) 400 memory hog processes running in the test memcg and repeatedly
> >    charging memory until oom killed. Then they repeat charging and oom
> >    killing.
> >
>
> Though this benchmark seems too extreme but userspace holding off irqs
> for that long time is bad. BTW are these memory hoggers, creating anon
> memory or file memory? Is [z]swap enabled?

The memory hoggers were anon, without any form of swap.

I think the other questions were answered in other replies, but feel free to
re-ask and I'll provide details.


> For the long term, I think we can use make this work without disabling
> irqs, similar to how networking manages sock lock.
>
> > v6.14-rc6 with CONFIG_IRQSOFF_TRACER with stat_reader and hogs, finds
> > interrupts are disabled by rstat for 45341 usec:
> >   #  => started at: _raw_spin_lock_irq
> >   #  => ended at:   cgroup_rstat_flush
> >   #
> >   #
> >   #                    _------=> CPU#
> >   #                   / _-----=> irqs-off/BH-disabled
> >   #                  | / _----=> need-resched
> >   #                  || / _---=> hardirq/softirq
> >   #                  ||| / _--=> preempt-depth
> >   #                  |||| / _-=> migrate-disable
> >   #                  ||||| /     delay
> >   #  cmd     pid     |||||| time  |   caller
> >   #     \   /        ||||||  \    |    /
> >   stat_rea-96532    52d....    0us*: _raw_spin_lock_irq
> >   stat_rea-96532    52d.... 45342us : cgroup_rstat_flush
> >   stat_rea-96532    52d.... 45342us : tracer_hardirqs_on <-cgroup_rstat_flush
> >   stat_rea-96532    52d.... 45343us : <stack trace>
> >    => memcg1_stat_format
> >    => memory_stat_format
> >    => memory_stat_show
> >    => seq_read_iter
> >    => vfs_read
> >    => ksys_read
> >    => do_syscall_64
> >    => entry_SYSCALL_64_after_hwframe
> >
> > With this patch the CONFIG_IRQSOFF_TRACER doesn't find rstat to be the
> > longest holder. The longest irqs-off holder has irqs disabled for
> > 4142 usec, a huge reduction from previous 45341 usec rstat finding.
> >
> > Running stat_reader memory.stat reader for 10 seconds:
> > - without memory hogs: 9.84M accesses => 12.7M accesses
> > -    with memory hogs: 9.46M accesses => 11.1M accesses
> > The throughput of memory.stat access improves.
> >
> > The mode of memory.stat access latency after grouping by of 2 buckets:
> > - without memory hogs: 64 usec => 16 usec
> > -    with memory hogs: 64 usec =>  8 usec
> > The memory.stat latency improves.
>
> So, things are improving even without batching. I wonder if there are
> less readers then how will this look like. Can you try with single
> reader as well?
>
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Greg Thelen <gthelen@google.com>
> > Tested-by: Greg Thelen <gthelen@google.com>
>
> Reviewed-by: Shakeel Butt <shakeel.butt@linux.dev>
>