public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] mm: io_uring: allow oom-killer from io_uring_setup
@ 2022-01-25  5:17 Shakeel Butt
  2022-01-25 18:35 ` David Rientjes
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Shakeel Butt @ 2022-01-25  5:17 UTC (permalink / raw)
  To: Jens Axboe, Pavel Begunkov
  Cc: Andrew Morton, linux-mm, linux-kernel, io-uring, Shakeel Butt

On an overcommitted system which is running multiple workloads of
varying priorities, it is preferred to trigger an oom-killer to kill a
low priority workload than to let the high priority workload receiving
ENOMEMs. On our memory overcommitted systems, we are seeing a lot of
ENOMEMs instead of oom-kills because io_uring_setup callchain is using
__GFP_NORETRY gfp flag which avoids the oom-killer. Let's remove it and
allow the oom-killer to kill a lower priority job.

Signed-off-by: Shakeel Butt <[email protected]>
---
 fs/io_uring.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e54c4127422e..d9eeb202363c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8928,10 +8928,9 @@ static void io_mem_free(void *ptr)
 
 static void *io_mem_alloc(size_t size)
 {
-	gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP |
-				__GFP_NORETRY | __GFP_ACCOUNT;
+	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
 
-	return (void *) __get_free_pages(gfp_flags, get_order(size));
+	return (void *) __get_free_pages(gfp, get_order(size));
 }
 
 static unsigned long rings_size(unsigned sq_entries, unsigned cq_entries,
-- 
2.35.0.rc0.227.g00780c9af4-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: io_uring: allow oom-killer from io_uring_setup
  2022-01-25  5:17 [PATCH] mm: io_uring: allow oom-killer from io_uring_setup Shakeel Butt
@ 2022-01-25 18:35 ` David Rientjes
  2022-01-25 22:57   ` Shakeel Butt
  2022-02-05  6:32 ` Shakeel Butt
  2022-02-07 15:44 ` Jens Axboe
  2 siblings, 1 reply; 6+ messages in thread
From: David Rientjes @ 2022-01-25 18:35 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Jens Axboe, Pavel Begunkov, Andrew Morton, linux-mm, linux-kernel,
	io-uring

On Mon, 24 Jan 2022, Shakeel Butt wrote:

> On an overcommitted system which is running multiple workloads of
> varying priorities, it is preferred to trigger an oom-killer to kill a
> low priority workload than to let the high priority workload receiving
> ENOMEMs. On our memory overcommitted systems, we are seeing a lot of
> ENOMEMs instead of oom-kills because io_uring_setup callchain is using
> __GFP_NORETRY gfp flag which avoids the oom-killer. Let's remove it and
> allow the oom-killer to kill a lower priority job.
> 

What is the size of the allocations that io_mem_alloc() is doing?

If get_order(size) > PAGE_ALLOC_COSTLY_ORDER, then this will fail even 
without the __GFP_NORETRY.  To make the guarantee that workloads are not 
receiving ENOMEM, it seems like we'd need to guarantee that allocations 
going through io_mem_alloc() are sufficiently small.

(And if we're really serious about it, then even something like a 
BUILD_BUG_ON().)

> Signed-off-by: Shakeel Butt <[email protected]>
> ---
>  fs/io_uring.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index e54c4127422e..d9eeb202363c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8928,10 +8928,9 @@ static void io_mem_free(void *ptr)
>  
>  static void *io_mem_alloc(size_t size)
>  {
> -	gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP |
> -				__GFP_NORETRY | __GFP_ACCOUNT;
> +	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
>  
> -	return (void *) __get_free_pages(gfp_flags, get_order(size));
> +	return (void *) __get_free_pages(gfp, get_order(size));
>  }
>  
>  static unsigned long rings_size(unsigned sq_entries, unsigned cq_entries,
> -- 
> 2.35.0.rc0.227.g00780c9af4-goog
> 
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: io_uring: allow oom-killer from io_uring_setup
  2022-01-25 18:35 ` David Rientjes
@ 2022-01-25 22:57   ` Shakeel Butt
  2022-01-26  1:42     ` David Rientjes
  0 siblings, 1 reply; 6+ messages in thread
From: Shakeel Butt @ 2022-01-25 22:57 UTC (permalink / raw)
  To: David Rientjes
  Cc: Jens Axboe, Pavel Begunkov, Andrew Morton, Linux MM, LKML,
	io-uring

On Tue, Jan 25, 2022 at 10:35 AM David Rientjes <[email protected]> wrote:
>
> On Mon, 24 Jan 2022, Shakeel Butt wrote:
>
> > On an overcommitted system which is running multiple workloads of
> > varying priorities, it is preferred to trigger an oom-killer to kill a
> > low priority workload than to let the high priority workload receiving
> > ENOMEMs. On our memory overcommitted systems, we are seeing a lot of
> > ENOMEMs instead of oom-kills because io_uring_setup callchain is using
> > __GFP_NORETRY gfp flag which avoids the oom-killer. Let's remove it and
> > allow the oom-killer to kill a lower priority job.
> >
>
> What is the size of the allocations that io_mem_alloc() is doing?
>
> If get_order(size) > PAGE_ALLOC_COSTLY_ORDER, then this will fail even
> without the __GFP_NORETRY.  To make the guarantee that workloads are not
> receiving ENOMEM, it seems like we'd need to guarantee that allocations
> going through io_mem_alloc() are sufficiently small.
>
> (And if we're really serious about it, then even something like a
> BUILD_BUG_ON().)
>

The test case provided to me for which the user was seeing ENOMEMs was
io_uring_setup() with 64 entries (nothing else).

If I understand rings_size() calculations correctly then the 0 order
allocation was requested in io_mem_alloc().

For order > PAGE_ALLOC_COSTLY_ORDER, maybe we can use
__GFP_RETRY_MAYFAIL. It will at least do more aggressive reclaim
though I think that is a separate discussion. For this issue, we are
seeing ENOMEMs even for order 0 allocations.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: io_uring: allow oom-killer from io_uring_setup
  2022-01-25 22:57   ` Shakeel Butt
@ 2022-01-26  1:42     ` David Rientjes
  0 siblings, 0 replies; 6+ messages in thread
From: David Rientjes @ 2022-01-26  1:42 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Jens Axboe, Pavel Begunkov, Andrew Morton, Linux MM, LKML,
	io-uring

On Tue, 25 Jan 2022, Shakeel Butt wrote:

> > > On an overcommitted system which is running multiple workloads of
> > > varying priorities, it is preferred to trigger an oom-killer to kill a
> > > low priority workload than to let the high priority workload receiving
> > > ENOMEMs. On our memory overcommitted systems, we are seeing a lot of
> > > ENOMEMs instead of oom-kills because io_uring_setup callchain is using
> > > __GFP_NORETRY gfp flag which avoids the oom-killer. Let's remove it and
> > > allow the oom-killer to kill a lower priority job.
> > >
> >
> > What is the size of the allocations that io_mem_alloc() is doing?
> >
> > If get_order(size) > PAGE_ALLOC_COSTLY_ORDER, then this will fail even
> > without the __GFP_NORETRY.  To make the guarantee that workloads are not
> > receiving ENOMEM, it seems like we'd need to guarantee that allocations
> > going through io_mem_alloc() are sufficiently small.
> >
> > (And if we're really serious about it, then even something like a
> > BUILD_BUG_ON().)
> >
> 
> The test case provided to me for which the user was seeing ENOMEMs was
> io_uring_setup() with 64 entries (nothing else).
> 
> If I understand rings_size() calculations correctly then the 0 order
> allocation was requested in io_mem_alloc().
> 
> For order > PAGE_ALLOC_COSTLY_ORDER, maybe we can use
> __GFP_RETRY_MAYFAIL. It will at least do more aggressive reclaim
> though I think that is a separate discussion. For this issue, we are
> seeing ENOMEMs even for order 0 allocations.
> 

Ah, gotcha, thanks for the background.  IIUC, io_uring_setup() can be done 
with anything with CAP_SYS_NICE so my only concern would be whether this 
could be used maliciously on a system not using memcg, but in that case we 
can already fork many small processes that consume all memory and oom kill 
everything else on the system already.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: io_uring: allow oom-killer from io_uring_setup
  2022-01-25  5:17 [PATCH] mm: io_uring: allow oom-killer from io_uring_setup Shakeel Butt
  2022-01-25 18:35 ` David Rientjes
@ 2022-02-05  6:32 ` Shakeel Butt
  2022-02-07 15:44 ` Jens Axboe
  2 siblings, 0 replies; 6+ messages in thread
From: Shakeel Butt @ 2022-02-05  6:32 UTC (permalink / raw)
  To: Jens Axboe, Pavel Begunkov; +Cc: Andrew Morton, Linux MM, LKML, io-uring

On Mon, Jan 24, 2022 at 9:17 PM Shakeel Butt <[email protected]> wrote:
>
> On an overcommitted system which is running multiple workloads of
> varying priorities, it is preferred to trigger an oom-killer to kill a
> low priority workload than to let the high priority workload receiving
> ENOMEMs. On our memory overcommitted systems, we are seeing a lot of
> ENOMEMs instead of oom-kills because io_uring_setup callchain is using
> __GFP_NORETRY gfp flag which avoids the oom-killer. Let's remove it and
> allow the oom-killer to kill a lower priority job.
>
> Signed-off-by: Shakeel Butt <[email protected]>

Jens, any comments or concerns on this patch?

> ---
>  fs/io_uring.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index e54c4127422e..d9eeb202363c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8928,10 +8928,9 @@ static void io_mem_free(void *ptr)
>
>  static void *io_mem_alloc(size_t size)
>  {
> -       gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP |
> -                               __GFP_NORETRY | __GFP_ACCOUNT;
> +       gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
>
> -       return (void *) __get_free_pages(gfp_flags, get_order(size));
> +       return (void *) __get_free_pages(gfp, get_order(size));
>  }
>
>  static unsigned long rings_size(unsigned sq_entries, unsigned cq_entries,
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: io_uring: allow oom-killer from io_uring_setup
  2022-01-25  5:17 [PATCH] mm: io_uring: allow oom-killer from io_uring_setup Shakeel Butt
  2022-01-25 18:35 ` David Rientjes
  2022-02-05  6:32 ` Shakeel Butt
@ 2022-02-07 15:44 ` Jens Axboe
  2 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2022-02-07 15:44 UTC (permalink / raw)
  To: Pavel Begunkov, Shakeel Butt
  Cc: linux-kernel, linux-mm, io-uring, Andrew Morton

On Mon, 24 Jan 2022 21:17:36 -0800, Shakeel Butt wrote:
> On an overcommitted system which is running multiple workloads of
> varying priorities, it is preferred to trigger an oom-killer to kill a
> low priority workload than to let the high priority workload receiving
> ENOMEMs. On our memory overcommitted systems, we are seeing a lot of
> ENOMEMs instead of oom-kills because io_uring_setup callchain is using
> __GFP_NORETRY gfp flag which avoids the oom-killer. Let's remove it and
> allow the oom-killer to kill a lower priority job.
> 
> [...]

Applied, thanks!

[1/1] mm: io_uring: allow oom-killer from io_uring_setup
      commit: 0a3f1e0beacf6cc8ae5f846b0641c1df476e83d6

Best regards,
-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-02-07 15:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-25  5:17 [PATCH] mm: io_uring: allow oom-killer from io_uring_setup Shakeel Butt
2022-01-25 18:35 ` David Rientjes
2022-01-25 22:57   ` Shakeel Butt
2022-01-26  1:42     ` David Rientjes
2022-02-05  6:32 ` Shakeel Butt
2022-02-07 15:44 ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox