From: Bruce Richardson <bruce.richardson@intel.com>
To: Emil Berg <emil.berg@ericsson.com>
Cc: "Morten Brørup" <mb@smartsharesystems.com>,
"Stephen Hemminger" <stephen@networkplumber.org>,
"stable@dpdk.org" <stable@dpdk.org>,
"bugzilla@dpdk.org" <bugzilla@dpdk.org>,
"hofors@lysator.liu.se" <hofors@lysator.liu.se>,
"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [PATCH] net: fix checksum with unaligned buffer
Date: Wed, 22 Jun 2022 10:18:14 +0100 [thread overview]
Message-ID: <YrLeVibepeSm4gHQ@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <AM8PR07MB766630B5F8D691118E5C6DE298B29@AM8PR07MB7666.eurprd07.prod.outlook.com>
On Wed, Jun 22, 2022 at 06:26:07AM +0000, Emil Berg wrote:
>
>
> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: den 21 juni 2022 11:35
> > To: Emil Berg <emil.berg@ericsson.com>
> > Cc: Bruce Richardson <bruce.richardson@intel.com>; Stephen Hemminger
> > <stephen@networkplumber.org>; stable@dpdk.org; bugzilla@dpdk.org;
> > hofors@lysator.liu.se; olivier.matz@6wind.com; dev@dpdk.org
> > Subject: RE: [PATCH] net: fix checksum with unaligned buffer
> >
> > > 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.)?
> >
>
> Well, if you append or adjust an odd number of bytes (e.g. a PDCP header) from a previously aligned payload the entire packet will then be unaligned.
>
If PDCP headers can leave the rest of the packet field unaligned, then we
had better remove the alignment restrictions through all of DPDK.
/Bruce
next prev parent reply other threads:[~2022-06-22 9:18 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 7:16 [Bug 1035] __rte_raw_cksum() crash with misaligned pointer bugzilla
2022-06-15 14:40 ` Morten Brørup
2022-06-16 5:44 ` Emil Berg
2022-06-16 6:27 ` Morten Brørup
2022-06-16 6:32 ` Emil Berg
2022-06-16 6:44 ` Morten Brørup
2022-06-16 13:58 ` Mattias Rönnblom
2022-06-16 14:36 ` Morten Brørup
2022-06-17 7:32 ` Morten Brørup
2022-06-17 8:45 ` [PATCH] net: fix checksum with unaligned buffer Morten Brørup
2022-06-17 9:06 ` Morten Brørup
2022-06-17 12:17 ` Emil Berg
2022-06-20 10:37 ` Emil Berg
2022-06-20 10:57 ` Morten Brørup
2022-06-21 7:16 ` Emil Berg
2022-06-21 8:05 ` Morten Brørup
2022-06-21 8:23 ` Bruce Richardson
2022-06-21 9:35 ` Morten Brørup
2022-06-22 6:26 ` Emil Berg
2022-06-22 9:18 ` Bruce Richardson [this message]
2022-06-22 11:26 ` Morten Brørup
2022-06-22 12:25 ` Emil Berg
2022-06-22 14:01 ` Morten Brørup
2022-06-22 14:03 ` Emil Berg
2022-06-23 5:21 ` Emil Berg
2022-06-23 7:01 ` Morten Brørup
2022-06-23 11:39 ` Emil Berg
2022-06-23 12:18 ` Morten Brørup
2022-06-22 13:44 ` [PATCH v2] " Morten Brørup
2022-06-22 13:54 ` [PATCH v3] " Morten Brørup
2022-06-23 12:39 ` [PATCH v4] " Morten Brørup
2022-06-23 12:51 ` Morten Brørup
2022-06-27 7:56 ` Emil Berg
2022-06-27 10:54 ` Morten Brørup
2022-06-27 12:28 ` Mattias Rönnblom
2022-06-27 12:46 ` Emil Berg
2022-06-27 12:50 ` Emil Berg
2022-06-27 13:22 ` Morten Brørup
2022-06-27 17:22 ` Mattias Rönnblom
2022-06-27 20:21 ` Morten Brørup
2022-06-28 6:28 ` Mattias Rönnblom
2022-06-30 16:28 ` Morten Brørup
2022-07-07 15:21 ` Stanisław Kardach
2022-07-07 18:34 ` [PATCH 1/2] app/test: add cksum performance test Mattias Rönnblom
2022-07-07 18:34 ` [PATCH 2/2] net: have checksum routines accept unaligned data Mattias Rönnblom
2022-07-07 21:44 ` Morten Brørup
2022-07-08 12:43 ` Mattias Rönnblom
2022-07-08 12:56 ` [PATCH v2 1/2] app/test: add cksum performance test Mattias Rönnblom
2022-07-08 12:56 ` [PATCH v2 2/2] net: have checksum routines accept unaligned data Mattias Rönnblom
2022-07-08 14:44 ` Ferruh Yigit
2022-07-11 9:53 ` Olivier Matz
2022-07-11 10:53 ` Mattias Rönnblom
2022-07-11 9:47 ` [PATCH v2 1/2] app/test: add cksum performance test Olivier Matz
2022-07-11 10:42 ` Mattias Rönnblom
2022-07-11 11:33 ` Olivier Matz
2022-07-11 12:11 ` [PATCH v3 " Mattias Rönnblom
2022-07-11 12:11 ` [PATCH v3 2/2] net: have checksum routines accept unaligned data Mattias Rönnblom
2022-07-11 13:25 ` Olivier Matz
2022-08-08 9:25 ` Mattias Rönnblom
2022-09-20 12:09 ` Mattias Rönnblom
2022-09-20 16:10 ` Thomas Monjalon
2022-07-11 13:20 ` [PATCH v3 1/2] app/test: add cksum performance test Olivier Matz
2022-07-08 13:02 ` [PATCH 2/2] net: have checksum routines accept unaligned data Morten Brørup
2022-07-08 13:52 ` Mattias Rönnblom
2022-07-08 14:10 ` Bruce Richardson
2022-07-08 14:30 ` Morten Brørup
2022-06-30 17:41 ` [PATCH v4] net: fix checksum with unaligned buffer Stephen Hemminger
2022-06-30 17:45 ` Stephen Hemminger
2022-07-01 4:11 ` Emil Berg
2022-07-01 16:50 ` Morten Brørup
2022-07-01 17:04 ` Stephen Hemminger
2022-07-01 20:46 ` Morten Brørup
2022-06-16 14:09 ` [Bug 1035] __rte_raw_cksum() crash with misaligned pointer Mattias Rönnblom
2022-10-10 10:40 ` bugzilla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YrLeVibepeSm4gHQ@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=bugzilla@dpdk.org \
--cc=dev@dpdk.org \
--cc=emil.berg@ericsson.com \
--cc=hofors@lysator.liu.se \
--cc=mb@smartsharesystems.com \
--cc=olivier.matz@6wind.com \
--cc=stable@dpdk.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).