RE: [PATCH] net: fix checksum with unaligned buffer

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Tuesday, 21 June 2022 10.23
> 
> On Tue, Jun 21, 2022 at 10:05:15AM +0200, Morten Brørup wrote:
> > +TO: @Bruce and @Stephen: You signed off on the 16 bit alignment
> requirement. We need background info on this.
> >
> > > From: Emil Berg [mailto:emil.berg@ericsson.com]
> > > Sent: Tuesday, 21 June 2022 09.17
> > >
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > Sent: den 20 juni 2022 12:58
> > > >
> > > > > From: Emil Berg [mailto:emil.berg@ericsson.com]
> > > > > Sent: Monday, 20 June 2022 12.38
> > > > >
> > > > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > > > Sent: den 17 juni 2022 11:07
> > > > > >
> > > > > > > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > > > > > > Sent: Friday, 17 June 2022 10.45
> > > > > > >
> > > > > > > With this patch, the checksum can be calculated on an
> unligned
> > > > > > > part
> > > > > of
> > > > > > > a packet buffer.
> > > > > > > I.e. the buf parameter is no longer required to be 16 bit
> > > aligned.
> > > > > > >
> > > > > > > The DPDK invariant that packet buffers must be 16 bit
> aligned
> > > > > remains
> > > > > > > unchanged.
> > > > > > > This invariant also defines how to calculate the 16 bit
> > > checksum
> > > > > > > on
> > > > > an
> > > > > > > unaligned part of a packet buffer.
> > > > > > >
> > > > > > > Bugzilla ID: 1035
> > > > > > > Cc: stable@dpdk.org
> > > > > > >
> > > > > > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > > > > > ---
> > > > > > >  lib/net/rte_ip.h | 17 +++++++++++++++--
> > > > > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h index
> > > > > > > b502481670..8e301d9c26 100644
> > > > > > > --- a/lib/net/rte_ip.h
> > > > > > > +++ b/lib/net/rte_ip.h
> > > > > > > @@ -162,9 +162,22 @@ __rte_raw_cksum(const void *buf,
> size_t
> > > len,
> > > > > > > uint32_t sum)  {
> > > > > > >  	/* extend strict-aliasing rules */
> > > > > > >  	typedef uint16_t __attribute__((__may_alias__))
> u16_p;
> > > > > > > -	const u16_p *u16_buf = (const u16_p *)buf;
> > > > > > > -	const u16_p *end = u16_buf + len / sizeof(*u16_buf);
> > > > > > > +	const u16_p *u16_buf;
> > > > > > > +	const u16_p *end;
> > > > > > > +
> > > > > > > +	/* if buffer is unaligned, keeping it byte order
> > > independent */
> > > > > > > +	if (unlikely((uintptr_t)buf & 1)) {
> > > > > > > +		uint16_t first = 0;
> > > > > > > +		if (unlikely(len == 0))
> > > > > > > +			return 0;
> > > > > > > +		((unsigned char *)&first)[1] = *(const unsigned
> > > > > > char *)buf;
> > > > > > > +		sum += first;
> > > > > > > +		buf = (const void *)((uintptr_t)buf + 1);
> > > > > > > +		len--;
> > > > > > > +	}
> > > > > > >
> > > > > > > +	u16_buf = (const u16_p *)buf;
> > > > > > > +	end = u16_buf + len / sizeof(*u16_buf);
> > > > > > >  	for (; u16_buf != end; ++u16_buf)
> > > > > > >  		sum += *u16_buf;
> > > > > > >
> > > > > > > --
> > > > > > > 2.17.1
> > > > > >
> > > > > > @Emil, can you please test this patch with an unaligned
> buffer on
> > > > > your
> > > > > > application to confirm that it produces the expected result.
> > > > >
> > > > > Hi!
> > > > >
> > > > > I tested the patch. It doesn't seem to produce the same
> results. I
> > > > > think the problem is that it always starts summing from an even
> > > > > address, the sum should always start from the first byte
> according
> > > to
> > > > > the checksum specification. Can I instead propose something
> Mattias
> > > > > Rönnblom sent me?
> > > >
> > > > I assume that it produces the same result when the "buf"
> parameter is
> > > > aligned?
> > > >
> > > > And when the "buf" parameter is unaligned, I don't expect it to
> > > produce the
> > > > same results as the simple algorithm!
> > > >
> > > > This was the whole point of the patch: I expect the overall
> packet
> > > buffer to
> > > > be 16 bit aligned, and the checksum to be a partial checksum of
> such
> > > a 16 bit
> > > > aligned packet buffer. When calling this function, I assume that
> the
> > > "buf" and
> > > > "len" parameters point to a part of such a packet buffer. If
> these
> > > > expectations are correct, the simple algorithm will produce
> incorrect
> > > results
> > > > when "buf" is unaligned.
> > > >
> > > > I was asking you to test if the checksum on the packet is correct
> > > when your
> > > > application modifies an unaligned part of the packet and uses
> this
> > > function to
> > > > update the checksum.
> > > >
> > >
> > > Now I understand your use case. Your use case seems to be about
> partial
> > > checksums, of which some partial checksums may start on unaligned
> > > addresses in an otherwise aligned packet.
> > >
> > > Our use case is about calculating the full checksum on a nested
> packet.
> > > That nested packet may start on unaligned addresses.
> > >
> > > The difference is basically if we want to sum over aligned
> addresses or
> > > not, handling the heading and trailing bytes appropriately.
> > >
> > > Your method does not work in our case since we want to treat the
> first
> > > two bytes as the first word in our case. But I do understand that
> both
> > > methods are useful.
> >
> > Yes, that certainly are two different use cases, requiring two
> different ways of calculating the 16 bit checksum.
> >
> > >
> > > Note that your method breaks the API. Previously (assuming no
> crashing
> > > due to low optimization levels, more accepting hardware, or a
> different
> > > compiler (version)) the current method would calculate the checksum
> > > assuming the first two bytes is the first word.
> > >
> >
> > Depending on the point of view, my patch either fixes a bug (where
> the checksum was calculated incorrectly when the buf pointer was
> unaligned) or breaks the API (by calculating the differently when the
> buffer is unaligned).
> >
> > I cannot say with certainty which one is correct, but perhaps some of
> the people with a deeper DPDK track record can...
> >
> > @Bruce and @Stephen, in 2019 you signed off on a patch [1]
> introducing a 16 bit alignment requirement to the Ethernet address
> structure.
> >
> > It is my understanding that DPDK has an invariant requiring packets
> to be 16 bit aligned, which that patch supports. Is this invariant
> documented anywhere, or am I completely wrong? If I'm wrong, then the
> alignment requirement introduced in that patch needs to be removed, as
> well as any similar alignment requirements elsewhere in DPDK.
> 
> I don't believe it is explicitly documented as a global invariant, but
> I
> think it should be unless there is a definite case where we need to
> allow
> packets to be completely unaligned. Across all packet headers we looked
> at,
> there was no tunneling protocol where the resulting packet was left
> unaligned.
> 
> That said, if there are real use cases where we need to allow packets
> to
> start at an unaligned address, then I agree with you that we need to
> roll
> back the patch and work to ensure everything works with unaligned
> addresses.
> 
> /Bruce
>

@Emil, can you please describe or refer to which tunneling protocol you are using, where the nested packet can be unaligned?

I am asking to determine if your use case is exotic (maybe some Ericsson proprietary protocol), or more generic (rooted in some standard protocol). This information affects the DPDK community's opinion about how it should be supported by DPDK.

If possible, please provide more details about the tunneling protocol and nested packets... E.g. do the nested packets also contain Layer 2 (Ethernet, VLAN, etc.) headers, or only Layer 3 (IP) or Layer 4 (TCP, UDP, etc.)? And how about ARP packets and Layer 2 control protocol packets (STP, LACP, etc.)?

> >
> > [1]
> http://git.dpdk.org/dpdk/commit/lib/librte_net/rte_ether.h?id=da5350ef2
> 9afd35c1adabe76f60832f3092269ad
> >
> > @Emil, we should wait for a conclusion about the alignment invariant
> before we proceed.
> >
> > If there is no such invariant, my patch is wrong, and we need to
> provide a v2 of the patch, which will then fit your use case.
> > If there is such an invariant, my patch is correct, and another
> function must be added for your use case.
> >