[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 <***@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;
}

/*

[Wiles, Keith]

Post by David Harton
The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.
---
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?

Post by David Harton
+ return new_ptr;
}
/*
--

[David Harton (dharton)]

-----Original Message-----
Sent: Friday, December 07, 2018 6:41 PM
Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_zalloc_socket to zero memory

Post by David Harton
The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.
---
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.

Post by David Harton
+ return new_ptr;
}
/*
--
2.19.1

[Wiles, Keith]

This post might be inappropriate. Click to display it.

[Wiles, Keith]

This post might be inappropriate. Click to display it.

[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 <***@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;
}

/*

[Burakov, Anatoly]

Post by David Harton
The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.
---
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.

[Burakov, Anatoly]

Post by David Harton
The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.
---
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

[Mattias Rönnblom]

Post by David Harton
The zalloc and calloc functions do not actually zero the memory.
Added memset to rte_zmalloc_socket() so allocated memory is cleared.
---
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.

Post by David Harton
+ return new_ptr;
}
/*