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

From: Yosry Ahmed <yosryahmed@google.com>
To: Michal Hocko <mhocko@suse.com>
Cc: "Johannes Weiner" <hannes@cmpxchg.org>,
	"Shakeel Butt" <shakeelb@google.com>, "Tejun Heo" <tj@kernel.org>,
	"Josef Bacik" <josef@toxicpanda.com>,
	"Jens Axboe" <axboe@kernel.dk>,
	"Zefan Li" <lizefan.x@bytedance.com>,
	"Roman Gushchin" <roman.gushchin@linux.dev>,
	"Muchun Song" <muchun.song@linux.dev>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Michal Koutný" <mkoutny@suse.com>,
	"Vasily Averin" <vasily.averin@linux.dev>,
	cgroups@vger.kernel.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	bpf@vger.kernel.org
Subject: Re: [PATCH v2 4/9] cgroup: rstat: add WARN_ON_ONCE() if flushing outside task context
Date: Thu, 30 Mar 2023 01:06:26 -0700	[thread overview]
Message-ID: <CAJD7tkaKd9Bcb2-e83Q-kzF7G+crr1U+7uqUPBARXWq-LpyKvw@mail.gmail.com> (raw)
In-Reply-To: <ZCU+8lSi+e4WgT3F@dhcp22.suse.cz>

On Thu, Mar 30, 2023 at 12:49 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 30-03-23 00:13:16, Yosry Ahmed wrote:
> > On Thu, Mar 30, 2023 at 12:06 AM Michal Hocko <mhocko@suse.com> wrote:
> [...]
> > > So the real question is. Do we really care so deeply? After all somebody
> > > might be calling this from within a spin lock or irq disabled section
> > > resulting in a similar situation without noticing.
> >
> > There are discussions in [1] about making atomic rstat flush not
> > disable irqs throughout the process, so in that case it would only
> > result in a similar situation if the caller has irq disabled. The only
> > caller that might have irq disabled is the same caller that might be
> > in irq context before this series: mem_cgroup_usage().
> >
> > On that note, and while I have your attention, I was wondering if we
> > can eliminate the flush call completely from mem_cgroup_usage(), and
> > read the global stats counters for root memcg instead of the root
> > counters. There might be subtle differences, but the root memcg usage
> > isn't super accurate now anyway (e.g. it doesn't include kernel
> > memory).
>
> root memcg stats are imprecise indeed and I have to admit I do not
> really know why we are adding more work for that case. I have tried to
> dig into git history for that yesterday but couldn't find a satisfying
> answer. It goes all the way down to 2d146aa3aa842 which has done the
> switch to rstat. Maybe Johannes remembers.

I think it goes back even before that. Even before rstat, we used to
get the root usage by iterating over the hierarchy. Maybe we didn't
have the global counters at some point or they weren't in line with
the root memcg (e.g. use_hierarchy = 0). It would be great if we can
just use the global counters here. Hopefully Johannes can confirm or
deny this.

>
> Anyway, back to this particular patch. I still do not see strong reasons
> to be verbose about !in_task case so I would just drop this patch.

I will drop this patch in the next iteration. If I implement a patch
that makes rstat flushing not disable irqs all throughout (like [1]),
whether part of this series or not, and we remove flushing from
mem_cgroup_usage(), then at this point:
- No existing flushers will be disabling irqs.
- rstat flushing itself will not be disabling irqs throughout the entire flush.

If we achieve that, do you think it makes sense to add
WARN_ON_ONCE(irqs_disabled()) instead to prevent future users from
flushing while disabling irqs or in irq context?

All I am trying to achieve here is make sure we don't regress. I don't
want to minimize the current atomic flushers now only to have them
increase later.

[1] https://lore.kernel.org/lkml/CAJD7tkZrp=4zWvjE9_010TAG1T_crCbf9P64UzJABspgcrGPKg@mail.gmail.com/

> --
> Michal Hocko
> SUSE Labs