On Tue, Aug 22, 2023 at 3:52 AM Zhang, Qi Z <qi.z.zh...@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.march...@redhat.com>
> > Sent: Tuesday, August 22, 2023 1:29 AM
> > To: Zhang, Qi Z <qi.z.zh...@intel.com>
> > Cc: dev@dpdk.org; echau...@redhat.com; m...@redhat.com;
> > sta...@dpdk.org; Wu, Jingjing <jingjing...@intel.com>; Xing, Beilei
> > <beilei.x...@intel.com>; Doherty, Declan <declan.dohe...@intel.com>; Sinha,
> > Abhijit <abhijit.si...@intel.com>; Nicolau, Radu <radu.nico...@intel.com>
> > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> >
> > On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zh...@intel.com> wrote:
> > > > Subject: [PATCH] net/iavf: fix checksum offloading
> > > >
> > > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > > indicator that a checksum offload has been requested by an application.
> > >
> > > According to current implementation, actually the only presence of
> > RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an
> > 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > > So, I assume in this situation, the PMD continues to operate under the
> > assumption that the application has not requested checksum offloading.
> > >
> > > Could you share more insight what is the failure, maybe we can perform a
> > more comprehensive investigation?
> >
> > I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> > In our tests, we could see that tx_errors get incremented for each failed
> > packet
> > to transmit.
>
> OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better fix?
Looking at the mbuf API, l2_len and l3_len should be considered by a
driver if ol_flags contains at least one of RTE_MBUF_F_TX_SEC_OFFLOAD,
RTE_MBUF_F_TX_TUNNEL_*, RTE_MBUF_F_TX_TCP_SEG,
RTE_MBUF_F_TX_(IP|TCP|UDP|SCTP)_CKSUM.
Here, it is not the case.
If the driver reads l2_len or l3_len, this is an undefined behavior:
for example, OVS might have been using l2_len or l3_len for its
internal uses (though I agree it would be risky for an application to
do so).
We probably need to fix access to l2_len a few lines before my patch.
if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
!(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
offset |= (m->outer_l2_len >> 1)
<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
else
offset |= (m->l2_len >> 1)
<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
But to be clear, no I don't think looking at l3_len value is better:
it should not be read at all.
--
David Marchand
