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

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 2:12 PM
> 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 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.

Yes, you may be correct; it appears that this issue is unrelated to l3_len. The primary concern is to prevent the configuration of Tx descriptors with incorrect values.

Based on your description, it seems the problem arises when the PMD sets MACLEN to 0 and also configures IIPT as 01b, Is this correct?

To prevent this issue, we could implement a check where, if l2_len is 0, we simply ignore the IIPT configuration and keep it at 0. (which leads to same configure with your patch)

Regarding your mention of 'fix access to l2_len,' if l2_len is 0, there's no change in the offset regardless of whether l2_len is accessed or not. Did you mean setting a fixed value of MACLEN to 14?"









> 
> 
> --
> David Marchand