From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CCD8EA04FD; Wed, 22 Jun 2022 11:18:24 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 896C54069C; Wed, 22 Jun 2022 11:18:24 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id 4C83040689; Wed, 22 Jun 2022 11:18:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655889502; x=1687425502; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=39gNf5QmRJr3YLsC8y4GPjpxJVhDicK0m2NrtTs6wSs=; b=noco+n65ZIXrvX8xiCYEVxEG8acmCDTcFed6rMSv/TdCnbEekitQTNEN 0wqkKmFqGY4eOAXeNHIcDjCJ5jy1pG8ohT94b/ZxQRMhPIAx9T8hMBNI/ 0X1W6SXzQ4/F2xetlGWQj0MS/mSisxcx68YY8bBTnIMrO8LrJyoKU2JAi zmsH1T7zd+5xm4ZKXd0AFnuFleqEUj2CFPpQyCfMweQtdV8LhCxeE6NxA oNnDTeKXK7Nm0dL5j2zKsORv2m/DB86wTmiQf4XBz+RNWTR7xmXhFAkYw +ubnjBJEoa5QP9mUGCyRQDL4oAovDCQyZHN00yH9VTsBiNlNNi7oWSH9+ w==; X-IronPort-AV: E=McAfee;i="6400,9594,10385"; a="277912689" X-IronPort-AV: E=Sophos;i="5.92,212,1650956400"; d="scan'208";a="277912689" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jun 2022 02:18:21 -0700 X-IronPort-AV: E=Sophos;i="5.92,212,1650956400"; d="scan'208";a="592092045" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.62]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 22 Jun 2022 02:18:19 -0700 Date: Wed, 22 Jun 2022 10:18:14 +0100 From: Bruce Richardson To: Emil Berg Cc: Morten =?iso-8859-1?Q?Br=F8rup?= , Stephen Hemminger , "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 Message-ID: References: <98CBD80474FA8B44BF855DF32C47DC35D87139@smartserver.smartshare.dk> <20220617084505.62071-1-mb@smartsharesystems.com> <98CBD80474FA8B44BF855DF32C47DC35D8713A@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87141@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87145@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87148@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Jun 22, 2022 at 06:26:07AM +0000, Emil Berg wrote: > > > > -----Original Message----- > > From: Morten Brørup > > Sent: den 21 juni 2022 11:35 > > To: Emil Berg > > Cc: Bruce Richardson ; Stephen Hemminger > > ; 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 > > > > > > 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 > > > > > > > > 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 > > > > > > > > > --- > > > > > > > > > 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