Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api

[Olivier Matz <olivier.matz@6wind.com>]

Removing wrong mails from the To and Cc. Sorry.

On Mon, Jun 25, 2018 at 04:20:57PM +0200, Olivier Matz wrote:
> The Tx descriptor status api was not behaving as expected. This API is
> used to inspect the content of the descriptors in the Tx ring to
> determine the length of the Tx queue.
> 
> Since the software advances the tail pointer and the hardware advances
> the head pointer, the Tx queue is located before txq->tx_tail in the
> ring. Therefore, a call to rte_eth_tx_descriptor_status(..., offset=20)
> should inspect the 20th descriptor before the tail, not after.
> 
> As before, we still need to take care about only checking descriptors
> that have the RS bit.
> 
> Additionally, we can avoid an access to the ring if offset is greater or
> equal to nb_tx_desc - nb_tx_free.
> 
> Fixes: a2919e13d95e ("net/ixgbe: implement descriptor status API")
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> ---
> 
> Hi Wei, Hi Qi,
> 
> We also recently found some issues in Tx descriptor status API for ixgbe,
> i40, e1000, igb. I'm preparing a clean patchset for all of them.
> 
> Here is already the patch for ixgbe, please let me know what you think.
> 
> The API comment of rte_eth_tx_descriptor_status() is incorrect and should
> be fixed too. The reference descriptor (when offset = 0) is not where the
> next packet will be sent, but where the latest packet has been enqueued.
> 
> Regards,
> Olivier
> 
> 
> 
>  drivers/net/ixgbe/ixgbe_rxtx.c | 45 +++++++++++++++++++++++++++++++-----------
>  drivers/net/ixgbe/ixgbe_rxtx.h |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 3e13d26ae..384587cc6 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -2606,10 +2606,15 @@ ixgbe_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  	    hw->mac.type == ixgbe_mac_X540_vf ||
>  	    hw->mac.type == ixgbe_mac_X550_vf ||
>  	    hw->mac.type == ixgbe_mac_X550EM_x_vf ||
> -	    hw->mac.type == ixgbe_mac_X550EM_a_vf)
> +	    hw->mac.type == ixgbe_mac_X550EM_a_vf) {
>  		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw, IXGBE_VFTDT(queue_idx));
> -	else
> +		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> +				IXGBE_VFTDH(queue_idx));
> +	} else {
>  		txq->tdt_reg_addr = IXGBE_PCI_REG_ADDR(hw, IXGBE_TDT(txq->reg_idx));
> +		txq->tdh_reg_addr = IXGBE_PCI_REG_ADDR(hw,
> +				IXGBE_TDH(txq->reg_idx));
> +	}
>  
>  	txq->tx_ring_phys_addr = tz->iova;
>  	txq->tx_ring = (union ixgbe_adv_tx_desc *) tz->addr;
> @@ -3140,22 +3145,38 @@ ixgbe_dev_tx_descriptor_status(void *tx_queue, uint16_t offset)
>  {
>  	struct ixgbe_tx_queue *txq = tx_queue;
>  	volatile uint32_t *status;
> -	uint32_t desc;
> +	int32_t desc, dd;
>  
>  	if (unlikely(offset >= txq->nb_tx_desc))
>  		return -EINVAL;
> +	if (offset >= txq->nb_tx_desc - txq->nb_tx_free)
> +		return RTE_ETH_TX_DESC_DONE;
> +
> +	desc = txq->tx_tail - offset - 1;
> +	if (desc < 0)
> +		desc += txq->nb_tx_desc;
>  
> -	desc = txq->tx_tail + offset;
> -	/* go to next desc that has the RS bit */
> -	desc = ((desc + txq->tx_rs_thresh - 1) / txq->tx_rs_thresh) *
> -		txq->tx_rs_thresh;
> -	if (desc >= txq->nb_tx_desc) {
> -		desc -= txq->nb_tx_desc;
> -		if (desc >= txq->nb_tx_desc)
> -			desc -= txq->nb_tx_desc;
> +	/* offset is too small, no other way than reading PCI reg */
> +	if (unlikely(offset < txq->tx_rs_thresh)) {
> +		int16_t tx_head, queue_size;
> +		tx_head = ixgbe_read_addr(txq->tdh_reg_addr);
> +		queue_size = txq->tx_tail - tx_head;
> +		if (queue_size < 0)
> +			queue_size += txq->nb_tx_desc;
> +		return queue_size > offset ? RTE_ETH_TX_DESC_FULL :
> +			RTE_ETH_TX_DESC_DONE;
>  	}
>  
> -	status = &txq->tx_ring[desc].wb.status;
> +	/* index of the dd bit to look at */
> +	dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;
> +
> +	/* In full featured mode, RS bit is only set in the last descriptor */
> +	/* of a multisegments packet */
> +	if (!((txq->offloads == 0) &&
> +	      (txq->tx_rs_thresh >= RTE_PMD_IXGBE_TX_MAX_BURST)))
> +		dd = txq->sw_ring[dd].last_id;
> +
> +	status = &txq->tx_ring[dd].wb.status;
>  	if (*status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD))
>  		return RTE_ETH_TX_DESC_DONE;
>  
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.h b/drivers/net/ixgbe/ixgbe_rxtx.h
> index 39378f754..384f6324d 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.h
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.h
> @@ -201,6 +201,7 @@ struct ixgbe_tx_queue {
>  		struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW ring for vector PMD */
>  	};
>  	volatile uint32_t   *tdt_reg_addr; /**< Address of TDT register. */
> +	volatile uint32_t   *tdh_reg_addr; /**< Address of TDH register. */
>  	uint16_t            nb_tx_desc;    /**< number of TX descriptors. */
>  	uint16_t            tx_tail;       /**< current value of TDT reg. */
>  	/**< Start freeing TX buffers if there are less free descriptors than
> -- 
> 2.11.0
>