From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 0B846559A for ; Thu, 24 Jan 2019 19:05:37 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Jan 2019 10:05:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,517,1539673200"; d="scan'208";a="128617316" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by orsmga002.jf.intel.com with ESMTP; 24 Jan 2019 10:05:34 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.116]) by IRSMSX154.ger.corp.intel.com ([169.254.12.253]) with mapi id 14.03.0415.000; Thu, 24 Jan 2019 18:05:33 +0000 From: "Ananyev, Konstantin" To: Honnappa Nagarahalli , "dev@dpdk.org" , "stephen@networkplumber.org" , "paulmck@linux.ibm.com" CC: "Gavin Hu (Arm Technology China)" , Dharmik Thakkar , nd , nd Thread-Topic: [RFC v2 1/2] rcu: add RCU library supporting QSBR mechanism Thread-Index: AQHUmZwkA3lN9JDuXEiclvOGJBGOtKWwUixggACXpdCAAUkpAIACgqOwgABi02CACb/TYIAAEgIQ Date: Thu, 24 Jan 2019 18:05:33 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258010D907734@irsmsx105.ger.corp.intel.com> References: <20181122033055.3431-1-honnappa.nagarahalli@arm.com> <20181222021420.5114-1-honnappa.nagarahalli@arm.com> <20181222021420.5114-2-honnappa.nagarahalli@arm.com> <2601191342CEEE43887BDE71AB977258010D904212@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258010D904AC7@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB977258010D9058F6@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjNiNDNmZWItYTg1Yi00YmRkLTg0N2EtNjYzYWJkMzVjOWU5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMVFiNktaMXVyRDRtZW1ndEZjNE4yZVwvZTUzcjVNVjgzYkdDako4cFI3YXpwU3N4VXNCb1Zzc1pkc0Q4SGdFUDkifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC v2 1/2] rcu: add RCU library supporting QSBR mechanism 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, 24 Jan 2019 18:05:38 -0000 > >=20 > > > > > > > +/** > > > > > > > + * RTE thread Quiescent State structure. > > > > > > > + */ > > > > > > > +struct rte_rcu_qsbr { > > > > > > > + uint64_t reg_thread_id[RTE_QSBR_BIT_MAP_ELEMS] > > > > > > __rte_cache_aligned; > > > > > > > + /**< Registered reader thread IDs - reader threads reportin= g > > > > > > > + * on this QS variable represented in a bit map. > > > > > > > + */ > > > > > > > + > > > > > > > + uint64_t token __rte_cache_aligned; > > > > > > > + /**< Counter to allow for multiple simultaneous QS queries > > > > > > > +*/ > > > > > > > + > > > > > > > + struct rte_rcu_qsbr_cnt w[RTE_RCU_MAX_THREADS] > > > > > > __rte_cache_aligned; > > > > > > > + /**< QS counter for each reader thread, counts upto > > > > > > > + * current value of token. > > > > > > > > > > > > As I understand you decided to stick with neutral thread_id and > > > > > > let user define what exactly thread_id is (lcore, syste, thread > > > > > > id, something > > > > else)? > > > > > Yes, that is correct. I will reply to the other thread to continu= e the > > discussion. > > > > > > > > > > > If so, can you probably get rid of RTE_RCU_MAX_THREADS limitati= on? > > > > > I am not seeing this as a limitation. The user can change this if > > > > > required. May > > > > be I should change it as follows: > > > > > #ifndef RTE_RCU_MAX_THREADS > > > > > #define RTE_RCU_MAX_THREADS 128 > > > > > #endif > > > > > > > > Yep, that's better, though it would still require user to rebuild > > > > the code if he would like to increase total number of threads suppo= rted. > > > Agree > > > > > > > Though it seems relatively simply to extend current code to support > > > > dynamic max thread num here (2 variable arrays plus shift value plu= s > > mask). > > > Agree, supporting dynamic 'max thread num' is simple. But this means > > > memory needs to be allocated to the arrays. The API > > > 'rte_rcu_qsbr_init' has to take max thread num as the parameter. We a= lso > > have to introduce another API to free this memory. This will become ver= y > > similar to alloc/free APIs I had in the v1. > > > I hope I am following you well, please correct me if not. > > > > I think we can still leave alloc/free tasks to the user. > > We probabply just need extra function rte_rcu_qsbr_size(uint32_ > > max_threads) to help user calculate required size. > > rte_rcu_qsbr_init() might take as an additional parameter 'size' to mak= e > > checks. > The size is returned by an API provided by the library. Why does it need = to be validated again? If 'size' is required for rte_rcu_qsbr_init, it > could calculate it again. Just as extra-safety check. I don't have strong opinion here - if you think it is overkill, let's drop = it. >=20 > > Thought about something like that: > > > > size_t sz =3D rte_rcu_qsbr_size(max_threads); struct rte_rcu_qsbr *qsbr= =3D > > alloc_aligned(CACHE_LINE, sz); rte_rcu_qsbr_init(qsbr, max_threads, sz)= ; ... > > > Do you see any advantage for allowing the user to allocate the memory? So user can choose where to allocate the memory (eal malloc, normal malloc,= stack, something else). Again user might decide to make rcu part of some complex data structure -=20 in that case he probably would like to allocate one big chunk of memory at = once and then provide part of it for rcu. Or some other usage scenario that I can't predict. > This approach requires the user to call 3 APIs (including memory allocati= on). These 3 can be abstracted in a rte_rcu_qsbr_alloc API, user has > to call just 1 API. >=20 > > Konstantin > > > > > > > > > > > > > > > > > > > > I.E. struct rte_rcu_qsbr_cnt w[] and allow user at init time to > > > > > > define max number of threads allowed. > > > > > > Or something like: > > > > > > #define RTE_RCU_QSBR_DEF(name, max_thread) struct name { \ > > > > > > uint64_t reg_thread_id[ALIGN_CEIL(max_thread, 64) >> 6]; \ > > > > > > ... > > > > > > struct rte_rcu_qsbr_cnt w[max_thread]; \ } > > > > > I am trying to understand this. I am not following why 'name' is > > > > > required? Would the user call 'RTE_RCU_QSBR_DEF' in the > > > > > application > > > > header file? > > > > > > > > My thought here was to allow user to define his own structures, > > > > depending on the number of max threads he needs/wants: > > > > RTE_RCU_QSBR_DEF(rte_rcu_qsbr_128, 128); > > > > RTE_RCU_QSBR_DEF(rte_rcu_qsbr_64, 64); ... > > > Thank you for the clarification, I follow you now. However, it will > > > not solve the problem of dynamic max thread num. Changes to the max > > number of threads will require recompilation. > > > > > > > Konstantin