[v2] net: mana: Batch ringing RX queue doorbell on receiving packets

Commit Message

Long Li June 22, 2023, 4:22 p.m. UTC

From: Long Li <[e-mail]>

It's inefficient to ring the doorbell page every time a WQE is posted to
the received queue.

Move the code for ringing doorbell page to where after we have posted all
WQEs to the receive queue during a callback from napi_poll().

Tests showed no regression in network latency benchmarks.

Cc: [e-mail]
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <[e-mail]>
---
Change log:
v2:
Check for comp_read > 0 as it might be negative on completion error.
Set rq.wqe_cnt to 0 according to BNIC spec.

 drivers/net/ethernet/microsoft/mana/gdma_main.c |  5 ++++-
 drivers/net/ethernet/microsoft/mana/mana_en.c   | 10 ++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

Haiyang Zhang June 22, 2023, 9:27 p.m. UTC | #1

> -----Original Message-----
> From: [e-mail] <[e-mail]>
> Sent: Thursday, June 22, 2023 12:23 PM
> To: Jason Gunthorpe <[e-mail]>; Leon Romanovsky <[e-mail]>;
> Ajay Sharma <[e-mail]>; Dexuan Cui
> <[e-mail]>; KY Srinivasan <[e-mail]>; Haiyang Zhang
> <[e-mail]>; Wei Liu <[e-mail]>; David S. Miller
> <[e-mail]>; Eric Dumazet <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Long Li
> <[e-mail]>; [e-mail]
> Subject: [PATCH v2] net: mana: Batch ringing RX queue doorbell on receiving
> packets
> 
> From: Long Li <[e-mail]>
> 
> It's inefficient to ring the doorbell page every time a WQE is posted to
> the received queue.
> 
> Move the code for ringing doorbell page to where after we have posted all
> WQEs to the receive queue during a callback from napi_poll().
> 
> Tests showed no regression in network latency benchmarks.
> 
> Cc: [e-mail]
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
> Adapter (MANA)")
> Signed-off-by: Long Li <[e-mail]>

Reviewed-by: Haiyang Zhang <[e-mail]>

Dexuan Cui June 23, 2023, 3 a.m. UTC | #2

> From: [e-mail]
> Sent: Thursday, June 22, 2023 9:23 AM
>  ...
> 
> It's inefficient to ring the doorbell page every time a WQE is posted to
> the received queue.
> 
> Move the code for ringing doorbell page to where after we have posted all
> WQEs to the receive queue during a callback from napi_poll().
> 
> Tests showed no regression in network latency benchmarks.
> 
> Cc: [e-mail]
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
> Adapter (MANA)")
> Signed-off-by: Long Li <[e-mail]>

Reviewed-by: Dexuan Cui <[e-mail]>

Jakub Kicinski June 24, 2023, 10:10 p.m. UTC | #3

On Thu, 22 Jun 2023 09:22:36 -0700 [e-mail] wrote:
> It's inefficient to ring the doorbell page every time a WQE is posted to
> the received queue.
> 
> Move the code for ringing doorbell page to where after we have posted all
> WQEs to the receive queue during a callback from napi_poll().
> 
> Tests showed no regression in network latency benchmarks.
> 
> Cc: [e-mail]
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")

If this is supposed to be a fix, you need to clearly explain what the
performance loss was, so that backporters can make an informed decision.

>  drivers/net/ethernet/microsoft/mana/gdma_main.c |  5 ++++-
>  drivers/net/ethernet/microsoft/mana/mana_en.c   | 10 ++++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8f3f78b68592..ef11d09a3655 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -300,8 +300,11 @@ static void mana_gd_ring_doorbell(struct gdma_context *gc, u32 db_index,
>  
>  void mana_gd_wq_ring_doorbell(struct gdma_context *gc, struct gdma_queue *queue)
>  {
> +	/* BNIC Spec specifies that client should set 0 for rq.wqe_cnt
> +	 * This value is not used in sq
> +	 */
>  	mana_gd_ring_doorbell(gc, queue->gdma_dev->doorbell, queue->type,
> -			      queue->id, queue->head * GDMA_WQE_BU_SIZE, 1);
> +			      queue->id, queue->head * GDMA_WQE_BU_SIZE, 0);
>  }

This change needs to be explained in the commit message, or should be 
a separate patch.

Long Li June 26, 2023, 10:55 p.m. UTC | #4

> Subject: Re: [PATCH v2] net: mana: Batch ringing RX queue doorbell on
> receiving packets
> 
> On Thu, 22 Jun 2023 09:22:36 -0700 [e-mail] wrote:
> > It's inefficient to ring the doorbell page every time a WQE is posted
> > to the received queue.
> >
> > Move the code for ringing doorbell page to where after we have posted
> > all WQEs to the receive queue during a callback from napi_poll().
> >
> > Tests showed no regression in network latency benchmarks.
> >
> > Cc: [e-mail]
> > Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure
> > Network Adapter (MANA)")
> 
> If this is supposed to be a fix, you need to clearly explain what the
> performance loss was, so that backporters can make an informed decision.

I'm sending v3 to include the details.

> 
> >  drivers/net/ethernet/microsoft/mana/gdma_main.c |  5 ++++-
> >  drivers/net/ethernet/microsoft/mana/mana_en.c   | 10 ++++++++--
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > index 8f3f78b68592..ef11d09a3655 100644
> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > @@ -300,8 +300,11 @@ static void mana_gd_ring_doorbell(struct
> > gdma_context *gc, u32 db_index,
> >
> >  void mana_gd_wq_ring_doorbell(struct gdma_context *gc, struct
> > gdma_queue *queue)  {
> > +	/* BNIC Spec specifies that client should set 0 for rq.wqe_cnt
> > +	 * This value is not used in sq
> > +	 */
> >  	mana_gd_ring_doorbell(gc, queue->gdma_dev->doorbell, queue-
> >type,
> > -			      queue->id, queue->head * GDMA_WQE_BU_SIZE,
> 1);
> > +			      queue->id, queue->head * GDMA_WQE_BU_SIZE,
> 0);
> >  }
> 
> This change needs to be explained in the commit message, or should be a
> separate patch.

Will explain the change in the commit message.

Thanks,
Long

> --
> pw-bot: cr

Patch

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 8f3f78b68592..ef11d09a3655 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -300,8 +300,11 @@  static void mana_gd_ring_doorbell(struct gdma_context *gc, u32 db_index,
 
 void mana_gd_wq_ring_doorbell(struct gdma_context *gc, struct gdma_queue *queue)
 {
+	/* BNIC Spec specifies that client should set 0 for rq.wqe_cnt
+	 * This value is not used in sq
+	 */
 	mana_gd_ring_doorbell(gc, queue->gdma_dev->doorbell, queue->type,
-			      queue->id, queue->head * GDMA_WQE_BU_SIZE, 1);
+			      queue->id, queue->head * GDMA_WQE_BU_SIZE, 0);
 }
 
 void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit)
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index cd4d5ceb9f2d..1d8abe63fcb8 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -1383,8 +1383,8 @@  static void mana_post_pkt_rxq(struct mana_rxq *rxq)
 
 	recv_buf_oob = &rxq->rx_oobs[curr_index];
 
-	err = mana_gd_post_and_ring(rxq->gdma_rq, &recv_buf_oob->wqe_req,
-				    &recv_buf_oob->wqe_inf);
+	err = mana_gd_post_work_request(rxq->gdma_rq, &recv_buf_oob->wqe_req,
+					&recv_buf_oob->wqe_inf);
 	if (WARN_ON_ONCE(err))
 		return;
 
@@ -1654,6 +1654,12 @@  static void mana_poll_rx_cq(struct mana_cq *cq)
 		mana_process_rx_cqe(rxq, cq, &comp[i]);
 	}
 
+	if (comp_read > 0) {
+		struct gdma_context *gc = rxq->gdma_rq->gdma_dev->gdma_context;
+
+		mana_gd_wq_ring_doorbell(gc, rxq->gdma_rq);
+	}
+
 	if (rxq->xdp_flush)
 		xdp_do_flush();
 }