diff mbox series

eal: fix rte_zalloc_socket to zero memory

Message ID 20181207222420.9508-1-dharton@cisco.com (mailing list archive)
State Superseded, archived
Headers
Series eal: fix rte_zalloc_socket to zero memory |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

David Harton Dec. 7, 2018, 10:24 p.m. UTC 
  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(-)

Comments

Wiles, Keith Dec. 7, 2018, 11:41 p.m. UTC | #1 
> 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
David Harton Dec. 7, 2018, 11:47 p.m. UTC | #2 
> -----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
Wiles, Keith Dec. 7, 2018, 11:51 p.m. UTC | #3 
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
Wiles, Keith Dec. 7, 2018, 11:54 p.m. UTC | #4 
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
Mattias Rönnblom Dec. 9, 2018, 7:20 p.m. UTC | #5 
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;
>   }
>   
>   /*
>
Bruce Richardson Dec. 10, 2018, 10:26 a.m. UTC | #6 
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
Burakov, Anatoly Dec. 10, 2018, 10:45 a.m. UTC | #7 
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
>
diff mbox series

Patch

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;
 }
 
 /*