Discussion:
[dpdk-dev] [PATCH] mempool: fix lack of free() registration
(too old to reply)
Zoltan Kiss
2016-07-19 14:37:40 UTC
Permalink
The new mempool handler interface forgets to register the free() function
of the ops. Introduced in this patch:

449c49b9 mempool: support handler operations

Signed-off-by: Zoltan Kiss <***@schaman.hu>
---
lib/librte_mempool/rte_mempool_ops.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index fd0b64c..5f24de2 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -81,6 +81,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
ops = &rte_mempool_ops_table.ops[ops_index];
snprintf(ops->name, sizeof(ops->name), "%s", h->name);
ops->alloc = h->alloc;
+ ops->free = h->free;
ops->enqueue = h->enqueue;
ops->dequeue = h->dequeue;
ops->get_count = h->get_count;
--
1.9.1
Zoltan Kiss
2016-07-19 14:37:41 UTC
Permalink
A recent fix brought up an issue about the size of the 'name' fields:

85cf0079 mem: avoid memzone/mempool/ring name truncation

These relations should be observed:

RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)

Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.

Signed-off-by: Zoltan Kiss <***@schaman.hu>
---
lib/librte_eal/common/include/rte_memzone.h | 2 +-
lib/librte_mempool/rte_mempool.h | 4 +++-
lib/librte_ring/rte_ring.h | 5 ++++-
3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_memzone.h b/lib/librte_eal/common/include/rte_memzone.h
index f69b5a8..ba3a1f0 100644
--- a/lib/librte_eal/common/include/rte_memzone.h
+++ b/lib/librte_eal/common/include/rte_memzone.h
@@ -74,7 +74,7 @@ extern "C" {
*/
struct rte_memzone {

-#define RTE_MEMZONE_NAMESIZE 32 /**< Maximum length of memory zone name.*/
+#define RTE_MEMZONE_NAMESIZE (32 + 6) /**< Maximum length of memory zone name.*/
char name[RTE_MEMZONE_NAMESIZE]; /**< Name of the memory zone. */

phys_addr_t phys_addr; /**< Start physical address. */
diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 4a8fbb1..61e8d19 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -123,7 +123,9 @@ struct rte_mempool_objsz {
/**< Total size of an object (header + elt + trailer). */
};

-#define RTE_MEMPOOL_NAMESIZE 32 /**< Maximum length of a memory pool. */
+/**< Maximum length of a memory pool's name. */
+#define RTE_MEMPOOL_NAMESIZE (RTE_RING_NAMESIZE - \
+ sizeof(RTE_MEMPOOL_MZ_PREFIX) + 1)
#define RTE_MEMPOOL_MZ_PREFIX "MP_"

/* "MP_<name>" */
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index eb45e41..d6185de 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -100,6 +100,7 @@ extern "C" {
#include <rte_lcore.h>
#include <rte_atomic.h>
#include <rte_branch_prediction.h>
+#include <rte_memzone.h>

#define RTE_TAILQ_RING_NAME "RTE_RING"

@@ -126,8 +127,10 @@ struct rte_ring_debug_stats {
} __rte_cache_aligned;
#endif

-#define RTE_RING_NAMESIZE 32 /**< The maximum length of a ring name. */
#define RTE_RING_MZ_PREFIX "RG_"
+/**< The maximum length of a ring name. */
+#define RTE_RING_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
+ sizeof(RTE_RING_MZ_PREFIX) + 1)

#ifndef RTE_RING_PAUSE_REP_COUNT
#define RTE_RING_PAUSE_REP_COUNT 0 /**< Yield after pause num of times, no yield
--
1.9.1
Olivier Matz
2016-07-19 15:37:28 UTC
Permalink
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.
I agree it is a problem for an application because it cannot know what
is the maximum name length. On the other hand, breaking the ABI for this
looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
we could keep the ABI as is.

It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I didn't
check it's feasible for memzones. What do you think?

In any case, I think it's a bit late for 16.07 for this kind of fix.

Regards,
Olivier
Zoltan Kiss
2016-07-19 15:59:28 UTC
Permalink
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.
I agree it is a problem for an application because it cannot know what
is the maximum name length. On the other hand, breaking the ABI for this
looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
we could keep the ABI as is.
But that would break the ABI too, wouldn't it? Unless you keep the array
the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
And even then, the API breaks anyway. There are applications - I have at
least some - which use all 32 bytes to store the name. Decrease that
would cause headache to change the naming scheme, because it's a 30
character long id, and chopping the last few chars would cause name
collisions and annoying bugs.
Post by Olivier Matz
It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I didn't
check it's feasible for memzones. What do you think?
It would work too, but I don't think it would help a lot. We would still
need max sizes for the names. Storing them somewhere else won't help us
in this problem.
Post by Olivier Matz
In any case, I think it's a bit late for 16.07 for this kind of fix.
Regards,
Olivier
Olivier Matz
2016-07-19 16:17:32 UTC
Permalink
Hi Zoltan,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.
I agree it is a problem for an application because it cannot know what
is the maximum name length. On the other hand, breaking the ABI for this
looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
we could keep the ABI as is.
But that would break the ABI too, wouldn't it? Unless you keep the array
the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
Yes, that was the idea.
Post by Zoltan Kiss
And even then, the API breaks anyway. There are applications - I have at
least some - which use all 32 bytes to store the name. Decrease that
would cause headache to change the naming scheme, because it's a 30
character long id, and chopping the last few chars would cause name
collisions and annoying bugs.
Before my patch (85cf0079), long names were silently truncated when
mempool created its ring and/or memzones. Now, it returns an error.

I'm not getting why changing the struct to something like below would
break the API, since it would already return an error today.

#define RTE_MEMPOOL_NAMESIZE \
(RTE_MEMZONE_NAMESIZE - sizeof(pool_prefix) - sizeof(ring prefix))
struct rte_mempool {
union {
char name[RTE_MEMPOOL_NAMESIZE];
char pad[32];
};
...
}

Anyway, it may not be the proper solution since it supposes that a
mempool includes a ring based on a memzone, which is not always true now
with mempool handlers.
Post by Zoltan Kiss
Post by Olivier Matz
It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I didn't
check it's feasible for memzones. What do you think?
It would work too, but I don't think it would help a lot. We would still
need max sizes for the names. Storing them somewhere else won't help us
in this problem.
Why should we have a maximum length for the names?


Thanks,
Olivier
Zoltan Kiss
2016-07-20 12:41:39 UTC
Permalink
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.
I agree it is a problem for an application because it cannot know what
is the maximum name length. On the other hand, breaking the ABI for this
looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
we could keep the ABI as is.
But that would break the ABI too, wouldn't it? Unless you keep the array
the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
Yes, that was the idea.
Post by Zoltan Kiss
And even then, the API breaks anyway. There are applications - I have at
least some - which use all 32 bytes to store the name. Decrease that
would cause headache to change the naming scheme, because it's a 30
character long id, and chopping the last few chars would cause name
collisions and annoying bugs.
Before my patch (85cf0079), long names were silently truncated when
mempool created its ring and/or memzones. Now, it returns an error.
With 16.04 an application could operate as expected if the first 26
character were unique. Your patch revealed the problem that characters
after these were left out of the name. Now applications fail where this
never been a bug because their naming scheme guarantees the uniqueness
on the first 26 chars (or makes it very unlikely)
Where the first 26 is not unique, it failed earlier too, because at
memzone creation it checks for duplicate names.
Post by Olivier Matz
I'm not getting why changing the struct to something like below would
break the API, since it would already return an error today.
#define RTE_MEMPOOL_NAMESIZE \
Wait, this would mean applications need to recompile to use the smaller
value. AFAIK that's an ABI break too, right? At the moment I don't see a
way to fix this without breaking the ABI
Post by Olivier Matz
(RTE_MEMZONE_NAMESIZE - sizeof(pool_prefix) - sizeof(ring prefix))
struct rte_mempool {
union {
char name[RTE_MEMPOOL_NAMESIZE];
char pad[32];
};
...
}
Anyway, it may not be the proper solution since it supposes that a
mempool includes a ring based on a memzone, which is not always true now
with mempool handlers.
Oh, as we dug deeper it gets better!
Indeed, we don't necessarily have this ring + memzone pair underneath,
but the user is not aware of that, and I think we should keep it that
way. It should only care that the string passed shouldn't be bigger than
a certain amount.
Also, even though we don't necessarily have the ring, we still reserve
memzone's in rte_mempool_populate_default(). And their name has a 3
letter prefix, and a "_%d" postfix, where the %d could be as much as
RTE_MAX_MEMZONE in worst case (2560 by default) So actually:

RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen("_2560")


As a side note, there is another bug around here: rte_ring_create()
doesn't check for name duplications. However the user of the library can
lookup based on the name with rte_ring_lookup(), and it will return the
first ring with that name
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I didn't
check it's feasible for memzones. What do you think?
It would work too, but I don't think it would help a lot. We would still
need max sizes for the names. Storing them somewhere else won't help us
in this problem.
Why should we have a maximum length for the names?
What happens if an application loads DPDK and create a mempool with a
name string 2 million characters long? Maybe nothing we should worry
about, but in general I think unlimited length function parameters are
problematic at the very least. The length should be passed at least
(which also creates a max due to the size of the param). But I think it
would be just easier to have these maximums set, observing the above
constrains.
Post by Olivier Matz
Thanks,
Olivier
Olivier Matz
2016-07-20 13:37:26 UTC
Permalink
Hi,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.
I agree it is a problem for an application because it cannot know what
is the maximum name length. On the other hand, breaking the ABI for this
looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
we could keep the ABI as is.
But that would break the ABI too, wouldn't it? Unless you keep the array
the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
Yes, that was the idea.
Post by Zoltan Kiss
And even then, the API breaks anyway. There are applications - I have at
least some - which use all 32 bytes to store the name. Decrease that
would cause headache to change the naming scheme, because it's a 30
character long id, and chopping the last few chars would cause name
collisions and annoying bugs.
Before my patch (85cf0079), long names were silently truncated when
mempool created its ring and/or memzones. Now, it returns an error.
With 16.04 an application could operate as expected if the first 26
character were unique. Your patch revealed the problem that characters
after these were left out of the name. Now applications fail where this
never been a bug because their naming scheme guarantees the uniqueness
on the first 26 chars (or makes it very unlikely)
Where the first 26 is not unique, it failed earlier too, because at
memzone creation it checks for duplicate names.
Yes, I understand that there is a behavior change for applications using
names larger than 26 between 16.04 and 16.07. I also understand that
there is no way for an application to know what is the maximum usable
size for a mempool or a ring.
Post by Zoltan Kiss
Post by Olivier Matz
I'm not getting why changing the struct to something like below would
break the API, since it would already return an error today.
#define RTE_MEMPOOL_NAMESIZE \
Wait, this would mean applications need to recompile to use the smaller
value. AFAIK that's an ABI break too, right? At the moment I don't see a
way to fix this without breaking the ABI
With this modification, if you don't recompile the application, its
behavior will still be the same as today -> it will return ENAMETOOLONG.
If you recompile it, the application will be aware of the maximum
length. To me, it seems to be a acceptable compromise for this release.

The patch you're proposing also changes the ABI of librte_ring and
librte_eal, which cannot be accepted for the release.
Post by Zoltan Kiss
Post by Olivier Matz
(RTE_MEMZONE_NAMESIZE - sizeof(pool_prefix) - sizeof(ring prefix))
struct rte_mempool {
union {
char name[RTE_MEMPOOL_NAMESIZE];
char pad[32];
};
...
}
Anyway, it may not be the proper solution since it supposes that a
mempool includes a ring based on a memzone, which is not always true now
with mempool handlers.
Oh, as we dug deeper it gets better!
Indeed, we don't necessarily have this ring + memzone pair underneath,
but the user is not aware of that, and I think we should keep it that
way. It should only care that the string passed shouldn't be bigger than
a certain amount.
Yes. What I'm just saying here is that it's not a good solution to write
in the #define that "a mempool is based on a ring + a memzone", because
if some someone adds a new mempool handler replacing the ring, and also
creating a memzone prefixed by something larger than "rg_", we will have
to break the ABI again.
Post by Zoltan Kiss
Also, even though we don't necessarily have the ring, we still reserve
memzone's in rte_mempool_populate_default(). And their name has a 3
letter prefix, and a "_%d" postfix, where the %d could be as much as
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen("_2560")
As a side note, there is another bug around here: rte_ring_create()
doesn't check for name duplications. However the user of the library can
lookup based on the name with rte_ring_lookup(), and it will return the
first ring with that name
The name uniqueness is checked by rte_memzone_reserve().
Post by Zoltan Kiss
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I didn't
check it's feasible for memzones. What do you think?
It would work too, but I don't think it would help a lot. We would still
need max sizes for the names. Storing them somewhere else won't help us
in this problem.
Why should we have a maximum length for the names?
What happens if an application loads DPDK and create a mempool with a
name string 2 million characters long? Maybe nothing we should worry
about, but in general I think unlimited length function parameters are
problematic at the very least. The length should be passed at least
(which also creates a max due to the size of the param). But I think it
would be just easier to have these maximums set, observing the above
constrains.
I think have a maximum name length brings more problems than not having
it, especially ABI problems.


Regards,
Olivier
Richardson, Bruce
2016-07-20 14:01:59 UTC
Permalink
-----Original Message-----
Sent: Wednesday, July 20, 2016 2:37 PM
Subject: Re: [dpdk-dev] [PATCH] mempool: adjust name string size in
related data types
Hi,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
A recent fix brought up an issue about the size of the 'name'
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_RING_MZ_PREFIX) RTE_MEMPOOL_NAMESIZE <=
RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the
application.
Post by Zoltan Kiss
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
Post by Zoltan Kiss
This patch increases the memzone string size to accomodate for
these prefixes, and the same happens with the ring name string.
The ABI needs to be broken to fix this API issue, this way doesn't
break applications previously not failing due to the truncating
bug now fixed.
I agree it is a problem for an application because it cannot know
what is the maximum name length. On the other hand, breaking the
ABI for this looks a bit overkill. Maybe we could reduce
RTE_MEMPOOL_NAMESIZE and RTE_RING_NAMESIZE instead of increasing
RTE_MEMZONE_NAMESIZE? That way, we could keep the ABI as is.
But that would break the ABI too, wouldn't it? Unless you keep the
array the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
Yes, that was the idea.
Post by Zoltan Kiss
And even then, the API breaks anyway. There are applications - I
have at least some - which use all 32 bytes to store the name.
Decrease that would cause headache to change the naming scheme,
because it's a 30 character long id, and chopping the last few chars
would cause name collisions and annoying bugs.
Before my patch (85cf0079), long names were silently truncated when
mempool created its ring and/or memzones. Now, it returns an error.
With 16.04 an application could operate as expected if the first 26
character were unique. Your patch revealed the problem that characters
after these were left out of the name. Now applications fail where
this never been a bug because their naming scheme guarantees the
uniqueness on the first 26 chars (or makes it very unlikely) Where the
first 26 is not unique, it failed earlier too, because at memzone
creation it checks for duplicate names.
Yes, I understand that there is a behavior change for applications using
names larger than 26 between 16.04 and 16.07. I also understand that there
is no way for an application to know what is the maximum usable size for a
mempool or a ring.
Post by Zoltan Kiss
Post by Olivier Matz
I'm not getting why changing the struct to something like below would
break the API, since it would already return an error today.
#define RTE_MEMPOOL_NAMESIZE \
Wait, this would mean applications need to recompile to use the
smaller value. AFAIK that's an ABI break too, right? At the moment I
don't see a way to fix this without breaking the ABI
With this modification, if you don't recompile the application, its
behavior will still be the same as today -> it will return ENAMETOOLONG.
If you recompile it, the application will be aware of the maximum length.
To me, it seems to be a acceptable compromise for this release.
The patch you're proposing also changes the ABI of librte_ring and
librte_eal, which cannot be accepted for the release.
Post by Zoltan Kiss
Post by Olivier Matz
(RTE_MEMZONE_NAMESIZE - sizeof(pool_prefix) - sizeof(ring
prefix))
Post by Zoltan Kiss
Post by Olivier Matz
struct rte_mempool {
union {
char name[RTE_MEMPOOL_NAMESIZE];
char pad[32];
};
...
}
Anyway, it may not be the proper solution since it supposes that a
mempool includes a ring based on a memzone, which is not always true
now with mempool handlers.
Oh, as we dug deeper it gets better!
Indeed, we don't necessarily have this ring + memzone pair underneath,
but the user is not aware of that, and I think we should keep it that
way. It should only care that the string passed shouldn't be bigger
than a certain amount.
Yes. What I'm just saying here is that it's not a good solution to write
in the #define that "a mempool is based on a ring + a memzone", because if
some someone adds a new mempool handler replacing the ring, and also
creating a memzone prefixed by something larger than "rg_", we will have
to break the ABI again.
Post by Zoltan Kiss
Also, even though we don't necessarily have the ring, we still reserve
memzone's in rte_mempool_populate_default(). And their name has a 3
letter prefix, and a "_%d" postfix, where the %d could be as much as
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen("_2560")
As a side note, there is another bug around here: rte_ring_create()
doesn't check for name duplications. However the user of the library
can lookup based on the name with rte_ring_lookup(), and it will
return the first ring with that name
The name uniqueness is checked by rte_memzone_reserve().
Post by Zoltan Kiss
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I
didn't check it's feasible for memzones. What do you think?
It would work too, but I don't think it would help a lot. We would
still need max sizes for the names. Storing them somewhere else
won't help us in this problem.
Why should we have a maximum length for the names?
What happens if an application loads DPDK and create a mempool with a
name string 2 million characters long? Maybe nothing we should worry
about, but in general I think unlimited length function parameters are
problematic at the very least. The length should be passed at least
(which also creates a max due to the size of the param). But I think
it would be just easier to have these maximums set, observing the
above constrains.
I think have a maximum name length brings more problems than not having
it, especially ABI problems.
I disagree. I think we should have reasonable max names, and allow functions to return an error in case of a name being too long. However, what I think we also need to do is to guarantee a minimum maximum name length to allow apps to ensure they never hit that name-too-long error. We can guarantee that for ring/mempool etc, that the max allowed name will always be at least 20 characters, for example. That gives plenty of scope for adjusting the max as we need to, while giving others reasonable guarantees too.

/Bruce
Zoltan Kiss
2016-07-20 17:20:24 UTC
Permalink
Post by Olivier Matz
Hi,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
Setting all of them to 32 hides this restriction from the application.
This patch increases the memzone string size to accomodate for these
prefixes, and the same happens with the ring name string. The ABI needs to
be broken to fix this API issue, this way doesn't break applications
previously not failing due to the truncating bug now fixed.
I agree it is a problem for an application because it cannot know what
is the maximum name length. On the other hand, breaking the ABI for this
looks a bit overkill. Maybe we could reduce RTE_MEMPOOL_NAMESIZE and
RTE_RING_NAMESIZE instead of increasing RTE_MEMZONE_NAMESIZE? That way,
we could keep the ABI as is.
But that would break the ABI too, wouldn't it? Unless you keep the array
the same size (32 bytes) by using RTE_MEMZONE_NAMESIZE.
Yes, that was the idea.
Post by Zoltan Kiss
And even then, the API breaks anyway. There are applications - I have at
least some - which use all 32 bytes to store the name. Decrease that
would cause headache to change the naming scheme, because it's a 30
character long id, and chopping the last few chars would cause name
collisions and annoying bugs.
Before my patch (85cf0079), long names were silently truncated when
mempool created its ring and/or memzones. Now, it returns an error.
With 16.04 an application could operate as expected if the first 26
character were unique. Your patch revealed the problem that characters
after these were left out of the name. Now applications fail where this
never been a bug because their naming scheme guarantees the uniqueness
on the first 26 chars (or makes it very unlikely)
Where the first 26 is not unique, it failed earlier too, because at
memzone creation it checks for duplicate names.
Yes, I understand that there is a behavior change for applications using
names larger than 26 between 16.04 and 16.07. I also understand that
there is no way for an application to know what is the maximum usable
size for a mempool or a ring.
Post by Zoltan Kiss
Post by Olivier Matz
I'm not getting why changing the struct to something like below would
break the API, since it would already return an error today.
#define RTE_MEMPOOL_NAMESIZE \
Wait, this would mean applications need to recompile to use the smaller
value. AFAIK that's an ABI break too, right? At the moment I don't see a
way to fix this without breaking the ABI
With this modification, if you don't recompile the application, its
behavior will still be the same as today -> it will return ENAMETOOLONG.
If you recompile it, the application will be aware of the maximum
length. To me, it seems to be a acceptable compromise for this release.
The patch you're proposing also changes the ABI of librte_ring and
librte_eal, which cannot be accepted for the release.
Ok, I've sent a new version with this approach.
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
(RTE_MEMZONE_NAMESIZE - sizeof(pool_prefix) - sizeof(ring prefix))
struct rte_mempool {
union {
char name[RTE_MEMPOOL_NAMESIZE];
char pad[32];
};
...
}
Anyway, it may not be the proper solution since it supposes that a
mempool includes a ring based on a memzone, which is not always true now
with mempool handlers.
Oh, as we dug deeper it gets better!
Indeed, we don't necessarily have this ring + memzone pair underneath,
but the user is not aware of that, and I think we should keep it that
way. It should only care that the string passed shouldn't be bigger than
a certain amount.
Yes. What I'm just saying here is that it's not a good solution to write
in the #define that "a mempool is based on a ring + a memzone", because
if some someone adds a new mempool handler replacing the ring, and also
creating a memzone prefixed by something larger than "rg_", we will have
to break the ABI again.
If someone adds a new handler, (s)he needs to keep in mind what's the
max size for pool name, and any derived object using that name as a base
should check if it fits.
Post by Olivier Matz
Post by Zoltan Kiss
Also, even though we don't necessarily have the ring, we still reserve
memzone's in rte_mempool_populate_default(). And their name has a 3
letter prefix, and a "_%d" postfix, where the %d could be as much as
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen("_2560")
As a side note, there is another bug around here: rte_ring_create()
doesn't check for name duplications. However the user of the library can
lookup based on the name with rte_ring_lookup(), and it will return the
first ring with that name
The name uniqueness is checked by rte_memzone_reserve().
Post by Zoltan Kiss
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
It would even be better to get rid of this static char[] for the
structure names and replace it by an allocated const char *. I didn't
check it's feasible for memzones. What do you think?
It would work too, but I don't think it would help a lot. We would still
need max sizes for the names. Storing them somewhere else won't help us
in this problem.
Why should we have a maximum length for the names?
What happens if an application loads DPDK and create a mempool with a
name string 2 million characters long? Maybe nothing we should worry
about, but in general I think unlimited length function parameters are
problematic at the very least. The length should be passed at least
(which also creates a max due to the size of the param). But I think it
would be just easier to have these maximums set, observing the above
constrains.
I think have a maximum name length brings more problems than not having
it, especially ABI problems.
Regards,
Olivier
Olivier Matz
2016-07-19 15:26:53 UTC
Permalink
Hi Zoltan,

I ran ./scripts/check-git-log.sh on your patch, showing some minor
[PATCH] mempool: fix lack of free() registration
"()" should be removed
The new mempool handler interface forgets to register the free() function
449c49b9 mempool: support handler operations
The format should be:
Fixes: 449c49b93a6b ("mempool: support handler operations")
---
lib/librte_mempool/rte_mempool_ops.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index fd0b64c..5f24de2 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -81,6 +81,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
ops = &rte_mempool_ops_table.ops[ops_index];
snprintf(ops->name, sizeof(ops->name), "%s", h->name);
ops->alloc = h->alloc;
+ ops->free = h->free;
ops->enqueue = h->enqueue;
ops->dequeue = h->dequeue;
ops->get_count = h->get_count;
Apart from that:
Acked-by: Olivier Matz <***@6wind.com>

+CC Thomas, I think it should be included in 16.07.

Thanks!
Zoltan Kiss
2016-07-19 16:17:20 UTC
Permalink
Post by Olivier Matz
Hi Zoltan,
I ran ./scripts/check-git-log.sh on your patch, showing some minor
Thanks, do you want me to resend it, or could Thomas fix them upon
commiting?
Post by Olivier Matz
[PATCH] mempool: fix lack of free() registration
"()" should be removed
The new mempool handler interface forgets to register the free() function
449c49b9 mempool: support handler operations
Fixes: 449c49b93a6b ("mempool: support handler operations")
---
lib/librte_mempool/rte_mempool_ops.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index fd0b64c..5f24de2 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -81,6 +81,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
ops = &rte_mempool_ops_table.ops[ops_index];
snprintf(ops->name, sizeof(ops->name), "%s", h->name);
ops->alloc = h->alloc;
+ ops->free = h->free;
ops->enqueue = h->enqueue;
ops->dequeue = h->dequeue;
ops->get_count = h->get_count;
+CC Thomas, I think it should be included in 16.07.
Thanks!
Zoltan Kiss
2016-07-20 17:14:00 UTC
Permalink
The new mempool handler interface forgets to register the free() function
of the ops. Introduced in this patch:

Fixes: 449c49b93a6b ("mempool: support handler operations")

Signed-off-by: Zoltan Kiss <***@schaman.hu>
Acked-by: Olivier Matz <***@6wind.com>
---

Notes:
v2: fix commit message

lib/librte_mempool/rte_mempool_ops.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
index fd0b64c..5f24de2 100644
--- a/lib/librte_mempool/rte_mempool_ops.c
+++ b/lib/librte_mempool/rte_mempool_ops.c
@@ -81,6 +81,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
ops = &rte_mempool_ops_table.ops[ops_index];
snprintf(ops->name, sizeof(ops->name), "%s", h->name);
ops->alloc = h->alloc;
+ ops->free = h->free;
ops->enqueue = h->enqueue;
ops->dequeue = h->dequeue;
ops->get_count = h->get_count;
--
1.9.1
Thomas Monjalon
2016-07-21 21:04:26 UTC
Permalink
Post by Zoltan Kiss
The new mempool handler interface forgets to register the free() function
Fixes: 449c49b93a6b ("mempool: support handler operations")
Applied, thanks
Zoltan Kiss
2016-07-20 17:16:38 UTC
Permalink
A recent patch brought up an issue about the size of the 'name' fields:

85cf0079 mem: avoid memzone/mempool/ring name truncation

These relations should be observed:

1. Each ring creates a memzone with a prefixed name:
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)

2. There are some mempool handlers which create a ring with a prefixed
name:
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)

3. A mempool can create up to RTE_MAX_MEMZONE pre and postfixed memzones:
sprintf(postfix, "_%d", RTE_MAX_MEMZONE)
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen(postfix)

Setting all of them to 32 hides this restriction from the application.
This patch decreases the mempool and ring string size to accommodate for
these prefixes, but it doesn't apply the 3rd constraint. Applications
relying on these constants need to be recompiled, otherwise they'll run
into ENAMETOOLONG issues.
The size of the arrays are kept 32 for ABI compatibility, it can be
decreased next time the ABI changes.

Signed-off-by: Zoltan Kiss <***@schaman.hu>
---

Notes:
v2: keep arrays 32 bytes and decrease the max sizes to maintain ABI
compatibility

lib/librte_mempool/rte_mempool.h | 11 +++++++++--
lib/librte_ring/rte_ring.h | 12 ++++++++++--
2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
index 4a8fbb1..059ad9e 100644
--- a/lib/librte_mempool/rte_mempool.h
+++ b/lib/librte_mempool/rte_mempool.h
@@ -123,7 +123,9 @@ struct rte_mempool_objsz {
/**< Total size of an object (header + elt + trailer). */
};

-#define RTE_MEMPOOL_NAMESIZE 32 /**< Maximum length of a memory pool. */
+/**< Maximum length of a memory pool's name. */
+#define RTE_MEMPOOL_NAMESIZE (RTE_RING_NAMESIZE - \
+ sizeof(RTE_MEMPOOL_MZ_PREFIX) + 1)
#define RTE_MEMPOOL_MZ_PREFIX "MP_"

/* "MP_<name>" */
@@ -208,7 +210,12 @@ struct rte_mempool_memhdr {
* The RTE mempool structure.
*/
struct rte_mempool {
- char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
+ /*
+ * Note: this field kept the RTE_MEMZONE_NAMESIZE size due to ABI
+ * compatibility requirements, it could be changed to
+ * RTE_MEMPOOL_NAMESIZE next time the ABI changes
+ */
+ char name[RTE_MEMZONE_NAMESIZE]; /**< Name of mempool. */
union {
void *pool_data; /**< Ring or pool to store objects. */
uint64_t pool_id; /**< External mempool identifier. */
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index eb45e41..0e22e69 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -100,6 +100,7 @@ extern "C" {
#include <rte_lcore.h>
#include <rte_atomic.h>
#include <rte_branch_prediction.h>
+#include <rte_memzone.h>

#define RTE_TAILQ_RING_NAME "RTE_RING"

@@ -126,8 +127,10 @@ struct rte_ring_debug_stats {
} __rte_cache_aligned;
#endif

-#define RTE_RING_NAMESIZE 32 /**< The maximum length of a ring name. */
#define RTE_RING_MZ_PREFIX "RG_"
+/**< The maximum length of a ring name. */
+#define RTE_RING_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
+ sizeof(RTE_RING_MZ_PREFIX) + 1)

#ifndef RTE_RING_PAUSE_REP_COUNT
#define RTE_RING_PAUSE_REP_COUNT 0 /**< Yield after pause num of times, no yield
@@ -147,7 +150,12 @@ struct rte_memzone; /* forward declaration, so as not to require memzone.h */
* a problem.
*/
struct rte_ring {
- char name[RTE_RING_NAMESIZE]; /**< Name of the ring. */
+ /*
+ * Note: this field kept the RTE_MEMZONE_NAMESIZE size due to ABI
+ * compatibility requirements, it could be changed to RTE_RING_NAMESIZE
+ * next time the ABI changes
+ */
+ char name[RTE_MEMZONE_NAMESIZE]; /**< Name of the ring. */
int flags; /**< Flags supplied at creation. */
const struct rte_memzone *memzone;
/**< Memzone, if any, containing the rte_ring */
--
1.9.1
Olivier Matz
2016-07-21 13:40:44 UTC
Permalink
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
2. There are some mempool handlers which create a ring with a prefixed
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)
sprintf(postfix, "_%d", RTE_MAX_MEMZONE)
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen(postfix)
Setting all of them to 32 hides this restriction from the application.
This patch decreases the mempool and ring string size to accommodate for
these prefixes, but it doesn't apply the 3rd constraint. Applications
relying on these constants need to be recompiled, otherwise they'll run
into ENAMETOOLONG issues.
The size of the arrays are kept 32 for ABI compatibility, it can be
decreased next time the ABI changes.
Looks like to be a good compromise for the 16.07 release. One question
however: why not taking in account the 3rd constraint? Because it may
not completly fix the issue if the mempool is fragmented.

We could define RTE_MEMPOOL_NAMESIZE to 24
= 32 - len('mp_') - len('_0123'))

Thanks,
Olivier
Zoltan Kiss
2016-07-21 13:47:48 UTC
Permalink
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
2. There are some mempool handlers which create a ring with a prefixed
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE - strlen(RTE_MEMPOOL_MZ_PREFIX)
sprintf(postfix, "_%d", RTE_MAX_MEMZONE)
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen(postfix)
Setting all of them to 32 hides this restriction from the application.
This patch decreases the mempool and ring string size to accommodate for
these prefixes, but it doesn't apply the 3rd constraint. Applications
relying on these constants need to be recompiled, otherwise they'll run
into ENAMETOOLONG issues.
The size of the arrays are kept 32 for ABI compatibility, it can be
decreased next time the ABI changes.
Looks like to be a good compromise for the 16.07 release. One question
however: why not taking in account the 3rd constraint? Because it may
not completly fix the issue if the mempool is fragmented.
We could define RTE_MEMPOOL_NAMESIZE to 24
= 32 - len('mp_') - len('_0123'))
I was trying to figure out a compile time macro for strlen(postfix), but
I could not. Your suggestion would work only until someone increases
RTE_MAX_MEMZONE above 9999. As the likelihood of fragmenting a pool over
99 memzones seems small, I did not bother to fix this with an ugly hack,
but if you think we should include it, let me know!
Post by Olivier Matz
Thanks,
Olivier
Olivier Matz
2016-07-21 14:25:41 UTC
Permalink
Post by Zoltan Kiss
Post by Olivier Matz
Hi Zoltan,
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
2. There are some mempool handlers which create a ring with a prefixed
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
sprintf(postfix, "_%d", RTE_MAX_MEMZONE)
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen(postfix)
Setting all of them to 32 hides this restriction from the application.
This patch decreases the mempool and ring string size to accommodate for
these prefixes, but it doesn't apply the 3rd constraint. Applications
relying on these constants need to be recompiled, otherwise they'll run
into ENAMETOOLONG issues.
The size of the arrays are kept 32 for ABI compatibility, it can be
decreased next time the ABI changes.
Looks like to be a good compromise for the 16.07 release. One question
however: why not taking in account the 3rd constraint? Because it may
not completly fix the issue if the mempool is fragmented.
We could define RTE_MEMPOOL_NAMESIZE to 24
= 32 - len('mp_') - len('_0123'))
I was trying to figure out a compile time macro for strlen(postfix), but
I could not. Your suggestion would work only until someone increases
RTE_MAX_MEMZONE above 9999. As the likelihood of fragmenting a pool over
99 memzones seems small, I did not bother to fix this with an ugly hack,
but if you think we should include it, let me know!
Ok, looks fair, thanks for the clarification.

Acked-by: Olivier Matz <***@6wind.com>
Thomas Monjalon
2016-07-21 21:16:21 UTC
Permalink
Post by Olivier Matz
Post by Zoltan Kiss
Post by Olivier Matz
Post by Zoltan Kiss
85cf0079 mem: avoid memzone/mempool/ring name truncation
RTE_RING_NAMESIZE <= RTE_MEMZONE_NAMESIZE - strlen(RTE_RING_MZ_PREFIX)
2. There are some mempool handlers which create a ring with a prefixed
RTE_MEMPOOL_NAMESIZE <= RTE_RING_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX)
sprintf(postfix, "_%d", RTE_MAX_MEMZONE)
RTE_MEMPOOL_NAMESIZE <= RTE_MEMZONE_NAMESIZE -
strlen(RTE_MEMPOOL_MZ_PREFIX) - strlen(postfix)
Setting all of them to 32 hides this restriction from the application.
This patch decreases the mempool and ring string size to accommodate for
these prefixes, but it doesn't apply the 3rd constraint. Applications
relying on these constants need to be recompiled, otherwise they'll run
into ENAMETOOLONG issues.
The size of the arrays are kept 32 for ABI compatibility, it can be
decreased next time the ABI changes.
Looks like to be a good compromise for the 16.07 release. One question
however: why not taking in account the 3rd constraint? Because it may
not completly fix the issue if the mempool is fragmented.
We could define RTE_MEMPOOL_NAMESIZE to 24
= 32 - len('mp_') - len('_0123'))
I was trying to figure out a compile time macro for strlen(postfix), but
I could not. Your suggestion would work only until someone increases
RTE_MAX_MEMZONE above 9999. As the likelihood of fragmenting a pool over
99 memzones seems small, I did not bother to fix this with an ugly hack,
but if you think we should include it, let me know!
Ok, looks fair, thanks for the clarification.
Applied, thanks

Continue reading on narkive:
Loading...