From: Bruce Richardson <bruce.richardson@intel.com>
To: Ciara Loftus <ciara.loftus@intel.com>
Cc: <dev@dpdk.org>
Subject: Re: [PATCH 2/2] net/iavf: fix single VLAN insertion positioning
Date: Tue, 4 Nov 2025 17:28:17 +0000 [thread overview]
Message-ID: <aQo3sVWdZuxxAmgh@bricha3-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <aQo2BiGru17VyD_j@bricha3-mobl1.ger.corp.intel.com>
On Tue, Nov 04, 2025 at 05:21:10PM +0000, Bruce Richardson wrote:
> On Fri, Oct 31, 2025 at 03:22:50PM +0000, Ciara Loftus wrote:
> > Commit fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar
> > Tx") broke single VLAN insertion offload in cases where the v2 offload
> > capability and both inner and outer insertion were supported because it
> > caused inner VLAN tags to be inserted instead of outer.
> >
> > When an iavf tx queue is being set up, if v2 offload capability is
> > supported, the driver queries the insertion capabilities and takes note
> > of where VLAN tags should be placed in the transmit and/or context
> > descriptors for insertion offload. In the offending commit, when both
> > inner and outer insertion was reported as supported, the flag
> > "vlan_flag" was changed to hold the location for inner VLAN tags.
> > However this caused inner VLAN tags to be inserted in the case of single
> > VLAN offload which is incorrect behaviour for this use case.
> >
> > To fix this, revert the "vlan_flag" back to holding the location for
> > outer VLAN tags and update the datapath code accordingly.
> >
> > Fixes: fdc37964c2bf ("net/iavf: support QinQ insertion offload in scalar Tx")
> >
> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
>
> One suggestion inline below.
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
>
> > ---
> > drivers/net/intel/iavf/iavf_rxtx.c | 50 +++++++------------
> > drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 24 ++++-----
> > drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 5 +-
> > 3 files changed, 32 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/net/intel/iavf/iavf_rxtx.c b/drivers/net/intel/iavf/iavf_rxtx.c
> > index a3ef13c791..66f718424a 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx.c
> > +++ b/drivers/net/intel/iavf/iavf_rxtx.c
> > @@ -799,32 +799,17 @@ iavf_dev_tx_queue_setup(struct rte_eth_dev *dev,
> > &adapter->vf.vlan_v2_caps.offloads.insertion_support;
> > uint32_t insertion_cap;
> >
> > - if (insertion_support->outer == VIRTCHNL_VLAN_UNSUPPORTED ||
> > - insertion_support->inner == VIRTCHNL_VLAN_UNSUPPORTED) {
> > - /* Only one insertion is supported. */
> > - if (insertion_support->outer)
> > - insertion_cap = insertion_support->outer;
> > - else
> > - insertion_cap = insertion_support->inner;
> > -
> > - if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> > - txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > - PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
> > - } else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
> > - txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> > - PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
> > - }
> > - } else {
> > - /* Both outer and inner insertion supported. */
> > - if (insertion_support->inner & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> > - txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > - PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG1");
> > - PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG2");
> > - } else {
> > - txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> > - PMD_INIT_LOG(DEBUG, "Inner VLAN insertion_cap: L2TAG2");
> > - PMD_INIT_LOG(DEBUG, "Outer VLAN insertion_cap: L2TAG1");
> > - }
> > + if (insertion_support->outer)
> > + insertion_cap = insertion_support->outer;
> > + else
> > + insertion_cap = insertion_support->inner;
> > +
> > + if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG1) {
> > + txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > + PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG1");
> > + } else if (insertion_cap & VIRTCHNL_VLAN_TAG_LOCATION_L2TAG2) {
> > + txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2;
> > + PMD_INIT_LOG(DEBUG, "VLAN insertion_cap: L2TAG2");
> > }
> > } else {
> > txq->vlan_flag = IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1;
> > @@ -2600,12 +2585,11 @@ iavf_fill_context_desc(volatile struct iavf_tx_context_desc *desc,
> > desc_qws->qw0 = rte_cpu_to_le_64(desc_qws->qw0);
> > desc_qws->qw1 = rte_cpu_to_le_64(desc_qws->qw1);
> >
> > + /* vlan_flag specifies VLAN tag location for VLAN, and outer tag location for QinQ. */
> > if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
> > + desc->l2tag2 = m->ol_flags & RTE_MBUF_F_TX_QINQ ? m->vlan_tci_outer : m->vlan_tci;
> > + else if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
> > desc->l2tag2 = m->vlan_tci;
>
> Minor issue, but the expression of the logic here is different to how its
> worked in the previous patch code. There we check the QINQ or VLAN tag
> first, and then have a condition based on the L2TAG* flag. Here we check
> the L2TAG? first then switch on the QINQ flag. Can you maybe rework to have
> all conditional checks for this consistent?
>
> > -
> > - if (m->ol_flags & RTE_MBUF_F_TX_QINQ)
> > - desc->l2tag2 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2 ? m->vlan_tci :
> > - m->vlan_tci_outer;
> > }
> >
> >
> > @@ -2660,11 +2644,11 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
> > l2tag1 |= m->vlan_tci;
> > }
> >
> > - /* Descriptor based QinQ insertion */
> > + /* Descriptor based QinQ insertion. vlan_flag specifies outer tag location. */
> > if (m->ol_flags & RTE_MBUF_F_TX_QINQ) {
> > command |= (uint64_t)IAVF_TX_DESC_CMD_IL2TAG1;
> > - l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci :
> > - m->vlan_tci_outer;
> > + l2tag1 = vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1 ? m->vlan_tci_outer :
> > + m->vlan_tci;
> > }
> >
> > if ((m->ol_flags &
> > diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> > index c800ae29e1..6f150cb1c1 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> > +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c
> > @@ -2136,17 +2136,17 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> > 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) {
> > - /* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > - low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> > - IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > - hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> > - IAVF_TXD_QW1_L2TAG1_SHIFT;
> > - } else {
> > /* Outer tag at L2TAG2, inner tag at L2TAG1. */
> > low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> > IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> > IAVF_TXD_QW1_L2TAG1_SHIFT;
> > + } else {
> > + /* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > + low_ctx_qw1 |= (uint64_t)pkt[1]->vlan_tci <<
> > + IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > + hi_data_qw1 |= (uint64_t)pkt[1]->vlan_tci_outer <<
> > + IAVF_TXD_QW1_L2TAG1_SHIFT;
> > }
> > } else if (pkt[1]->ol_flags & RTE_MBUF_F_TX_VLAN) {
> > if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2) {
> > @@ -2166,17 +2166,17 @@ ctx_vtx(volatile struct iavf_tx_desc *txdp,
> > 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) {
> > - /* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > - low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> > - IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > - hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> > - IAVF_TXD_QW1_L2TAG1_SHIFT;
> > - } else {
> > /* Outer tag at L2TAG2, inner tag at L2TAG1. */
> > low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> > IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> > IAVF_TXD_QW1_L2TAG1_SHIFT;
> > + } else {
> > + /* Inner tag at L2TAG2, outer tag at L2TAG1. */
> > + low_ctx_qw0 |= (uint64_t)pkt[0]->vlan_tci <<
> > + IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
> > + hi_data_qw0 |= (uint64_t)pkt[0]->vlan_tci_outer <<
> > + 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) {
On first review, I missed the fact that you were also changing the logic in
the AVX512 code paths too. Since I don't see the update to the ctx_vtx1
function I assume that the previous patch updated that correctly the first
time, and that this patch corrects the ctx_vtx function only. Is that
correct?
In that case, I'd suggest that this patch should come first (or else be
split and put this AVX-512 bit first), so that we correct the ctx_vtx bulk
before adding the new and correct ctx_vtx1 implementation. As it now
stands, after patch 1 we have the two different ctx_vtx functions in the AVX512
path behaving in reversed fashion, which is rather strange.
/Bruce
> > diff --git a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> > index bf8faf3632..86523a7d2b 100644
> > --- a/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> > +++ b/drivers/net/intel/iavf/iavf_rxtx_vec_common.h
> > @@ -227,11 +227,12 @@ iavf_txd_enable_offload(__rte_unused struct rte_mbuf *tx_pkt,
> > #ifdef IAVF_TX_VLAN_QINQ_OFFLOAD
> > if (ol_flags & RTE_MBUF_F_TX_QINQ) {
> > td_cmd |= IAVF_TX_DESC_CMD_IL2TAG1;
> > + /* vlan_flag specifies outer tag location for QinQ. */
> > if (vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG1)
> > - *txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> > + *txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
> > IAVF_TXD_QW1_L2TAG1_SHIFT);
> > else
> > - *txd_hi |= ((uint64_t)tx_pkt->vlan_tci_outer <<
> > + *txd_hi |= ((uint64_t)tx_pkt->vlan_tci <<
> > 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;
> > --
> > 2.34.1
> >
prev parent reply other threads:[~2025-11-04 17:28 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
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 [this message]
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=aQo3sVWdZuxxAmgh@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).