[dpdk-dev] [PATCH] lib/librte_eal/common/rte_malloc: remove redundant check for size and alignment

[dpdk-dev] [PATCH] lib/librte_eal/common/rte_malloc: remove redundant check for size and alignment

[Sarosh Arif]

Since mallock_socket() always calls malloc_heap_alloc() and
this check is present inside malloc_heap_alloc() so there is
no need to place it in mallock_socket(). 

Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
---
 lib/librte_eal/common/rte_malloc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
index 9d39e58c0..51256117b 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -61,10 +61,6 @@ malloc_socket(const char *type, size_t size, unsigned int align,
 {
 	void *ptr;
 
-	/* return NULL if size is 0 or alignment is not power-of-2 */
-	if (size == 0 || (align && !rte_is_power_of_2(align)))
-		return NULL;
-
 	/* if there are no hugepages and if we are not allocating from an
 	 * external heap, use memory from any socket available. checking for
 	 * socket being external may return -1 in case of invalid socket, but
-- 
2.17.1

Re: [dpdk-dev] [PATCH] lib/librte_eal/common/rte_malloc: remove redundant check for size and alignment

[Lukasz Wojciechowski]

W dniu 15.07.2020 o 15:31, Sarosh Arif pisze:
> Since mallock_socket() always calls malloc_heap_alloc() and
> this check is present inside malloc_heap_alloc() so there is
> no need to place it in mallock_socket().
>
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---
>   lib/librte_eal/common/rte_malloc.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index 9d39e58c0..51256117b 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -61,10 +61,6 @@ malloc_socket(const char *type, size_t size, unsigned int align,
>   {
>   	void *ptr;
>   
> -	/* return NULL if size is 0 or alignment is not power-of-2 */
> -	if (size == 0 || (align && !rte_is_power_of_2(align)))
> -		return NULL;
> -
>   	/* if there are no hugepages and if we are not allocating from an
>   	 * external heap, use memory from any socket available. checking for
>   	 * socket being external may return -1 in case of invalid socket, but
Yes the check is duplicated.
However it allows to avoid many unnecessary operations that can cost a 
lot, e.g. locking memory inside rte_malloc_heap_socket_is_external().
I would keep it.

Please also note, that the checks are not quite the same as the 
malloc_heap_alloc is called with changed align value (align == 0 ? 1 : 
align):
         ptr = malloc_heap_alloc(type, size, socket_arg, 0, align == 0 ? 
1 : align, 0, false);
Although there should be no change in check behavior as for both align 
==0 and align == 1 the check will fail and won't be the cause of 
returning NULL.

-- 
Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH] lib/librte_eal/common/rte_malloc: remove redundant check for size and alignment

[Burakov, Anatoly]

On 15-Jul-20 2:31 PM, Sarosh Arif wrote:
> Since mallock_socket() always calls malloc_heap_alloc() and
> this check is present inside malloc_heap_alloc() so there is
> no need to place it in mallock_socket().
> 
> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
> ---

Technically not a bug, so i don't think backporting this is necessary.

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] lib/librte_eal/common/rte_malloc: remove redundant check for size and alignment

[Burakov, Anatoly]

On 17-Jul-20 3:40 PM, Burakov, Anatoly wrote:
> On 15-Jul-20 2:31 PM, Sarosh Arif wrote:
>> Since mallock_socket() always calls malloc_heap_alloc() and
>> this check is present inside malloc_heap_alloc() so there is
>> no need to place it in mallock_socket().
>>
>> Signed-off-by: Sarosh Arif <sarosh.arif@emumba.com>
>> ---
> 
> Technically not a bug, so i don't think backporting this is necessary.
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
Actually, now that i think of it, i agree with Lukasz - while this check 
isn't necessary, it's an early exit to avoid memory locking. So, NACK.

-- 
Thanks,
Anatoly