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

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

[longli]

From: Long Li <longli@microsoft.com>

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: stable@vger.kernel.org
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
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(-)

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();
 }
-- 
2.34.1

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

[Haiyang Zhang]

> -----Original Message-----
> From: longli@linuxonhyperv.com <longli@linuxonhyperv.com>
> Sent: Thursday, June 22, 2023 12:23 PM
> To: Jason Gunthorpe <jgg@ziepe.ca>; Leon Romanovsky <leon@kernel.org>;
> Ajay Sharma <sharmaajay@microsoft.com>; Dexuan Cui
> <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> Cc: linux-rdma@vger.kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Long Li
> <longli@microsoft.com>; stable@vger.kernel.org
> Subject: [PATCH v2] net: mana: Batch ringing RX queue doorbell on receiving
> packets
> 
> From: Long Li <longli@microsoft.com>
> 
> 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: stable@vger.kernel.org
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
> Adapter (MANA)")
> Signed-off-by: Long Li <longli@microsoft.com>

Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>

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

[Dexuan Cui]

> From: longli@linuxonhyperv.com
> 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: stable@vger.kernel.org
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network
> Adapter (MANA)")
> Signed-off-by: Long Li <longli@microsoft.com>

Reviewed-by: Dexuan Cui <decui@microsoft.com>

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

[Jakub Kicinski]

On Thu, 22 Jun 2023 09:22:36 -0700 longli@linuxonhyperv.com 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: stable@vger.kernel.org
> 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.
-- 
pw-bot: cr

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

[Long Li]

> Subject: Re: [PATCH v2] net: mana: Batch ringing RX queue doorbell on
> receiving packets
> 
> On Thu, 22 Jun 2023 09:22:36 -0700 longli@linuxonhyperv.com 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: stable@vger.kernel.org
> > 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