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 41C5EA0032; Fri, 24 Jun 2022 10:34:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2E263427EE; Fri, 24 Jun 2022 10:34:17 +0200 (CEST) Received: from EUR04-HE1-obe.outbound.protection.outlook.com (mail-eopbgr70077.outbound.protection.outlook.com [40.107.7.77]) by mails.dpdk.org (Postfix) with ESMTP id F02984003F; Thu, 23 Jun 2022 13:39:12 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KqD8nGch02FGdCudeZ4d4UyooQ7JvNEOv9k+O049MqVtiYCEV8+wdmHc0P98x6tRDSOjDv8slwVxgd6t0boDXna0qwOtctSaNvKqxYEjIh3JuljyR+yJ0Ik7sZ5BObS9vAYGvHhTdILDqlbXgv/t60Uhdun+VQrTT8TrtrFA6Rf8caUcmDjZs6xkx8sdOLtSV53A+hsDsGLQCUKF2590A7Wf/vUBeoCzJLsRw9v2j8TTwWbBgZaU3ZHHjanbaWkWWgoF9+jr88/stjrzjtzBLElF8s2H4nPvLnuspbE+zIFw98pbZhj5hdREsRA/Us1hPA13BEbCqqqDPDcqpZrfvA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=g1CcxvDBauo1mqtwMWa5owRfg59epYGD6LS4bSBvF1c=; b=CauGEevMprdpgZu1nW7A5UEOe+JIgtsnP4UlTA0YGLEwApfrxGDkKZUyaudzSuzqt1+pfSmQyNai7EFWbK6KcpYTWj5/+ksHCpRRaJfUaOaymAQ7mJQM7+wcc4IB0SvDE2RTjoZuOfq44nqdvuphvG1ut2CpQz0iOSUXj7b331qLLCBbySA8+fTtd5vU5qZXBJoW44jPnDcVNXDNFtkdZehHw8Nn3CnkyLhlv4vum6EVzqNxEdYd1dVChDtaYwqHWFhD/aMFjdJ3rLWGOGhffSRZJpbABArI1/07BJey3tQqFoa9eMry+eqxblUywELLUYbrXabOHDOd7Uvk0q2bJg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=ericsson.com; dmarc=pass action=none header.from=ericsson.com; dkim=pass header.d=ericsson.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=g1CcxvDBauo1mqtwMWa5owRfg59epYGD6LS4bSBvF1c=; b=RWL6d4BTqQYo98VBz35OgmNGPvOE9C4R231MSt0i5hYcPWqXl7/vYHurwYgqPhGRccUnZIPBMyS4YkfpuMUIYrDAhr8XyZ4eDFJtYzxjMpCjAGO+CEcRMR0GtUkWBAoNlDQy+lOBhnP5MH2e/rSA4L9YiXBczPDB8jhO9qSRqF8= Received: from AM8PR07MB7666.eurprd07.prod.outlook.com (2603:10a6:20b:240::23) by DU2PR07MB8045.eurprd07.prod.outlook.com (2603:10a6:10:2b6::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5373.9; Thu, 23 Jun 2022 11:39:11 +0000 Received: from AM8PR07MB7666.eurprd07.prod.outlook.com ([fe80::188:e139:774e:cea1]) by AM8PR07MB7666.eurprd07.prod.outlook.com ([fe80::188:e139:774e:cea1%7]) with mapi id 15.20.5373.016; Thu, 23 Jun 2022 11:39:11 +0000 From: Emil Berg To: =?iso-8859-1?Q?Morten_Br=F8rup?= , Bruce Richardson CC: 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 Thread-Topic: [PATCH] net: fix checksum with unaligned buffer Thread-Index: AQHYgiaa5HfwvbWOiEyQ5g/wygBtSq1TTuaAgATMtJCAAAQg8IABVuVggAAFVwCAABAOAIAAFBGAgAFa5uCAADK3AIAAI7mAgAAOwgCAABrZ4IABAW+ggAATMKCAAFOrkA== Date: Thu, 23 Jun 2022 11:39:11 +0000 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> <98CBD80474FA8B44BF855DF32C47DC35D87152@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87154@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D87159@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87159@smartserver.smartshare.dk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=ericsson.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: bc6f6948-fcc5-40e9-9376-08da550cfdd9 x-ms-traffictypediagnostic: DU2PR07MB8045:EE_ x-microsoft-antispam-prvs: x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: YRoM6X2wDqWA1nBPEcfRsDcLoMVk0hvryqgRPv2ABAI09XwEtvNkPhjfZVl/5WA9S4FvkCjAx3qSXK5I9sj/wymqcHb38mBFh9v725X+Op1kDJ8/mnJaT8tzN7maG78As8Tq1slP26xqEzHDFLA+3KpRUNxa1SKHzy0YiQccbq+DNDEqrUeXjeG7BRkKZJk6x6wA9oJpL2HnQKKlROjZBAovr04R+bZbQIJ9Buq6k4Lxh0oEigVFvRIciHDEPbPqIP7wxOhMnlcn8l+mmz9sYiLFw/tgpqXOwaBTh4p6DFL+0AhsmlZebgJbKRtnB9XnsdBdtKCMLtncvM+qqaoNskmy+g/C69dCvNaHOwPLIQBlzTMz58WZ9GkOXPxC6bjvoeIuGsp8QGDz6NEJvwx39q3l8mlruDiVkTFJZ/MLKzNSz7WWgonL/0YtT/hbgen4qyI+ugJlktc5cWjmnAl1MY7+Naeb7iK8XV09K2N+oqTUwV1HEpF+IBWC96JCQv592qR7k88lWAZmZU/8xt/gnO10xUw9rJl909jDM8mHpU9+hfUKKGguzSslkpJRJzU3WdYi0nQlMV/UNqExpQnziTYELxlSS0OKKAjfeQSaZzm2Hr88m9HMjq/ojvf41WpUTX1vgBm42VPSGgpCyjBELdXNILNSGquUOGooye3QB5P4kEPFEpGTYBU5qHGI8f1SOUsFarmUQmYUoVbHvUXt71YINSa15Dm7t/r1X157yrsmwRBaDlpB2Rb5mML2qgcgg5Y8qapPEH+0EgUp/WjEiES6a9SYV/8e3kzkJEWl8C0= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:AM8PR07MB7666.eurprd07.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230016)(4636009)(396003)(39860400002)(376002)(346002)(136003)(366004)(71200400001)(54906003)(76116006)(4326008)(478600001)(2906002)(30864003)(52536014)(5660300002)(66446008)(66946007)(26005)(316002)(66476007)(8936002)(9686003)(110136005)(53546011)(7696005)(64756008)(44832011)(55016003)(8676002)(186003)(41300700001)(38070700005)(6506007)(122000001)(83380400001)(66574015)(82960400001)(966005)(33656002)(66556008)(38100700002)(86362001); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?g9tUCLuZ+ZX1xrqiL/F55vfjg7/JIKuHWd/oA+hZ1Gy1YT8E30HvxzJRO6?= =?iso-8859-1?Q?ZmuikkEqjmthGhh3M/LsNbRMsMdL75tNMimnf2z22JT0GaXHV46K58AvB2?= =?iso-8859-1?Q?PjxMmHHv/oiojX0y66egvojNfk6KwAYUIgffljln4KvjbjRyc6B8gXNk65?= =?iso-8859-1?Q?WGXdpW84ZwOZ2OuAyGfwvUMvhTh95ZpIsyu8DSJUveqHLIwzv8M0/uREmU?= =?iso-8859-1?Q?dNgXe7fvmOFcSLqGQ3sqYO1+V7KKMEcQv7hXFEdqZLSHRjHKmQzTkD/dul?= =?iso-8859-1?Q?xfiEB4B2fxtjY4yhDXc+7tGUmf57CDg77VmhpICfmPxclQlDrKd6FrSMoj?= =?iso-8859-1?Q?vk9N5ChGcMno7dnAlPQxZfuRwBmTu09JJdVHxZ+HTKrP4EtKGqhYjtzT0d?= =?iso-8859-1?Q?Zh/nNEEAk6C1cvSRxQCggK0DMNtwmJSJFyhPB0sSUV9XjnlPL+G2CefteM?= =?iso-8859-1?Q?oAPYEj1dbKnIyGqkc32dQJwkNtAqQFpd0tRNwUkAF+SUQPrvYbk9oAnkkF?= =?iso-8859-1?Q?SWpCIG6+T7iecRAL6mUuqPLGWOygAcL7/OGMCCsUncyZMo0mfFVDbXMzXm?= =?iso-8859-1?Q?SPW0IHvdfy/QspBNK9+FlVccy3CPfkeD+YfzaACHyP8Ih/RXsz6hODQKRv?= =?iso-8859-1?Q?WEPBuOi5V8lJXuQUJWJFLJmpFK5OYCldyuOh8dHd6hP6XxMtgzQndp587v?= =?iso-8859-1?Q?QJ4PrjUnju6y9Pf2tOA+opIM2fXEchgPNtCse/OGRheO6dM5FbOKdxWQzw?= =?iso-8859-1?Q?QN0atPZ00c+Pd6mfLuSl1rzt7BH8j2flkJGPvrpvFBVLq2sM3sfcYAXaXK?= =?iso-8859-1?Q?QfPeD90n0jbbYzCPH71n4VFfQJtIkX5andZvhHZ0BZ/HxO0G2CQ4jBqp/L?= =?iso-8859-1?Q?WJuTxK21j1lw6Uv/7ryfCOSykEjQCJl34ZFK4+e98zG/02/VjH50ynvTWm?= =?iso-8859-1?Q?6JYY4EaxNMHq5Y7nDkB0oeN5WEG1YKPLjE/KxZiiDUNA7pYfanT8G4ZdGr?= =?iso-8859-1?Q?+ZRNMrsWSrKiIImFPKq1N0rIT6PNZ/fVpNy9I1EdZJurmslJ1kvCwVCp/U?= =?iso-8859-1?Q?IMsxYn8pk02KBr9366ErdHqXTDlRiP9azUMeCezqxiM3/aIyDlSRz4qtQL?= =?iso-8859-1?Q?1lDZVPOzAGOG0BIdbJy4igjRDA7aiqVxpI02qKugkXQ+sDkl5sHHpiu1Ew?= =?iso-8859-1?Q?muHkgvvYUar0dnD/fmCpxaasV/oaz1dckF53RQUhLFcbxeQ4xDHddLiQ26?= =?iso-8859-1?Q?ygtEZaUyulhZdjTE0FXZ3y9cZ4QGEc3FlFSrzoITAhklbZSKCbEtb0IcMi?= =?iso-8859-1?Q?Hs6I9mQLV+y/UGuiwwKuA/Ss26o0EsPc4kN0ej6oCV1Z5ApLdXU1pHyO7l?= =?iso-8859-1?Q?FiOXb3unz9I7FVmisT2io1zwMHKtTewmcc64n591ZtAVrsBA9ChVrVd9te?= =?iso-8859-1?Q?e36S22wy9g/o+enW0qasPP4oKNYGPZ3SnKOePzs6OZVpKyiiiXEmW5Afhc?= =?iso-8859-1?Q?gjCGuZidJSokHIdsi3uF25Ybd4x9OOe4mDlhlMUthT554zJZuVMlcZJHae?= =?iso-8859-1?Q?JGwXY9GH/pKE0fxQm96t3ihJAHGyMrOY/RscP9L84GfCnSY6dRw2uGseAR?= =?iso-8859-1?Q?DHi59QzF4Cj9U/grjct7uTGTjhfAlZ/C0m?= Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: ericsson.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: AM8PR07MB7666.eurprd07.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: bc6f6948-fcc5-40e9-9376-08da550cfdd9 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jun 2022 11:39:11.2897 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 92e84ceb-fbfd-47ab-be52-080c6b87953f X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: SIzZnRtI0tCPR7I4nZZG1mJvPKfEXEua9ICiRvdC+fApg74Y/WM8Z6TwFjkW4Qgt1vzyJnGFHl9LpXb7vJdX2A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR07MB8045 X-Mailman-Approved-At: Fri, 24 Jun 2022 10:34:14 +0200 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 > -----Original Message----- > From: Morten Br=F8rup > Sent: den 23 juni 2022 09:01 > To: Emil Berg ; Bruce Richardson > > Cc: 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 >=20 > > From: Emil Berg [mailto:emil.berg@ericsson.com] > > Sent: Thursday, 23 June 2022 07.22 > > > > > From: Morten Br=F8rup > > > Sent: den 22 juni 2022 16:02 > > > > > > > From: Emil Berg [mailto:emil.berg@ericsson.com] > > > > Sent: Wednesday, 22 June 2022 14.25 > > > > > > > > > From: Morten Br=F8rup > > > > > Sent: den 22 juni 2022 13:26 > > > > > > > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > > > > Sent: Wednesday, 22 June 2022 11.18 > > > > > > > > > > > > On Wed, Jun 22, 2022 at 06:26:07AM +0000, Emil Berg wrote: > > > > > > > > > > > > > > > From: Morten Br=F8rup > > > > > > > > Sent: den 21 juni 2022 11:35 > > > > > > > > > > > > > > > > > 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=F8rup > > > > 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=F8rup > > > > > > > > > > > > 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=F8rup > > > > > > > > > > > > > > > Sent: den 17 juni 2022 11:07 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Morten Br=F8rup > > > > > > > > > > > > > > > [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=F8rup > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > 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 =3D (const u16_p > > *)buf; > > > > > > > > > > > > > > > - const u16_p *end =3D 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 =3D 0; > > > > > > > > > > > > > > > + if (unlikely(len =3D=3D 0)) > > > > > > > > > > > > > > > + return 0; > > > > > > > > > > > > > > > + ((unsigned char *)&first)[1] =3D > > > > > *(const > > > > > > unsigned > > > > > > > > > > > > > > char *)buf; > > > > > > > > > > > > > > > + sum +=3D first; > > > > > > > > > > > > > > > + buf =3D (const void *)((uintptr_t)buf > > > > > + 1); > > > > > > > > > > > > > > > + len--; > > > > > > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + u16_buf =3D (const u16_p *)buf; > > > > > > > > > > > > > > > + end =3D u16_buf + len / sizeof(*u16_buf); > > > > > > > > > > > > > > > for (; u16_buf !=3D end; ++u16_buf) > > > > > > > > > > > > > > > sum +=3D *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=F6nnblom 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 > > > > > > > > > > Re-reading the details regarding unaligned pointers in C11, as > > > > > posted > > > > by Emil > > > > > in Bugzilla [2], I interpret it as follows: Any 16 bit or wider > > > > pointer type a must > > > > > point to data aligned with that type, i.e. a pointer of the type > > > > "uint16_t *" > > > > > must point to 16 bit aligned data, and a pointer of the type > > > > "uint64_t *" must > > > > > point to 64 bit aligned data. Please, someone tell me I got this > > > > wrong, and > > > > > wake me up from my nightmare! > > > > > > > > > > Updating DPDK's packet structures to fully support this C11 > > > > limitation with > > > > > unaligned access would be a nightmare, as we would need to use > > byte > > > > arrays > > > > > for all structure fields. Functions would also be unable to use > > > > > other > > > > pointer > > > > > types than "void *" and "char *", which seems to be the actual > > > > problem in > > > > > the __rte_raw_cksum() function. I guess that it also would > > prevent > > > > the > > > > > compiler from auto-vectorizing the functions. > > > > > > > > > > I am usually a big proponent of academically correct solutions, > > but > > > > such a > > > > > change would be too wide ranging, so I would like to narrow it > > down > > > > to the > > > > > actual use case, and perhaps extrapolate a bit from there. > > > > > > > > > > @Emil: Do you only need to calculate the checksum of the > > > > > (potentially > > > > > unaligned) embedded packet? Or do you also need to use other > > > > > DPDK functions with the embedded packet, potentially accessing > > > > > it at > > an > > > > unaligned > > > > > address? > > > > > > > > > > I'm trying to determine the scope of this C11 pointer alignment > > > > limitation for > > > > > your use case, i.e. whether or not other DPDK functions need to > > be > > > > updated > > > > > to support unaligned packet access too. > > > > > > > > > > [2] > > > > > https://protect2.fireeye.com/v1/url?k=3D31323334-501cfaf3-313273a= f > > > > > - > > > > > 454445554331-2ffe58e5caaeb74e&q=3D1&e=3D3f0544d3-8a71-4676-b4f9- > > > > > > > > > 27e0952f7de0&u=3Dhttps%3A%2F%2Fbugs.dpdk.org%2Fshow_bug.cgi%3Fid% > > > > > 3D1035 > > > > > > > > That's my interpretation of the standard as well; For example an > > > > uint16_t* must be on even addresses. If not it is undefined > > behavior. > > > > I think this is a bigger problem on ARM for example. > > > > > > > > Without being that invested in dpdk, adding unaligned support for > > > > everything seems like a steep step, but I'm not sure what it > > entails > > > > in practice. > > > > > > > > We are actually only interested in the checksumming. > > > > > > Great! Then we can cancel the panic about rewriting DPDK Core > > completely. > > > Although it might still need some review for similar alignment bugs, > > where > > > we have been forcing the compiler shut up when trying to warn us. > > > :-) > > > > > > I have provided v3 of the patch, which should do as requested - and > > still allow > > > the compiler to auto-vectorize. > > > > > > @Emil, will you please test v3 of the patch? > > > > It seems to work in these two cases: > > * Even address, even length > > * Even address, odd length > > But it breaks in these two cases: > > * Odd address, even length (although it works for small buffers, > > probably when the sum fits inside a uint16_t integer or something) >=20 > Interesting observation, good analysis. >=20 > > * Odd address, odd length >=20 > Does this also work for small buffers? >=20 > > I get (and like) the main idea of the algorithm but haven't yet > > figured out what the problem is with odd addresses. >=20 > I wonder if I messed up the algorithm for swapping back the bytes in bsum > after the calculation... Is the checksum also wrong when compiling withou= t > optimization? >=20 > And just to be sure: The algorithm requires that __rte_raw_cksum_reduce() > is also applied to the sum. Please confirm that you call rte_raw_cksum() = (or > __rte_raw_cksum() followed by __rte_raw_cksum_reduce())? >=20 Yes, I messed up. I didn't run the reduction part. When I do the output see= ms to be the same. It seems to be about as fast as the previous algorithm, obviously. Both val= grind and fsanitize=3Dundefined are happy. Some minor improvements: * #include ? * Use RTE_PTR_ADD to make the casts cleaner? * I guess you could skip using 'bsum' and add to 'sum' instead, but that's = a matter of preference * Can't you just do bsum +=3D *(const unsigned char *)buf; to avoid 'first'= , making it a bit more readable? > > > > /Emil