Re: [PATCH v2 4/9] cgroup: rstat: add WARN_ON_ONCE() if flushing outside task context

[Michal Hocko <mhocko@suse.com>]

On Tue 28-03-23 22:16:39, Yosry Ahmed wrote:
> rstat flushing is too expensive to perform in irq context.
> The previous patch removed the only context that may invoke an rstat
> flush from irq context, add a WARN_ON_ONCE() to detect future
> violations, or those that we are not aware of.
> 
> Ideally, we wouldn't flush with irqs disabled either, but we have one
> context today that does so in mem_cgroup_usage(). Forbid callers from
> irq context for now, and hopefully we can also forbid callers with irqs
> disabled in the future when we can get rid of this callsite.

I am sorry to be late to the discussion. I wanted to follow up on
Johannes reply in the previous version but you are too fast ;)

I do agree that this looks rather arbitrary. You do not explain how the
warning actually helps. Is the intention to be really verbose to the
kernel log when somebody uses this interface from the IRQ context and
get bug reports? What about configurations with panic on warn? Do we
really want to crash their systems for something like that?

> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> Reviewed-by: Shakeel Butt <shakeelb@google.com>
> ---
>  kernel/cgroup/rstat.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index d3252b0416b6..c2571939139f 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c
> @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup *cgrp, bool may_sleep)
>  {
>  	int cpu;
>  
> +	/* rstat flushing is too expensive for irq context */
> +	WARN_ON_ONCE(!in_task());
>  	lockdep_assert_held(&cgroup_rstat_lock);
>  
>  	for_each_possible_cpu(cpu) {
> -- 
> 2.40.0.348.gf938b09366-goog

-- 
Michal Hocko
SUSE Labs