Re: [PATCH] mm: memcg: provide accurate stats for userspace reads

[Yosry Ahmed]

On Wed, Aug 16, 2023 at 3:35 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Wed, Aug 16, 2023 at 12:08 PM Tejun Heo <tj@xxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > On Wed, Aug 16, 2023 at 10:11:20AM -0700, Shakeel Butt wrote:
> > > These options are not white and black and there can be something in
> > > between but let me be very clear on what I don't want and would NACK.
> >
> > I'm not a big fan of interfaces with hidden states. What you're proposing
> > isn't strictly that but it's still a bit nasty. So, if we can get by without
> > doing that, that'd be great.
>
> Agreed. I will try to send patches soon implementing option (2) above,
> basically removing unified flushing. I will try to document any
> potential regressions that may happen and how we may fix them. Ideally
> we see no regressions.
>
> >
> > > I don't want a global sleepable lock which can be taken by potentially
> > > any application running on the system. We have seen similar global
> > > locks causing isolation and priority inversion issues in production.
> > > So, not another lock which needs to be taken under extreme condition
> > > (reading stats under OOM) by a high priority task (node controller)
> > > and might be held by a low priority task.
> >
> > Yeah, this is a real concern. Those priority inversions do occur and can be
> > serious but causing serious problems under memory pressure usually requires
> > involving memory allocations and IOs. Here, it's just all CPU. So, at least
> > in OOM conditions, this shouldn't be in the way (the system wouldn't have
> > anything else to do anyway).
> >
> > It is true that this still can lead to priority through CPU competition tho.
> > However, that problem isn't necessarily solved by what you're suggesting
> > either unless you want to restrict explicit flushing based on permissions
> > which is another can of worms.
>
> Right. Also in the case of a mutex, if we disable preemption while
> holding the mutex, this makes sure that whoever holding the mutex does
> not starve waiters. Essentially the difference would be that waiters
> will sleep with the mutex instead of spinning, but the mutex holder
> itself wouldn't sleep.
>
> I will make this a separate patch, just in case it's too
> controversial. Switching the spinlock to a mutex should not block
> removing unified flushing.
>
> >
> > My preference is not exposing this in user interface. This is mostly arising
> > from internal implementation details and isn't what users necessarily care
> > about. There are many things we can do on the kernel side to make trade-offs
> > among overhead, staleness and priority inversions. If we make this an
> > explicit userland interface behavior, we get locked into that semantics
> > which we'll likely regret in some future.
> >
>
> Yeah that's what I am trying to do here as well.  I will try to follow
> up on this discussion with patches soon.
>
> Thanks everyone!

So status update. I tried implementing removing unified flushing. I
can send the patches if it helps visualize things, but essentially
mem_cgroup_flush_stats{_ratelimited} takes a memcg argument that it
passes to cgroup_flush_stats(). No skipping if someone else is
flushing (stats_flush_ongoing) and no threshold at which we start
flushing (stats_flush_threshold).

I tested this by some synthetic reclaim stress test that I wrote,
because reclaim is the easiest way to stress in-kernel flushing. I
basically create 10s or 100s cgroups and run workers in them that keep
allocating memory beyond the limit. I also run workers that
periodically read the stats.

Removing unified flushing makes such a synthetic benchmark 2-3 times
slower. This is not surprising, as all these concurrent reclaimers
used to just skip flushing, regardless of whether or not they get
accurate stats. Now I don't know how realistic such a benchmark is,
but if there's a workload somewhere that runs into such conditions it
will surely regress.

Sorry if the above is difficult to visualize. I can send the patches
and the test script if it makes things easier, I just didn't want to
overwhelm the thread.

I think there are 2 options going forward:
(a) If we believe removing unified flushing is the best way forward
for most use cases, then we need to find a better way of testing this
approach practically. Any suggestions here are useful.

(b) Decide that it's too expensive to remove unified flushing
completely, at least for in-kernel flushers that can see a lot of
concurrency. We can only remove unified flushing for userspace reads.
Only flush the memcg being read and never skip (essentially what Ivan
tried). There are already some flushing contexts that do this (e.g.
zswap memcg charging code). I believe as long as there isn't a lot of
concurrency in these paths it should be fine to skip unified flushing
for them, and keep it only for in-kernel flushers.

Any thoughts on this?