RE: [PATCH 1/1] net/mana: add 32 bit short doorbell

[Long Li <longli@microsoft.com>]

> Subject: [PATCH 1/1] net/mana: add 32 bit short doorbell
> 
> Add 32 bit short doorbell support. Ring short doorbell when running in 32 bit
> applicactions.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
>  drivers/net/mana/gdma.c | 95
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/net/mana/mana.h | 25 +++++++++++
>  drivers/net/mana/rx.c   | 52 ++++++++++++++++++++++
>  drivers/net/mana/tx.c   | 28 ++++++++++++
>  4 files changed, 200 insertions(+)
> 
> diff --git a/drivers/net/mana/gdma.c b/drivers/net/mana/gdma.c index
> 65685fe236..d1da025d1b 100644
> --- a/drivers/net/mana/gdma.c
> +++ b/drivers/net/mana/gdma.c
> @@ -166,6 +166,97 @@ gdma_post_work_request(struct
> mana_gdma_queue *queue,
>  	return 0;
>  }
> 
> +#ifdef RTE_ARCH_32
> +union gdma_short_doorbell_entry {
> +	uint32_t     as_uint32;
> +
> +	struct {
> +		uint32_t tail_ptr_incr	: 16; /* Number of CQEs */
> +		uint32_t id		: 12;
> +		uint32_t reserved	: 3;
> +		uint32_t arm		: 1;
> +	} cq;
> +
> +	struct {
> +		uint32_t tail_ptr_incr	: 16; /* In number of bytes */
> +		uint32_t id		: 12;
> +		uint32_t reserved	: 4;
> +	} rq;
> +
> +	struct {
> +		uint32_t tail_ptr_incr	: 16; /* In number of bytes */
> +		uint32_t id		: 12;
> +		uint32_t reserved	: 4;
> +	} sq;
> +
> +	struct {
> +		uint32_t tail_ptr_incr	: 16; /* Number of EQEs */
> +		uint32_t id		: 12;
> +		uint32_t reserved	: 3;
> +		uint32_t arm		: 1;
> +	} eq;
> +}; /* HW DATA */
> +
> +enum {
> +	DOORBELL_SHORT_OFFSET_SQ = 0x10,
> +	DOORBELL_SHORT_OFFSET_RQ = 0x410,
> +	DOORBELL_SHORT_OFFSET_CQ = 0x810,
> +	DOORBELL_SHORT_OFFSET_EQ = 0xFF0,
> +};
> +
> +/*
> + * Write to hardware doorbell to notify new activity.
> + */
> +int
> +mana_ring_short_doorbell(void *db_page, enum gdma_queue_types
> queue_type,
> +			 uint32_t queue_id, uint32_t tail_incr, uint8_t arm) {
> +	uint8_t *addr = db_page;
> +	union gdma_short_doorbell_entry e = {};
> +
> +	if ((queue_id & ~GDMA_SHORT_DB_QID_MASK) ||
> +	    (tail_incr & ~GDMA_SHORT_DB_INC_MASK)) {
> +		DP_LOG(ERR, "%s: queue_id %u or "
> +		       "tail_incr %u overflowed, queue type %d",
> +		       __func__, queue_id, tail_incr, queue_type);
> +		return -EINVAL;
> +	}
> +
> +	switch (queue_type) {
> +	case GDMA_QUEUE_SEND:
> +		e.sq.id = queue_id;
> +		e.sq.tail_ptr_incr = tail_incr;
> +		addr += DOORBELL_SHORT_OFFSET_SQ;
> +		break;
> +
> +	case GDMA_QUEUE_RECEIVE:
> +		e.rq.id = queue_id;
> +		e.rq.tail_ptr_incr = tail_incr;
> +		addr += DOORBELL_SHORT_OFFSET_RQ;
> +		break;
> +
> +	case GDMA_QUEUE_COMPLETION:
> +		e.cq.id = queue_id;
> +		e.cq.tail_ptr_incr = tail_incr;
> +		e.cq.arm = arm;
> +		addr += DOORBELL_SHORT_OFFSET_CQ;
> +		break;
> +
> +	default:
> +		DP_LOG(ERR, "Unsupported queue type %d", queue_type);
> +		return -1;
> +	}
> +
> +	/* Ensure all writes are done before ringing doorbell */
> +	rte_wmb();
> +
> +	DP_LOG(DEBUG, "db_page %p addr %p queue_id %u type %u tail %u
> arm %u",
> +	       db_page, addr, queue_id, queue_type, tail_incr, arm);
> +
> +	rte_write32(e.as_uint32, addr);
> +	return 0;
> +}
> +#else
>  union gdma_doorbell_entry {
>  	uint64_t     as_uint64;
> 
> @@ -248,6 +339,7 @@ mana_ring_doorbell(void *db_page, enum
> gdma_queue_types queue_type,
>  	rte_write64(e.as_uint64, addr);
>  	return 0;
>  }
> +#endif
> 
>  /*
>   * Poll completion queue for completions.
> @@ -287,6 +379,9 @@ gdma_poll_completion_queue(struct
> mana_gdma_queue *cq,
>  		num_comp++;
> 
>  		cq->head++;
> +#ifdef RTE_ARCH_32
> +		cq->head_incr_to_short_db++;
> +#endif
> 
>  		DP_LOG(DEBUG, "comp new 0x%x old 0x%x cqe 0x%x wq %u
> sq %u head %u",
>  		       new_owner_bits, old_owner_bits, cqe_owner_bits, diff --
> git a/drivers/net/mana/mana.h b/drivers/net/mana/mana.h index
> 5801491d75..848d87c096 100644
> --- a/drivers/net/mana/mana.h
> +++ b/drivers/net/mana/mana.h
> @@ -50,6 +50,19 @@ struct mana_shared_data {  #define
> MAX_TX_WQE_SIZE 512  #define MAX_RX_WQE_SIZE 256
> 
> +/* For 32 bit only */
> +#ifdef RTE_ARCH_32
> +#define	GDMA_SHORT_DB_INC_MASK 0xffff
> +#define	GDMA_SHORT_DB_QID_MASK 0xfff
> +
> +#define GDMA_SHORT_DB_MAX_WQE	(0x10000 /
> GDMA_WQE_ALIGNMENT_UNIT_SIZE)
> +
> +#define TX_WQE_SHORT_DB_THRESHOLD \
> +	(GDMA_SHORT_DB_MAX_WQE - (2 * MAX_TX_WQE_SIZE)) #define
> +RX_WQE_SHORT_DB_THRESHOLD \

GDMA_SHORT_DB_MAX_WQE is in BU, MAX_TX_WQE_SIZE is in bytes.

Doing math on two different units...

And why using "2*", using "1*" is good enough?




> +	(GDMA_SHORT_DB_MAX_WQE - (2 * MAX_RX_WQE_SIZE)) #endif
> +
>  /* Values from the GDMA specification document, WQE format description */
> #define INLINE_OOB_SMALL_SIZE_IN_BYTES 8  #define
> INLINE_OOB_LARGE_SIZE_IN_BYTES 24 @@ -375,6 +388,9 @@ struct
> mana_gdma_queue {
>  	uint32_t id;
>  	uint32_t head;
>  	uint32_t tail;
> +#ifdef RTE_ARCH_32
> +	uint32_t head_incr_to_short_db;
> +#endif
>  };
> 
>  #define MANA_MR_BTREE_PER_QUEUE_N	64
> @@ -425,6 +441,9 @@ struct mana_rxq {
>  	 */
>  	uint32_t desc_ring_head, desc_ring_tail;
> 
> +	/* For storing wqe increment count btw each short doorbell ring */
> +	uint32_t wqe_cnt_to_short_db;
> +
>  	struct mana_gdma_queue gdma_rq;
>  	struct mana_gdma_queue gdma_cq;
>  	struct gdma_comp *gdma_comp_buf;
> @@ -455,8 +474,14 @@ extern int mana_logtype_init;
> 
>  #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> 
> +#ifdef RTE_ARCH_32
> +int mana_ring_short_doorbell(void *db_page, enum gdma_queue_types
> queue_type,
> +			     uint32_t queue_id, uint32_t tail_incr,
> +			     uint8_t arm);
> +#else
>  int mana_ring_doorbell(void *db_page, enum gdma_queue_types
> queue_type,
>  		       uint32_t queue_id, uint32_t tail, uint8_t arm);
> +#endif
>  int mana_rq_ring_doorbell(struct mana_rxq *rxq);
> 
>  int gdma_post_work_request(struct mana_gdma_queue *queue, diff --git
> a/drivers/net/mana/rx.c b/drivers/net/mana/rx.c index
> 14d9085801..303d129e5b 100644
> --- a/drivers/net/mana/rx.c
> +++ b/drivers/net/mana/rx.c
> @@ -39,10 +39,23 @@ mana_rq_ring_doorbell(struct mana_rxq *rxq)
>  	/* Hardware Spec specifies that software client should set 0 for
>  	 * wqe_cnt for Receive Queues.
>  	 */
> +#ifdef RTE_ARCH_32
> +	if (rxq->wqe_cnt_to_short_db) {
> +		ret = mana_ring_short_doorbell(db_page,
> GDMA_QUEUE_RECEIVE,
> +				 rxq->gdma_rq.id,
> +				 rxq->wqe_cnt_to_short_db *
> +
> 	GDMA_WQE_ALIGNMENT_UNIT_SIZE,
> +				 0);
> +	} else {

Is it possible that rxq->wqe_cnt_to_short_db might be 0 from the caller? The caller shouldn't ring the doorbell if there is nothing to ring.





> +		/* No need to ring, just return */
> +		ret = 0;
> +	}
> +#else
>  	ret = mana_ring_doorbell(db_page, GDMA_QUEUE_RECEIVE,
>  			 rxq->gdma_rq.id,
>  			 rxq->gdma_rq.head *
> GDMA_WQE_ALIGNMENT_UNIT_SIZE,
>  			 0);
> +#endif
> 
>  	if (ret)
>  		DP_LOG(ERR, "failed to ring RX doorbell ret %d", ret); @@ -
> 97,6 +110,7 @@ mana_alloc_and_post_rx_wqe(struct mana_rxq *rxq)
>  		/* update queue for tracking pending packets */
>  		desc->pkt = mbuf;
>  		desc->wqe_size_in_bu = wqe_size_in_bu;
> +		rxq->wqe_cnt_to_short_db += wqe_size_in_bu;
>  		rxq->desc_ring_head = (rxq->desc_ring_head + 1) % rxq-
> >num_desc;
>  	} else {
>  		DP_LOG(DEBUG, "failed to post recv ret %d", ret); @@ -
> 115,12 +129,22 @@ mana_alloc_and_post_rx_wqes(struct mana_rxq *rxq)
>  	int ret;
>  	uint32_t i;
> 
> +#ifdef RTE_ARCH_32
> +	rxq->wqe_cnt_to_short_db = 0;
> +#endif
>  	for (i = 0; i < rxq->num_desc; i++) {
>  		ret = mana_alloc_and_post_rx_wqe(rxq);
>  		if (ret) {
>  			DP_LOG(ERR, "failed to post RX ret = %d", ret);
>  			return ret;
>  		}
> +
> +#ifdef RTE_ARCH_32
> +		if (rxq->wqe_cnt_to_short_db >
> RX_WQE_SHORT_DB_THRESHOLD) {
> +			mana_rq_ring_doorbell(rxq);
> +			rxq->wqe_cnt_to_short_db = 0;
> +		}
> +#endif
>  	}
> 
>  	mana_rq_ring_doorbell(rxq);
> @@ -349,6 +373,9 @@ mana_start_rx_queues(struct rte_eth_dev *dev)
> 
>  		/* CQ head starts with count */
>  		rxq->gdma_cq.head = rxq->gdma_cq.count;
> +#ifdef RTE_ARCH_32
> +		rxq->gdma_cq.head_incr_to_short_db = 0; #endif
> 
>  		DRV_LOG(INFO, "rxq cq id %u buf %p count %u size %u",
>  			rxq->gdma_cq.id, rxq->gdma_cq.buffer, @@ -397,6
> +424,10 @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
> uint16_t pkts_n)
>  	uint32_t i;
>  	int polled = 0;
> 
> +#ifdef RTE_ARCH_32
> +	rxq->wqe_cnt_to_short_db = 0;
> +#endif
> +
>  repoll:
>  	/* Polling on new completions if we have no backlog */
>  	if (rxq->comp_buf_idx == rxq->comp_buf_len) { @@ -505,6 +536,16
> @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t
> pkts_n)
>  		wqe_posted++;
>  		if (pkt_received == pkts_n)
>  			break;
> +
> +#ifdef RTE_ARCH_32
> +		/* Ring short doorbell if approaching the wqe increment
> +		 * limit.
> +		 */
> +		if (rxq->wqe_cnt_to_short_db >
> RX_WQE_SHORT_DB_THRESHOLD) {
> +			mana_rq_ring_doorbell(rxq);
> +			rxq->wqe_cnt_to_short_db = 0;
> +		}
> +#endif
>  	}
> 
>  	rxq->backlog_idx = pkt_idx;
> @@ -529,6 +570,16 @@ static int
>  mana_arm_cq(struct mana_rxq *rxq, uint8_t arm)  {
>  	struct mana_priv *priv = rxq->priv;
> +#ifdef RTE_ARCH_32
> +	uint16_t cqe_incr = (uint16_t)rxq->gdma_cq.head_incr_to_short_db;

How do you make sure head_incr_to_short_db doesn't overflow?


Thanks,

Long