lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 Jan 2021 14:45:43 -0500 From: Johannes Weiner <hannes@...xchg.org> To: Roman Gushchin <guro@...com> Cc: Andrew Morton <akpm@...ux-foundation.org>, Tejun Heo <tj@...nel.org>, Michal Hocko <mhocko@...e.com>, linux-mm@...ck.org, cgroups@...r.kernel.org, linux-kernel@...r.kernel.org, kernel-team@...com Subject: Re: [PATCH] mm: memcontrol: prevent starvation when writing memory.high On Tue, Jan 12, 2021 at 09:03:22AM -0800, Roman Gushchin wrote: > On Tue, Jan 12, 2021 at 11:30:11AM -0500, Johannes Weiner wrote: > > When a value is written to a cgroup's memory.high control file, the > > write() context first tries to reclaim the cgroup to size before > > putting the limit in place for the workload. Concurrent charges from > > the workload can keep such a write() looping in reclaim indefinitely. > > > > In the past, a write to memory.high would first put the limit in place > > for the workload, then do targeted reclaim until the new limit has > > been met - similar to how we do it for memory.max. This wasn't prone > > to the described starvation issue. However, this sequence could cause > > excessive latencies in the workload, when allocating threads could be > > put into long penalty sleeps on the sudden memory.high overage created > > by the write(), before that had a chance to work it off. > > > > Now that memory_high_write() performs reclaim before enforcing the new > > limit, reflect that the cgroup may well fail to converge due to > > concurrent workload activity. Bail out of the loop after a few tries. > > > > Fixes: 536d3bf261a2 ("mm: memcontrol: avoid workload stalls when lowering memory.high") > > Cc: <stable@...r.kernel.org> # 5.8+ > > Reported-by: Tejun Heo <tj@...nel.org> > > Signed-off-by: Johannes Weiner <hannes@...xchg.org> > > --- > > mm/memcontrol.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 605f671203ef..63a8d47c1cd3 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -6275,7 +6275,6 @@ static ssize_t memory_high_write(struct kernfs_open_file *of, > > > > for (;;) { > > unsigned long nr_pages = page_counter_read(&memcg->memory); > > - unsigned long reclaimed; > > > > if (nr_pages <= high) > > break; > > @@ -6289,10 +6288,10 @@ static ssize_t memory_high_write(struct kernfs_open_file *of, > > continue; > > } > > > > - reclaimed = try_to_free_mem_cgroup_pages(memcg, nr_pages - high, > > - GFP_KERNEL, true); > > + try_to_free_mem_cgroup_pages(memcg, nr_pages - high, > > + GFP_KERNEL, true); > > > > - if (!reclaimed && !nr_retries--) > > + if (!nr_retries--) > > Shouldn't it be (!reclaimed || !nr_retries) instead? > > If reclaimed == 0, it probably doesn't make much sense to retry. We usually allow nr_retries worth of no-progress reclaim cycles to make up for intermittent reclaim failures. The difference to OOMs/memory.max is that we don't want to loop indefinitely on forward progress, but we should allow the usual number of no-progress loops.
Powered by blists - more mailing lists