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:16:19 +0000 [thread overview]
Message-ID: <aQo049BYyDIxh_JE@bricha3-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <aQo0QLplPKmTbZTS@bricha3-mobl1.ger.corp.intel.com>
On Tue, Nov 04, 2025 at 05:13:36PM +0000, Bruce Richardson wrote:
> 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);
Very minor nit - this line doesn't need to be wrapped, it fits in 100
chars.
> > }
> > #endif
> >
> > --
> > 2.34.1
> >
next prev parent reply other threads:[~2025-11-04 17:16 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
2025-11-04 17:16 ` Bruce Richardson [this message]
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=aQo049BYyDIxh_JE@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).