From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 91054A04BC; Thu, 8 Oct 2020 16:55:15 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2979A1C138; Thu, 8 Oct 2020 16:55:13 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id ADA691C12A for ; Thu, 8 Oct 2020 16:55:11 +0200 (CEST) IronPort-SDR: hAXztLRpyK07e5hu8ghSz8SKgTMABXfMf2iYsiDmiPZ3GToDPRGqsoeNk9fePQ6yHuxgyXzxTQ jGxazhp98how== X-IronPort-AV: E=McAfee;i="6000,8403,9768"; a="162709057" X-IronPort-AV: E=Sophos;i="5.77,351,1596524400"; d="scan'208";a="162709057" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Oct 2020 07:55:09 -0700 IronPort-SDR: xszJe0n0fgW3KpKUEX5ymQyXlqsR5I08n7TI56QueEf/1ca48faiEwnyhr6vXDKcBxkzSe+jnS P1/1Ct80liSA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,351,1596524400"; d="scan'208";a="388819458" Received: from orsmsx605.amr.corp.intel.com ([10.22.229.18]) by orsmga001.jf.intel.com with ESMTP; 08 Oct 2020 07:55:09 -0700 Received: from orsmsx601.amr.corp.intel.com (10.22.229.14) by ORSMSX605.amr.corp.intel.com (10.22.229.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 8 Oct 2020 07:55:08 -0700 Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) by orsmsx601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5 via Frontend Transport; Thu, 8 Oct 2020 07:55:08 -0700 Received: from NAM02-SN1-obe.outbound.protection.outlook.com (104.47.36.58) by edgegateway.intel.com (134.134.137.103) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.1713.5; Thu, 8 Oct 2020 07:55:07 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YOe8qegHowxrPqm8vz9gO88ZRz7fq2jl9dYSH/blMLxcxdldJL3avRa1HhJ4xdl/0E4skCi7dH5jHz3q93x+CoDCzjrymAcSr/5/0Twk6ExM0yPriINHSNklL916kTqkmiePDaRngfMfAPEIn43Nvef1rwR7rkQp+tlKXzIe2S86CdMFyEFzXaUBhgxPa3tCWfiVfQ/DBGZZDAh1pKULoTa0VGRKmzNgifGkQnJRyvST7yu7DPUcv21mDmFU7WaOVYI8DaGpY730YOWiEqlj/781hvJ0HG6eIbFrysFtdQkmcNPvq3GjLlzX4ku7ra9WXfTJZtXn7eG5Jgte+nBJUw== 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-SenderADCheck; bh=O2Yj2Zn84Id6Uy20ynxsyWyiWaFcg9BLlM2MpkmIiVU=; b=Cg08hbsU9Zb4qBgLbp6BEPXjz/xI0Wxq1pP8uL4BxzfGEcFYc5Qju7RGU94oJAF2DSiMg2ZFBwpk+MuXC4D/5GPsZTc93k0OMs6OThx+LLVUPMKXy8io4x/CEn6ak1cy17apRMkVrPSjCO9H3PoOk+5AJnmQFc0wAPGwKYl0Lxx9VdSMeh6W7NyRCSusnJEnP1Jtliosfz2vuGIIxUdeKnUmRf27ioSXZpEywheoV2dQdHl1yCiHgo9U3CxYqGMBFl8ODNPWXXIplqyJlpbn+IpZcd3pgQVFA1ZeZ82lyTAY2VPvCEfCVeCNBBANaiFHY9e9aQ+Jz8uxBq7A1ytNvA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=O2Yj2Zn84Id6Uy20ynxsyWyiWaFcg9BLlM2MpkmIiVU=; b=s+azbNSx2dbhbg0m0WsIMZE1YGe6D7sP4CSbRiMN98CaAkxLMPEO5sRKOXTg7npMmny+EgGBUVchNhs4duOujBP5R2uH9kmhWSv0FE24zP+yVf095ZusTB7hRJ7oDW0Ar264nH6FcpyZPpKvY7LZ7yuUD14ecpbM/TCp7lYA240= Received: from BYAPR11MB3301.namprd11.prod.outlook.com (2603:10b6:a03:7f::26) by BY5PR11MB3976.namprd11.prod.outlook.com (2603:10b6:a03:187::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3455.22; Thu, 8 Oct 2020 14:55:01 +0000 Received: from BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f5a4:3f6b:ade3:296b]) by BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f5a4:3f6b:ade3:296b%3]) with mapi id 15.20.3455.023; Thu, 8 Oct 2020 14:55:01 +0000 From: "Ananyev, Konstantin" To: Olivier Matz , "Power, Ciara" CC: "Coyle, David" , "Singh, Jasvinder" , "dev@dpdk.org" , "O'loingsigh, Mairtin" , "Ryan, Brendan" , "Richardson, Bruce" Thread-Topic: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD bitwidth Thread-Index: AQHWlzsWlg9TcWIRv0ulP6ug42lSyqmBVCiAgAF4TYCAAADdgIAHk06AgAN1fuA= Date: Thu, 8 Oct 2020 14:55:01 +0000 Message-ID: References: <20200807155859.63888-1-ciara.power@intel.com> <20200930130415.11211-1-ciara.power@intel.com> <20200930130415.11211-18-ciara.power@intel.com> <20201006100039.GI21395@platinum> In-Reply-To: <20201006100039.GI21395@platinum> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 authentication-results: 6wind.com; dkim=none (message not signed) header.d=none;6wind.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [46.7.39.127] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 43d8b7a7-da54-42f7-b4e3-08d86b9a221f x-ms-traffictypediagnostic: BY5PR11MB3976: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: YdS00Ibo/33ga28GHUgeCXUGz+/S/oxvyqih2+bbaR8vW3DDgMi8tdj+wI9bbMVd8JyUZhx0yiclCylJD9N/zxhgGmAJVjc7r5MRP0wFI6WJYCOxTBPoALWHN/0Zc/iw0cc3JIvnP0nKpQeU1hq60DnlEnaUZmrtixXi56zkEWYHuaxnAoDiILXYEMZLe3TnJuFB9PtznYhZMOqGFKgbkicK+92OxKA84ZM5VfJECVzL7AcPg/ulEoCLWsmuQwIvpsS9eaeAimS5GR8ZvzqKvVz1TQmn1j0JZ0NCqDHqGUowjQFofDMarbcweOE7ZfSSca0s+AWe0a3tt52twr7Nvgn4KDGeKSOE3QcB6E89LVYY0uLNdurgMEFYunmd3vzumAFyrohK5cJ1NxIuwCAPGg== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR11MB3301.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(376002)(346002)(39860400002)(396003)(136003)(52536014)(966005)(110136005)(316002)(54906003)(5660300002)(83080400001)(86362001)(66446008)(64756008)(83380400001)(66556008)(66476007)(66946007)(76116006)(33656002)(6636002)(26005)(9686003)(107886003)(186003)(8936002)(7696005)(2906002)(478600001)(71200400001)(55016002)(6506007)(4326008); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: o1mfhI9s7fGAB/lYFbMz1ZABREqVB4AfMSzLa80pCqmxwWY/G4sYrItOYxjIHDB2l2oMQaTz2ZodSJnqhFUAfO/uK8879qJKhHPUa2LKRZtmrJWeaLeGiCshgv/kfcWrxy9i4aR7JQfHK6LhPJjvcd5BBBpPiLwKqXg+Cv4LriaITLK9fHVz+8ujlojZnYYSio1Yt35J3A4+2WZgB0/huRAa0xfxdI+BihmONnhppN6dfiILePm8ODSGFH0GBiIY5yBnBSHYKq8myxdVS29xOYzsCaXtpnp9L3tDbuYhhEW+sTiOHfFfDvGEDbPz9pWYc28wGgwdXUYDr4QPejLcJtkAiBXvVLy5F1Sd/vwF3HhbOJWhIyh84U8LIPiQwWRBZ8sOqsxK4nQX7ROPuEE3OGdC4pOmH/weQJ0Pw7dSgl1Su+JnVwP1XMLgODgwkL3HVTMRuj9RsJoPRuB806jdSt+ZrZXQ8A0PejoQ8n+rLstydy6C9cvsCc8LmziNW4txErEZN6FOmeHAmLHlbJgYwfzmmqz/T0SG1LbdTD2xKUU/hN9JSMqa9Y/vE6NBm2Ujja6rTbIlQJreEREr3WMrzygMnzx7cEM5Ir6foVbe3o2sratoh1YS4ANWroGPB8OPMBhsLR79wuFbf/VOmHW5Ug== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3301.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 43d8b7a7-da54-42f7-b4e3-08d86b9a221f X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Oct 2020 14:55:01.2655 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: KJYmj8om35O8qlF9ajJtXyVL5IN1Hh+upIIn/+jOWjanvvulH3oiqNWHiSrfX9BgpTjCqSjQFlRFGK44RYBaHqyUP/qGllTt8oPAsv8dZ5M= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB3976 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH v3 17/18] net: add checks for max SIMD bitwidth X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > > >> > > From: dev On Behalf Of Ciara Power When > > >> > > choosing a vector path to take, an extra condition must be > > >> > > satisfied to ensure the max SIMD bitwidth allows for the CPU ena= bled > > >path. > > >> > > > > >> > > The vector path was initially chosen in RTE_INIT, however this i= s > > >> > > no longer suitable as we cannot check the max SIMD bitwidth at t= hat > > >time. > > >> > > The default chosen in RTE_INIT is now scalar. For best performan= ce > > >> > > and to use vector paths, apps must explicitly call the set > > >> > > algorithm function before using other functions from this librar= y, > > >> > > as this is where vector handlers are now chosen. > > >> > > > >> > [DC] Has it been decided that it is ok to now require applications > > >> > to pick the CRC algorithm they want to use? > > >> > > > >> > An application which previously automatically got SSE4.2 CRC, for > > >> > example, will now automatically only get scalar. > > >> > > > >> > If this is ok, this should probably be called out explicitly in > > >> > release notes as it may not be Immediately noticeable to users tha= t > > >> > they now need to select the CRC algo. > > >> > > > >> > Actually, in general, the release notes need to be updated for thi= s > > >> patchset. > > >> > > >> The decision to move rte_set_alg() out of RTE_INIT was taken to avoi= d > > >> check on max_simd_bitwidth in data path for every single time when > > >> crc_calc() api is invoked. Based on my understanding, > > >> max_simd_bitwidth is set after eal init, and when used in crc_calc()= , > > >> it might override the default crc algo set during RTE_INIT. Therefor= e, > > >> to avoid extra check on max_simd_bitwidth in data path, better opti= on > > >> will be to use this static configuration one time after eal init in = the set_algo > > >API. > > > > > >[DC] Yes that is a good change to have made to avoid extra datapath ch= ecks. > > > > > >Based on off-list discussion, I now also know the reason behind now > > >defaulting to scalar CRC in RTE_INIT. If a higher bitwidth CRC was cho= sen by > > >RTE_INIT (e.g. > > >SSE4.2 CRC) but the max_simd_bitwidth was then set to RTE_NO_SIMD (64) > > >through the EAL parameter or call to rte_set_max_simd_bitwidth(), then > > >there is a mismatch if rte_net_crc_set_alg() is not then called to rec= onfigure > > >the CRC. Defaulting to scalar avoids this mismatch and works on all ar= chs > > > > > >As I mentioned before, I think this needs to be called out in release = notes, as > > >it's an under-the-hood change which could cause app performance to dro= p if > > >app developers aren't aware of it - the API itself hasn't changed, so = they may > > >not read the doxygen :) > > > > > > > Yes that is a good point, I can add to the release notes for this to ca= ll it out. >=20 > I don't think it is a good idea to have the scalar crc by default. > To me, the fastest available CRC has to be enabled by default. >=20 > I understand the technical reason why you did it like this however: the > SIMD bitwidth may not be known at the time the > RTE_INIT(rte_net_crc_init) function is called. >=20 > A simple approach to solve this issue would be to initialize the > rte_net_crc_handler pointer to a handlers_default. The first time a crc > is called, the rte_crc32_*_default_handler() function would check the > configured SIMD bitwidth, and set the handler to the correct one, to > avoid to do the test for next time. >=20 > This approach still does not solve the case where the SIMD bitwidth is > modified during the life of the application. In this case, a callback > would have to be registered to notify SIMD bitwidth changes... but I > don't think it is worth to do it. Instead, it can be documented that > rte_set_max_simd_bitwidth() has to be called early, before > rte_eal_init(). Actually I also thought about callback approach. It does complicate things a bit for sure, but on a positive side - it allows to solve RTE_INIT() code-path selection problem in a generic way, plus it means zero changes in the data-path.=20 So probably worth to consider it. >=20 >=20 >=20 > > >> > > >> > > >> > > > > >> > > Suggested-by: Jasvinder Singh > > >> > > > > >> > > Signed-off-by: Ciara Power > > >> > > > > >> > > --- > > >> > > v3: > > >> > > - Moved choosing vector paths out of RTE_INIT. > > >> > > - Moved checking max_simd_bitwidth into the set_alg function. > > >> > > --- > > >> > > lib/librte_net/rte_net_crc.c | 26 +++++++++++++++++--------- > > >> > > lib/librte_net/rte_net_crc.h | 3 ++- > > >> > > 2 files changed, 19 insertions(+), 10 deletions(-) > > >> > > > > >> > > diff --git a/lib/librte_net/rte_net_crc.c > > >> > > b/lib/librte_net/rte_net_crc.c index > > >> > > 9fd4794a9d..241eb16399 100644 > > >> > > --- a/lib/librte_net/rte_net_crc.c > > >> > > +++ b/lib/librte_net/rte_net_crc.c > > >> > > > >> > > > >> > > > >> > > @@ -145,18 +149,26 @@ rte_crc32_eth_handler(const uint8_t *data, > > >> > > uint32_t data_len) void rte_net_crc_set_alg(enum rte_net_crc_a= lg > > >> > > alg) { > > >> > > + if (max_simd_bitwidth =3D=3D 0) > > >> > > + max_simd_bitwidth =3D rte_get_max_simd_bitwidth(); > > >> > > + > > >> > > switch (alg) { > > >> > > #ifdef X86_64_SSE42_PCLMULQDQ > > >> > > case RTE_NET_CRC_SSE42: > > >> > > - handlers =3D handlers_sse42; > > >> > > - break; > > >> > > + if (max_simd_bitwidth >=3D RTE_MAX_128_SIMD) { > > >> > > + handlers =3D handlers_sse42; > > >> > > + return; > > >> > > + } > > >> > > + RTE_LOG(INFO, NET, "Max SIMD Bitwidth too low, using > > >> > > scalar\n"); > > >> > > > >> > [DC] Not sure if you're aware but there is another patchset which > > >> > adds an > > >> > AVX512 CRC implementation and run-time checking of cpuflags to > > >> > select the CRC path to use: > > >> > https://patchwork.dpdk.org/project/dpdk/list/?series=3D12596 > > >> > > > >> > There will be a task to merge these 2 patchsets if both are merged= . > > >> > It looks fairly straightforward to me to merge these, but it would > > >> > be good if you take a look too > > > > I have looked at that patchset, I agree, I think they will be straightf= orward to merge together. > > > > Thanks, > > Ciara