From: "Zhao1, Wei" <wei.zhao1@intel.com>
To: Olivier Matz <olivier.matz@6wind.com>,
"Zhao1@6wind.com" <Zhao1@6wind.com>,
"Zhang@6wind.com" <Zhang@6wind.com>,
"Zhang, Qi Z" <qi.z.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "Lu@6wind.com" <Lu@6wind.com>,
"Lu, Wenzhuo" <wenzhuo.lu@intel.com>
Subject: Re: [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api
Date: Tue, 26 Jun 2018 01:38:22 +0000 [thread overview]
Message-ID: <A2573D2ACFCADC41BB3BE09C6DE313CA07DFF21F@PGSMSX103.gar.corp.intel.com> (raw)
In-Reply-To: <20180625142057.6296-1-olivier.matz@6wind.com>
Hi, Olivier Matz
Will you commit fix patch for i40e and ixgbe and em?
And the code " dd = (desc / txq->tx_rs_thresh + 1) * txq->tx_rs_thresh - 1;"
Is only proper for tx function ixgbe_xmit_pkts_simple and ixgbe_xmit_pkts_vec ().
But not proper for ixgbe_xmit_pkts (), the RS bit set rule is different from all these two.
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, June 25, 2018 10:21 PM
> To: Zhao1@6wind.com; Zhao1, Wei <wei.zhao1@intel.com>;
> Zhang@6wind.com; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Lu@6wind.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: [RFC] net/ixgbe: fix Tx descriptor status api
>
> 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
next prev parent reply other threads:[~2018-06-26 1:38 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-22 8:38 [dpdk-dev] [PATCH] net/ixgbe: fix Tx check descriptor status APIs error Wei Zhao
2018-06-22 13:47 ` Zhang, Qi Z
2018-06-25 1:57 ` Zhao1, Wei
2018-06-25 2:48 ` Zhang, Qi Z
2018-06-25 5:58 ` Zhao1, Wei
2018-06-25 6:49 ` Zhao1, Wei
2018-06-25 7:47 ` Zhang, Qi Z
2018-06-25 7:52 ` Zhao1, Wei
2018-06-25 7:55 ` Zhang, Qi Z
2018-06-25 8:41 ` Zhao1, Wei
2018-06-25 14:20 ` [dpdk-dev] [RFC] net/ixgbe: fix Tx descriptor status api Olivier Matz
2018-06-25 14:30 ` Olivier Matz
2018-06-26 1:38 ` Zhao1, Wei [this message]
2018-06-26 8:46 ` Olivier Matz
2018-06-27 2:07 ` Zhao1, Wei
2018-06-27 13:38 ` Zhang, Qi Z
2018-06-26 1:24 ` [dpdk-dev] [PATCH v2] net/ixgbe: fix Tx check descriptor status APIs error Wei Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A2573D2ACFCADC41BB3BE09C6DE313CA07DFF21F@PGSMSX103.gar.corp.intel.com \
--to=wei.zhao1@intel.com \
--cc=Lu@6wind.com \
--cc=Zhang@6wind.com \
--cc=Zhao1@6wind.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.com \
--cc=qi.z.zhang@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).