Re: [PATCH] net/iavf: fix checksum offloading

[David Marchand <david.marchand@redhat.com>]

On Tue, Aug 22, 2023 at 3:52 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, August 22, 2023 1:29 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> > Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> >
> > On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@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