[DPDK/ethdev Bug 1392] mlx5: random superfluous setting mbuf:hash.fdir.hi

[bugzilla@dpdk.org]

[-- Attachment #1: Type: text/plain, Size: 6683 bytes --]

https://bugs.dpdk.org/show_bug.cgi?id=1392

            Bug ID: 1392
           Summary: mlx5: random superfluous setting mbuf:hash.fdir.hi
           Product: DPDK
           Version: unspecified
          Hardware: x86
                OS: All
            Status: UNCONFIRMED
          Severity: minor
          Priority: Normal
         Component: ethdev
          Assignee: dev@dpdk.org
          Reporter: konstantin.v.ananyev@yandex.ru
  Target Milestone: ---

Reproducible on latest version of DPDK main branch (24.03 rc1).

While mocking with DPDK+mlx5 device(MT27800 Family [ConnectX-5] 1017)
noticed that mlx5_rx_burst_vec() on x86 can sometimes store
junk values inside  mbuf:hash.fdir.hi, even though rte_flow 'MARK'
action was not requested and RTE_MBUF_F_RX_FDIR is not set in the
mbuf:ol_flags.
It can be easily reproduced with testpmd too,
to catch it I added the following code into it:

--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -872,6 +872,10 @@ common_fwd_stream_receive(struct fwd_stream *fs, struct
rte_mbuf **burst,
        if (record_burst_stats)
                fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
        fs->rx_packets += nb_rx;
+
+       for (uint16_t i = 0; i != nb_rx; i++)
+               RTE_VERIFY(burst[i]->hash.fdir.hi == 0 ||
+                       (burst[i]->ol_flags & RTE_MBUF_F_RX_FDIR) != 0);
        return nb_rx;
 }

Then:
./dpdk-testpmd --lcores='49,51' -n 6 -a ca:00.0 -a ca:00.1 -a cb:00.0 -a
cb:00.1 --  --rxd=1024 --txd=1024 --rss-ip --rxq=2 --txq=2
...
EAL: PANIC in common_fwd_stream_receive():
line 877        assert "burst[i]->hash.fdir.hi == 0 || (burst[i]->ol_flags &
RTE_MBUF_F_RX_FDIR) != 0" failed
0: /home/kananyev/dpdk.org/x84_64-default-linuxapp-gcc10-dbg/app/dpdk-testpmd
(rte_dump_stack+0x1f) [1781b0d]
...
(gdb) print/x burst[i]->hash.fdir
$5 = {{{hash = 0xc176, id = 0xd46c}, lo = 0xd46cc176}, hi = 0x76c16c}
(gdb) print/x burst[i]->ol_flags
$6 = 0x182 // RTE_MBUF_F_RX_RSS_HASH |  RTE_MBUF_F_RX_IP_CKSUM_GOOD |
RTE_MBUF_F_RX_L4_CKSUM_GOOD

A bit of analysis (to put it upfront - I am not that familiar with mlx5 PMD,
so would need mlx5 maintainer to have a proper look):

first, hash.fdir.hi are being updated here:
#0  _mm_storeu_si128 (__B=..., __P=0x17eb98aa4)
    at /usr/lib64/gcc/x86_64-suse-linux/10/include/emmintrin.h:728
#1  rxq_cq_process_v (rxq=0x1727b1f80, cq=0x11f3c0a800, elts=0x1727b3240,
    pkts=0x7ffff2f23ea0, pkts_n=32, err=0x7ffff2f23d98, comp=0x7ffff2f21038)
    at ../drivers/net/mlx5/mlx5_rxtx_vec_sse.h:697

It just copies cq[..].sop_drop_qpn value here:
(gdb) print/x cq[pos + p3].sop_drop_qpn
$30 = 0x2b1fc59d

What is interesting, it copies it unconditionally:
const __m128i flow_mark_adj = _mm_set_epi32(rxq->mark * (-1), 0, 0, 0);
...
pkt_mb3 = _mm_shuffle_epi8(cqes[3], shuf_mask);
...
pkt_mb3 = _mm_add_epi32(pkt_mb3, flow_mark_adj);
...
_mm_storeu_si128((void *)&pkts[pos + 3]->pkt_len, pkt_mb3);

While scalar version of RX updates hash.fdir.hi conditionally, i.e. only when
rxq->mark is set.
Then later vector version realizes that corresponding cqe[] entry was a
compressed one, so
it continues with   rxq_cq_decompress_v() for the same mbuf.
But rxq_cq_decompress_v() updates hash.fdir.hi (and corresponding ol_flags)
*only* when rxq->mark is set.
Otherwise, it doesn't touch hash.fdir.hi at all.
So, as rxq->mark==0,  we ended up with 'mbuf.hash.fdir.hi' set to some junk
value,
while (ol_flags & RTE_MBUF_F_RX_FDIR) == 0.  


Note that by itself, it is probably not a big issue, as that happens when
RTE_MBUF_F_RX_FDIR is not set.
So majority of well behaving app will run just fine, and this small problem
will be un-noticed.
Though event (and probably sched) framework silently and un-conditionally
re-uses mbuf::hash.fdir.hi for its own purposes:
struct {
  uint32_t reserved1;
  uint16_t reserved2;
  uint16_t txq;
  /**< The event eth Tx adapter uses this field
   * to store Tx queue id.
   * @see rte_event_eth_tx_adapter_txq_set()
   */
} txadapter; /**< Eventdev ethdev Tx adapter */

So some of libevent functions expects hash.txadapter.txq to be set into
particular 
value, and such superfluous update of mbuf:hash.fdir.hi can cause a crash.
That's how I hit that issue first: https://bugs.dpdk.org/show_bug.cgi?id=1391

Plus it still looks like sort of inconsistency and undefined bheaviour.
So I decided to report it.
Possible fix (again, I am not sure that it the best way, but at least it
works):
inside rxq_cq_process_v(),  just zero-out hash.fdir.hi when rxq->mark is not
set:    

diff --git a/drivers/net/mlx5/mlx5_rxtx_vec_sse.h
b/drivers/net/mlx5/mlx5_rxtx_vec_sse.h
index 2bdd1f676d..8ab5dcc19e 100644
--- a/drivers/net/mlx5/mlx5_rxtx_vec_sse.h
+++ b/drivers/net/mlx5/mlx5_rxtx_vec_sse.h
@@ -600,6 +600,8 @@ rxq_cq_process_v(struct mlx5_rxq_data *rxq, volatile struct
mlx5_cqe *cq,
                              0,
                              rxq->crc_present * RTE_ETHER_CRC_LEN);
        const __m128i flow_mark_adj = _mm_set_epi32(rxq->mark * (-1), 0, 0, 0);
+       const __m128i flow_mark_msk =
+               _mm_set_epi32(rxq->mark * (-1), -1, -1, -1);
        /*
         * A. load first Qword (8bytes) in one loop.
         * B. copy 4 mbuf pointers from elts ring to returning pkts.
@@ -693,6 +695,8 @@ rxq_cq_process_v(struct mlx5_rxq_data *rxq, volatile struct
mlx5_cqe *cq,
                /* C.4 adjust flow mark. */
                pkt_mb3 = _mm_add_epi32(pkt_mb3, flow_mark_adj);
                pkt_mb2 = _mm_add_epi32(pkt_mb2, flow_mark_adj);
+               pkt_mb3 = _mm_and_si128(pkt_mb3, flow_mark_msk);
+               pkt_mb2 = _mm_and_si128(pkt_mb2, flow_mark_msk);
                /* D.1 fill in mbuf - rx_descriptor_fields1. */
                _mm_storeu_si128((void *)&pkts[pos + 3]->pkt_len, pkt_mb3);
                _mm_storeu_si128((void *)&pkts[pos + 2]->pkt_len, pkt_mb2);
@@ -720,6 +724,8 @@ rxq_cq_process_v(struct mlx5_rxq_data *rxq, volatile struct
mlx5_cqe *cq,
                /* C.4 adjust flow mark. */
                pkt_mb1 = _mm_add_epi32(pkt_mb1, flow_mark_adj);
                pkt_mb0 = _mm_add_epi32(pkt_mb0, flow_mark_adj);
+               pkt_mb1 = _mm_and_si128(pkt_mb1, flow_mark_msk);
+               pkt_mb0 = _mm_and_si128(pkt_mb0, flow_mark_msk);
                /* E.1 extract op_own byte. */
                op_own_tmp1 = _mm_unpacklo_epi32(cqes[0], cqes[1]);
                op_own = _mm_unpackhi_epi64(op_own_tmp1, op_own_tmp2);

-- 
You are receiving this mail because:
You are the assignee for the bug.

[-- Attachment #2: Type: text/html, Size: 8911 bytes --]