Messages in this thread | | | Date | Tue, 14 Sep 2021 11:31:17 +1000 | From | Dave Chinner <> | Subject | Re: [PATCH 5/6] XFS: remove congestion_wait() loop from kmem_alloc() |
| |
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote: > Documentation commment in gfp.h discourages indefinite retry loops on > ENOMEM and says of __GFP_NOFAIL that it > > is definitely preferable to use the flag rather than opencode > endless loop around allocator. > > So remove the loop, instead specifying __GFP_NOFAIL if KM_MAYFAIL was > not given. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/xfs/kmem.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c > index 6f49bf39183c..f545f3633f88 100644 > --- a/fs/xfs/kmem.c > +++ b/fs/xfs/kmem.c > @@ -13,19 +13,11 @@ kmem_alloc(size_t size, xfs_km_flags_t flags) > { > int retries = 0; > gfp_t lflags = kmem_flags_convert(flags); > - void *ptr; > > trace_kmem_alloc(size, flags, _RET_IP_); > > - do { > - ptr = kmalloc(size, lflags); > - if (ptr || (flags & KM_MAYFAIL)) > - return ptr; > - if (!(++retries % 100)) > - xfs_err(NULL, > - "%s(%u) possible memory allocation deadlock size %u in %s (mode:0x%x)", > - current->comm, current->pid, > - (unsigned int)size, __func__, lflags); > - congestion_wait(BLK_RW_ASYNC, HZ/50); > - } while (1); > + if (!(flags & KM_MAYFAIL)) > + lflags |= __GFP_NOFAIL; > + > + return kmalloc(size, lflags); > }
Which means we no longer get warnings about memory allocation failing - kmem_flags_convert() sets __GFP_NOWARN for all allocations in this loop. Hence we'll now get silent deadlocks through this code instead of getting warnings that memory allocation is failing repeatedly.
I also wonder about changing the backoff behaviour here (it's a 20ms wait right now because there are not early wakeups) will affect the behaviour, as __GFP_NOFAIL won't wait for that extra time between allocation attempts....
And, of course, how did you test this? Sometimes we see unpredicted behaviours as a result of "simple" changes like this under low memory conditions...
Cheers,
Dave. -- Dave Chinner david@fromorbit.com
| |