From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id CD36011A4 for ; Fri, 29 Mar 2019 20:23:39 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Mar 2019 12:23:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,285,1549958400"; d="scan'208";a="157044171" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 29 Mar 2019 12:23:38 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 29 Mar 2019 12:23:38 -0700 Received: from fmsmsx108.amr.corp.intel.com ([169.254.9.216]) by fmsmsx117.amr.corp.intel.com ([169.254.3.142]) with mapi id 14.03.0415.000; Fri, 29 Mar 2019 12:23:38 -0700 From: "Eads, Gage" To: Honnappa Nagarahalli , "dev@dpdk.org" CC: "olivier.matz@6wind.com" , "arybchenko@solarflare.com" , "Richardson, Bruce" , "Ananyev, Konstantin" , "Gavin Hu (Arm Technology China)" , nd , "thomas@monjalon.net" , nd , Thomas Monjalon Thread-Topic: [PATCH v3 1/8] stack: introduce rte stack library Thread-Index: AQHU1CtVrsaMA7YCkEOnYy6dB4cOVqYgkw6AgAJQ+XA= Date: Fri, 29 Mar 2019 19:23:37 +0000 Message-ID: <9184057F7FC11744A2107296B6B8EB1E5420D923@FMSMSX108.amr.corp.intel.com> References: <20190305164256.2367-1-gage.eads@intel.com> <20190306144559.391-1-gage.eads@intel.com> <20190306144559.391-2-gage.eads@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzE2ZTU5YTUtYjk0ZS00NDMyLTllYmQtYjQzNWI2Y2ExNDJkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidndTUmo4SGNpdW9KbFJxclpEN0NwQ1wvWFJkTWpodnA4bFBQTnlKd0hzbTRoSWpFSkpjNDFvQWF5cWJxNExhRE4ifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.1.200.108] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 1/8] stack: introduce rte stack library 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: Fri, 29 Mar 2019 19:23:40 -0000 @Thomas: I expect I can address Honnappa's feedback within a day or two. Si= nce today is the 19.05 merge deadline, what do you think about these option= s for merging? 1. Merge V4 now and address these comments during RC1. 2. Delay merge until RC2, with all comments addressed. In terms of risk, Honnappa identified an incorrect memory ordering argument= (patch 6/8), but that doesn't affect the one platform (x86-64) that can (c= urrently) use this library. His other comments address readability, error-c= hecking, and performance, but aren't critical. Beyond that, this patchset = is isolated from the rest of DPDK. So, I think the risk to the project is v= ery low. (Also, note that I accidentally left off Olivier's Reviewed-by tag in V4's = patches 1, 3, 5, and 6 -- I'll address that as well) > -----Original Message----- > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Thursday, March 28, 2019 6:27 PM > To: Eads, Gage ; dev@dpdk.org > Cc: olivier.matz@6wind.com; arybchenko@solarflare.com; Richardson, Bruce > ; Ananyev, Konstantin > ; Gavin Hu (Arm Technology China) > ; nd ; thomas@monjalon.net; nd > > Subject: RE: [PATCH v3 1/8] stack: introduce rte stack library >=20 > Hi Gage, > Apologies for the late comments. >=20 No problem, I appreciate the feedback. [snip] > > +static ssize_t > > +rte_stack_get_memsize(unsigned int count) { > > + ssize_t sz =3D sizeof(struct rte_stack); > > + > > + /* Add padding to avoid false sharing conflicts */ > > + sz +=3D RTE_CACHE_LINE_ROUNDUP(count * sizeof(void *)) + > > + 2 * RTE_CACHE_LINE_SIZE; > I did not understand how the false sharing is caused and how this padding= is > solving the issue. Verbose comments would help. The additional padding (beyond the CACHE_LINE_ROUNDUP) is to prevent false = sharing caused by adjacent/next-line hardware prefetchers. I'll address thi= s. [snip] > > +struct rte_stack * > > +rte_stack_create(const char *name, unsigned int count, int socket_id, > > + uint32_t flags) > > +{ > > + char mz_name[RTE_MEMZONE_NAMESIZE]; > > + struct rte_stack_list *stack_list; > > + const struct rte_memzone *mz; > > + struct rte_tailq_entry *te; > > + struct rte_stack *s; > > + unsigned int sz; > > + int ret; > > + > > + RTE_SET_USED(flags); > > + > > + sz =3D rte_stack_get_memsize(count); > > + > > + ret =3D snprintf(mz_name, sizeof(mz_name), "%s%s", > > + RTE_STACK_MZ_PREFIX, name); > > + if (ret < 0 || ret >=3D (int)sizeof(mz_name)) { > > + rte_errno =3D ENAMETOOLONG; > > + return NULL; > > + } > > + > > + te =3D rte_zmalloc("STACK_TAILQ_ENTRY", sizeof(*te), 0); > > + if (te =3D=3D NULL) { > > + STACK_LOG_ERR("Cannot reserve memory for tailq\n"); > > + rte_errno =3D ENOMEM; > > + return NULL; > > + } > > + > > + rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK); > > + > I think there is a need to check if a stack with the same name exists alr= eady. rte_memzone_reserve_aligned() does just that. This behavior is tested in th= e function test_stack_name_reuse(), added in commit " test/stack: add stack= test". > > + mz =3D rte_memzone_reserve_aligned(mz_name, sz, socket_id, > > + 0, __alignof__(*s)); > > + if (mz =3D=3D NULL) { > > + STACK_LOG_ERR("Cannot reserve stack memzone!\n"); > > + rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK); > > + rte_free(te); > > + return NULL; > > + } [snip] > > +void > > +rte_stack_free(struct rte_stack *s) > > +{ > > + struct rte_stack_list *stack_list; > > + struct rte_tailq_entry *te; > > + > > + if (s =3D=3D NULL) > > + return; > > + > Adding a check to make sure the length of the stack is 0 would help catch > issues? My preference is to leave that check to the user, for any apps that want to= /can safely free non-empty stacks. [snip] > > +#define RTE_TAILQ_STACK_NAME "RTE_STACK" > > +#define RTE_STACK_MZ_PREFIX "STK_" > Nit, "STACK_" would be easier to debug Since RTE_MEMZONE_NAMESIZE (32) doesn't give us a lot of space, I kept the = prefix short. Adding 2 more characters *probably* won't make a difference..= .but I'd prefer the shortened name. > > +/** The maximum length of a stack name. */ #define > RTE_STACK_NAMESIZE > > +(RTE_MEMZONE_NAMESIZE - \ > > + sizeof(RTE_STACK_MZ_PREFIX) + 1) > > + [snip] > > +/** > > + * @internal Push several objects on the stack (MT-safe). > > + * > > + * @param s > > + * A pointer to the stack structure. > > + * @param obj_table > > + * A pointer to a table of void * pointers (objects). > > + * @param n > > + * The number of objects to push on the stack from the obj_table. > > + * @return > > + * Actual number of objects pushed (either 0 or *n*). > > + */ > > +static __rte_always_inline unsigned int __rte_experimental > This is an internal function. Is '__rte_experimental' tag required? I don't think so, but I erred on the side of caution. I don't think the tag= causes any problems. >=20 > > +rte_stack_std_push(struct rte_stack *s, void * const *obj_table, > > +unsigned int n) { > Since this is an internal function, does it make sense to add '__' to the > beginning of the function name (similar to what is done in rte_ring?). Makes sense. I'll address this. [snip] > > +/** > > + * @internal Pop several objects from the stack (MT-safe). > > + * > > + * @param s > > + * A pointer to the stack structure. > > + * @param obj_table > > + * A pointer to a table of void * pointers (objects). > > + * @param n > > + * The number of objects to pull from the stack. > > + * @return > > + * Actual number of objects popped (either 0 or *n*). > > + */ > > +static __rte_always_inline unsigned int __rte_experimental > This is an internal function. Is '__rte_experimental' tag required? (see above) [snip] > > +static __rte_always_inline unsigned int __rte_experimental > > +rte_stack_pop(struct rte_stack *s, void **obj_table, unsigned int n) { > > + if (unlikely(n =3D=3D 0 || obj_table =3D=3D NULL)) > > + return 0; > 's =3D=3D NULL' can be added as well. Similar check is missing in 'rte_st= ack_push'. > Since these are data-path APIs, RTE_ASSERT would be better. >=20 Good point. I'll add RTE_ASSERT for obj_table and s. That won't work for "= n =3D=3D 0" -- the pop code assumes n > 0, so we can't allow that check to = be compiled out. [snip] > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Create a new stack named *name* in memory. > > + * > > + * This function uses ``memzone_reserve()`` to allocate memory for a > > +stack of > > + * size *count*. The behavior of the stack is controlled by the *flags= *. > > + * > > + * @param name > > + * The name of the stack. > > + * @param count > > + * The size of the stack. > > + * @param socket_id > > + * The *socket_id* argument is the socket identifier in case of > > + * NUMA. The value can be *SOCKET_ID_ANY* if there is no NUMA > > + * constraint for the reserved zone. > > + * @param flags > > + * Reserved for future use. > > + * @return > > + * On success, the pointer to the new allocated stack. NULL on error= with > > + * rte_errno set appropriately. Possible errno values include: > > + * - ENOSPC - the maximum number of memzones has already been > > allocated > > + * - EEXIST - a stack with the same name already exists > This is not implemented currently It is -- see above. Thanks, Gage From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 0F9EFA05D3 for ; Fri, 29 Mar 2019 20:23:43 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 91E8E1DB8; Fri, 29 Mar 2019 20:23:41 +0100 (CET) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id CD36011A4 for ; Fri, 29 Mar 2019 20:23:39 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Mar 2019 12:23:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,285,1549958400"; d="scan'208";a="157044171" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 29 Mar 2019 12:23:38 -0700 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 29 Mar 2019 12:23:38 -0700 Received: from fmsmsx108.amr.corp.intel.com ([169.254.9.216]) by fmsmsx117.amr.corp.intel.com ([169.254.3.142]) with mapi id 14.03.0415.000; Fri, 29 Mar 2019 12:23:38 -0700 From: "Eads, Gage" To: Honnappa Nagarahalli , "dev@dpdk.org" CC: "olivier.matz@6wind.com" , "arybchenko@solarflare.com" , "Richardson, Bruce" , "Ananyev, Konstantin" , "Gavin Hu (Arm Technology China)" , nd , "thomas@monjalon.net" , nd , Thomas Monjalon Thread-Topic: [PATCH v3 1/8] stack: introduce rte stack library Thread-Index: AQHU1CtVrsaMA7YCkEOnYy6dB4cOVqYgkw6AgAJQ+XA= Date: Fri, 29 Mar 2019 19:23:37 +0000 Message-ID: <9184057F7FC11744A2107296B6B8EB1E5420D923@FMSMSX108.amr.corp.intel.com> References: <20190305164256.2367-1-gage.eads@intel.com> <20190306144559.391-1-gage.eads@intel.com> <20190306144559.391-2-gage.eads@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzE2ZTU5YTUtYjk0ZS00NDMyLTllYmQtYjQzNWI2Y2ExNDJkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidndTUmo4SGNpdW9KbFJxclpEN0NwQ1wvWFJkTWpodnA4bFBQTnlKd0hzbTRoSWpFSkpjNDFvQWF5cWJxNExhRE4ifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.1.200.108] Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 1/8] stack: introduce rte stack library 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" Message-ID: <20190329192337.CKKAzdapXFFYWvFybiNkE-__-1C57Smk9bXZxwqN15M@z> @Thomas: I expect I can address Honnappa's feedback within a day or two. Si= nce today is the 19.05 merge deadline, what do you think about these option= s for merging? 1. Merge V4 now and address these comments during RC1. 2. Delay merge until RC2, with all comments addressed. In terms of risk, Honnappa identified an incorrect memory ordering argument= (patch 6/8), but that doesn't affect the one platform (x86-64) that can (c= urrently) use this library. His other comments address readability, error-c= hecking, and performance, but aren't critical. Beyond that, this patchset = is isolated from the rest of DPDK. So, I think the risk to the project is v= ery low. (Also, note that I accidentally left off Olivier's Reviewed-by tag in V4's = patches 1, 3, 5, and 6 -- I'll address that as well) > -----Original Message----- > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Thursday, March 28, 2019 6:27 PM > To: Eads, Gage ; dev@dpdk.org > Cc: olivier.matz@6wind.com; arybchenko@solarflare.com; Richardson, Bruce > ; Ananyev, Konstantin > ; Gavin Hu (Arm Technology China) > ; nd ; thomas@monjalon.net; nd > > Subject: RE: [PATCH v3 1/8] stack: introduce rte stack library >=20 > Hi Gage, > Apologies for the late comments. >=20 No problem, I appreciate the feedback. [snip] > > +static ssize_t > > +rte_stack_get_memsize(unsigned int count) { > > + ssize_t sz =3D sizeof(struct rte_stack); > > + > > + /* Add padding to avoid false sharing conflicts */ > > + sz +=3D RTE_CACHE_LINE_ROUNDUP(count * sizeof(void *)) + > > + 2 * RTE_CACHE_LINE_SIZE; > I did not understand how the false sharing is caused and how this padding= is > solving the issue. Verbose comments would help. The additional padding (beyond the CACHE_LINE_ROUNDUP) is to prevent false = sharing caused by adjacent/next-line hardware prefetchers. I'll address thi= s. [snip] > > +struct rte_stack * > > +rte_stack_create(const char *name, unsigned int count, int socket_id, > > + uint32_t flags) > > +{ > > + char mz_name[RTE_MEMZONE_NAMESIZE]; > > + struct rte_stack_list *stack_list; > > + const struct rte_memzone *mz; > > + struct rte_tailq_entry *te; > > + struct rte_stack *s; > > + unsigned int sz; > > + int ret; > > + > > + RTE_SET_USED(flags); > > + > > + sz =3D rte_stack_get_memsize(count); > > + > > + ret =3D snprintf(mz_name, sizeof(mz_name), "%s%s", > > + RTE_STACK_MZ_PREFIX, name); > > + if (ret < 0 || ret >=3D (int)sizeof(mz_name)) { > > + rte_errno =3D ENAMETOOLONG; > > + return NULL; > > + } > > + > > + te =3D rte_zmalloc("STACK_TAILQ_ENTRY", sizeof(*te), 0); > > + if (te =3D=3D NULL) { > > + STACK_LOG_ERR("Cannot reserve memory for tailq\n"); > > + rte_errno =3D ENOMEM; > > + return NULL; > > + } > > + > > + rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK); > > + > I think there is a need to check if a stack with the same name exists alr= eady. rte_memzone_reserve_aligned() does just that. This behavior is tested in th= e function test_stack_name_reuse(), added in commit " test/stack: add stack= test". > > + mz =3D rte_memzone_reserve_aligned(mz_name, sz, socket_id, > > + 0, __alignof__(*s)); > > + if (mz =3D=3D NULL) { > > + STACK_LOG_ERR("Cannot reserve stack memzone!\n"); > > + rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK); > > + rte_free(te); > > + return NULL; > > + } [snip] > > +void > > +rte_stack_free(struct rte_stack *s) > > +{ > > + struct rte_stack_list *stack_list; > > + struct rte_tailq_entry *te; > > + > > + if (s =3D=3D NULL) > > + return; > > + > Adding a check to make sure the length of the stack is 0 would help catch > issues? My preference is to leave that check to the user, for any apps that want to= /can safely free non-empty stacks. [snip] > > +#define RTE_TAILQ_STACK_NAME "RTE_STACK" > > +#define RTE_STACK_MZ_PREFIX "STK_" > Nit, "STACK_" would be easier to debug Since RTE_MEMZONE_NAMESIZE (32) doesn't give us a lot of space, I kept the = prefix short. Adding 2 more characters *probably* won't make a difference..= .but I'd prefer the shortened name. > > +/** The maximum length of a stack name. */ #define > RTE_STACK_NAMESIZE > > +(RTE_MEMZONE_NAMESIZE - \ > > + sizeof(RTE_STACK_MZ_PREFIX) + 1) > > + [snip] > > +/** > > + * @internal Push several objects on the stack (MT-safe). > > + * > > + * @param s > > + * A pointer to the stack structure. > > + * @param obj_table > > + * A pointer to a table of void * pointers (objects). > > + * @param n > > + * The number of objects to push on the stack from the obj_table. > > + * @return > > + * Actual number of objects pushed (either 0 or *n*). > > + */ > > +static __rte_always_inline unsigned int __rte_experimental > This is an internal function. Is '__rte_experimental' tag required? I don't think so, but I erred on the side of caution. I don't think the tag= causes any problems. >=20 > > +rte_stack_std_push(struct rte_stack *s, void * const *obj_table, > > +unsigned int n) { > Since this is an internal function, does it make sense to add '__' to the > beginning of the function name (similar to what is done in rte_ring?). Makes sense. I'll address this. [snip] > > +/** > > + * @internal Pop several objects from the stack (MT-safe). > > + * > > + * @param s > > + * A pointer to the stack structure. > > + * @param obj_table > > + * A pointer to a table of void * pointers (objects). > > + * @param n > > + * The number of objects to pull from the stack. > > + * @return > > + * Actual number of objects popped (either 0 or *n*). > > + */ > > +static __rte_always_inline unsigned int __rte_experimental > This is an internal function. Is '__rte_experimental' tag required? (see above) [snip] > > +static __rte_always_inline unsigned int __rte_experimental > > +rte_stack_pop(struct rte_stack *s, void **obj_table, unsigned int n) { > > + if (unlikely(n =3D=3D 0 || obj_table =3D=3D NULL)) > > + return 0; > 's =3D=3D NULL' can be added as well. Similar check is missing in 'rte_st= ack_push'. > Since these are data-path APIs, RTE_ASSERT would be better. >=20 Good point. I'll add RTE_ASSERT for obj_table and s. That won't work for "= n =3D=3D 0" -- the pop code assumes n > 0, so we can't allow that check to = be compiled out. [snip] > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice > > + * > > + * Create a new stack named *name* in memory. > > + * > > + * This function uses ``memzone_reserve()`` to allocate memory for a > > +stack of > > + * size *count*. The behavior of the stack is controlled by the *flags= *. > > + * > > + * @param name > > + * The name of the stack. > > + * @param count > > + * The size of the stack. > > + * @param socket_id > > + * The *socket_id* argument is the socket identifier in case of > > + * NUMA. The value can be *SOCKET_ID_ANY* if there is no NUMA > > + * constraint for the reserved zone. > > + * @param flags > > + * Reserved for future use. > > + * @return > > + * On success, the pointer to the new allocated stack. NULL on error= with > > + * rte_errno set appropriately. Possible errno values include: > > + * - ENOSPC - the maximum number of memzones has already been > > allocated > > + * - EEXIST - a stack with the same name already exists > This is not implemented currently It is -- see above. Thanks, Gage