RE: [dpdk-dev] [PATCH v3] app/testpmd: fix l4 sw csum over multi segments

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

Hi

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Friday, October 29, 2021 09:29
> To: Morten Brørup <mb@smartsharesystems.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> stephen@networkplumber.org; dev@dpdk.org; stable@dpdk.org;
> Medvedkin, Vladimir <vladimir.medvedkin@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3] app/testpmd: fix l4 sw csum over multi
> segments
> 
> On Wed, Oct 27, 2021 at 01:29:52PM +0200, Morten Brørup wrote:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> > > Sent: Wednesday, 27 October 2021 12.49
> > >
> > > On 10/20/2021 11:12 AM, Xiaoyun Li wrote:
> > > > In csum forwarding mode, software UDP/TCP csum calculation only
> > > > takes the first segment into account while using the whole packet
> > > > length so the calculation will read invalid memory region with
> > > > multi-segments packets and will get wrong value.
> > > > This patch fixes this issue.
> > > >
> > > > Fixes: af75078fece3 ("first public release")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > > > ---
> > > > v3:
> > > >   * Use rte_raw_cksum() for multi-segs case instead of copying the
> > > whole
> > > >   * packet.
> > > > v2:
> > > >   * Use static stack memory instead of dynamic allocating in
> > > > datapath
> > > > ---
> > > >   app/test-pmd/csumonly.c | 68
> > > > ++++++++++++++++++++++++++++++++------
> > > ---
> > > >   1 file changed, 53 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> > > > index 090797318a..f3e60eb3c3 100644
> > > > --- a/app/test-pmd/csumonly.c
> > > > +++ b/app/test-pmd/csumonly.c
> > > > @@ -91,12 +91,41 @@ struct simple_gre_hdr {
> > > >   } __rte_packed;
> > > >
> > > >   static uint16_t
> > > > -get_udptcp_checksum(void *l3_hdr, void *l4_hdr, uint16_t
> > > > ethertype)
> > > > +get_udptcp_checksum(void *l3_hdr, struct rte_mbuf *m, uint16_t
> > > l4_off,
> > > > +		    uint16_t ethertype)
> > > >   {
> > > > +	uint16_t off = l4_off;
> > > > +	uint32_t cksum = 0;
> > > > +	char *buf;
> > > > +
> > > > +	while (m != NULL) {
> > > > +		buf = rte_pktmbuf_mtod_offset(m, char *, off);
> > > > +		cksum += rte_raw_cksum(buf, m->data_len - off);
> > > > +		off = 0;
> > > > +		m = m->next;
> > > > +	}
> > > >   	if (ethertype == _htons(RTE_ETHER_TYPE_IPV4))
> > > > -		return rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > > > +		cksum += rte_ipv4_phdr_cksum(l3_hdr, 0);
> > > >   	else /* assume ethertype == RTE_ETHER_TYPE_IPV6 */
> > > > -		return rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > > > +		cksum += rte_ipv6_phdr_cksum(l3_hdr, 0);
> > > > +
> > >
> > > Hi Xiaoyun,
> > >
> > > I can see 'rte_ipv[46]_udptcp_cksum()' is not taking multi segment
> > > mbuf into account, so this fix is required, but instead of
> > > implementing this logic into testpmd, what do you think to have APIs
> > > to support multi segment mbufs?
> > > This way other applications also benefit from it and we don't need
> > > to maintain ip4/6 checksum related code in testpmd.
> >
> > +1
> >
> > Also, there is no need to implement the multi-segment raw checksum loop
> in test-pmd.
> >
> > You can use the multi-segment raw checksum function in the net library
> instead:
> > http://code.dpdk.org/dpdk/latest/source/lib/net/rte_ip.h#L224
> 
> +1
> 
> We can have mbuf variants of udptcp checksum functions:
> 
> rte_ipv4_udptcp_cksum()
> rte_ipv4_udptcp_cksum_verify()
> rte_ipv6_udptcp_cksum()
> rte_ipv6_udptcp_cksum_verify()
> 
> Adding a "_mbuf" suffix would be consistent with rte_raw_cksum_mbuf().

Thanks for the suggestion. Since it's API change, I'll send these in 22.03 release with release note.
V4 will come soon.

> 
> 
> Olivier
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.