[dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[David Harton]

The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.

Signed-off-by: David Harton <dharton@cisco.com>
---
 lib/librte_eal/common/rte_malloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
index 0da5ad5e8..be382e534 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned align)
 void *
 rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
 {
-	return rte_malloc_socket(type, size, align, socket);
+	void *new_ptr = rte_malloc_socket(type, size, align, socket);
+	if (new_ptr) memset(new_ptr, 0, size);
+	return new_ptr;
 }
 
 /*
-- 
2.19.1

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[Wiles, Keith]

> On Dec 7, 2018, at 3:24 PM, David Harton <dharton@cisco.com> wrote:
> 
> The zalloc and calloc functions do not actually zero the memory.
> Added memset to rte_zmalloc_socket() so allocated memory is cleared.
> 
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
> lib/librte_eal/common/rte_malloc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index 0da5ad5e8..be382e534 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned align)
> void *
> rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
> {
> -	return rte_malloc_socket(type, size, align, socket);
> +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
> +	if (new_ptr) memset(new_ptr, 0, size);

Someone will hate me, but the memset() line should be on the next line not on the ‘if’ line. It does not explicitly state in the coding style, but do not see any example in the coding style on having the one line statement on the line of the ‘if’.

What is the ruling here, I would suggest it be on the next line?

> +	return new_ptr;
> }
> 
> /*
> -- 
> 2.19.1
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[David Harton (dharton)]

> -----Original Message-----
> From: Wiles, Keith <keith.wiles@intel.com>
> Sent: Friday, December 07, 2018 6:41 PM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory
> 
> 
> 
> > On Dec 7, 2018, at 3:24 PM, David Harton <dharton@cisco.com> wrote:
> >
> > The zalloc and calloc functions do not actually zero the memory.
> > Added memset to rte_zmalloc_socket() so allocated memory is cleared.
> >
> > Signed-off-by: David Harton <dharton@cisco.com>
> > ---
> > lib/librte_eal/common/rte_malloc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/common/rte_malloc.c
> > b/lib/librte_eal/common/rte_malloc.c
> > index 0da5ad5e8..be382e534 100644
> > --- a/lib/librte_eal/common/rte_malloc.c
> > +++ b/lib/librte_eal/common/rte_malloc.c
> > @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned
> > align) void * rte_zmalloc_socket(const char *type, size_t size,
> > unsigned align, int socket) {
> > -	return rte_malloc_socket(type, size, align, socket);
> > +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
> > +	if (new_ptr) memset(new_ptr, 0, size);
> 
> Someone will hate me, but the memset() line should be on the next line not
> on the ‘if’ line. It does not explicitly state in the coding style, but do
> not see any example in the coding style on having the one line statement
> on the line of the ‘if’.
> 
> What is the ruling here, I would suggest it be on the next line?

FWIW, I copied the pattern from rte_free() but I it is the only use in the file.

I have no problems changing it and fixing rte_free() too if that is the desire.

> 
> > +	return new_ptr;
> > }
> >
> > /*
> > --
> > 2.19.1
> >
> 
> Regards,
> Keith

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[Wiles, Keith]

On Dec 7, 2018, at 4:47 PM, David Harton (dharton) <dharton@cisco.com<mailto:dharton@cisco.com>> wrote:



-----Original Message-----
From: Wiles, Keith <keith.wiles@intel.com<mailto:keith.wiles@intel.com>>
Sent: Friday, December 07, 2018 6:41 PM
To: David Harton (dharton) <dharton@cisco.com<mailto:dharton@cisco.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Burakov, Anatoly <anatoly.burakov@intel.com<mailto:anatoly.burakov@intel.com>>
Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory



On Dec 7, 2018, at 3:24 PM, David Harton <dharton@cisco.com<mailto:dharton@cisco.com>> wrote:

The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.

Signed-off-by: David Harton <dharton@cisco.com<mailto:dharton@cisco.com>>
---
lib/librte_eal/common/rte_malloc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/rte_malloc.c
b/lib/librte_eal/common/rte_malloc.c
index 0da5ad5e8..be382e534 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned
align) void * rte_zmalloc_socket(const char *type, size_t size,
unsigned align, int socket) {
- return rte_malloc_socket(type, size, align, socket);
+ void *new_ptr = rte_malloc_socket(type, size, align, socket);
+ if (new_ptr) memset(new_ptr, 0, size);

Someone will hate me, but the memset() line should be on the next line not
on the ‘if’ line. It does not explicitly state in the coding style, but do
not see any example in the coding style on having the one line statement
on the line of the ‘if’.

What is the ruling here, I would suggest it be on the next line?

FWIW, I copied the pattern from rte_free() but I it is the only use in the file.

I have no problems changing it and fixing rte_free() too if that is the desire.

Lets wait for the big guns to decide what is the correct method and updated the coding style.

This also points to a problem as we need a tool to run and fix up the code, like uncrustify or similar tool, this way I can stop being the code style police :-)


+ return new_ptr;
}

/*
--
2.19.1


Regards,
Keith

Regards,
Keith

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[Wiles, Keith]

fix up my emailer issue, sending out in non-plain text. :-(

> On Dec 7, 2018, at 4:47 PM, David Harton (dharton) <dharton@cisco.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Wiles, Keith <keith.wiles@intel.com>
>> Sent: Friday, December 07, 2018 6:41 PM
>> To: David Harton (dharton) <dharton@cisco.com>
>> Cc: dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory
>> 
>> 
>> 
>>> On Dec 7, 2018, at 3:24 PM, David Harton <dharton@cisco.com> wrote:
>>> 
>>> The zalloc and calloc functions do not actually zero the memory.
>>> Added memset to rte_zmalloc_socket() so allocated memory is cleared.
>>> 
>>> Signed-off-by: David Harton <dharton@cisco.com>
>>> ---
>>> lib/librte_eal/common/rte_malloc.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/lib/librte_eal/common/rte_malloc.c
>>> b/lib/librte_eal/common/rte_malloc.c
>>> index 0da5ad5e8..be382e534 100644
>>> --- a/lib/librte_eal/common/rte_malloc.c
>>> +++ b/lib/librte_eal/common/rte_malloc.c
>>> @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned
>>> align) void * rte_zmalloc_socket(const char *type, size_t size,
>>> unsigned align, int socket) {
>>> -	return rte_malloc_socket(type, size, align, socket);
>>> +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
>>> +	if (new_ptr) memset(new_ptr, 0, size);
>> 
>> Someone will hate me, but the memset() line should be on the next line not
>> on the ‘if’ line. It does not explicitly state in the coding style, but do
>> not see any example in the coding style on having the one line statement
>> on the line of the ‘if’.
>> 
>> What is the ruling here, I would suggest it be on the next line?
> 
> FWIW, I copied the pattern from rte_free() but I it is the only use in the file.
> 
> I have no problems changing it and fixing rte_free() too if that is the desire.
> 

Let's wait for the big guns to decide what is the correct method and updated the coding style.

This also points to a problem as we need a tool to run and fix up the code, like uncrustify or similar tool, this way I can stop being the code style police :-)

>> 
>>> +	return new_ptr;
>>> }
>>> 
>>> /*
>>> --
>>> 2.19.1
>>> 
>> 
>> Regards,
>> Keith
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[Mattias Rönnblom]

On 2018-12-07 23:24, David Harton wrote:
> The zalloc and calloc functions do not actually zero the memory.
> Added memset to rte_zmalloc_socket() so allocated memory is cleared.
> 
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
>   lib/librte_eal/common/rte_malloc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index 0da5ad5e8..be382e534 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned align)
>   void *
>   rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
>   {
> -	return rte_malloc_socket(type, size, align, socket);
> +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
> +	if (new_ptr) memset(new_ptr, 0, size);

Maybe it would be worth to have a likely() here.

> +	return new_ptr;
>   }
>   
>   /*
> 

[dpdk-dev] [PATCH v2] eal: fix rte_zalloc_socket to zero memory

[David Harton]

The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.

Signed-off-by: David Harton <dharton@cisco.com>
---

v2 Indented if clause

 lib/librte_eal/common/rte_malloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
index 0da5ad5e8..40a5384ff 100644
--- a/lib/librte_eal/common/rte_malloc.c
+++ b/lib/librte_eal/common/rte_malloc.c
@@ -74,7 +74,10 @@ rte_malloc(const char *type, size_t size, unsigned align)
 void *
 rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
 {
-	return rte_malloc_socket(type, size, align, socket);
+	void *new_ptr = rte_malloc_socket(type, size, align, socket);
+	if (new_ptr)
+		memset(new_ptr, 0, size);
+	return new_ptr;
 }
 
 /*
-- 
2.19.1

Re: [dpdk-dev] [PATCH v2] eal: fix rte_zalloc_socket to zero memory

[Burakov, Anatoly]

On 09-Dec-18 8:11 PM, David Harton wrote:
> The zalloc and calloc functions do not actually zero the memory.
> Added memset to rte_zmalloc_socket() so allocated memory is cleared.
> 
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
> 
> v2 Indented if clause
> 
>   lib/librte_eal/common/rte_malloc.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index 0da5ad5e8..40a5384ff 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -74,7 +74,10 @@ rte_malloc(const char *type, size_t size, unsigned align)
>   void *
>   rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
>   {
> -	return rte_malloc_socket(type, size, align, socket);
> +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
> +	if (new_ptr)
> +		memset(new_ptr, 0, size);
> +	return new_ptr;
>   }
>   
>   /*
> 
NAK, see comments to v1.

TL;DR this is not needed, rte_free does memset(0). If there are 
circumstances where some memory is not erased, this is a bug and should 
be fixed at the source.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[Bruce Richardson]

On Fri, Dec 07, 2018 at 05:24:20PM -0500, David Harton wrote:
> The zalloc and calloc functions do not actually zero the memory.
> Added memset to rte_zmalloc_socket() so allocated memory is cleared.
> 
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
>  lib/librte_eal/common/rte_malloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
> index 0da5ad5e8..be382e534 100644
> --- a/lib/librte_eal/common/rte_malloc.c
> +++ b/lib/librte_eal/common/rte_malloc.c
> @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned align)
>  void *
>  rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
>  {
> -	return rte_malloc_socket(type, size, align, socket);
> +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
> +	if (new_ptr) memset(new_ptr, 0, size);
> +	return new_ptr;
>  }

While this is functionally correct, I believe this memset should not
actually be needed. A few years back we changed the behaviour in DPDK to
always zero memory on free, rather than zeroing on allocate. This worked
fine because the kernel always gave us zeroed hugepages and zeroing them a
second time was a waste of cycles. The percentage of memory that was
subsequently freed and reallocated was small so zeroing on free saved quite
a bit of processing time, especially at app startup.

If, following all the memory rework in recent releases, this scheme of
zeroing on free no longer works, I'd rather see that fixed than go back to
the scheme of zeroing on allocation. [Assuming we do fix it, a comment
explaining the missing memset would also be good to avoid future patches
here]

Regards,
/Bruce

Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

[Burakov, Anatoly]

On 10-Dec-18 10:26 AM, Bruce Richardson wrote:
> On Fri, Dec 07, 2018 at 05:24:20PM -0500, David Harton wrote:
>> The zalloc and calloc functions do not actually zero the memory.
>> Added memset to rte_zmalloc_socket() so allocated memory is cleared.
>>
>> Signed-off-by: David Harton <dharton@cisco.com>
>> ---
>>   lib/librte_eal/common/rte_malloc.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/rte_malloc.c b/lib/librte_eal/common/rte_malloc.c
>> index 0da5ad5e8..be382e534 100644
>> --- a/lib/librte_eal/common/rte_malloc.c
>> +++ b/lib/librte_eal/common/rte_malloc.c
>> @@ -74,7 +74,9 @@ rte_malloc(const char *type, size_t size, unsigned align)
>>   void *
>>   rte_zmalloc_socket(const char *type, size_t size, unsigned align, int socket)
>>   {
>> -	return rte_malloc_socket(type, size, align, socket);
>> +	void *new_ptr = rte_malloc_socket(type, size, align, socket);
>> +	if (new_ptr) memset(new_ptr, 0, size);
>> +	return new_ptr;
>>   }
> 
> While this is functionally correct, I believe this memset should not
> actually be needed. A few years back we changed the behaviour in DPDK to
> always zero memory on free, rather than zeroing on allocate. This worked
> fine because the kernel always gave us zeroed hugepages and zeroing them a
> second time was a waste of cycles. The percentage of memory that was
> subsequently freed and reallocated was small so zeroing on free saved quite
> a bit of processing time, especially at app startup.
> 
> If, following all the memory rework in recent releases, this scheme of
> zeroing on free no longer works, I'd rather see that fixed than go back to
> the scheme of zeroing on allocation. [Assuming we do fix it, a comment
> explaining the missing memset would also be good to avoid future patches
> here]

Bruce is correct. Memory is zeroed-out on free, and we get zeroed-out 
pages from the kernel, so memory from rte_malloc and rte_zmalloc are (or 
should be) functionally equivalent.

If there are any circumstances where memory is not being freed (and 
there were a few bugs like that), then i'd rather fix those too.

> 
> Regards,
> /Bruce
> 


-- 
Thanks,
Anatoly