Re: [PATCH v2 net] net: mana: Fix doorbell out of order violation and avoid unnecessary doorbell rings

[Shradha Gupta]

On Wed, Aug 07, 2024 at 04:17:06PM -0700, longli@xxxxxxxxxxxxxxxxx wrote:
> From: Long Li <longli@xxxxxxxxxxxxx>
> 
> After napi_complete_done() is called when NAPI is polling in the current
> process context, another NAPI may be scheduled and start running in
> softirq on another CPU and may ring the doorbell before the current CPU
> does. When combined with unnecessary rings when there is no need to arm
> the CQ, it triggers error paths in the hardware.
> 
> This patch fixes this by calling napi_complete_done() after doorbell
> rings. It limits the number of unnecessary rings when there is
> no need to arm. MANA hardware specifies that there must be one doorbell
> ring every 8 CQ wraparounds. This driver guarantees one doorbell ring as
> soon as the number of consumed CQEs exceeds 4 CQ wraparounds. In pratical
> workloads, the 4 CQ wraparounds proves to be big enough that it rarely
> exceeds this limit before all the napi weight is consumed.
> 
> To implement this, add a per-CQ counter cq->work_done_since_doorbell,
> and make sure the CQ is armed as soon as passing 4 wraparounds of the CQ.
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: e1b5683ff62e ("net: mana: Move NAPI from EQ to CQ")
> 
> Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Signed-off-by: Long Li <longli@xxxxxxxxxxxxx>
> ---
> 
> change in v2:
> Added more details to comments to explain the patch
> 
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 24 ++++++++++++-------
>  include/net/mana/mana.h                       |  1 +
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index d2f07e179e86..f83211f9e737 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1788,7 +1788,6 @@ static void mana_poll_rx_cq(struct mana_cq *cq)
>  static int mana_cq_handler(void *context, struct gdma_queue *gdma_queue)
>  {
>  	struct mana_cq *cq = context;
> -	u8 arm_bit;
>  	int w;
>  
>  	WARN_ON_ONCE(cq->gdma_cq != gdma_queue);
> @@ -1799,16 +1798,23 @@ static int mana_cq_handler(void *context, struct gdma_queue *gdma_queue)
>  		mana_poll_tx_cq(cq);
>  
>  	w = cq->work_done;
> -
> -	if (w < cq->budget &&
> -	    napi_complete_done(&cq->napi, w)) {
> -		arm_bit = SET_ARM_BIT;
> -	} else {
> -		arm_bit = 0;
> +	cq->work_done_since_doorbell += w;
> +
> +	if (w < cq->budget) {
> +		mana_gd_ring_cq(gdma_queue, SET_ARM_BIT);
> +		cq->work_done_since_doorbell = 0;
> +		napi_complete_done(&cq->napi, w);
> +	} else if (cq->work_done_since_doorbell >
> +		   cq->gdma_cq->queue_size / COMP_ENTRY_SIZE * 4) {

should we define a macro for 4? may be 'CQ_WRAPAROUND_LIMIT'

> +		/* MANA hardware requires at least one doorbell ring every 8
> +		 * wraparounds of CQ even if there is no need to arm the CQ.
> +		 * This driver rings the doorbell as soon as we have exceeded
> +		 * 4 wraparounds.
> +		 */
> +		mana_gd_ring_cq(gdma_queue, 0);
> +		cq->work_done_since_doorbell = 0;
>  	}
>  
> -	mana_gd_ring_cq(gdma_queue, arm_bit);
> -
>  	return w;
>  }
>  
> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 6439fd8b437b..7caa334f4888 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -275,6 +275,7 @@ struct mana_cq {
>  	/* NAPI data */
>  	struct napi_struct napi;
>  	int work_done;
> +	int work_done_since_doorbell;
>  	int budget;
>  };
>  
> -- 
> 2.17.1