| Message ID | 20220210081437.1884008-3-shakeelb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg: robust enforcement of memory.high | expand |
On Thu, Feb 10, 2022 at 12:14:35AM -0800, Shakeel Butt wrote: > Currently the kernel force charges the allocations which have __GFP_HIGH > flag without triggering the memory reclaim. __GFP_HIGH indicates that > the caller is high priority and since commit 869712fd3de5 ("mm: > memcontrol: fix network errors from failing __GFP_ATOMIC charges") the > kernel let such allocations do force charging. Please note that > __GFP_ATOMIC has been replaced by __GFP_HIGH. > > __GFP_HIGH does not tell if the caller can block or can trigger reclaim. > There are separate checks to determine that. So, there is no need to > skip reclaim for __GFP_HIGH allocations. So, handle __GFP_HIGH together > with __GFP_NOFAIL which also does force charging. This sounds very reasonable. But shouldn't we check if __GFP_DIRECT_RECLAIM is set and bail out otherwise? Thanks! > > Please note that this is a noop change as there are no __GFP_HIGH > allocators in kernel which also have __GFP_ACCOUNT (or SLAB_ACCOUNT) and > does not allow reclaim for now. The reason for this patch is to simplify > the reasoning of the following patches. > > Signed-off-by: Shakeel Butt <shakeelb@google.com> > --- > mm/memcontrol.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index c40c27822802..ae73a40818b0 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2560,15 +2560,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, > goto retry; > } > > - /* > - * Memcg doesn't have a dedicated reserve for atomic > - * allocations. But like the global atomic pool, we need to > - * put the burden of reclaim on regular allocation requests > - * and let these go through as privileged allocations. > - */ > - if (gfp_mask & __GFP_HIGH) > - goto force; > - > /* > * Prevent unbounded recursion when reclaim operations need to > * allocate memory. This might exceed the limits temporarily, > @@ -2642,7 +2633,13 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, > goto retry; > } > nomem: > - if (!(gfp_mask & __GFP_NOFAIL)) > + /* > + * Memcg doesn't have a dedicated reserve for atomic > + * allocations. But like the global atomic pool, we need to > + * put the burden of reclaim on regular allocation requests > + * and let these go through as privileged allocations. > + */ > + if (!(gfp_mask & (__GFP_NOFAIL | __GFP_HIGH))) > return -ENOMEM; > force: > /* > -- > 2.35.1.265.g69c8d7142f-goog >
On Thu, Feb 10, 2022 at 12:03 PM Roman Gushchin <guro@fb.com> wrote: > > On Thu, Feb 10, 2022 at 12:14:35AM -0800, Shakeel Butt wrote: > > Currently the kernel force charges the allocations which have __GFP_HIGH > > flag without triggering the memory reclaim. __GFP_HIGH indicates that > > the caller is high priority and since commit 869712fd3de5 ("mm: > > memcontrol: fix network errors from failing __GFP_ATOMIC charges") the > > kernel let such allocations do force charging. Please note that > > __GFP_ATOMIC has been replaced by __GFP_HIGH. > > > > __GFP_HIGH does not tell if the caller can block or can trigger reclaim. > > There are separate checks to determine that. So, there is no need to > > skip reclaim for __GFP_HIGH allocations. So, handle __GFP_HIGH together > > with __GFP_NOFAIL which also does force charging. > > This sounds very reasonable. But shouldn't we check if __GFP_DIRECT_RECLAIM > is set and bail out otherwise? > We already have a gfpflags_allow_blocking() check which checks for __GFP_DIRECT_RECLAIM.
On Thu, Feb 10, 2022 at 02:25:01PM -0800, Shakeel Butt wrote: > On Thu, Feb 10, 2022 at 12:03 PM Roman Gushchin <guro@fb.com> wrote: > > > > On Thu, Feb 10, 2022 at 12:14:35AM -0800, Shakeel Butt wrote: > > > Currently the kernel force charges the allocations which have __GFP_HIGH > > > flag without triggering the memory reclaim. __GFP_HIGH indicates that > > > the caller is high priority and since commit 869712fd3de5 ("mm: > > > memcontrol: fix network errors from failing __GFP_ATOMIC charges") the > > > kernel let such allocations do force charging. Please note that > > > __GFP_ATOMIC has been replaced by __GFP_HIGH. > > > > > > __GFP_HIGH does not tell if the caller can block or can trigger reclaim. > > > There are separate checks to determine that. So, there is no need to > > > skip reclaim for __GFP_HIGH allocations. So, handle __GFP_HIGH together > > > with __GFP_NOFAIL which also does force charging. > > > > This sounds very reasonable. But shouldn't we check if __GFP_DIRECT_RECLAIM > > is set and bail out otherwise? > > > > We already have a gfpflags_allow_blocking() check which checks for > __GFP_DIRECT_RECLAIM. Indeed. Please, feel free to add Reviewed-by: Roman Gushchin <guro@fb.com> to the patch. Thank you!
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index c40c27822802..ae73a40818b0 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2560,15 +2560,6 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, goto retry; } - /* - * Memcg doesn't have a dedicated reserve for atomic - * allocations. But like the global atomic pool, we need to - * put the burden of reclaim on regular allocation requests - * and let these go through as privileged allocations. - */ - if (gfp_mask & __GFP_HIGH) - goto force; - /* * Prevent unbounded recursion when reclaim operations need to * allocate memory. This might exceed the limits temporarily, @@ -2642,7 +2633,13 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask, goto retry; } nomem: - if (!(gfp_mask & __GFP_NOFAIL)) + /* + * Memcg doesn't have a dedicated reserve for atomic + * allocations. But like the global atomic pool, we need to + * put the burden of reclaim on regular allocation requests + * and let these go through as privileged allocations. + */ + if (!(gfp_mask & (__GFP_NOFAIL | __GFP_HIGH))) return -ENOMEM; force: /*
Currently the kernel force charges the allocations which have __GFP_HIGH flag without triggering the memory reclaim. __GFP_HIGH indicates that the caller is high priority and since commit 869712fd3de5 ("mm: memcontrol: fix network errors from failing __GFP_ATOMIC charges") the kernel let such allocations do force charging. Please note that __GFP_ATOMIC has been replaced by __GFP_HIGH. __GFP_HIGH does not tell if the caller can block or can trigger reclaim. There are separate checks to determine that. So, there is no need to skip reclaim for __GFP_HIGH allocations. So, handle __GFP_HIGH together with __GFP_NOFAIL which also does force charging. Please note that this is a noop change as there are no __GFP_HIGH allocators in kernel which also have __GFP_ACCOUNT (or SLAB_ACCOUNT) and does not allow reclaim for now. The reason for this patch is to simplify the reasoning of the following patches. Signed-off-by: Shakeel Butt <shakeelb@google.com> --- mm/memcontrol.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)