From: Bruce Richardson <bruce.richardson@intel.com>
To: Ciara Loftus <ciara.loftus@intel.com>
Cc: <dev@dpdk.org>
Subject: Re: [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion
Date: Tue, 4 Nov 2025 17:13:36 +0000 [thread overview]
Message-ID: <aQo0QLplPKmTbZTS@bricha3-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <20251031152250.2441980-2-ciara.loftus@intel.com>
On Fri, Oct 31, 2025 at 03:22:49PM +0000, Ciara Loftus wrote:
> QinQ insertion was enabled in the bulk transmit function but not the
> single packet transmit function. Implement it in the single packet
> function.
>
> Also, fix an issue that would arise in the event an mbuf had both the
> VLAN and QINQ offload flags set. In this case the L2TAG2 field would be
> written to twice and could cause the tag to be corrupted. Reorder the
> logic of populating the L2TAG2 field and ensure that the field is only
> written to once.
>
While both these described changes fix the same commit, can you please
split this patch into two, because it fixes two separate issues with that
code.
Just one review comment inline below. You can keep my ack on any new
versions.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Fixes: 3aa4efa36438 ("net/iavf: support VLAN insertion in AVX512 Tx")
>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
> drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 50 ++++++++++---------
> drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 10 ++--
> 2 files changed, 31 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> index d40a858413..c800ae29e1 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> @@ -2077,7 +2077,13 @@ ctx_vtx1(volatile struct iavf_tx_desc *txdp, struct rte_mbuf *pkt,
> if (((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) || offload)) {
> if (offload)
> iavf_fill_ctx_desc_tunneling_avx512(&low_ctx_qw, pkt);
> - if ((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) ||
> + if (pkt->ol_flags & RTE_MBUF_F_TX_QINQ) {
> + uint64_t qinq_tag = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ?
> + (uint64_t)pkt->vlan_tci_outer :
> + (uint64_t)pkt->vlan_tci;
> + high_ctx_qw |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> + low_ctx_qw |= qinq_tag << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> + } else if ((pkt->ol_flags & RTE_MBUF_F_TX_VLAN) &&
> (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)) {
> high_ctx_qw |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> low_ctx_qw |= (uint64_t)pkt->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
We are missing handling here for the case of vlan_flag set and L2TAG2 flag
not set. However, that is handled by iavf_txd_enable_offload - but only if
IAVF_TX_VLAN_QINQ_OFFLOAD is set, which it is by default as far as I can see.
[Maybe a future patch we can remove that define???]. If it's not set, do we
need to handle the vlan case here? There also seems to be a little
duplication between the code here and in that function.
> @@ -2127,17 +2133,6 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> ((uint64_t)pkt[0]->data_len <<
> IAVF_TXD_QW1_TX_BUF_SZ_SHIFT);
>
> - if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> - if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> - hi_ctx_qw1 |=
> - IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> - low_ctx_qw1 |=
> - (uint64_t)pkt[1]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> - } else {
> - hi_data_qw1 |=
> - (uint64_t)pkt[1]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> - }
> - }
> if (pkt[1]->ol_flags & RTE_MBUF_F_TX_QINQ) {
> hi_ctx_qw1 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> @@ -2153,22 +2148,21 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> IAVF_TXD_QW1_L2TAG1_SHIFT;
> }
> - }
> - if (IAVF_CHECK_TX_LLDP(pkt[1]))
> - hi_ctx_qw1 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
> - << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> -
> - if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> + } else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> - hi_ctx_qw0 |=
> + hi_ctx_qw1 |=
> IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> - low_ctx_qw0 |=
> - (uint64_t)pkt[0]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> + low_ctx_qw1 |=
> + (uint64_t)pkt[1]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> } else {
> - hi_data_qw0 |=
> - (uint64_t)pkt[0]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> + hi_data_qw1 |=
> + (uint64_t)pkt[1]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> }
> }
> + if (IAVF_CHECK_TX_LLDP(pkt[1]))
> + hi_ctx_qw1 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
> + << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> +
> if (pkt[0]->ol_flags & RTE_MBUF_F_TX_QINQ) {
> hi_ctx_qw0 |= IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> @@ -2184,6 +2178,16 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> IAVF_TXD_QW1_L2TAG1_SHIFT;
> }
> + } else if (pkt[0]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> + if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> + hi_ctx_qw0 |=
> + IAVF_TX_CTX_DESC_IL2TAG2 << IAVF_TXD_CTX_QW1_CMD_SHIFT;
> + low_ctx_qw0 |=
> + (uint64_t)pkt[0]->vlan_tci << IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> + } else {
> + hi_data_qw0 |=
> + (uint64_t)pkt[0]->vlan_tci << IAVF_TXD_QW1_L2TAG1_SHIFT;
> + }
> }
> if (IAVF_CHECK_TX_LLDP(pkt[0]))
> hi_ctx_qw0 |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
> diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> index f513777663..bf8faf3632 100644
> --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> @@ -225,12 +225,6 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
> #endif
>
> #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
> - if (ol_flags & RTE_MBUF_F_TX_VLAN && vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1) {
> - td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> - *txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> - IAVF_TXD_QW1_L2TAG1_SHIFT);
> - }
> -
> if (ol_flags & RTE_MBUF_F_TX_QINQ) {
> td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1)
> @@ -239,6 +233,10 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
> else
> *txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
> IAVF_TXD_QW1_L2TAG1_SHIFT);
> + } else if (ol_flags & RTE_MBUF_F_TX_VLAN && vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1) {
> + td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> + *txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> + IAVF_TXD_QW1_L2TAG1_SHIFT);
> }
> #endif
>
> --
> 2.34.1
>
next prev parent reply other threads:[~2025-11-04 17:13 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-31 15:22 [PATCH 0/2] Fixes for iavf VLAN insertion offload Ciara Loftus
2025-10-31 15:22 ` [PATCH 1/2] net/iavf: fix AVX-512 double VLAN (QinQ) insertion Ciara Loftus
2025-11-04 17:13 ` Bruce Richardson [this message]
2025-11-04 17:16 ` Bruce Richardson
2025-10-31 15:22 ` [PATCH 2/2] net/iavf: fix single VLAN insertion positioning Ciara Loftus
2025-11-04 17:21 ` Bruce Richardson
2025-11-04 17:28 ` Bruce Richardson
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=aQo0QLplPKmTbZTS@bricha3-mobl1.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=ciara.loftus@intel.com \
--cc=dev@dpdk.org \
/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).