Re: [PATCH v4] mm: vmpressure: don't count proactive reclaim in vmpressure

[Yosry Ahmed <yosryahmed@google.com>]

On Thu, Jul 21, 2022 at 9:42 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 21-07-22 08:58:06, Yosry Ahmed wrote:
> > On Thu, Jul 21, 2022 at 4:44 AM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Wed 20-07-22 11:02:56, Yosry Ahmed wrote:
> > > > On Wed, Jul 20, 2022 at 10:50 AM Shakeel Butt <shakeelb@google.com> wrote:
> > > > >
> > > > > On Wed, Jul 20, 2022 at 2:24 AM Michal Hocko <mhocko@suse.com> wrote:
> > > > > >
> > > > > [...]
> > > > > >
> > > > > > I think what we are missing here is
> > > > > > - explain that this doesn't have any effect on existing users of
> > > > > >   vmpressure user interface because that is cgroup v1 and memory.reclaim
> > > > > >   is v2 feature. This is a trivial statement but quite useful for future
> > > > > >   readers of this commit
> > > > > > - explain the effect on the networking layer and typical usecases
> > > > > >   memory.reclaim is used for currently and ideally document that.
> > > > >
> > > > > I agree with the above two points (Yosry, please address those) but
> > > > > the following third point is orthogonal and we don't really need to
> > > > > have an answer for this patch to be accepted.
> > > > >
> > > >
> > > > That's great feedback, thanks Michal and Shakeel!
> > > >
> > > > How do you feel about the following commit message instead? Does it
> > > > address your concerns?:
> > > >
> > > > memory.reclaim is a cgroup v2 interface that allows users to
> > > > proactively reclaim memory from a memcg, without real memory pressure.
> > > > Reclaim operations invoke vmpressure, which is used in cgroup v1 to
> > > > notify userspace of reclaim efficiency, and used in both v1 and v2 as
> > > > a signal for a memcg being under memory pressure for networking (see
> > > > mem_cgroup_under_socket_pressure()). For the former, vmpressure
> > > > notifications in v1 are not affected by this change since
> > > > memory.reclaim is a v2 feature.
> > > >
> > > > For the latter, the effects of the vmpressure signal (according to
> > > > Shakeel [1]) are as follows:
> > > > 1. Reducing send and receive buffers of the current socket.
> > > > 2. May drop packets on the rx path.
> > > > 3. May throttle current thread on the tx path.
> > > >
> > > > Since proactive reclaim is invoked directly by userspace, not by
> > > > memory pressure, it makes sense not to throttle networking. Hence,
> > > > this change makes sure that proactive reclaim caused by memory.reclaim
> > > > does not trigger vmpressure.
> > >
> > > OK, looks much better. Please also add a note to the documentation about
> > > this side effect.
> >
> > I don't want to add something to the documentation about throttling
> > networking because it seems like these are implementation details that
> > we may change in the future. I don't know if we can document this
> > behavior today and then change it later.
>
> The exact mechanism on how the throttling is done is one thing. This can
> change. But the fact that _no_ throttling is applied is something that
> we shouldn't change of course. If we were really strict we shouldn't
> change it even now but considering that the interface is new and
> usecases still shaping then better now than later.
>
> > How about we document a more generic statement in memory.reclaim
> > documentation, like:
> >
> > "With reactive reclaim operations triggered by the kernel, the kernel
> > may take further actions to alleviate memory pressure (such as
> > throttling networking memory consumption). For proactive reclaim
> > operations triggered by this interface, the kernel may choose to skip
> > such actions as reclaim is not an indication of memory pressure."
>
> IDK, this sounds too much word lawyering to me TBH. It is better to be clear
> about explicitly known side effects. For example where do shrinkers
> stand in the light of above wording? Kernel can chose to do almost
> anything and I do not think we want to control which shrinkers are
> triggered and what they do.
>
> So I would really prefer to say something like:
> "
> Please note that the proactive reclaim (triggered by this interface) is
> not meant to indicate memory pressure on the memory cgroup. Therefore
> socket memory balancing triggered by the memory reclaim normally is not
> exercised in this case. This means that the networking layer will not
> adapt based on reclaim induced by memory.reclaim.
> "

Sounds good to me! Will send v5 shortly with added doc changes and the
newly agreed upon commit log. Thanks Michal!

> --
> Michal Hocko
> SUSE Labs