Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Apr 2022 11:26:20 +0200
From:   Michal =?iso-8859-1?Q?Koutn=FD?= <mkoutny@suse.com>
To:     David Vernet <void@manifault.com>
Cc:     akpm@linux-foundation.org, tj@kernel.org, roman.gushchin@linux.dev,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        cgroups@vger.kernel.org, hannes@cmpxchg.org, mhocko@kernel.org,
        shakeelb@google.com, kernel-team@fb.com,
        Richard Palethorpe <rpalethorpe@suse.com>
Subject: Re: [PATCH v2 2/5] cgroup: Account for memory_recursiveprot in
 test_memcg_low()
Message-ID: <20220429092620.GA23621@blackbody.suse.cz>
References: <20220423155619.3669555-1-void@manifault.com>
 <20220423155619.3669555-3-void@manifault.com>
 <20220427140928.GD9823@blackbody.suse.cz>
 <20220429010333.5rt2jwpiumnbuapf@dev0025.ash9.facebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220429010333.5rt2jwpiumnbuapf@dev0025.ash9.facebook.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Thu, Apr 28, 2022 at 06:03:33PM -0700, David Vernet <void@manifault.com> wrote:
> but my interpretation of the rest of that discussion with Roman is
> that we haven't yet decided whether we don't want to propagate
> memory.low events from children cgroups with memory.low == 0. Or at
> the very least, some more justification was requested on why not
> counting such events was prudent.

I'm not a fan of that original proposal of mine anymore (to be more
precise, of _only_ that patch, there's still the RFCness reason 1) to
consider).
As I shared with the last reply there, there's a problem in the behavior
which shouldn't be masked by filtering some events.

> Would you be ok with merging this patch so that the cgroup selftests can
> pass again based on the current behavior of the kernel, and we can then
> revert the changes to test_memcg_low() later on if and when we decide that
> we don't want to propagate memory.low events for memory.low == 0 children?

I still think that the behavior when there's no protection left for the
memory.low == 0 child, there should be no memory.low events (not just
uncounted but not happening) and test should not accept this (even
though it's the current behavior).

What might improve the test space would be to have two configs like

Original one (simplified here)
	parent		memory.low=50M	memory.current=100M
	` child1	memory.low=50M	memory.current=50M
	` child2	memory.low=0M	memory.current=50M

New one (checks events due to recursive protection)
	parent		memory.low=50M	memory.current=100M
	` child1	memory.low=40M	memory.current=50M
	` child2	memory.low=0M	memory.current=50M

The second config assigns recursive protection to child2 and should
therefore cause memory.low events in child2 (with memory_recursiveprot
enabled of course).

Or alternative new one (checks events due to recursive protection)
	parent		memory.low=50M	memory.current=100M
	` child1	memory.low=0M	memory.current=50M
	` child2	memory.low=0M	memory.current=50M

HTH,
Michal