[1/1] net/rds: suppress page allocation failure error in recv buffer refill

Commit Message

Manjunath Patil Oct. 2, 2020, 8:05 p.m. UTC

RDS/IB tries to refill the recv buffer in softirq context using
GFP_NOWAIT flag. However alloc failure is handled by queueing a work to
refill the recv buffer with GFP_KERNEL flag. This means failure to
allocate with GFP_NOWAIT isn't fatal. Do not print the PAF warnings if
softirq context fails to refill the recv buffer, instead print rate
limited warnings.

Signed-off-by: Manjunath Patil <[e-mail]>
Reviewed-by: Aruna Ramakrishna <[e-mail]>
---
 net/rds/ib_recv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Santosh Shilimkar Oct. 2, 2020, 8:10 p.m. UTC | #1

On 10/2/20 1:05 PM, Manjunath Patil wrote:
> RDS/IB tries to refill the recv buffer in softirq context using
> GFP_NOWAIT flag. However alloc failure is handled by queueing a work to
> refill the recv buffer with GFP_KERNEL flag. This means failure to
> allocate with GFP_NOWAIT isn't fatal. Do not print the PAF warnings if
> softirq context fails to refill the recv buffer, instead print rate
> limited warnings.
> 
> Signed-off-by: Manjunath Patil <[e-mail]>
> Reviewed-by: Aruna Ramakrishna <[e-mail]>
> ---
Thanks for the updated version. Whenever you send updated patch,
you should add version so that it helps for archiving as well as
review.

Acked-by: Santosh Shilimkar <[e-mail]>

Manjunath Patil Oct. 2, 2020, 8:23 p.m. UTC | #2

Thanks for ack'ing.

Yeah, sorry about version. I had it in my mind to add it when I started, 
but forgot it at the last moment.

-Thanks,
Manjunath
On 10/2/2020 1:10 PM, [e-mail] wrote:
> On 10/2/20 1:05 PM, Manjunath Patil wrote:
>> RDS/IB tries to refill the recv buffer in softirq context using
>> GFP_NOWAIT flag. However alloc failure is handled by queueing a work to
>> refill the recv buffer with GFP_KERNEL flag. This means failure to
>> allocate with GFP_NOWAIT isn't fatal. Do not print the PAF warnings if
>> softirq context fails to refill the recv buffer, instead print rate
>> limited warnings.
>>
>> Signed-off-by: Manjunath Patil <[e-mail]>
>> Reviewed-by: Aruna Ramakrishna <[e-mail]>
>> ---
> Thanks for the updated version. Whenever you send updated patch,
> you should add version so that it helps for archiving as well as
> review.
>
> Acked-by: Santosh Shilimkar <[e-mail]>

David Miller Oct. 4, 2020, 12:26 a.m. UTC | #3

From: Manjunath Patil <[e-mail]>
Date: Fri,  2 Oct 2020 13:05:45 -0700

> RDS/IB tries to refill the recv buffer in softirq context using
> GFP_NOWAIT flag. However alloc failure is handled by queueing a work to
> refill the recv buffer with GFP_KERNEL flag. This means failure to
> allocate with GFP_NOWAIT isn't fatal. Do not print the PAF warnings if
> softirq context fails to refill the recv buffer, instead print rate
> limited warnings.
> 
> Signed-off-by: Manjunath Patil <[e-mail]>
> Reviewed-by: Aruna Ramakrishna <[e-mail]>

Honestly I don't think the subsystem should print any warning at all.

Either it's a softirq failure, and that's ok because you will push
the allocation to GFP_KERNEL via a work job.  Or it's a GFP_KERNEL
failure in non-softirq context and the kernel will print a warning
and a stack backtrace from the memory allocator.

Therefore, please remove all of the warnings in the rds code.

Thanks.

Manjunath Patil Oct. 5, 2020, 9:39 p.m. UTC | #4

Thanks David for your feedback.

I will submit v3 of this patch removing the warning.

-Manjunath
On 10/3/2020 5:26 PM, David Miller wrote:
> From: Manjunath Patil <[e-mail]>
> Date: Fri,  2 Oct 2020 13:05:45 -0700
>
>> RDS/IB tries to refill the recv buffer in softirq context using
>> GFP_NOWAIT flag. However alloc failure is handled by queueing a work to
>> refill the recv buffer with GFP_KERNEL flag. This means failure to
>> allocate with GFP_NOWAIT isn't fatal. Do not print the PAF warnings if
>> softirq context fails to refill the recv buffer, instead print rate
>> limited warnings.
>>
>> Signed-off-by: Manjunath Patil <[e-mail]>
>> Reviewed-by: Aruna Ramakrishna <[e-mail]>
> Honestly I don't think the subsystem should print any warning at all.
>
> Either it's a softirq failure, and that's ok because you will push
> the allocation to GFP_KERNEL via a work job.  Or it's a GFP_KERNEL
> failure in non-softirq context and the kernel will print a warning
> and a stack backtrace from the memory allocator.
>
> Therefore, please remove all of the warnings in the rds code.
>
> Thanks.

Patch

diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 694d411dc72f..b4ed421acc7b 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -310,8 +310,8 @@  static int rds_ib_recv_refill_one(struct rds_connection *conn,
 	struct rds_ib_connection *ic = conn->c_transport_data;
 	struct ib_sge *sge;
 	int ret = -ENOMEM;
-	gfp_t slab_mask = GFP_NOWAIT;
-	gfp_t page_mask = GFP_NOWAIT;
+	gfp_t slab_mask = gfp;
+	gfp_t page_mask = gfp;
 
 	if (gfp & __GFP_DIRECT_RECLAIM) {
 		slab_mask = GFP_KERNEL;
@@ -406,6 +406,9 @@  void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 		recv = &ic->i_recvs[pos];
 		ret = rds_ib_recv_refill_one(conn, recv, gfp);
 		if (ret) {
+			pr_warn_ratelimited("RDS/IB: failed to refill recv buffer for <%pI6c,%pI6c,%d>, waking worker\n",
+					    &conn->c_laddr, &conn->c_faddr,
+					    conn->c_tos);
 			must_wake = true;
 			break;
 		}
@@ -1020,7 +1023,7 @@  void rds_ib_recv_cqe_handler(struct rds_ib_connection *ic,
 		rds_ib_stats_inc(s_ib_rx_ring_empty);
 
 	if (rds_ib_ring_low(&ic->i_recv_ring)) {
-		rds_ib_recv_refill(conn, 0, GFP_NOWAIT);
+		rds_ib_recv_refill(conn, 0, GFP_NOWAIT | __GFP_NOWARN);
 		rds_ib_stats_inc(s_ib_rx_refill_from_cq);
 	}
 }