| Message ID | 1421421030-22261-1-git-send-email-declan.doherty@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Declan Doherty > Sent: Friday, January 16, 2015 3:11 PM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] eal / malloc : alignment parameter check failing > due to changes in rte_is_power_of_2 > > In commit 2fc8d6d the behaviour of function rte_is_power_of_2 was > changed to not return true for 0. memzone_reserve_aligned_thread_unsafe > and rte_malloc_socket both make the assumption that for align = 0 > !rte_is_power_of_2(align) will return false. This patch adds a check > that align parameter is non-zero before doing the power of 2 check > > Signed-off-by: Declan Doherty <declan.doherty@intel.com> Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
2015-01-16 15:10, Declan Doherty: > In commit 2fc8d6d the behaviour of function rte_is_power_of_2 was > changed to not return true for 0. memzone_reserve_aligned_thread_unsafe > and rte_malloc_socket both make the assumption that for align = 0 > !rte_is_power_of_2(align) will return false. This patch adds a check > that align parameter is non-zero before doing the power of 2 check > > Signed-off-by: Declan Doherty <declan.doherty@intel.com> [...] > - if (!rte_is_power_of_2(align)) { > + if (align ? !rte_is_power_of_2(align) : 0) { [...] > - if (size == 0 || !rte_is_power_of_2(align)) > + if (size == 0 || align ? !rte_is_power_of_2(align) : 0) I don't understand why you write "align ? !rte_is_power_of_2(align) : 0" instead of the more readable "align && !rte_is_power_of_2(align)" ? Pablo acked it so I guess there is something obvious I'm missing.
> -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Friday, January 16, 2015 5:02 PM > To: Doherty, Declan > Cc: dev@dpdk.org; De Lara Guarch, Pablo > Subject: Re: [dpdk-dev] [PATCH] eal / malloc : alignment parameter check failing > due to changes in rte_is_power_of_2 > > 2015-01-16 15:10, Declan Doherty: > > In commit 2fc8d6d the behaviour of function rte_is_power_of_2 was > > changed to not return true for 0. memzone_reserve_aligned_thread_unsafe > > and rte_malloc_socket both make the assumption that for align = 0 > > !rte_is_power_of_2(align) will return false. This patch adds a check > > that align parameter is non-zero before doing the power of 2 check > > > > Signed-off-by: Declan Doherty <declan.doherty@intel.com> > [...] > > - if (!rte_is_power_of_2(align)) { > > + if (align ? !rte_is_power_of_2(align) : 0) { > [...] > > - if (size == 0 || !rte_is_power_of_2(align)) > > + if (size == 0 || align ? !rte_is_power_of_2(align) : 0) > > I don't understand why you write "align ? !rte_is_power_of_2(align) : 0" > instead of the more readable "align && !rte_is_power_of_2(align)" ? > > Pablo acked it so I guess there is something obvious I'm missing. > > -- > Thomas No there's nothing you're missing, this is just the way I saw the logic, if align is none zero, then test the power of 2 condition otherwise return 0. I have no problem with your suggestion in you prefer that, at the end of the day the logic test works out equivalent.
2015-01-18 19:26, Doherty, Declan: > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > 2015-01-16 15:10, Declan Doherty: > > > In commit 2fc8d6d the behaviour of function rte_is_power_of_2 was > > > changed to not return true for 0. memzone_reserve_aligned_thread_unsafe > > > and rte_malloc_socket both make the assumption that for align = 0 > > > !rte_is_power_of_2(align) will return false. This patch adds a check > > > that align parameter is non-zero before doing the power of 2 check > > > > > > Signed-off-by: Declan Doherty <declan.doherty@intel.com> > > [...] > > > - if (!rte_is_power_of_2(align)) { > > > + if (align ? !rte_is_power_of_2(align) : 0) { > > [...] > > > - if (size == 0 || !rte_is_power_of_2(align)) > > > + if (size == 0 || align ? !rte_is_power_of_2(align) : 0) There is an operator precedence bug here. Parens are needed after ||. > > I don't understand why you write "align ? !rte_is_power_of_2(align) : 0" > > instead of the more readable "align && !rte_is_power_of_2(align)" ? > > > > Pablo acked it so I guess there is something obvious I'm missing. > > No there's nothing you're missing, this is just the way I saw the logic, if align is none > zero, then test the power of 2 condition otherwise return 0. I have no problem with > your suggestion in you prefer that, at the end of the day the logic test works out equivalent. So I change to the simpler && form and I add parentheses after || to correctly check size==0. Applied with above changes. Thanks
diff --git a/lib/librte_eal/common/eal_common_memzone.c b/lib/librte_eal/common/eal_common_memzone.c index b5a5d72..3f09338 100644 --- a/lib/librte_eal/common/eal_common_memzone.c +++ b/lib/librte_eal/common/eal_common_memzone.c @@ -156,7 +156,7 @@ memzone_reserve_aligned_thread_unsafe(const char *name, size_t len, } /* if alignment is not a power of two */ - if (!rte_is_power_of_2(align)) { + if (align ? !rte_is_power_of_2(align) : 0) { RTE_LOG(ERR, EAL, "%s(): Invalid alignment: %u\n", __func__, align); rte_errno = EINVAL; diff --git a/lib/librte_malloc/rte_malloc.c b/lib/librte_malloc/rte_malloc.c index b966fc7..15c7e20 100644 --- a/lib/librte_malloc/rte_malloc.c +++ b/lib/librte_malloc/rte_malloc.c @@ -75,7 +75,7 @@ rte_malloc_socket(const char *type, size_t size, unsigned align, int socket_arg) void *ret; /* return NULL if size is 0 or alignment is not power-of-2 */ - if (size == 0 || !rte_is_power_of_2(align)) + if (size == 0 || align ? !rte_is_power_of_2(align) : 0) return NULL; if (socket_arg == SOCKET_ID_ANY)
In commit 2fc8d6d the behaviour of function rte_is_power_of_2 was changed to not return true for 0. memzone_reserve_aligned_thread_unsafe and rte_malloc_socket both make the assumption that for align = 0 !rte_is_power_of_2(align) will return false. This patch adds a check that align parameter is non-zero before doing the power of 2 check Signed-off-by: Declan Doherty <declan.doherty@intel.com> --- lib/librte_eal/common/eal_common_memzone.c | 2 +- lib/librte_malloc/rte_malloc.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)