While working on an NFS issue recently I was informed (or maybe
"reminded") that congestion_wait() doesn't really do what we think it
does. It is indistinguishable from schedule_timeout_uninterruptible().
Some current users for congestion_wait() would be better suited by
__GFP_NOFAIL. In related discussions it was pointed out that the
__GFP_NOFAIL documentation could usefully clarify the costs of its use.
So this set of patch addresses some of these issues. The patches are
all independent and can safely be applied separately in different tress
as appropriate.
They:
- add or improve documentation relating to these issues
- make a tiny fix to the page_alloc_bulk_*
- replace those calls to congestion_wait() which are simply waiting
to retry a memory allocation.
These are the easy bits. There are 5 calls to congestion_wait() and one
to wait_iff_congested() in mm/ which need consideration. There are
multiple calls to congestion_wait in fs/, particularly fs/f2fs/ which
need to be addressed too. I'll try to form an opinion about these in
coming weeks.
Thanks,
NeilBrown
---
NeilBrown (6):
MM: improve documentation for __GFP_NOFAIL
MM: annotate congestion_wait() and wait_iff_congested() as ineffective.
EXT4: Remove ENOMEM/congestion_wait() loops.
EXT4: remove congestion_wait from ext4_bio_write_page, and simplify
XFS: remove congestion_wait() loop from kmem_alloc()
XFS: remove congestion_wait() loop from xfs_buf_alloc_pages()
fs/ext4/ext4.h | 2 +-
fs/ext4/ext4_jbd2.c | 8 +++++-
fs/ext4/extents.c | 49 ++++++++++++++-----------------------
fs/ext4/extents_status.c | 35 ++++++++++++++------------
fs/ext4/extents_status.h | 2 +-
fs/ext4/indirect.c | 2 +-
fs/ext4/inode.c | 6 ++---
fs/ext4/ioctl.c | 4 +--
fs/ext4/page-io.c | 13 ++++------
fs/ext4/super.c | 2 +-
fs/jbd2/transaction.c | 8 +++---
fs/xfs/kmem.c | 16 +++---------
fs/xfs/xfs_buf.c | 6 ++---
include/linux/backing-dev.h | 7 ++++++
mm/backing-dev.c | 9 +++++++
15 files changed, 86 insertions(+), 83 deletions(-)
--
Signature
__GFP_NOFAIL is documented both in gfp.h and memory-allocation.rst.
The details are not entirely consistent.
This patch ensures both places state that:
- there is a cost potentially imposed on other subsystems
- it should only be used when there is no real alternative
- it is preferable to an endless loop
- it is strongly discourages for costly-order allocations.
Signed-off-by: NeilBrown <redacted>
---
Documentation/core-api/memory-allocation.rst | 9 ++++++++-
include/linux/gfp.h | 4 ++++
2 files changed, 12 insertions(+), 1 deletion(-)
@@ -126,7 +126,14 @@ or another request.*``GFP_KERNEL | __GFP_NOFAIL`` - overrides the default allocator behavior and all allocation requests will loop endlessly until they succeed.- This might be really dangerous especially for larger orders.+ The allocator may provide access to memory that would otherwise be+ reserved in order to satisfy this allocation which might adversely+ affect other subsystems. So it should only be used when there is no+ reasonable failure policy and when the memory is likely to be freed+ again in the near future. Its use is strong discourage (via a+ WARN_ON) for allocations larger than ``PAGE_ALLOC_COSTLY_ORDER``.+ While this flag is best avoided, it is still preferable to endless+ loops around the allocator. Selecting memory allocator ==========================
Only 4 subsystems call set_bdi_congested() or clear_bdi_congested():
block/pktcdvd, fs/ceph fs/fuse fs/nfs
It may make sense to use congestion_wait() or wait_iff_congested()
within these subsystems, but they have no value outside of these.
Add documentation comments to these functions to discourage further use.
Signed-off-by: NeilBrown <redacted>
---
include/linux/backing-dev.h | 7 +++++++
mm/backing-dev.c | 9 +++++++++
2 files changed, 16 insertions(+)
Indefinite loops waiting for memory allocation are discouraged by
documentation in gfp.h which says the use of __GFP_NOFAIL that it
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
Such loops that use congestion_wait() are particularly unwise as
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice - and should be
deprecated.
So this patch changes the two loops in ext4_ext_truncate() to use
__GFP_NOFAIL instead of looping.
As the allocation is multiple layers deeper in the call stack, this
requires passing the EXT4_EX_NOFAIL flag down and handling it in various
places.
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
jbd2__journal_start() is enhanced so that the gfp_t flags passed are
used for *all* allocations.
Signed-off-by: NeilBrown <redacted>
---
fs/ext4/ext4.h | 2 +-
fs/ext4/ext4_jbd2.c | 8 +++++++-
fs/ext4/extents.c | 49 +++++++++++++++++-----------------------------
fs/ext4/extents_status.c | 35 +++++++++++++++++++--------------
fs/ext4/extents_status.h | 2 +-
fs/ext4/indirect.c | 2 +-
fs/ext4/inode.c | 6 +++---
fs/ext4/ioctl.c | 4 ++--
fs/ext4/super.c | 2 +-
fs/jbd2/transaction.c | 8 ++++----
10 files changed, 58 insertions(+), 60 deletions(-)
@@ -1488,7 +1488,7 @@ static int ext4_ext_search_left(struct inode *inode,staticintext4_ext_search_right(structinode*inode,structext4_ext_path*path,ext4_lblk_t*logical,ext4_fsblk_t*phys,-structext4_extent*ret_ex)+structext4_extent*ret_ex,intnofail){structbuffer_head*bh=NULL;structext4_extent_header*eh;
@@ -1565,7 +1565,7 @@ static int ext4_ext_search_right(struct inode *inode,while(++depth<path->p_depth){/* subtract from p_depth to get proper eh_depth */bh=read_extent_tree_block(inode,block,-path->p_depth-depth,0);+path->p_depth-depth,nofail);if(IS_ERR(bh))returnPTR_ERR(bh);eh=ext_block_hdr(bh);
@@ -1574,7 +1574,7 @@ static int ext4_ext_search_right(struct inode *inode,put_bh(bh);}-bh=read_extent_tree_block(inode,block,path->p_depth-depth,0);+bh=read_extent_tree_block(inode,block,path->p_depth-depth,nofail);if(IS_ERR(bh))returnPTR_ERR(bh);eh=ext_block_hdr(bh);
@@ -2789,7 +2789,8 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,ext_debug(inode,"truncate since %u to %u\n",start,end);/* probably first extent we're gonna free will be last in block */-handle=ext4_journal_start_with_revoke(inode,EXT4_HT_TRUNCATE,+handle=ext4_journal_start_with_revoke(inode,+EXT4_HT_TRUNCATE|nofail,depth+1,ext4_free_metadata_revoke_credits(inode->i_sb,depth));if(IS_ERR(handle))
@@ -2877,7 +2878,7 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,*/lblk=ex_end+1;err=ext4_ext_search_right(inode,path,&lblk,&pblk,-NULL);+NULL,nofail);if(err<0)gotoout;if(pblk){
@@ -2899,10 +2900,6 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,}else{path=kcalloc(depth+1,sizeof(structext4_ext_path),GFP_NOFS|__GFP_NOFAIL);-if(path==NULL){-ext4_journal_stop(handle);-return-ENOMEM;-}path[0].p_maxdepth=path[0].p_depth=depth;path[0].p_hdr=ext_inode_hdr(inode);i=0;
@@ -2955,7 +2952,7 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,memset(path+i+1,0,sizeof(*path));bh=read_extent_tree_block(inode,ext4_idx_pblock(path[i].p_idx),depth-i-1,-EXT4_EX_NOCACHE);+EXT4_EX_NOCACHE|nofail);if(IS_ERR(bh)){/* should we reset i_size? */err=PTR_ERR(bh);
@@ -4186,7 +4183,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,if(err)gotoout;ar.lright=map->m_lblk;-err=ext4_ext_search_right(inode,path,&ar.lright,&ar.pright,&ex2);+err=ext4_ext_search_right(inode,path,&ar.lright,&ar.pright,&ex2,0);if(err<0)gotoout;
@@ -4368,23 +4365,13 @@ int ext4_ext_truncate(handle_t *handle, struct inode *inode)last_block=(inode->i_size+sb->s_blocksize-1)>>EXT4_BLOCK_SIZE_BITS(sb);-retry:err=ext4_es_remove_extent(inode,last_block,-EXT_MAX_BLOCKS-last_block);-if(err==-ENOMEM){-cond_resched();-congestion_wait(BLK_RW_ASYNC,HZ/50);-gotoretry;-}+EXT_MAX_BLOCKS-last_block,+EXT4_EX_NOFAIL);if(err)returnerr;-retry_remove_space:-err=ext4_ext_remove_space(inode,last_block,EXT_MAX_BLOCKS-1);-if(err==-ENOMEM){-cond_resched();-congestion_wait(BLK_RW_ASYNC,HZ/50);-gotoretry_remove_space;-}+err=ext4_ext_remove_space(inode,last_block,EXT_MAX_BLOCKS-1,+EXT4_EX_NOFAIL);returnerr;}
@@ -476,9 +476,9 @@ static int start_this_handle(journal_t *journal, handle_t *handle,}/* Allocate a new handle. This should probably be in a slab... */-statichandle_t*new_handle(intnblocks)+statichandle_t*new_handle(intnblocks,gfp_tgfp){-handle_t*handle=jbd2_alloc_handle(GFP_NOFS);+handle_t*handle=jbd2_alloc_handle(gfp);if(!handle)returnNULL;handle->h_total_credits=nblocks;
@@ -505,13 +505,13 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int rsv_blocks,nblocks+=DIV_ROUND_UP(revoke_records,journal->j_revoke_records_per_block);-handle=new_handle(nblocks);+handle=new_handle(nblocks,gfp_mask);if(!handle)returnERR_PTR(-ENOMEM);if(rsv_blocks){handle_t*rsv_handle;-rsv_handle=new_handle(rsv_blocks);+rsv_handle=new_handle(rsv_blocks,gfp_mask);if(!rsv_handle){jbd2_free_handle(handle);returnERR_PTR(-ENOMEM);
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible(). It is best avoided and should be
deprecated.
It is not needed in ext4_bio_write_page(). There are two cases.
If there are no ->io_bio yet, then it is appropriate to use __GFP_NOFAIL
which does the waiting in a better place. The code already uses this
flag on the second attempt. This patch changes to it always use that
flag for this case.
If there *are* ->io_bio (in which case the allocation was non-blocking)
we submit the io and return the first case. No waiting is needed in
this case.
So remove the congestion_wait() call, and simplify the code so that the
two cases are somewhat clearer.
Remove the "if (io->io_bio)" before calling ext4_io_submit() as that
test is performed internally by that function.
Signed-off-by: NeilBrown <redacted>
---
fs/ext4/page-io.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
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 <redacted>
---
fs/xfs/kmem.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
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.
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice and it is not a good way
to wait for memory to become available.
So instead of waiting, allocate a single page using __GFP_NOFAIL, then
loop around and try to get any more pages that might be needed with a
bulk allocation. This single-page allocation will wait in the most
appropriate way.
Signed-off-by: NeilBrown <redacted>
---
fs/xfs/xfs_buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-14 01:31:26
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted hunk
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 <redacted>
---
fs/xfs/kmem.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
@@ -13,19 +13,11 @@ kmem_alloc(size_t size, xfs_km_flags_t flags){intretries=0;gfp_tlflags=kmem_flags_convert(flags);-void*ptr;trace_kmem_alloc(size,flags,_RET_IP_);-do{-ptr=kmalloc(size,lflags);-if(ptr||(flags&KM_MAYFAIL))-returnptr;-if(!(++retries%100))-xfs_err(NULL,-"%s(%u) possible memory allocation deadlock size %u in %s (mode:0x%x)",-current->comm,current->pid,-(unsignedint)size,__func__,lflags);-congestion_wait(BLK_RW_ASYNC,HZ/50);-}while(1);+if(!(flags&KM_MAYFAIL))+lflags|=__GFP_NOFAIL;++returnkmalloc(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
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-14 02:08:47
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted hunk
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.
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice and it is not a good way
to wait for memory to become available.
So instead of waiting, allocate a single page using __GFP_NOFAIL, then
loop around and try to get any more pages that might be needed with a
bulk allocation. This single-page allocation will wait in the most
appropriate way.
Signed-off-by: NeilBrown <redacted>
---
fs/xfs/xfs_buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
This smells wrong - the whole point of using the bulk page allocator
in this loop is to avoid the costly individual calls to
alloc_page().
What we are implementing here fail-fast semantics for readahead and
fail-never for everything else. If the bulk allocator fails to get
a page from the fast path free lists, it already falls back to
__alloc_pages(gfp, 0, ...) to allocate a single page. So AFAICT
there's no need to add another call to alloc_page() because we can
just do this instead:
if (flags & XBF_READ_AHEAD)
gfp_mask |= __GFP_NORETRY;
else
- gfp_mask |= GFP_NOFS;
+ gfp_mask |= GFP_NOFS | __GFP_NOFAIL;
Which should make the __alloc_pages() call in
alloc_pages_bulk_array() do a __GFP_NOFAIL allocation and hence
provide the necessary never-fail guarantee that is needed here.
At which point, the bulk allocation loop can be simplified because
we can only fail bulk allocation for readahead, so something like:
if (filled == bp->b_page_count) {
XFS_STATS_INC(bp->b_mount, xb_page_found);
break;
}
- if (filled != last)
+ if (filled == last) {
- continue;
-
- if (flags & XBF_READ_AHEAD) {
ASSERT(flags & XBF_READ_AHEAD);
xfs_buf_free_pages(bp);
return -ENOMEM;
}
XFS_STATS_INC(bp->b_mount, xb_page_retries);
- congestion_wait(BLK_RW_ASYNC, HZ / 50);
}
return 0;
}
would do the right thing and still record that we are doing
blocking allocations (via the xb_page_retries stat) in this loop.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
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.
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice and it is not a good way
to wait for memory to become available.
So instead of waiting, allocate a single page using __GFP_NOFAIL, then
loop around and try to get any more pages that might be needed with a
bulk allocation. This single-page allocation will wait in the most
appropriate way.
Signed-off-by: NeilBrown <redacted>
---
fs/xfs/xfs_buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
This smells wrong - the whole point of using the bulk page allocator
in this loop is to avoid the costly individual calls to
alloc_page().
What we are implementing here fail-fast semantics for readahead and
fail-never for everything else. If the bulk allocator fails to get
a page from the fast path free lists, it already falls back to
__alloc_pages(gfp, 0, ...) to allocate a single page. So AFAICT
there's no need to add another call to alloc_page() because we can
just do this instead:
if (flags & XBF_READ_AHEAD)
gfp_mask |= __GFP_NORETRY;
else
- gfp_mask |= GFP_NOFS;
+ gfp_mask |= GFP_NOFS | __GFP_NOFAIL;
Which should make the __alloc_pages() call in
alloc_pages_bulk_array() do a __GFP_NOFAIL allocation and hence
provide the necessary never-fail guarantee that is needed here.
That is a nice simplification.
Mel Gorman told me
https://lore.kernel.org/linux-nfs/20210907153116.GJ3828@suse.com/
that alloc_pages_bulk ignores GFP_NOFAIL. I added that to the
documentation comment in an earlier patch.
I had a look at the code and cannot see how it would fail to allocate at
least one page. Maybe Mel can help....
NeilBrown
At which point, the bulk allocation loop can be simplified because
we can only fail bulk allocation for readahead, so something like:
if (filled == bp->b_page_count) {
XFS_STATS_INC(bp->b_mount, xb_page_found);
break;
}
- if (filled != last)
+ if (filled == last) {
- continue;
-
- if (flags & XBF_READ_AHEAD) {
ASSERT(flags & XBF_READ_AHEAD);
xfs_buf_free_pages(bp);
return -ENOMEM;
}
XFS_STATS_INC(bp->b_mount, xb_page_retries);
- congestion_wait(BLK_RW_ASYNC, HZ / 50);
}
return 0;
}
would do the right thing and still record that we are doing
blocking allocations (via the xb_page_retries stat) in this loop.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
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 <redacted>
---
fs/xfs/kmem.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
@@ -13,19 +13,11 @@ kmem_alloc(size_t size, xfs_km_flags_t flags){intretries=0;gfp_tlflags=kmem_flags_convert(flags);-void*ptr;trace_kmem_alloc(size,flags,_RET_IP_);-do{-ptr=kmalloc(size,lflags);-if(ptr||(flags&KM_MAYFAIL))-returnptr;-if(!(++retries%100))-xfs_err(NULL,-"%s(%u) possible memory allocation deadlock size %u in %s (mode:0x%x)",-current->comm,current->pid,-(unsignedint)size,__func__,lflags);-congestion_wait(BLK_RW_ASYNC,HZ/50);-}while(1);+if(!(flags&KM_MAYFAIL))+lflags|=__GFP_NOFAIL;++returnkmalloc(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.
Yes, that is a problem. Could we just clear __GFP_NOWARN when setting
__GFP_NOFAIL?
Or is the 1-in-100 important? I think default warning is 1 every 10
seconds.
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....
The internal backoff is 100ms if there is much pending writeout, and
there are 16 internal retries. If there is not much pending writeout, I
think it just loops with cond_resched().
So adding 20ms can only be at all interesting when the only way to
reclaim memory is something other than writeout. I don't know how to
think about that.
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...
I suspect this is close to untestable. While I accept that there might
be a scenario where the change might cause some macro effect, it would
most likely be some interplay with some other subsystem struggling with
memory. Testing XFS by itself would be unlikely to find it.
Thanks,
NeilBrown
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-14 05:33:09
On Tue, Sep 14, 2021 at 12:35:59PM +1000, NeilBrown wrote:
On Tue, 14 Sep 2021, Dave Chinner wrote:
quoted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
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.
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice and it is not a good way
to wait for memory to become available.
So instead of waiting, allocate a single page using __GFP_NOFAIL, then
loop around and try to get any more pages that might be needed with a
bulk allocation. This single-page allocation will wait in the most
appropriate way.
Signed-off-by: NeilBrown <redacted>
---
fs/xfs/xfs_buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
This smells wrong - the whole point of using the bulk page allocator
in this loop is to avoid the costly individual calls to
alloc_page().
What we are implementing here fail-fast semantics for readahead and
fail-never for everything else. If the bulk allocator fails to get
a page from the fast path free lists, it already falls back to
__alloc_pages(gfp, 0, ...) to allocate a single page. So AFAICT
there's no need to add another call to alloc_page() because we can
just do this instead:
if (flags & XBF_READ_AHEAD)
gfp_mask |= __GFP_NORETRY;
else
- gfp_mask |= GFP_NOFS;
+ gfp_mask |= GFP_NOFS | __GFP_NOFAIL;
Which should make the __alloc_pages() call in
alloc_pages_bulk_array() do a __GFP_NOFAIL allocation and hence
provide the necessary never-fail guarantee that is needed here.
Well, that's a surprise to me - I can't see where it masked out
NOFAIL, and it seems quite arbitrary to just say "different code
needs different fallbacks, so you can't have NOFAIL" despite NOFAIL
being the exact behavioural semantics one of only three users of the
bulk allocator really needs...
I had a look at the code and cannot see how it would fail to allocate at
least one page. Maybe Mel can help....
Yup, clarification is definitely needed here.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-14 06:05:30
On Tue, Sep 14, 2021 at 01:27:31PM +1000, NeilBrown wrote:
On Tue, 14 Sep 2021, Dave Chinner wrote:
quoted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
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 <redacted>
---
fs/xfs/kmem.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
@@ -13,19 +13,11 @@ kmem_alloc(size_t size, xfs_km_flags_t flags){intretries=0;gfp_tlflags=kmem_flags_convert(flags);-void*ptr;trace_kmem_alloc(size,flags,_RET_IP_);-do{-ptr=kmalloc(size,lflags);-if(ptr||(flags&KM_MAYFAIL))-returnptr;-if(!(++retries%100))-xfs_err(NULL,-"%s(%u) possible memory allocation deadlock size %u in %s (mode:0x%x)",-current->comm,current->pid,-(unsignedint)size,__func__,lflags);-congestion_wait(BLK_RW_ASYNC,HZ/50);-}while(1);+if(!(flags&KM_MAYFAIL))+lflags|=__GFP_NOFAIL;++returnkmalloc(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.
Yes, that is a problem. Could we just clear __GFP_NOWARN when setting
__GFP_NOFAIL?
Probably.
Or is the 1-in-100 important? I think default warning is 1 every 10
seconds.
1-in-100 is an arbitrary number to prevent spamming of logs unless
there is a real likelihood of there being a memory allocation
deadlock. We've typically only ever seen this when trying to do
high-order allocations (e.g. 64kB for xattr buffers) and failing
repeatedly in extreme memory pressure events. It's a canary that we
leave in the logs so that when a user reports problems we know that
they've been running under extended extreme low memory conditions
and can adjust the triage process accordingly.
So, we could remove __GFP_NOWARN, as long as the core allocator code
has sufficient rate limiting that it won't spam the logs due to
extended failure looping...
quoted
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....
The internal backoff is 100ms if there is much pending writeout, and
there are 16 internal retries. If there is not much pending writeout, I
think it just loops with cond_resched().
So adding 20ms can only be at all interesting when the only way to
reclaim memory is something other than writeout. I don't know how to
think about that.
Any cache that uses a shrinker to reclaim (e.g. dentry, inodes, fs
metadata, etc due to recursive directory traversals) can cause
reclaim looping and priority escalation without there being any page
cache writeback or reclaim possible. Especially when you have
GFP_NOFS allocation context and all your memory is in VFS level
caches. At that point, direct reclaim cannot (and will not) make
forwards progress, so we still have to wait for some other
GFP_KERNEL context reclaim (e.g. kswapd) to make progress reclaiming
memory while we wait.
Fundamentally, the memory reclaim backoff code doesn't play well
with shrinkers. Patches from an old patchset which pushed lack of
shrinker progress back up into the vmscan level backoff algorithms
was something I was experimenting with a few years ago. e.g.
https://lore.kernel.org/linux-xfs/20191031234618.15403-16-david@fromorbit.com/https://lore.kernel.org/linux-xfs/20191031234618.15403-17-david@fromorbit.com/
We didn't end up going this way to solve the XFS inode reclaim
problems - I ended up solving that entirely by pinning XFS buffer
cache memory and modifying the XFS inode shrinker - but it was this
patchset that first exposed the fact that congestion_wait() was no
longer functioning as intended. See the last few paragraphs of the
(long) cover letter for v1 of that patchset here:
https://lore.kernel.org/linux-xfs/20190801021752.4986-1-david@fromorbit.com/
So, yeah, I know full well that congestion_wait() is mostly just
an unconditional timeout these days...
quoted
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...
I suspect this is close to untestable. While I accept that there might
be a scenario where the change might cause some macro effect, it would
most likely be some interplay with some other subsystem struggling with
memory. Testing XFS by itself would be unlikely to find it.
Filesystem traversal workloads (e.g. chown -R) are the ones that
hammer memory allocation from GFP_NOFS context which creates memory
pressure that cannot be balanced by direct reclaim as direct reclaim
cannot reclaim filesystem caches in this situation. This is where I
would expect extra backoff on failing GFP_NOFS allocations to have
some effect...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
Indefinite loops waiting for memory allocation are discouraged by
documentation in gfp.h which says the use of __GFP_NOFAIL that it
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
Such loops that use congestion_wait() are particularly unwise as
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice - and should be
deprecated.
So this patch changes the two loops in ext4_ext_truncate() to use
__GFP_NOFAIL instead of looping.
As the allocation is multiple layers deeper in the call stack, this
requires passing the EXT4_EX_NOFAIL flag down and handling it in various
places.
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
jbd2__journal_start() is enhanced so that the gfp_t flags passed are
used for *all* allocations.
Signed-off-by: NeilBrown <redacted>
I'm not a fan. GFP_NOFAIL allows access to emergency reserves increasing
the risk of a livelock if memory is completely depleted where as some
callers can afford to wait.
The key event should be reclaim making progress. The hack below is
intended to vaguely demonstrate how blocking can be based on reclaim
making progress instead of "congestion" but has not even been booted. A
more complete overhaul may involve introducing
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout, nodemask_t *nodemask)
and
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout)
and converting congestion_wait and wait_iff_congestion to calling
reclaim_congestion_wait_nodemask which waits on the first usable node
and then audit every single congestion_wait() user to see which API
they should call. Further work would be to establish whether the page allocator should
call reclaim_congestion_wait_nodemask() if direct reclaim is not making
progress or whether that should be in vmscan.c. Conceivably, GFP_NOFAIL
could then soften its access to emergency reserves but I haven't given
it much thought.
Yes it's significant work, but it would be a better than letting
__GFP_NOFAIL propagate further and kicking us down the road.
This hack is terrible, it's not the right way to do it, it's just to
illustrate the idea of "waiting on memory should be based on reclaim
making progress and not the state of storage" is not impossible.
--8<--
@@ -832,6 +832,7 @@ typedef struct pglist_data {unsignedlongnode_spanned_pages;/* total size of physical pagerange,includingholes*/intnode_id;+wait_queue_head_treclaim_wait;wait_queue_head_tkswapd_wait;wait_queue_head_tpfmemalloc_wait;structtask_struct*kswapd;/* Protected by
@@ -1013,25 +1014,41 @@ void set_bdi_congested(struct backing_dev_info *bdi, int sync)EXPORT_SYMBOL(set_bdi_congested);/**-*congestion_wait-waitforabacking_devtobecomeuncongested-*@sync:SYNCorASYNCIO-*@timeout:timeoutinjiffies+*congestion_wait-thedocsarenowworthlessbutavoidingarename*-*Waitsforupto@timeoutjiffiesforabacking_dev(anybacking_dev)toexit-*writecongestion.Ifnobacking_devsarecongestedthenjustwaitforthe-*nextwritetobecompleted.+*Newthing--waitforatimeoutorreclaimtomakeprogress*/longcongestion_wait(intsync,longtimeout){+pg_data_t*pgdat;longret;unsignedlongstart=jiffies;DEFINE_WAIT(wait);-wait_queue_head_t*wqh=&congestion_wqh[sync];+wait_queue_head_t*wqh;-prepare_to_wait(wqh,&wait,TASK_UNINTERRUPTIBLE);-ret=io_schedule_timeout(timeout);+/* Never let kswapd sleep on itself */+if(current_is_kswapd())+gototrace;++/*+*Dangerous,localmemorymaybeforbiddenbycpusetorpolicies,+*usefirsteligiblezoneinzonelistsnodeinstead+*/+preempt_disable();+pgdat=NODE_DATA(smp_processor_id());+preempt_enable();+wqh=&pgdat->reclaim_wait;++/*+*Shouldprobablycheckwatermarkofsuitablezoneshere+*incasethisisspuriouslycalled+*/++prepare_to_wait(wqh,&wait,TASK_INTERRUPTIBLE);+ret=schedule_timeout(timeout);finish_wait(wqh,&wait);+trace:trace_writeback_congestion_wait(jiffies_to_usecs(timeout),jiffies_to_usecs(jiffies-start));
On Tue, Sep 14, 2021 at 12:35:59PM +1000, NeilBrown wrote:
On Tue, 14 Sep 2021, Dave Chinner wrote:
quoted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
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.
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice and it is not a good way
to wait for memory to become available.
So instead of waiting, allocate a single page using __GFP_NOFAIL, then
loop around and try to get any more pages that might be needed with a
bulk allocation. This single-page allocation will wait in the most
appropriate way.
Signed-off-by: NeilBrown <redacted>
---
fs/xfs/xfs_buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
This smells wrong - the whole point of using the bulk page allocator
in this loop is to avoid the costly individual calls to
alloc_page().
What we are implementing here fail-fast semantics for readahead and
fail-never for everything else. If the bulk allocator fails to get
a page from the fast path free lists, it already falls back to
__alloc_pages(gfp, 0, ...) to allocate a single page. So AFAICT
there's no need to add another call to alloc_page() because we can
just do this instead:
if (flags & XBF_READ_AHEAD)
gfp_mask |= __GFP_NORETRY;
else
- gfp_mask |= GFP_NOFS;
+ gfp_mask |= GFP_NOFS | __GFP_NOFAIL;
Which should make the __alloc_pages() call in
alloc_pages_bulk_array() do a __GFP_NOFAIL allocation and hence
provide the necessary never-fail guarantee that is needed here.
That is a nice simplification.
Mel Gorman told me
https://lore.kernel.org/linux-nfs/20210907153116.GJ3828@suse.com/
that alloc_pages_bulk ignores GFP_NOFAIL. I added that to the
documentation comment in an earlier patch.
I had a look at the code and cannot see how it would fail to allocate at
least one page. Maybe Mel can help....
If there are already at least one page an the array and the first attempt
at bulk allocation fails, it'll simply return. It's an odd corner case
that may never apply but it's possible. That said, I'm of the opinion that
__GFP_NOFAIL should not be expanded and instead congestion_wait should be
deleted and replaced with something triggered by reclaim making progress.
--
Mel Gorman
SUSE Labs
On Tue, Sep 14, 2021 at 12:35:59PM +1000, NeilBrown wrote:
quoted
On Tue, 14 Sep 2021, Dave Chinner wrote:
quoted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
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.
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice and it is not a good way
to wait for memory to become available.
So instead of waiting, allocate a single page using __GFP_NOFAIL, then
loop around and try to get any more pages that might be needed with a
bulk allocation. This single-page allocation will wait in the most
appropriate way.
Signed-off-by: NeilBrown <redacted>
---
fs/xfs/xfs_buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
This smells wrong - the whole point of using the bulk page allocator
in this loop is to avoid the costly individual calls to
alloc_page().
What we are implementing here fail-fast semantics for readahead and
fail-never for everything else. If the bulk allocator fails to get
a page from the fast path free lists, it already falls back to
__alloc_pages(gfp, 0, ...) to allocate a single page. So AFAICT
there's no need to add another call to alloc_page() because we can
just do this instead:
if (flags & XBF_READ_AHEAD)
gfp_mask |= __GFP_NORETRY;
else
- gfp_mask |= GFP_NOFS;
+ gfp_mask |= GFP_NOFS | __GFP_NOFAIL;
Which should make the __alloc_pages() call in
alloc_pages_bulk_array() do a __GFP_NOFAIL allocation and hence
provide the necessary never-fail guarantee that is needed here.
That is a nice simplification.
Mel Gorman told me
https://lore.kernel.org/linux-nfs/20210907153116.GJ3828@suse.com/
that alloc_pages_bulk ignores GFP_NOFAIL. I added that to the
documentation comment in an earlier patch.
I had a look at the code and cannot see how it would fail to allocate at
least one page. Maybe Mel can help....
If there are already at least one page an the array and the first attempt
at bulk allocation fails, it'll simply return. It's an odd corner case
that may never apply but it's possible. That said, I'm of the opinion that
__GFP_NOFAIL should not be expanded and instead congestion_wait should be
deleted and replaced with something triggered by reclaim making progress.
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
Indefinite loops waiting for memory allocation are discouraged by
documentation in gfp.h which says the use of __GFP_NOFAIL that it
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
Such loops that use congestion_wait() are particularly unwise as
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice - and should be
deprecated.
So this patch changes the two loops in ext4_ext_truncate() to use
__GFP_NOFAIL instead of looping.
As the allocation is multiple layers deeper in the call stack, this
requires passing the EXT4_EX_NOFAIL flag down and handling it in various
places.
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
jbd2__journal_start() is enhanced so that the gfp_t flags passed are
used for *all* allocations.
Signed-off-by: NeilBrown <redacted>
I'm not a fan. GFP_NOFAIL allows access to emergency reserves increasing
the risk of a livelock if memory is completely depleted where as some
callers can afford to wait.
Maybe we should wind back and focus on the documentation patches.
As quoted above, mm.h says:
quoted
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
but you seem to be saying that is wrong. I'd certainly like to get the
documentation right before changing any code.
Why does __GFP_NOFAIL access the reserves? Why not require that the
relevant "Try harder" flag (__GFP_ATOMIC or __GFP_MEMALLOC) be included
with __GFP_NOFAIL if that is justified?
There are over 100 __GFP_NOFAIL allocation sites. I don't feel like
reviewing them all and seeing if any really need a try-harder flag.
Can we rename __GFP_NOFAIL to __GFP_NEVERFAIL and then
#define __GFP_NOFAIL (__GFP_NEVERFAIL | __GFP_ATOMIC)
and encourage the use of __GFP_NEVERFAIL in future?
When __GFP_NOFAIL loops, it calls congestion_wait() internally. That
certainly needs to be fixed and the ideas you present below are
certainly worth considering when trying to understand how to address
that. I'd rather fix it once there in page_alloc.c rather then export a
waiting API like congestion_wait(). That would provide more
flexibility. e.g. a newly freed page could be handed directly back to
the waiter.
Thanks,
NeilBrown
quoted hunk
The key event should be reclaim making progress. The hack below is
intended to vaguely demonstrate how blocking can be based on reclaim
making progress instead of "congestion" but has not even been booted. A
more complete overhaul may involve introducing
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout, nodemask_t *nodemask)
and
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout)
and converting congestion_wait and wait_iff_congestion to calling
reclaim_congestion_wait_nodemask which waits on the first usable node
and then audit every single congestion_wait() user to see which API
they should call. Further work would be to establish whether the page allocator should
call reclaim_congestion_wait_nodemask() if direct reclaim is not making
progress or whether that should be in vmscan.c. Conceivably, GFP_NOFAIL
could then soften its access to emergency reserves but I haven't given
it much thought.
Yes it's significant work, but it would be a better than letting
__GFP_NOFAIL propagate further and kicking us down the road.
This hack is terrible, it's not the right way to do it, it's just to
illustrate the idea of "waiting on memory should be based on reclaim
making progress and not the state of storage" is not impossible.
--8<--
@@ -832,6 +832,7 @@ typedef struct pglist_data {unsignedlongnode_spanned_pages;/* total size of physical pagerange,includingholes*/intnode_id;+wait_queue_head_treclaim_wait;wait_queue_head_tkswapd_wait;wait_queue_head_tpfmemalloc_wait;structtask_struct*kswapd;/* Protected by
@@ -1013,25 +1014,41 @@ void set_bdi_congested(struct backing_dev_info *bdi, int sync)EXPORT_SYMBOL(set_bdi_congested);/**-*congestion_wait-waitforabacking_devtobecomeuncongested-*@sync:SYNCorASYNCIO-*@timeout:timeoutinjiffies+*congestion_wait-thedocsarenowworthlessbutavoidingarename*-*Waitsforupto@timeoutjiffiesforabacking_dev(anybacking_dev)toexit-*writecongestion.Ifnobacking_devsarecongestedthenjustwaitforthe-*nextwritetobecompleted.+*Newthing--waitforatimeoutorreclaimtomakeprogress*/longcongestion_wait(intsync,longtimeout){+pg_data_t*pgdat;longret;unsignedlongstart=jiffies;DEFINE_WAIT(wait);-wait_queue_head_t*wqh=&congestion_wqh[sync];+wait_queue_head_t*wqh;-prepare_to_wait(wqh,&wait,TASK_UNINTERRUPTIBLE);-ret=io_schedule_timeout(timeout);+/* Never let kswapd sleep on itself */+if(current_is_kswapd())+gototrace;++/*+*Dangerous,localmemorymaybeforbiddenbycpusetorpolicies,+*usefirsteligiblezoneinzonelistsnodeinstead+*/+preempt_disable();+pgdat=NODE_DATA(smp_processor_id());+preempt_enable();+wqh=&pgdat->reclaim_wait;++/*+*Shouldprobablycheckwatermarkofsuitablezoneshere+*incasethisisspuriouslycalled+*/++prepare_to_wait(wqh,&wait,TASK_INTERRUPTIBLE);+ret=schedule_timeout(timeout);finish_wait(wqh,&wait);+trace:trace_writeback_congestion_wait(jiffies_to_usecs(timeout),jiffies_to_usecs(jiffies-start));
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-14 23:55:43
On Tue, Sep 14, 2021 at 05:34:32PM +0100, Mel Gorman wrote:
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
Indefinite loops waiting for memory allocation are discouraged by
documentation in gfp.h which says the use of __GFP_NOFAIL that it
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
Such loops that use congestion_wait() are particularly unwise as
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice - and should be
deprecated.
So this patch changes the two loops in ext4_ext_truncate() to use
__GFP_NOFAIL instead of looping.
As the allocation is multiple layers deeper in the call stack, this
requires passing the EXT4_EX_NOFAIL flag down and handling it in various
places.
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
jbd2__journal_start() is enhanced so that the gfp_t flags passed are
used for *all* allocations.
Signed-off-by: NeilBrown <redacted>
I'm not a fan. GFP_NOFAIL allows access to emergency reserves increasing
the risk of a livelock if memory is completely depleted where as some
callers can afford to wait.
Undocumented behaviour, never mentioned or communicated to users in
any __GFP_NOFAIL discussion I've taken part in until now.
How is it different to, say, GFP_ATOMIC? i.e. Does GFP_NOFAIL
actually imply GFP_ATOMIC, or is there some other undocumented
behaviour going on here?
We've already go ~80 __GFP_NOFAIL allocation contexts in fs/ and the
vast majority of the are GFP_KERNEL | __GFP_NOFAIL or GFP_NOFS |
__GFP_NOFAIL, so some clarification on what this actually means
would be really good...
The key event should be reclaim making progress.
Yup, that's what we need, but I don't see why it needs to be exposed
outside the allocation code at all.
The hack below is
intended to vaguely demonstrate how blocking can be based on reclaim
making progress instead of "congestion" but has not even been booted. A
more complete overhaul may involve introducing
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout, nodemask_t *nodemask)
and
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout)
I think that's racy. There's no guarantee that the node we are
currently running on matches the cpu/node id that we failed to
allocate from. Pre-emptible kernels and all that. IOWs, I think
needs to be completely internal to the reclaim infrastructure and
based on the current context we are trying to reclaim from.
That way "GFP_RETRY_FOREVER" allocation contexts don't have to jump
through an ever changing tangle of hoops to make basic "never-fail"
allocation semantics behave correctly.
and converting congestion_wait and wait_iff_congestion to calling
reclaim_congestion_wait_nodemask which waits on the first usable node
and then audit every single congestion_wait() user to see which API
they should call. Further work would be to establish whether the page allocator should
call reclaim_congestion_wait_nodemask() if direct reclaim is not making
progress or whether that should be in vmscan.c. Conceivably, GFP_NOFAIL
could then soften its access to emergency reserves but I haven't given
it much thought.
Yes it's significant work, but it would be a better than letting
__GFP_NOFAIL propagate further and kicking us down the road.
Unfortunately, that seems to ignore the fact that we still need
never-fail allocation semantics for stable system performance. Like
it or not the requirements for __GFP_NOFAIL (and "retry forever"
equivalent semantics) or open coded endless retry loops
are *never* going away.
IOWs, I'd suggest that we should think about how to formally
support "never-fail" allocation semantics in both the API and the
implementation in such a way that we don't end up with this
__GFP_NOFAIL catch-22 ever again. Having the memory reclaim code
wait on forwards progress instead of congestion as you propose here
would be a core part of providing "never-fail" allocations...
This hack is terrible, it's not the right way to do it, it's just to
illustrate the idea of "waiting on memory should be based on reclaim
making progress and not the state of storage" is not impossible.
I've been saying that is how reclaim should work for years. :/
It was LFSMM 2013 or 2014 that I was advocating for memory reclaim
to move to IO-less reclaim throttling based on the rate at which
free pages are returned to the freelists similar to the way IO-less
dirty page throttling is based on the rate dirty pages are cleaned.
Relying on IO interactions (submitting IO or waiting for completion)
for high level page state management has always been a bad way to
throttle demand because it only provides indirect control and has
poor feedback indication.
It's also a good way to remove the dependency on direct reclaim -
just sleep instead of duplicating the work that kswapd should
already be doing in the background to reclaim pages...
@@ -832,6 +832,7 @@ typedef struct pglist_data {unsignedlongnode_spanned_pages;/* total size of physical pagerange,includingholes*/intnode_id;+wait_queue_head_treclaim_wait;wait_queue_head_tkswapd_wait;wait_queue_head_tpfmemalloc_wait;structtask_struct*kswapd;/* Protected by
@@ -1013,25 +1014,41 @@ void set_bdi_congested(struct backing_dev_info *bdi, int sync)EXPORT_SYMBOL(set_bdi_congested);/**-*congestion_wait-waitforabacking_devtobecomeuncongested-*@sync:SYNCorASYNCIO-*@timeout:timeoutinjiffies+*congestion_wait-thedocsarenowworthlessbutavoidingarename*-*Waitsforupto@timeoutjiffiesforabacking_dev(anybacking_dev)toexit-*writecongestion.Ifnobacking_devsarecongestedthenjustwaitforthe-*nextwritetobecompleted.+*Newthing--waitforatimeoutorreclaimtomakeprogress*/longcongestion_wait(intsync,longtimeout){+pg_data_t*pgdat;longret;unsignedlongstart=jiffies;DEFINE_WAIT(wait);-wait_queue_head_t*wqh=&congestion_wqh[sync];+wait_queue_head_t*wqh;-prepare_to_wait(wqh,&wait,TASK_UNINTERRUPTIBLE);-ret=io_schedule_timeout(timeout);+/* Never let kswapd sleep on itself */+if(current_is_kswapd())+gototrace;
I think this breaks the kswapd 100ms immediate reclaim backoff in
shrink_node().
+
+ /*
+ * Dangerous, local memory may be forbidden by cpuset or policies,
+ * use first eligible zone in zonelists node instead
+ */
+ preempt_disable();
+ pgdat = NODE_DATA(smp_processor_id());
+ preempt_enable();
+ wqh = &pgdat->reclaim_wait;
This goes away if it is kept internal and is passed the reclaim
pgdat context we just failed to reclaim pages from.
+
+ /*
+ * Should probably check watermark of suitable zones here
+ * in case this is spuriously called
+ */
Ditto.
These hacks really make me think that an external "wait for memory
reclaim to make progress before retrying allocation" behaviour is
the wrong way to tackle this. It's always been a hack because
open-coded retry loops had to be implemented everywhere for
never-fail allocation semantics.
Neil has the right idea by replacing such fail-never back-offs with
actual allocation attempts that encapsulate waiting for reclaim to
make progress. This needs to be a formally supported function of
memory allocation, and then these backoffs can be properly
integrated into the memory reclaim retry mechanism instead of being
poorly grafted onto the side...
Whether that be __GFP_NOFAIL or GFP_RETRY_FOREVER that doesn't have
the "dip into reserves" behaviour of __GFP_NOFAIL (which we clearly
don't need because open coded retry loops have clearly work well
enough for production systems for many years), I don't really care.
But I think the memory allocation subsystem needs to move beyond
"ahhhh, never-fail is too hard!!!!" and take steps to integrate this
behaviour properly so that it can be made to work a whole lot better
than it currently does....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
I'm really not fond of this type blurring. What I'd suggeset doing
instead is adding a "gfp_t gfp_mask" parameter to the
__ext4_journal_start_sb(). With the exception of one call site in
fs/ext4/ialloc.c, most of the callers of __ext4_journal_start_sb() are
via #define helper macros or inline funcions. So it would just
require adding a GFP_NOFS as an extra parameter to the various macros
and inline functions which call __ext4_journal_start_sb() in
ext4_jbd2.h.
The function ext4_journal_start_with_revoke() is called exactly once
so we could just bury the __GFP_NOFAIL in the definition of that
macros, e.g.:
#define ext4_journal_start_with_revoke(inode, type, blocks, revoke_creds) \
__ext4_journal_start((inode), __LINE__, (type), (blocks), 0, \
GFP_NOFS | __GFP_NOFAIL, (revoke_creds))
but it's probably better to do something like this:
#define ext4_journal_start_with_revoke(gfp_mask, inode, type, blocks, revoke_creds) \
__ext4_journal_start((inode), __LINE__, (type), (blocks), 0, \
gfp_mask, (revoke_creds))
So it's explicit in the C function ext4_ext_remove_space() in
fs/ext4/extents.c that we are explicitly requesting the __GFP_NOFAIL
behavior.
Does that make sense?
- Ted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
I'm really not fond of this type blurring. What I'd suggeset doing
instead is adding a "gfp_t gfp_mask" parameter to the
__ext4_journal_start_sb(). With the exception of one call site in
fs/ext4/ialloc.c, most of the callers of __ext4_journal_start_sb() are
via #define helper macros or inline funcions. So it would just
require adding a GFP_NOFS as an extra parameter to the various macros
and inline functions which call __ext4_journal_start_sb() in
ext4_jbd2.h.
The function ext4_journal_start_with_revoke() is called exactly once
so we could just bury the __GFP_NOFAIL in the definition of that
macros, e.g.:
#define ext4_journal_start_with_revoke(inode, type, blocks, revoke_creds) \
__ext4_journal_start((inode), __LINE__, (type), (blocks), 0, \
GFP_NOFS | __GFP_NOFAIL, (revoke_creds))
but it's probably better to do something like this:
#define ext4_journal_start_with_revoke(gfp_mask, inode, type, blocks, revoke_creds) \
__ext4_journal_start((inode), __LINE__, (type), (blocks), 0, \
gfp_mask, (revoke_creds))
So it's explicit in the C function ext4_ext_remove_space() in
fs/ext4/extents.c that we are explicitly requesting the __GFP_NOFAIL
behavior.
Does that make sense?
Mostly.
Adding gfp_mask to __ext4_journal_start_sb() make perfect sense.
There doesn't seem much point adding one to __ext4_journal_start(),
we can have ext4_journal_start_with_revoke() call
__ext4_journal_start_sb() directly.
But I cannot see what it doesn't already do that.
i.e. why have the inline __ext4_journal_start() at all?
Is it OK if I don't use that for ext4_journal_start_with_revoke()?
Thanks,
NeilBrown
On Wed, Sep 15, 2021 at 09:55:35AM +1000, Dave Chinner wrote:
On Tue, Sep 14, 2021 at 05:34:32PM +0100, Mel Gorman wrote:
quoted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
Indefinite loops waiting for memory allocation are discouraged by
documentation in gfp.h which says the use of __GFP_NOFAIL that it
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
Such loops that use congestion_wait() are particularly unwise as
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice - and should be
deprecated.
So this patch changes the two loops in ext4_ext_truncate() to use
__GFP_NOFAIL instead of looping.
As the allocation is multiple layers deeper in the call stack, this
requires passing the EXT4_EX_NOFAIL flag down and handling it in various
places.
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
jbd2__journal_start() is enhanced so that the gfp_t flags passed are
used for *all* allocations.
Signed-off-by: NeilBrown <redacted>
I'm not a fan. GFP_NOFAIL allows access to emergency reserves increasing
the risk of a livelock if memory is completely depleted where as some
callers can afford to wait.
Undocumented behaviour, never mentioned or communicated to users in
any __GFP_NOFAIL discussion I've taken part in until now.
How is it different to, say, GFP_ATOMIC? i.e. Does GFP_NOFAIL
actually imply GFP_ATOMIC, or is there some other undocumented
behaviour going on here?
Hmm, it's similar but not the same as GFP_ATOMIC. The most severe aspect
of depleting emergency reserves comes from this block which is relevant
when the system is effectively OOM
/*
* XXX: GFP_NOFS allocations should rather fail than rely on
* other request to make a forward progress.
* We are in an unfortunate situation where out_of_memory cannot
* do much for this context but let's try it to at least get
* access to memory reserved if the current task is killed (see
* out_of_memory). Once filesystems are ready to handle allocation
* failures more gracefully we should just bail out here.
*/
/* Exhausted what can be done so it's blame time */
if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) {
*did_some_progress = 1;
/*
* Help non-failing allocations by giving them access to memory
* reserves
*/
if (gfp_mask & __GFP_NOFAIL)
page = __alloc_pages_cpuset_fallback(gfp_mask, order,
ALLOC_NO_WATERMARKS, ac);
}
The less severe aspect comes from
/*
* Help non-failing allocations by giving them access to memory
* reserves but do not use ALLOC_NO_WATERMARKS because this
* could deplete whole memory reserves which would just make
* the situation worse
*/
page = __alloc_pages_cpuset_fallback(gfp_mask, order, ALLOC_HARDER, ac);
if (page)
goto got_pg;
This doesn't dip into reserves as much as an atomic allocation does but
it competes with them.
We've already go ~80 __GFP_NOFAIL allocation contexts in fs/ and the
vast majority of the are GFP_KERNEL | __GFP_NOFAIL or GFP_NOFS |
__GFP_NOFAIL, so some clarification on what this actually means
would be really good...
I'm not sure how much clarity can be given. Whatever the documented
semantics, at some point under the current implementation __GFP_NOFAIL
potentially competes with the same reserves as GFP_ATOMIC and has a path
where watermarks are ignored entirely.
quoted
The key event should be reclaim making progress.
Yup, that's what we need, but I don't see why it needs to be exposed
outside the allocation code at all.
Probably not. At least some of it could be contained within reclaim
itself to block when reclaim is not making progress as opposed to anything
congestion related. That might still livelock if no progress can be made
but that's not new, the OOM hammer should eventually kick in.
quoted
The hack below is
intended to vaguely demonstrate how blocking can be based on reclaim
making progress instead of "congestion" but has not even been booted. A
more complete overhaul may involve introducing
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout, nodemask_t *nodemask)
and
reclaim_congestion_wait_nodemask(gfp_t gfp_mask, long timeout)
I think that's racy. There's no guarantee that the node we are
currently running on matches the cpu/node id that we failed to
allocate from.
I know, I commented
+ /*
+ * Dangerous, local memory may be forbidden by cpuset or policies,
+ * use first eligible zone in zonelists node instead
+ */
There may be multiple nodes "we failed to allocate from", but the first
eligible node is definitely one of them. There is the possibility that
the first eligible node may be completely unreclaimable (all anonymous,
no swap) in which case the timeout kicks in. I don't think this should
be a global workqueue because there will be spurious wakeups.
Pre-emptible kernels and all that. IOWs, I think
needs to be completely internal to the reclaim infrastructure and
based on the current context we are trying to reclaim from.
A further step could be something similar to capture_control
whereby reclaimed pages are immediately assigned to tasks blocked on
reclaim_congestion_wait. It may be excessively complicated and overkill.
That way "GFP_RETRY_FOREVER" allocation contexts don't have to jump
through an ever changing tangle of hoops to make basic "never-fail"
allocation semantics behave correctly.
True and I can see what that is desirable. What I'm saying is that right
now, increasing the use of __GFP_NOFAIL may cause a different set of
problems (unbounded retries combined with ATOMIC allocation failures) as
they compete for similar resources.
quoted
and converting congestion_wait and wait_iff_congestion to calling
reclaim_congestion_wait_nodemask which waits on the first usable node
and then audit every single congestion_wait() user to see which API
they should call. Further work would be to establish whether the page allocator should
call reclaim_congestion_wait_nodemask() if direct reclaim is not making
progress or whether that should be in vmscan.c. Conceivably, GFP_NOFAIL
could then soften its access to emergency reserves but I haven't given
it much thought.
Yes it's significant work, but it would be a better than letting
__GFP_NOFAIL propagate further and kicking us down the road.
Unfortunately, that seems to ignore the fact that we still need
never-fail allocation semantics for stable system performance. Like
it or not the requirements for __GFP_NOFAIL (and "retry forever"
equivalent semantics) or open coded endless retry loops
are *never* going away.
I'm aware there will be cases where never-fail allocation semantics are
required, particularly in GFP_NOFS contexts. What I'm saying is that right
now because throttling is based on imaginary "congestion" that increasing
the use could result in live-lock like bugs when multiple users complete
for similar emergency resources to atomic. Note that I didn't NACK this.
IOWs, I'd suggest that we should think about how to formally
support "never-fail" allocation semantics in both the API and the
implementation in such a way that we don't end up with this
__GFP_NOFAIL catch-22 ever again. Having the memory reclaim code
wait on forwards progress instead of congestion as you propose here
would be a core part of providing "never-fail" allocations...
quoted
This hack is terrible, it's not the right way to do it, it's just to
illustrate the idea of "waiting on memory should be based on reclaim
making progress and not the state of storage" is not impossible.
I've been saying that is how reclaim should work for years. :/
It was LFSMM 2013 or 2014 that I was advocating for memory reclaim
to move to IO-less reclaim throttling based on the rate at which
free pages are returned to the freelists similar to the way IO-less
dirty page throttling is based on the rate dirty pages are cleaned.
I'm going to guess no one ever tried.
Relying on IO interactions (submitting IO or waiting for completion)
for high level page state management has always been a bad way to
throttle demand because it only provides indirect control and has
poor feedback indication.
Also true.
It's also a good way to remove the dependency on direct reclaim -
just sleep instead of duplicating the work that kswapd should
already be doing in the background to reclaim pages...
Even for direct reclaim, I do think that the number of direct reclaimers
should be limited with the rest going to sleep. At some point, excessive
direct reclaim tasks are simply hammering the lru lock.
@@ -832,6 +832,7 @@ typedef struct pglist_data {unsignedlongnode_spanned_pages;/* total size of physical pagerange,includingholes*/intnode_id;+wait_queue_head_treclaim_wait;wait_queue_head_tkswapd_wait;wait_queue_head_tpfmemalloc_wait;structtask_struct*kswapd;/* Protected by
@@ -1013,25 +1014,41 @@ void set_bdi_congested(struct backing_dev_info *bdi, int sync)EXPORT_SYMBOL(set_bdi_congested);/**-*congestion_wait-waitforabacking_devtobecomeuncongested-*@sync:SYNCorASYNCIO-*@timeout:timeoutinjiffies+*congestion_wait-thedocsarenowworthlessbutavoidingarename*-*Waitsforupto@timeoutjiffiesforabacking_dev(anybacking_dev)toexit-*writecongestion.Ifnobacking_devsarecongestedthenjustwaitforthe-*nextwritetobecompleted.+*Newthing--waitforatimeoutorreclaimtomakeprogress*/longcongestion_wait(intsync,longtimeout){+pg_data_t*pgdat;longret;unsignedlongstart=jiffies;DEFINE_WAIT(wait);-wait_queue_head_t*wqh=&congestion_wqh[sync];+wait_queue_head_t*wqh;-prepare_to_wait(wqh,&wait,TASK_UNINTERRUPTIBLE);-ret=io_schedule_timeout(timeout);+/* Never let kswapd sleep on itself */+if(current_is_kswapd())+gototrace;
I think this breaks the kswapd 100ms immediate reclaim backoff in
shrink_node().
Yep, it is. That would definitely need better care.
quoted
+
+ /*
+ * Dangerous, local memory may be forbidden by cpuset or policies,
+ * use first eligible zone in zonelists node instead
+ */
+ preempt_disable();
+ pgdat = NODE_DATA(smp_processor_id());
+ preempt_enable();
+ wqh = &pgdat->reclaim_wait;
This goes away if it is kept internal and is passed the reclaim
pgdat context we just failed to reclaim pages from.
Yep, that would also work if this was called only from reclaim contexts
or mm internally. Some helper would still be needed to implement an
alternative congestion_wait that looks up the same information until
congestion_wait callers can be removed.
Again, I wasn't trying to offer a correct implementation, only illustrating
that it's perfectly possible to throttle based on reclaim making progress
instead of "congestion".
quoted
+
+ /*
+ * Should probably check watermark of suitable zones here
+ * in case this is spuriously called
+ */
Ditto.
These hacks really make me think that an external "wait for memory
reclaim to make progress before retrying allocation" behaviour is
the wrong way to tackle this. It's always been a hack because
open-coded retry loops had to be implemented everywhere for
never-fail allocation semantics.
Neil has the right idea by replacing such fail-never back-offs with
actual allocation attempts that encapsulate waiting for reclaim to
make progress. This needs to be a formally supported function of
memory allocation, and then these backoffs can be properly
integrated into the memory reclaim retry mechanism instead of being
poorly grafted onto the side...
I'm not necessarily opposed to this. What I'm saying is that doing the
conversion now *MIGHT* mean an increase in live-lock-like bugs because
with the current implementation, the callers may not sleep/throttle in
the same way the crappy "loop around congestion_wait" implementations did.
Whether that be __GFP_NOFAIL or GFP_RETRY_FOREVER that doesn't have
the "dip into reserves" behaviour of __GFP_NOFAIL (which we clearly
don't need because open coded retry loops have clearly work well
enough for production systems for many years), I don't really care.
I suspected this was true and that it might be appropriate for __GFP_NOFAIL
to obey normal watermarks unless __GFP_HIGH is also specified if it's
absolutely necessary but I'm not sure because I haven't put enough thought
into it.
But I think the memory allocation subsystem needs to move beyond
"ahhhh, never-fail is too hard!!!!" and take steps to integrate this
behaviour properly so that it can be made to work a whole lot better
than it currently does....
Again, not opposed. It's simply a heads-up that converting now may cause
problems that manifest as livelock-like bugs unless, at minimum, internal
reclaim bases throttling on some reclaim making progress instead of
congestion_wait. Given my current load, I can't promise I'd find the time
to follow through with converting the hack into a proper implementation
but someone reading linux-mm might. Either way, I felt it was necessary
to at least warn about the hazards.
--
Mel Gorman
SUSE Labs
From: Michal Hocko <mhocko@suse.com> Date: 2021-09-15 11:51:59
On Tue 14-09-21 10:13:04, Neil Brown wrote:
__GFP_NOFAIL is documented both in gfp.h and memory-allocation.rst.
The details are not entirely consistent.
This patch ensures both places state that:
- there is a cost potentially imposed on other subsystems
- it should only be used when there is no real alternative
- it is preferable to an endless loop
- it is strongly discourages for costly-order allocations.
Yes this is a useful addition to the documentation. Thanks!
@@ -126,7 +126,14 @@ or another request.*``GFP_KERNEL | __GFP_NOFAIL`` - overrides the default allocator behavior and all allocation requests will loop endlessly until they succeed.- This might be really dangerous especially for larger orders.+ The allocator may provide access to memory that would otherwise be+ reserved in order to satisfy this allocation which might adversely+ affect other subsystems. So it should only be used when there is no+ reasonable failure policy and when the memory is likely to be freed+ again in the near future. Its use is strong discourage (via a+ WARN_ON) for allocations larger than ``PAGE_ALLOC_COSTLY_ORDER``.+ While this flag is best avoided, it is still preferable to endless+ loops around the allocator. Selecting memory allocator ==========================
From: Michal Hocko <mhocko@suse.com> Date: 2021-09-15 11:56:43
On Tue 14-09-21 10:13:04, Neil Brown wrote:
Only 4 subsystems call set_bdi_congested() or clear_bdi_congested():
block/pktcdvd, fs/ceph fs/fuse fs/nfs
It may make sense to use congestion_wait() or wait_iff_congested()
within these subsystems, but they have no value outside of these.
Add documentation comments to these functions to discourage further use.
This is an unfortunate state. The MM layer still relies on the API.
While adding a documentation to clarify the current status can stop more
usage I am wondering what is a real alternative. My experience tells me
that a lack of real alternative will lead to new creative ways of doing
things instead.
From: Michal Hocko <mhocko@suse.com> Date: 2021-09-15 12:06:54
On Wed 15-09-21 07:48:11, Neil Brown wrote:
On Wed, 15 Sep 2021, Mel Gorman wrote:
quoted
On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote:
quoted
Indefinite loops waiting for memory allocation are discouraged by
documentation in gfp.h which says the use of __GFP_NOFAIL that it
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
Such loops that use congestion_wait() are particularly unwise as
congestion_wait() is indistinguishable from
schedule_timeout_uninterruptible() in practice - and should be
deprecated.
So this patch changes the two loops in ext4_ext_truncate() to use
__GFP_NOFAIL instead of looping.
As the allocation is multiple layers deeper in the call stack, this
requires passing the EXT4_EX_NOFAIL flag down and handling it in various
places.
Of particular interest is the ext4_journal_start family of calls which
can now have EXT4_EX_NOFAIL 'or'ed in to the 'type'. This could be seen
as a blurring of types. However 'type' is 8 bits, and EXT4_EX_NOFAIL is
a high bit, so it is safe in practice.
jbd2__journal_start() is enhanced so that the gfp_t flags passed are
used for *all* allocations.
Signed-off-by: NeilBrown <redacted>
I'm not a fan. GFP_NOFAIL allows access to emergency reserves increasing
the risk of a livelock if memory is completely depleted where as some
callers can afford to wait.
Maybe we should wind back and focus on the documentation patches.
As quoted above, mm.h says:
quoted
quoted
is definitely preferable to use the flag rather than opencode endless
loop around allocator.
but you seem to be saying that is wrong. I'd certainly like to get the
documentation right before changing any code.
Why does __GFP_NOFAIL access the reserves? Why not require that the
relevant "Try harder" flag (__GFP_ATOMIC or __GFP_MEMALLOC) be included
with __GFP_NOFAIL if that is justified?
Does 5020e285856c ("mm, oom: give __GFP_NOFAIL allocations access to
memory reserves") help?
I would be worried to make the semantic even more complex than already
is. Access to memory reserves is an implementation detail that the page
allocator does currently. Callers shouldn't really be worried about
that. I do not ever remember any actual NOFAIL triggered memory
exhaustion. I have seen that to happen for unrestricted access to memory
reserves by OOM victim though. Hence cd04ae1e2dc8 ("mm, oom: do not rely
on TIF_MEMDIE for memory reserves access"). We can consider something
similar if NOFAIL allocation really tend to show a similar problem. We
do not want callers to care about OOM sitauations for this kind of
requests.
__GFP_NOFAIL | __GFP_HIGH is certainly something that is a valid usage
but I wouldn't base OOM behavior based on that.
There are over 100 __GFP_NOFAIL allocation sites. I don't feel like
reviewing them all and seeing if any really need a try-harder flag.
Can we rename __GFP_NOFAIL to __GFP_NEVERFAIL and then
#define __GFP_NOFAIL (__GFP_NEVERFAIL | __GFP_ATOMIC)
and encourage the use of __GFP_NEVERFAIL in future?
Doesn't this add even more complexity?
When __GFP_NOFAIL loops, it calls congestion_wait() internally. That
certainly needs to be fixed and the ideas you present below are
certainly worth considering when trying to understand how to address
that. I'd rather fix it once there in page_alloc.c rather then export a
waiting API like congestion_wait(). That would provide more
flexibility. e.g. a newly freed page could be handed directly back to
the waiter.
Completely agreed here. We really do not want people to open code NOFAIL
unless they can do something really subsystem specific that would help
to make a forward progress.
--
Michal Hocko
SUSE Labs
From: Michal Hocko <mhocko@suse.com> Date: 2021-09-15 12:20:15
On Wed 15-09-21 09:59:04, Mel Gorman wrote:
On Wed, Sep 15, 2021 at 09:55:35AM +1000, Dave Chinner wrote:
quoted
That way "GFP_RETRY_FOREVER" allocation contexts don't have to jump
through an ever changing tangle of hoops to make basic "never-fail"
allocation semantics behave correctly.
True and I can see what that is desirable. What I'm saying is that right
now, increasing the use of __GFP_NOFAIL may cause a different set of
problems (unbounded retries combined with ATOMIC allocation failures) as
they compete for similar resources.
I have commented on reasoning behind the above code in other reply. Let
me just comment on this particular concern. I completely do agree that
any use of __GFP_NOFAIL should be carefully evaluated. This is a very
strong recuirement and it should be used only as a last resort.
On the other hand converting an existing open coded nofail code that
_doesn't_ really do any clever tricks to allow a forward progress (e.g.
dropping locks, kicking some internal caching mechinisms etc.) should
just be turned into __GPF_NOFAIL. Not only it makes it easier to spot
that code but it also allows the page allocator to behave consistently
and predictably.
If the existing heuristic wrt. memory reserves to GFP_NOFAIL turns out
to be suboptimal we can fix it for all those users.
Dropping the rest of the email which talks about reclaim changes because
I will need much more time to digest that.
[...]
--
Michal Hocko
SUSE Labs
On Wed, Sep 15, 2021 at 09:59:04AM +0100, Mel Gorman wrote:
quoted
Yup, that's what we need, but I don't see why it needs to be exposed
outside the allocation code at all.
Probably not. At least some of it could be contained within reclaim
itself to block when reclaim is not making progress as opposed to anything
congestion related. That might still livelock if no progress can be made
but that's not new, the OOM hammer should eventually kick in.
There are two sides to the reclaim-related throttling
1. throttling because zero progress is being made
2. throttling because there are too many dirty pages or pages under
writeback cycling through the LRU too quickly.
The dirty page aspects (and the removal of wait_iff_congested which is
almost completely broken) could be done with something like the following
(completly untested). The downside is that end_page_writeback() takes an
atomic penalty if reclaim is throttled but at that point the system is
struggling anyway so I doubt it matters.
--8<--
@@ -841,6 +841,9 @@ typedef struct pglist_data {intnode_id;wait_queue_head_tkswapd_wait;wait_queue_head_tpfmemalloc_wait;+wait_queue_head_treclaim_wait;/* wq for throttling reclaim */+atomic_tnr_reclaim_throttled;/* nr of throtted tasks */+atomic_tnr_reclaim_written;/* nr pages written since throttled */structtask_struct*kswapd;/* Protected bymem_hotplug_begin/end()*/intkswapd_order;
@@ -1041,51 +1041,3 @@ long congestion_wait(int sync, long timeout)returnret;}EXPORT_SYMBOL(congestion_wait);--/**-*wait_iff_congested-Conditionallywaitforabacking_devtobecomeuncongestedorapgdattocompletewrites-*@sync:SYNCorASYNCIO-*@timeout:timeoutinjiffies-*-*Intheeventofacongestedbacking_dev(anybacking_dev)thiswaits-*forupto@timeoutjiffiesforeitheraBDItoexitcongestionofthe-*given@syncqueueorawritetocomplete.-*-*Thereturnvalueis0ifthesleepisforthefulltimeout.Otherwise,-*itisthenumberofjiffiesthatwerestillremainingwhenthefunction-*returned.return_value==timeoutimpliesthefunctiondidnotsleep.-*/-longwait_iff_congested(intsync,longtimeout)-{-longret;-unsignedlongstart=jiffies;-DEFINE_WAIT(wait);-wait_queue_head_t*wqh=&congestion_wqh[sync];--/*-*Ifthereisnocongestion,yieldifnecessaryinstead-*ofsleepingonthecongestionqueue-*/-if(atomic_read(&nr_wb_congested[sync])==0){-cond_resched();--/* In case we scheduled, work out time remaining */-ret=timeout-(jiffies-start);-if(ret<0)-ret=0;--gotoout;-}--/* Sleep until uncongested or a write happens */-prepare_to_wait(wqh,&wait,TASK_UNINTERRUPTIBLE);-ret=io_schedule_timeout(timeout);-finish_wait(wqh,&wait);--out:-trace_writeback_wait_iff_congested(jiffies_to_usecs(timeout),-jiffies_to_usecs(jiffies-start));--returnret;-}-EXPORT_SYMBOL(wait_iff_congested);
On Wed, Sep 15, 2021 at 03:25:40PM +1000, NeilBrown wrote:
Adding gfp_mask to __ext4_journal_start_sb() make perfect sense.
There doesn't seem much point adding one to __ext4_journal_start(),
we can have ext4_journal_start_with_revoke() call
__ext4_journal_start_sb() directly.
But I cannot see what it doesn't already do that.
i.e. why have the inline __ext4_journal_start() at all?
Is it OK if I don't use that for ext4_journal_start_with_revoke()?
Sure. I think the only reason why we have __ext4_journal_start() as
an inline function at all was for historical reasons. That is, we
modified __ext4_journal_start() so that it took a struct super, and
instead of changing all of the macros which called
__ext4_journal_start(), we named it to be __ext4_journal_start_sb()
and added the inline definition of __ext4_journal_start() to avoid
changing all of the existing users of __ext4_journal_start().
So sure, it's fine not to use that for
ext4_journal_start_with_revoke(), and we probably should clean up the
use of __ext4_journal_start() at some point. That's unrelated to your
work, though.
Cheers,
- Ted
Why does __GFP_NOFAIL access the reserves? Why not require that the
relevant "Try harder" flag (__GFP_ATOMIC or __GFP_MEMALLOC) be included
with __GFP_NOFAIL if that is justified?
Does 5020e285856c ("mm, oom: give __GFP_NOFAIL allocations access to
memory reserves") help?
Yes, that helps. A bit.
I'm not fond of the clause "the allocation request might have come with some
locks held". What if it doesn't? Does it still have to pay the price.
Should we not require that the caller indicate if any locks are held?
That way callers which don't hold locks can use __GFP_NOFAIL without
worrying about imposing on other code.
Or is it so rare that __GFP_NOFAIL would be used without holding a lock
that it doesn't matter?
The other commit of interest is
Commit: 6c18ba7a1899 ("mm: help __GFP_NOFAIL allocations which do not trigger OOM killer")
I don't find the reasoning convincing. It is a bit like "Robbing Peter
to pay Paul". It takes from the reserves to allow a __GFP_NOFAIL to
proceed, with out any reason to think this particular allocation has any
more 'right' to the reserves than anything else.
While I don't like the reasoning in either of these, they do make it
clear (to me) that the use of reserves is entirely an internal policy
decision. They should *not* be seen as part of the API and callers
should not have to be concerned about it when deciding whether to use
__GFP_NOFAIL or not.
The use of these reserves is, at most, a hypothetical problem. If it
ever looks like becoming a real practical problem, it needs to be fixed
internally to the page allocator. Maybe an extra water-mark which isn't
quite as permissive as ALLOC_HIGH...
I'm inclined to drop all references to reserves from the documentation
for __GFP_NOFAIL. I think there are enough users already that adding a
couple more isn't going to make problems substantially more likely. And
more will be added anyway that the mm/ team won't have the opportunity
or bandwidth to review.
Meanwhile I'll see if I can understand the intricacies of alloc_page so
that I can contibute to making it more predictable.
Question: In those cases where an open-coded loop is appropriate, such
as when you want to handle signals or can drop locks, how bad would it
be to have a tight loop without any sleep?
should_reclaim_retry() will sleep 100ms (sometimes...). Is that enough?
__GFP_NOFAIL doesn't add any sleep when looping.
Thanks,
NeilBrown
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-15 22:39:10
On Wed, Sep 15, 2021 at 03:35:10PM +0100, Mel Gorman wrote:
On Wed, Sep 15, 2021 at 09:59:04AM +0100, Mel Gorman wrote:
quoted
quoted
Yup, that's what we need, but I don't see why it needs to be exposed
outside the allocation code at all.
Probably not. At least some of it could be contained within reclaim
itself to block when reclaim is not making progress as opposed to anything
congestion related. That might still livelock if no progress can be made
but that's not new, the OOM hammer should eventually kick in.
There are two sides to the reclaim-related throttling
1. throttling because zero progress is being made
2. throttling because there are too many dirty pages or pages under
writeback cycling through the LRU too quickly.
The dirty page aspects (and the removal of wait_iff_congested which is
almost completely broken) could be done with something like the following
(completly untested). The downside is that end_page_writeback() takes an
atomic penalty if reclaim is throttled but at that point the system is
struggling anyway so I doubt it matters.
The atomics are pretty nasty, as is directly accessing the pgdat on
every call to end_page_writeback(). Those will be performance
limiting factors. Indeed, we don't use atomics for dirty page
throttling, which does dirty page accounting via
percpu counters on the BDI and doesn't require wakeups.
Also, we've already got per-node and per-zone counters there for
dirty/write pending stats, so do we actually need new counters and
wakeups here?
i.e. balance_dirty_pages() does not have an explicit wakeup - it
bases it's sleep time on the (memcg aware) measured writeback rate
on the BDI the page belongs to and the amount of outstanding dirty
data on that BDI. i.e. it estimates fairly accurately what the wait
time for this task should be given the dirty page demand and current
writeback progress being made is and just sleeps for that length of
time.
Ideally, that's what should be happening here - we should be able to
calculate a page cleaning rate estimation and then base the sleep
time on that. No wakeups needed - when we've waited for the
estimated time, we try to reclaim again...
In fact, why can't this "too many dirty pages" case just use the
balance_dirty_pages() infrastructure to do the "wait for writeback"
reclaim backoff? Why do we even need to re-invent the wheel here?
From: Dave Chinner <david@fromorbit.com> Date: 2021-09-16 00:38:02
On Thu, Sep 16, 2021 at 08:35:40AM +1000, NeilBrown wrote:
On Wed, 15 Sep 2021, Michal Hocko wrote:
quoted
On Wed 15-09-21 07:48:11, Neil Brown wrote:
quoted
Why does __GFP_NOFAIL access the reserves? Why not require that the
relevant "Try harder" flag (__GFP_ATOMIC or __GFP_MEMALLOC) be included
with __GFP_NOFAIL if that is justified?
Does 5020e285856c ("mm, oom: give __GFP_NOFAIL allocations access to
memory reserves") help?
Yes, that helps. A bit.
I'm not fond of the clause "the allocation request might have come with some
locks held". What if it doesn't? Does it still have to pay the price.
Should we not require that the caller indicate if any locks are held?
That way callers which don't hold locks can use __GFP_NOFAIL without
worrying about imposing on other code.
Or is it so rare that __GFP_NOFAIL would be used without holding a lock
that it doesn't matter?
The other commit of interest is
Commit: 6c18ba7a1899 ("mm: help __GFP_NOFAIL allocations which do not trigger OOM killer")
I don't find the reasoning convincing. It is a bit like "Robbing Peter
to pay Paul". It takes from the reserves to allow a __GFP_NOFAIL to
proceed, with out any reason to think this particular allocation has any
more 'right' to the reserves than anything else.
While I don't like the reasoning in either of these, they do make it
clear (to me) that the use of reserves is entirely an internal policy
decision. They should *not* be seen as part of the API and callers
should not have to be concerned about it when deciding whether to use
__GFP_NOFAIL or not.
Agree totally with this - we just want to block until allocation
succeeds, and if the -filesystem- deadlocks because allocation never
succeeds then that's a problem that needs to be solved in the
filesystem with a different memory allocation strategy...
OTOH, setting up a single __GFP_NOFAIL call site with the ability to
take the entire system down seems somewhat misguided.
The use of these reserves is, at most, a hypothetical problem. If it
ever looks like becoming a real practical problem, it needs to be fixed
internally to the page allocator. Maybe an extra water-mark which isn't
quite as permissive as ALLOC_HIGH...
I'm inclined to drop all references to reserves from the documentation
for __GFP_NOFAIL. I think there are enough users already that adding a
couple more isn't going to make problems substantially more likely. And
more will be added anyway that the mm/ team won't have the opportunity
or bandwidth to review.
Yup, we've been replacing open coded loops like in kmem_alloc() with
explicit __GFP_NOFAIL usage for a while now:
$ ▶ git grep __GFP_NOFAIL fs/xfs |wc -l
33
$
ANd we've got another 100 or so call sites planned for conversion to
__GFP_NOFAIL. Hence the suggestion to remove the use of
reserves from __GFP_NOFAIL seems like a sensible plan because it has
never been necessary in the past for all the allocation sites we are
converting from open coded loops to __GFP_NOFAIL...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
From: Michal Hocko <mhocko@suse.com> Date: 2021-09-16 06:52:30
On Thu 16-09-21 08:35:40, Neil Brown wrote:
On Wed, 15 Sep 2021, Michal Hocko wrote:
quoted
On Wed 15-09-21 07:48:11, Neil Brown wrote:
quoted
Why does __GFP_NOFAIL access the reserves? Why not require that the
relevant "Try harder" flag (__GFP_ATOMIC or __GFP_MEMALLOC) be included
with __GFP_NOFAIL if that is justified?
Does 5020e285856c ("mm, oom: give __GFP_NOFAIL allocations access to
memory reserves") help?
Yes, that helps. A bit.
I'm not fond of the clause "the allocation request might have come with some
locks held". What if it doesn't? Does it still have to pay the price.
Should we not require that the caller indicate if any locks are held?
I do not think this would help much TBH. What if the lock in question
doesn't impose any dependency through allocation problem?
That way callers which don't hold locks can use __GFP_NOFAIL without
worrying about imposing on other code.
Or is it so rare that __GFP_NOFAIL would be used without holding a lock
that it doesn't matter?
The other commit of interest is
Commit: 6c18ba7a1899 ("mm: help __GFP_NOFAIL allocations which do not trigger OOM killer")
I don't find the reasoning convincing. It is a bit like "Robbing Peter
to pay Paul". It takes from the reserves to allow a __GFP_NOFAIL to
proceed, with out any reason to think this particular allocation has any
more 'right' to the reserves than anything else.
I do agree that this is not really optimal. I do not remember exact
details but these changes were mostly based or inspired by extreme
memory pressure testing by Tetsuo who has managed to trigger quite some
corner cases. Especially those where NOFS was involved were problematic.
While I don't like the reasoning in either of these, they do make it
clear (to me) that the use of reserves is entirely an internal policy
decision. They should *not* be seen as part of the API and callers
should not have to be concerned about it when deciding whether to use
__GFP_NOFAIL or not.
Yes. NOFAIL should have high enough bar to use - essentially there is no
other way than use it - that memory reserves shouldn't be a road block.
If we learn that existing users can seriously deplete memory reserves
then we might need to reconsider the existing logic. So far there are no
indications that NOFAIL would really cause any problems in that area.
The use of these reserves is, at most, a hypothetical problem. If it
ever looks like becoming a real practical problem, it needs to be fixed
internally to the page allocator. Maybe an extra water-mark which isn't
quite as permissive as ALLOC_HIGH...
I'm inclined to drop all references to reserves from the documentation
for __GFP_NOFAIL.
I have found your additions to the documentation useful.
I think there are enough users already that adding a
couple more isn't going to make problems substantially more likely. And
more will be added anyway that the mm/ team won't have the opportunity
or bandwidth to review.
Meanwhile I'll see if I can understand the intricacies of alloc_page so
that I can contibute to making it more predictable.
Question: In those cases where an open-coded loop is appropriate, such
as when you want to handle signals or can drop locks, how bad would it
be to have a tight loop without any sleep?
should_reclaim_retry() will sleep 100ms (sometimes...). Is that enough?
__GFP_NOFAIL doesn't add any sleep when looping.
Yeah, NOFAIL doesn't add any explicit sleep points. In general there is
no guarantee that a sleepable allocation will sleep. We do cond_resched
in general but sleeping is enforced only for worker contexts because WQ
concurrency depends on an explicit sleeping. So to answer your question,
if you really need to sleep between retries then you should do it
manually but cond_resched can be implied.
--
Michal Hocko
SUSE Labs
On Thu, Sep 16, 2021 at 08:38:58AM +1000, Dave Chinner wrote:
On Wed, Sep 15, 2021 at 03:35:10PM +0100, Mel Gorman wrote:
quoted
On Wed, Sep 15, 2021 at 09:59:04AM +0100, Mel Gorman wrote:
quoted
quoted
Yup, that's what we need, but I don't see why it needs to be exposed
outside the allocation code at all.
Probably not. At least some of it could be contained within reclaim
itself to block when reclaim is not making progress as opposed to anything
congestion related. That might still livelock if no progress can be made
but that's not new, the OOM hammer should eventually kick in.
There are two sides to the reclaim-related throttling
1. throttling because zero progress is being made
2. throttling because there are too many dirty pages or pages under
writeback cycling through the LRU too quickly.
The dirty page aspects (and the removal of wait_iff_congested which is
almost completely broken) could be done with something like the following
(completly untested). The downside is that end_page_writeback() takes an
atomic penalty if reclaim is throttled but at that point the system is
struggling anyway so I doubt it matters.
The atomics are pretty nasty, as is directly accessing the pgdat on
every call to end_page_writeback(). Those will be performance
limiting factors. Indeed, we don't use atomics for dirty page
throttling, which does dirty page accounting via
percpu counters on the BDI and doesn't require wakeups.
Thanks for taking a look!
From end_page_writeback, the first atomic operation is an atomic read
which is READ_ONCE on most architectures (alpha is a counter example as it
has a memory barrier but alpha is niche). The main atomic penalty is when
the system is reclaim throttled but it can be a per-cpu node page state
counter instead. That sacrifices accuracy for speed but in this context,
I think that's ok. As for accessing the pgdat structure, every vmstat
counter for the node involves a pgdat lookup as the API is page-based
and so there are already a bunch of pgdat lookups in the IO path.
Also, we've already got per-node and per-zone counters there for
dirty/write pending stats, so do we actually need new counters and
wakeups here?
I think we need at least a new counter because dirty/write pending
stats do not tell us how many pages were cleaned since reclaim started
hitting problems with dirty pages at the tail of the LRU. Reading
dirty/write_pending stats at two points of time cannot be used to
infer how many pages were cleaned during the same interval. At minimum,
we'd need nr_dirtied and a new nr_cleaned stat to infer pages cleaned
between two points in time. That can be done but if the new counters is
NR_THROTTLED_WRITTEN (NR_WRITTEN while reclaim throttled), we only need one
field in struct zone to track nr_reclaim_throttled when throttling
startsi (updated patch at the end of the mail).
i.e. balance_dirty_pages() does not have an explicit wakeup - it
bases it's sleep time on the (memcg aware) measured writeback rate
on the BDI the page belongs to and the amount of outstanding dirty
data on that BDI. i.e. it estimates fairly accurately what the wait
time for this task should be given the dirty page demand and current
writeback progress being made is and just sleeps for that length of
time.
Ideally, that's what should be happening here - we should be able to
calculate a page cleaning rate estimation and then base the sleep
time on that. No wakeups needed - when we've waited for the
estimated time, we try to reclaim again...
In fact, why can't this "too many dirty pages" case just use the
balance_dirty_pages() infrastructure to do the "wait for writeback"
reclaim backoff? Why do we even need to re-invent the wheel here?
Conceptually I can see what you are asking for but am finding it hard to
translate it into an implementation. Dirty page throttling is throttling
heavy writers on a task and bdi basis but does not care about the
positioning of pages on the LRU or what node the page is allocated from.
On the reclaim side, the concern is how many pages that are dirty or
writeback at the tail of the LRU regardless of what task dirtied that
page or BDI it belongs to.
Hence I'm failing to see how the same rate-limiting mechanism could be
used on the reclaim side.
I guess we could look at the reclaim efficiency for a given task by
tracking pages that could not be reclaimed due to dirty/writeback relative
to pages that could be reclaimed and sleeping for increasing lengths of
time unconditionally when the reclaim efficiency is low. However it's
complex and would be hard to debug. It could hit serious problems in
cases where there are both fast and slow bdi's with the pages backed by a
slow bdi dominating the tail of the LRU -- it could throttle excessively
prematurely. Alternatively, look at taking pages that are dirty/writeback
off the inactive list like what is done for LRU_UNEVICTABLE pages
and throttling based on a high rate of INACTIVE_FILE:LRU_UNEVICTABLE,
but again, it's complex and could incur additional penalties in the
end_page_writeback due to LRU manipulations. Both are essentially
re-inventing a very complex wheel.
I'm aware that what I'm proposing also has its problems. It could wake
prematurely because all the pages cleaned were backed by a fast bdi when
the pages it scanned were backed by a slow bdi. Prehaps this could
be dealt with by tracking the estimated writeback speed of pages cleaned
and comparing it against the estimated writeback speed of pages at the
tail of the LRU but again, the complexity may be excessive.
If the first solution is too complex, it'll get hit with the KISS hammer
with a request to justify the complexity when the basis for comparison is
a broken concept. So I want to start simple, all it has to be is better
than congestion_wait/wait_iff_congested. If that still is not good enough,
the more complex options will have a basis for comparison.
UAF - that would need to be before the put_page() call...
UAF indeed.
Here is another version of the same concept that avoids atomic updates
from end_page_writeback() context and limits pgdat lookups. It's still
not tested other than "it boots under kvm".
@@ -199,6 +199,7 @@ enum node_stat_item {NR_VMSCAN_IMMEDIATE,/* Prioritise for reclaim when writeback ends */NR_DIRTIED,/* page dirtyings since bootup */NR_WRITTEN,/* page writings since bootup */+NR_THROTTLED_WRITTEN,/* NR_WRITTEN while reclaim throttled */NR_KERNEL_MISC_RECLAIMABLE,/* reclaimable non-slab kernel pages */NR_FOLL_PIN_ACQUIRED,/* via: pin_user_page(), gup flag: FOLL_PIN */NR_FOLL_PIN_RELEASED,/* pages returned via unpin_user_page() */
@@ -841,6 +842,10 @@ typedef struct pglist_data {intnode_id;wait_queue_head_tkswapd_wait;wait_queue_head_tpfmemalloc_wait;+wait_queue_head_treclaim_wait;/* wq for throttling reclaim */+atomic_tnr_reclaim_throttled;/* nr of throtted tasks */+unsignedlongnr_reclaim_start;/* nr pages written while throttled+*whenthrottlingstarted.*/structtask_struct*kswapd;/* Protected bymem_hotplug_begin/end()*/intkswapd_order;
@@ -1041,51 +1041,3 @@ long congestion_wait(int sync, long timeout)returnret;}EXPORT_SYMBOL(congestion_wait);--/**-*wait_iff_congested-Conditionallywaitforabacking_devtobecomeuncongestedorapgdattocompletewrites-*@sync:SYNCorASYNCIO-*@timeout:timeoutinjiffies-*-*Intheeventofacongestedbacking_dev(anybacking_dev)thiswaits-*forupto@timeoutjiffiesforeitheraBDItoexitcongestionofthe-*given@syncqueueorawritetocomplete.-*-*Thereturnvalueis0ifthesleepisforthefulltimeout.Otherwise,-*itisthenumberofjiffiesthatwerestillremainingwhenthefunction-*returned.return_value==timeoutimpliesthefunctiondidnotsleep.-*/-longwait_iff_congested(intsync,longtimeout)-{-longret;-unsignedlongstart=jiffies;-DEFINE_WAIT(wait);-wait_queue_head_t*wqh=&congestion_wqh[sync];--/*-*Ifthereisnocongestion,yieldifnecessaryinstead-*ofsleepingonthecongestionqueue-*/-if(atomic_read(&nr_wb_congested[sync])==0){-cond_resched();--/* In case we scheduled, work out time remaining */-ret=timeout-(jiffies-start);-if(ret<0)-ret=0;--gotoout;-}--/* Sleep until uncongested or a write happens */-prepare_to_wait(wqh,&wait,TASK_UNINTERRUPTIBLE);-ret=io_schedule_timeout(timeout);-finish_wait(wqh,&wait);--out:-trace_writeback_wait_iff_congested(jiffies_to_usecs(timeout),-jiffies_to_usecs(jiffies-start));--returnret;-}-EXPORT_SYMBOL(wait_iff_congested);
Only 4 subsystems call set_bdi_congested() or clear_bdi_congested():
block/pktcdvd, fs/ceph fs/fuse fs/nfs
It may make sense to use congestion_wait() or wait_iff_congested()
within these subsystems, but they have no value outside of these.
Add documentation comments to these functions to discourage further use.
This is an unfortunate state. The MM layer still relies on the API.
While adding a documentation to clarify the current status can stop more
usage I am wondering what is a real alternative. My experience tells me
that a lack of real alternative will lead to new creative ways of doing
things instead.
That is a valid concern. Discouraging the use of an interface without
providing a clear alternative risks people doing worse things.
At lease if people continue to use congestion_wait(), then we will be
able to find those uses when we are able to provide a better approach.
I'll drop this patch.
Thanks,
NeilBrown