From: Johannes Weiner <hannes@cmpxchg.org> To: "Michal Koutný" <mkoutny@suse.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Leon Yang <lnyng@fb.com>, Chris Down <chris@chrisdown.name>, Roman Gushchin <guro@fb.com>, Michal Hocko <mhocko@suse.com>, linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH] mm: memcontrol: fix occasional OOMs due to proportional memory.low reclaim Date: Mon, 23 Aug 2021 13:48:43 -0400 [thread overview] Message-ID: <YSPfe4yf2fRdzijh@cmpxchg.org> (raw) In-Reply-To: <YSPIOZOVG2qplLIW@blackbook> Hi Michal, On Mon, Aug 23, 2021 at 06:09:29PM +0200, Michal Koutný wrote: > Hello > > (and sorry for a belated reply). It's never too late, thanks for taking a look. > On Tue, Aug 17, 2021 at 02:05:06PM -0400, Johannes Weiner <hannes@cmpxchg.org> wrote: > > @@ -2576,6 +2578,15 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc, > > [...] > > + /* memory.low scaling, make sure we retry before OOM */ > > + if (!sc->memcg_low_reclaim && low > min) { > > + protection = low; > > + sc->memcg_low_skipped = 1; > > IIUC, this won't result in memory.events:low increment although the > effect is similar (breaching (partial) memory.low protection) and signal > to the user is comparable (overcommited memory.low). Good observation. I think you're right, we should probably count such partial breaches as LOW events as well. Note that this isn't new behavior. My patch merely moved this part from mem_cgroup_protection(): - if (in_low_reclaim) - return READ_ONCE(memcg->memory.emin); Even before, if we retried due to just one (possibly insignificant) cgroup below low, we'd ignore proportional reclaim and partially breach ALL protected cgroups, while only counting a low event for the one group that is usage < low. > Admittedly, this patch's behavior adheres to the current documentation > (Documentation/admin-guide/cgroup-v2.rst): > > > The number of times the cgroup is reclaimed due to high memory > > pressure even though its usage is under the low boundary, > > however, that definition might not be what the useful indicator would > be now. > Is it worth including these partial breaches into memory.events:low? I think it is. How about: "The number of times the cgroup's memory.low-protected memory was reclaimed in order to avoid OOM during high memory pressure." And adding a MEMCG_LOW event to partial breaches. BTW, the comment block above this code is also out-of-date, because it says we're honoring memory.low on the retries, but that's not the case. I'll prepare a follow-up patch for these 3 things as well as the more verbose comment that Michal Hocko asked for on the retry logic.
WARNING: multiple messages have this Message-ID (diff)
From: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> To: "Michal Koutný" <mkoutny-IBi9RG/b67k@public.gmane.org> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Leon Yang <lnyng-b10kYP2dOMg@public.gmane.org>, Chris Down <chris-6Bi1550iOqEnzZ6mRAm98g@public.gmane.org>, Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>, Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kernel-team-b10kYP2dOMg@public.gmane.org Subject: Re: [PATCH] mm: memcontrol: fix occasional OOMs due to proportional memory.low reclaim Date: Mon, 23 Aug 2021 13:48:43 -0400 [thread overview] Message-ID: <YSPfe4yf2fRdzijh@cmpxchg.org> (raw) In-Reply-To: <YSPIOZOVG2qplLIW@blackbook> Hi Michal, On Mon, Aug 23, 2021 at 06:09:29PM +0200, Michal Koutný wrote: > Hello > > (and sorry for a belated reply). It's never too late, thanks for taking a look. > On Tue, Aug 17, 2021 at 02:05:06PM -0400, Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> wrote: > > @@ -2576,6 +2578,15 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc, > > [...] > > + /* memory.low scaling, make sure we retry before OOM */ > > + if (!sc->memcg_low_reclaim && low > min) { > > + protection = low; > > + sc->memcg_low_skipped = 1; > > IIUC, this won't result in memory.events:low increment although the > effect is similar (breaching (partial) memory.low protection) and signal > to the user is comparable (overcommited memory.low). Good observation. I think you're right, we should probably count such partial breaches as LOW events as well. Note that this isn't new behavior. My patch merely moved this part from mem_cgroup_protection(): - if (in_low_reclaim) - return READ_ONCE(memcg->memory.emin); Even before, if we retried due to just one (possibly insignificant) cgroup below low, we'd ignore proportional reclaim and partially breach ALL protected cgroups, while only counting a low event for the one group that is usage < low. > Admittedly, this patch's behavior adheres to the current documentation > (Documentation/admin-guide/cgroup-v2.rst): > > > The number of times the cgroup is reclaimed due to high memory > > pressure even though its usage is under the low boundary, > > however, that definition might not be what the useful indicator would > be now. > Is it worth including these partial breaches into memory.events:low? I think it is. How about: "The number of times the cgroup's memory.low-protected memory was reclaimed in order to avoid OOM during high memory pressure." And adding a MEMCG_LOW event to partial breaches. BTW, the comment block above this code is also out-of-date, because it says we're honoring memory.low on the retries, but that's not the case. I'll prepare a follow-up patch for these 3 things as well as the more verbose comment that Michal Hocko asked for on the retry logic.
next prev parent reply other threads:[~2021-08-23 17:47 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-17 18:05 [PATCH] mm: memcontrol: fix occasional OOMs due to proportional memory.low reclaim Johannes Weiner 2021-08-17 18:05 ` Johannes Weiner 2021-08-17 18:44 ` Rik van Riel 2021-08-17 18:44 ` Rik van Riel 2021-08-17 19:10 ` Shakeel Butt 2021-08-17 19:10 ` Shakeel Butt 2021-08-17 19:10 ` Shakeel Butt 2021-08-18 14:16 ` Johannes Weiner 2021-08-18 14:16 ` Johannes Weiner 2021-08-17 19:14 ` Andrew Morton 2021-08-17 19:45 ` Roman Gushchin 2021-08-17 19:45 ` Roman Gushchin 2021-08-18 14:15 ` Johannes Weiner 2021-08-18 14:15 ` Johannes Weiner 2021-08-18 20:18 ` Chris Down 2021-08-18 20:18 ` Chris Down 2021-08-19 15:01 ` Michal Hocko 2021-08-19 20:38 ` Johannes Weiner 2021-08-20 15:44 ` Michal Hocko 2021-08-23 16:09 ` Michal Koutný 2021-08-23 16:09 ` Michal Koutný 2021-08-23 17:48 ` Johannes Weiner [this message] 2021-08-23 17:48 ` Johannes Weiner 2021-08-24 13:01 ` Michal Koutný 2021-08-24 13:01 ` Michal Koutný
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YSPfe4yf2fRdzijh@cmpxchg.org \ --to=hannes@cmpxchg.org \ --cc=akpm@linux-foundation.org \ --cc=cgroups@vger.kernel.org \ --cc=chris@chrisdown.name \ --cc=guro@fb.com \ --cc=kernel-team@fb.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=lnyng@fb.com \ --cc=mhocko@suse.com \ --cc=mkoutny@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.