From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30067.outbound.protection.outlook.com [40.107.3.67]) by dpdk.org (Postfix) with ESMTP id 7F2FD7CE2 for ; Thu, 1 Nov 2018 20:43:31 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=+s0/w9nveA3vpAEEiCzfdlA/SmUgPVklaG96nntQjL4=; b=EEvOWnoejHeqBFq/2scOn4GVIa5KvsZTnxeBsmG2blOuF/EJCIfqWVscmDq+VMTmlSBfkT3ckJN5Y0ipw2IqZNpYsP64tECNHgia5QbdO5QJoL0Q4UteYFaKbJCoXtjBWxdSxdwimz7a3wm0x4m/fAmtGi9n/IB4U+Y7BcDyTiE= Received: from AM6PR08MB3672.eurprd08.prod.outlook.com (20.177.115.29) by AM6PR08MB3829.eurprd08.prod.outlook.com (20.178.90.26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1294.21; Thu, 1 Nov 2018 19:43:29 +0000 Received: from AM6PR08MB3672.eurprd08.prod.outlook.com ([fe80::c1a0:51bf:cd33:2b27]) by AM6PR08MB3672.eurprd08.prod.outlook.com ([fe80::c1a0:51bf:cd33:2b27%6]) with mapi id 15.20.1273.025; Thu, 1 Nov 2018 19:43:29 +0000 From: Honnappa Nagarahalli To: Bruce Richardson CC: "pablo.de.lara.guarch@intel.com" , "dev@dpdk.org" , "Gavin Hu (Arm Technology China)" , Dharmik Thakkar , nd , "yipeng1.wang@intel.com" , "sameh.gobriel@intel.com" , nd Thread-Topic: [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags Thread-Index: AQHUccemtAnjeXccJE2VJo/HhbpiIqU7Evpw Date: Thu, 1 Nov 2018 19:43:29 +0000 Message-ID: References: <1539208085-30756-4-git-send-email-yipeng1.wang@intel.com> <20181101045454.632-1-honnappa.nagarahalli@arm.com> <20181101045454.632-2-honnappa.nagarahalli@arm.com> <20181101094530.GA5004@bricha3-MOBL.ger.corp.intel.com> In-Reply-To: <20181101094530.GA5004@bricha3-MOBL.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Honnappa.Nagarahalli@arm.com; x-originating-ip: [217.140.111.135] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM6PR08MB3829; 6:YA/6zXeZWTCNNw3lAhiNmtUgREZSmN8im40wa6bhZbs1rwHvk8rYp9TFDdoxIz/l7n7I2HVX9i0cRub7BqQjdrLPPA9/0ofROFS5dENWjxJO8j8JhJhtv9NGpyzL+jgsGRHwtHmP2v1/rrrOw9+O/v9UCl0tv6gb6SVnBDiUipFCR/g4L5vNG1sn5S13b8uXJkMj3WbwxOmpXB+gVSHQH+jhsq79aaLUCLBMxsalf6JkvNSX2xl2E45MWENWh937hkmY4qz8Q5ZLb6mSUSn4qHFtwFvLPSeLM4rROUDn5bS9EvSJ2dZY91EJPR2ijW+QN03urC4M8WoyuKd0imvThNQez3hXkipDBHcvmHGlovnJ/Tno32J63lZqkc/JcjuAMmIdBadsP1CnC0g/jPtKl+N+4YmQdR4/DV9I/GcxdwIrmW9brgGvdL7Ui+AkFRIXcjW3eGiejddt39OV1BV8zA==; 5:f4PA/Uic19qo1+skLxYYa5jeh4Aayd2v1m/uzmWQIe5F+jjyX7YPoO9O/D+K4t6k85ULiHtBBx/o6gATKGFgXXAlYpcxf6DBdBCRprstY6v5ubS6CGCgnF3n9zW5KDYvxfjLl4z1z2jUDZ91ONGaO3Md9P7mFmB7XXmIcxR1l+0=; 7:s2mxk2AtY3wKAmEWSBqOnPbGNq7Drx71x4jDCTtm2/qcuL3hybvQfPkwRjGwadEXG+YNLKlVRQb9i2VgdcrWtXRGCw1PbFEKQ4lx9NIxLf6LxGXos3Bgnzdqb0XxUmyuAnffsCT8bG7WDeH5DH6dtQ== x-ms-exchange-antispam-srfa-diagnostics: SOS;SOR; x-ms-office365-filtering-correlation-id: 90b0da56-b90c-417e-50ce-08d640324c89 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:AM6PR08MB3829; x-ms-traffictypediagnostic: AM6PR08MB3829: nodisclaimer: True x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3231382)(944501410)(52105095)(3002001)(10201501046)(93006095)(93001095)(6055026)(148016)(149066)(150057)(6041310)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123562045)(20161123558120)(20161123564045)(201708071742011)(7699051)(76991095); SRVR:AM6PR08MB3829; BCL:0; PCL:0; RULEID:; SRVR:AM6PR08MB3829; x-forefront-prvs: 0843C17679 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(396003)(136003)(39860400002)(346002)(376002)(189003)(199004)(7736002)(76176011)(305945005)(14454004)(6506007)(478600001)(6916009)(11346002)(446003)(26005)(25786009)(486006)(86362001)(476003)(68736007)(102836004)(7696005)(8676002)(81156014)(2900100001)(74316002)(72206003)(256004)(99286004)(14444005)(8936002)(81166006)(71190400001)(71200400001)(3846002)(33656002)(186003)(5660300001)(6246003)(6116002)(2906002)(575784001)(97736004)(66066001)(551934003)(93886005)(53936002)(316002)(4326008)(105586002)(106356001)(229853002)(9686003)(5250100002)(55016002)(54906003)(6436002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR08MB3829; H:AM6PR08MB3672.eurprd08.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: T7ahIMDYrPIo12PcBkpZ6qwJW1n9/Hrds/wImINVsLeVDfYOEmEagK+M4rfusF7owFAwWNyy7+9U5RmZXj6+SXt1iNoxk5qArj4l6/xCSjDe4hUn+NCDn1ebSuUkevnGrmgzgnt0+GHsUr2A3oIBMMiGkFp1GCQBpEKKcr9eP5RA93xybmwi593CuMSuJf7m/gMTPOxNbqD8B0XQ9I3b82QOx8lhEHm5M1r4+sCeLW2C3huFyui6JPHStUFgFzWuPGGYPndmD86AWKdc46IQlPxhhp+kz/QdtNQkOaSKVxsz8dzx/6K2dmxvbTjjBh02r6F/4L379REr1hH9kuUVtjYcFFDeHWLfHqQFMrZIsKQ= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-Network-Message-Id: 90b0da56-b90c-417e-50ce-08d640324c89 X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Nov 2018 19:43:29.5483 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR08MB3829 Subject: Re: [dpdk-dev] [PATCH 1/3] hash: deprecate lock ellision and read/write concurreny flags 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: , X-List-Received-Date: Thu, 01 Nov 2018 19:43:31 -0000 > > > > diff --git a/lib/librte_hash/rte_cuckoo_hash.c > > b/lib/librte_hash/rte_cuckoo_hash.c > > index 5ddcccd87..a11de22be 100644 > > --- a/lib/librte_hash/rte_cuckoo_hash.c > > +++ b/lib/librte_hash/rte_cuckoo_hash.c > > @@ -121,7 +121,7 @@ get_alt_bucket_index(const struct rte_hash *h, } > > > > struct rte_hash * > > -rte_hash_create(const struct rte_hash_parameters *params) > > +rte_hash_create_v20(const struct rte_hash_parameters *params) > > { > > struct rte_hash *h =3D NULL; > > struct rte_tailq_entry *te =3D NULL; > > @@ -446,6 +446,323 @@ rte_hash_create(const struct > rte_hash_parameters *params) > > rte_free(tbl_chng_cnt); > > return NULL; > > } > > +VERSION_SYMBOL(rte_hash_create, _v20, 2.0); > > + >=20 > To make reviewing this easier, can I ask if in V2 you can put the v18.11 > function first, before the existing one. Hopefully that means that the "n= ew" > function from git's point of view is the existing one, showing it as a bl= ock > add that we can pretty much skip reviewing. The benefit of this is that t= he > changes in the v1811 should then show as line-by-line diffs in the patch,= so > we can easily review the changes made in the new case. It's hard to spot > them in the whole function below. >=20 I agree it is painful to review. I tried what you suggested here, it still = shows v18.11 function as a complete new block of code. I will create anothe= r commit in the series. The first commit will have just the exact duplicati= on of the function (but renamed to v18.11). The next commit will have all t= he required changes for the v18.11 function and deprecation related changes= . We can squash both of them once the review completes. Let me know if there is a better solution. > > +struct rte_hash * > > +rte_hash_create_v1811(const struct rte_hash_parameters *params) { > > + struct rte_hash *h =3D NULL; > > + struct rte_tailq_entry *te =3D NULL; > > + struct rte_hash_list *hash_list; > > + struct rte_ring *r =3D NULL; > > + struct rte_ring *r_ext =3D NULL; > > + char hash_name[RTE_HASH_NAMESIZE]; > > + void *k =3D NULL; > > + void *buckets =3D NULL; > > + void *buckets_ext =3D NULL; > > + char ring_name[RTE_RING_NAMESIZE]; > > + char ext_ring_name[RTE_RING_NAMESIZE]; > > + unsigned num_key_slots; > > + unsigned i; > > + > > + /* Validate correct usage of extra options */ > > + if ((params->extra_flag & > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF) && > > + (params->extra_flag & RTE_HASH_EXTRA_FLAGS_EXT_TABLE)) { > > + rte_errno =3D EINVAL; > > + RTE_LOG(ERR, HASH, "rte_hash_create: extendable bucket " > > + "feature not supported with rw concurrency " > > + "lock free\n"); >=20 > Please don't split the error text across lines. If you put it on a line b= y itself, > hopefully checkpatch should not complain. If checkpatch does complain > about the long line, just ignore it! > [Yes, I know this is copied from original code, but since it appears as n= ew > code in this patch, we should fix it] >=20 Ok, I can do that. There are other check patch issues which I did not fix (= thinking it is old code), will fix those as well. > > + return NULL; > > + } > > + > > + /* Check extra flags field to check extra options. */ > > + if (params->extra_flag & > RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD) > > + use_local_cache =3D 1; > > + > > > > void > > rte_hash_free(struct rte_hash *h) > > diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h > > index c93d1a137..93c7019ec 100644 > > --- a/lib/librte_hash/rte_hash.h > > +++ b/lib/librte_hash/rte_hash.h > > @@ -30,13 +30,27 @@ extern "C" { > > #define RTE_HASH_LOOKUP_BULK_MAX 64 > > #define RTE_HASH_LOOKUP_MULTI_MAX > RTE_HASH_LOOKUP_BULK_MAX > > > > -/** Enable Hardware transactional memory support. */ > > +/** > > + * @deprecated > > + * This define will be removed in the next release. > > + * If the target platform supports hardware transactional memory > > + * it will be used without user consent as it provides the best > > +possible > > + * performance. > > + * > > + * Enable Hardware transactional memory support. > > + */ > > #define RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT 0x01 > > > > /** Default behavior of insertion, single writer/multi writer */ > > #define RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x02 > > > > -/** Flag to support reader writer concurrency */ > > +/** > > + * @deprecated > > + * This define will be removed in the next release. > > + * This library should be thread-safe by default. > > + * > > + * Flag to support reader writer concurrency */ > > #define RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY 0x04 > > >=20 > Do we not need/want to add some new flags to disable these features, in > case there are cases where either RW concurrency, or transaction memory i= s > unwanted? >=20 I cannot think of a case where RW concurrency is not required, except for s= ingle thread use case. In the single thread use case, the existing flags pr= ovide the most optimal solution. Hopefully, it will be clear from the chang= es. I am not sure on if there is a case for no wanting transactional memory whi= le using lock based RW concurrency. We have a flag for non-lock based RW co= ncurrency in which case transactional memory will not be used. IMO, I do not see a need for additional flags. =20 > > /** Flag to indicate the extendabe bucket table feature should be > > used */ @@ -105,6 +119,10 @@ struct rte_hash; > > */ > > struct rte_hash * > > rte_hash_create(const struct rte_hash_parameters *params); > > +struct rte_hash * > > +rte_hash_create_v20(const struct rte_hash_parameters *params); struct > > +rte_hash * rte_hash_create_v1811(const struct rte_hash_parameters > > +*params); > > > > /** > > * Set a new hash compare function other than the default one. > > diff --git a/lib/librte_hash/rte_hash_version.map > > b/lib/librte_hash/rte_hash_version.map > > index 734ae28b0..c72469099 100644 > > --- a/lib/librte_hash/rte_hash_version.map > > +++ b/lib/librte_hash/rte_hash_version.map > > @@ -54,6 +54,13 @@ DPDK_18.08 { > > > > } DPDK_16.07; > > > > +DPDK_18.11 { > > + global: > > + > > + rte_hash_create; > > + > > +} DPDK_18.08; > > + > > EXPERIMENTAL { > > global: > > > > -- > > 2.17.1 > >