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

[Olivier Matz <olivier.matz@6wind.com>]

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().


Olivier