Date:   Thu, 21 Jul 2022 09:49:12 -0700
From:   Yosry Ahmed <yosryahmed@...gle.com>
To:     Michal Hocko <mhocko@...e.com>
Cc:     Shakeel Butt <shakeelb@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Muchun Song <songmuchun@...edance.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Matthew Wilcox <willy@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        David Hildenbrand <david@...hat.com>,
        Miaohe Lin <linmiaohe@...wei.com>, NeilBrown <neilb@...e.de>,
        Alistair Popple <apopple@...dia.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Peter Xu <peterx@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Cgroups <cgroups@...r.kernel.org>, Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH v4] mm: vmpressure: don't count proactive reclaim in vmpressure

On Thu, Jul 21, 2022 at 9:42 AM Michal Hocko <mhocko@...e.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@...e.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@...gle.com> wrote:
> > > > >
> > > > > On Wed, Jul 20, 2022 at 2:24 AM Michal Hocko <mhocko@...e.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

Powered by blists - more mailing lists