[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
Hi David, On 03/09/2016 05:28 PM, Hunt, David wrote: >> Sorry, maybe I wasn't very clear in my previous messages. For me, the >> NEXT_ABI is not the proper solution because, as Panu stated, it makes >> the patch hard to read. My understanding of NEXT_ABI is that it should >> only be used if the changes are small enough. Duplicating the code with >> a big #ifdef NEXT_ABI is not an option to me either. >> >> So that's why the deprecation notice should be used instead. But in this >> case, it means that this patch won't be present in 16.04, but will be >> added in 16.07. >> > Sure, v4 will remove the NEXT_ABI patch , and replace it with just the > ABI break announcement for 16.07. For anyone who what's to try out the > patch, they can always get it from patchwork, but not as part 16.04. I think it's better to have the deprecation notice in a separate mail, outside of the patch series, so Thomas can just apply this one and let the series pending for 16.07. Thanks, Olivier
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
Hi Olivier, On 3/9/2016 4:31 PM, Olivier MATZ wrote: > Hi David, > > On 03/09/2016 05:28 PM, Hunt, David wrote: > >> Sure, v4 will remove the NEXT_ABI patch , and replace it with just the >> ABI break announcement for 16.07. For anyone who what's to try out the >> patch, they can always get it from patchwork, but not as part 16.04. > I think it's better to have the deprecation notice in a separate > mail, outside of the patch series, so Thomas can just apply this > one and let the series pending for 16.07. > > Thanks, > Olivier Yes, sure, makes perfect sense. Thanks, David.
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
Hi David, On 03/09/2016 12:30 PM, Hunt, David wrote: > Hi Panu, > > On 3/9/2016 10:46 AM, Panu Matilainen wrote: >> On 03/09/2016 11:50 AM, David Hunt wrote: >>> This patch is for those people who want to be easily able to switch >>> between the new mempool layout and the old. Change the value of >>> RTE_NEXT_ABI in common_base config file >> >> I guess the idea here is to document how to switch between the ABIs >> but to me this reads as if this patch is supposed to change the value >> in common_base. Of course there's no such change included (nor should >> there be) here, but the description could use some fine-tuning perhaps. >> > > You're right, I'll clarify the comments. v4 due soon. > >>> >>> v3: Updated to take re-work of file layouts into consideration >>> >>> v2: Kept all the NEXT_ABI defs to this patch so as to make the >>> previous patches easier to read, and also to imake it clear what >>> code is necessary to keep ABI compatibility when NEXT_ABI is >>> disabled. >> >> Maybe its just me, but: >> I can see why NEXT_ABI is in a separate patch for review purposes but >> for final commit this split doesn't seem right to me. In any case its >> quite a large change for NEXT_ABI. >> > > The patch basically re-introduces the old (pre-mempool) code as the > refactoring of the code would have made the NEXT_ABI additions totally > unreadable. I think this way is the lesser of two evils. > >> In any case, you should add a deprecation notice for the oncoming ABI >> break in 16.07. >> > > Sure, I'll add that in v4. Sorry, maybe I wasn't very clear in my previous messages. For me, the NEXT_ABI is not the proper solution because, as Panu stated, it makes the patch hard to read. My understanding of NEXT_ABI is that it should only be used if the changes are small enough. Duplicating the code with a big #ifdef NEXT_ABI is not an option to me either. So that's why the deprecation notice should be used instead. But in this case, it means that this patch won't be present in 16.04, but will be added in 16.07. Regards, Olivier
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
On 03/09/2016 11:50 AM, David Hunt wrote: > This patch is for those people who want to be easily able to switch > between the new mempool layout and the old. Change the value of > RTE_NEXT_ABI in common_base config file I guess the idea here is to document how to switch between the ABIs but to me this reads as if this patch is supposed to change the value in common_base. Of course there's no such change included (nor should there be) here, but the description could use some fine-tuning perhaps. > > v3: Updated to take re-work of file layouts into consideration > > v2: Kept all the NEXT_ABI defs to this patch so as to make the > previous patches easier to read, and also to imake it clear what > code is necessary to keep ABI compatibility when NEXT_ABI is > disabled. Maybe its just me, but: I can see why NEXT_ABI is in a separate patch for review purposes but for final commit this split doesn't seem right to me. In any case its quite a large change for NEXT_ABI. In any case, you should add a deprecation notice for the oncoming ABI break in 16.07. - Panu -
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
Hi Panu, On 3/9/2016 10:46 AM, Panu Matilainen wrote: > On 03/09/2016 11:50 AM, David Hunt wrote: >> This patch is for those people who want to be easily able to switch >> between the new mempool layout and the old. Change the value of >> RTE_NEXT_ABI in common_base config file > > I guess the idea here is to document how to switch between the ABIs > but to me this reads as if this patch is supposed to change the value > in common_base. Of course there's no such change included (nor should > there be) here, but the description could use some fine-tuning perhaps. > You're right, I'll clarify the comments. v4 due soon. >> >> v3: Updated to take re-work of file layouts into consideration >> >> v2: Kept all the NEXT_ABI defs to this patch so as to make the >> previous patches easier to read, and also to imake it clear what >> code is necessary to keep ABI compatibility when NEXT_ABI is >> disabled. > > Maybe its just me, but: > I can see why NEXT_ABI is in a separate patch for review purposes but > for final commit this split doesn't seem right to me. In any case its > quite a large change for NEXT_ABI. > The patch basically re-introduces the old (pre-mempool) code as the refactoring of the code would have made the NEXT_ABI additions totally unreadable. I think this way is the lesser of two evils. > In any case, you should add a deprecation notice for the oncoming ABI > break in 16.07. > Sure, I'll add that in v4. > - Panu - > Thanks for the comments, Regards, David.
[dpdk-dev] [PATCH v3 4/4] mempool: add in the RTE_NEXT_ABI for ABI breakages
This patch is for those people who want to be easily able to switch
between the new mempool layout and the old. Change the value of
RTE_NEXT_ABI in common_base config file
v3: Updated to take re-work of file layouts into consideration
v2: Kept all the NEXT_ABI defs to this patch so as to make the
previous patches easier to read, and also to imake it clear what
code is necessary to keep ABI compatibility when NEXT_ABI is
disabled.
Signed-off-by: David Hunt
---
app/test/Makefile| 2 +
app/test/test_mempool_perf.c | 3 +
lib/librte_mbuf/rte_mbuf.c | 7 ++
lib/librte_mempool/Makefile | 2 +
lib/librte_mempool/rte_mempool.c | 245 ++-
lib/librte_mempool/rte_mempool.h | 59 +-
6 files changed, 315 insertions(+), 3 deletions(-)
diff --git a/app/test/Makefile b/app/test/Makefile
index 9a2f75f..8fcf0c2 100644
--- a/app/test/Makefile
+++ b/app/test/Makefile
@@ -74,7 +74,9 @@ SRCS-$(CONFIG_RTE_LIBRTE_TIMER) += test_timer_perf.c
SRCS-$(CONFIG_RTE_LIBRTE_TIMER) += test_timer_racecond.c
SRCS-y += test_mempool.c
+ifeq ($(CONFIG_RTE_NEXT_ABI),y)
SRCS-y += test_ext_mempool.c
+endif
SRCS-y += test_mempool_perf.c
SRCS-y += test_mbuf.c
diff --git a/app/test/test_mempool_perf.c b/app/test/test_mempool_perf.c
index 091c1df..ca69e49 100644
--- a/app/test/test_mempool_perf.c
+++ b/app/test/test_mempool_perf.c
@@ -161,6 +161,9 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg)
n_get_bulk);
if (unlikely(ret < 0)) {
rte_mempool_dump(stdout, mp);
+#ifndef RTE_NEXT_ABI
+ rte_ring_dump(stdout, mp->ring);
+#endif
/* in this case, objects are lost... */
return -1;
}
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 42b0cd1..967d987 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -167,6 +167,7 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
mbp_priv.mbuf_data_room_size = data_room_size;
mbp_priv.mbuf_priv_size = priv_size;
+#ifdef RTE_NEXT_ABI
#ifdef RTE_MEMPOOL_HANDLER_EXT
return rte_mempool_create_ext(name, n, elt_size,
cache_size, sizeof(struct rte_pktmbuf_pool_private),
@@ -179,6 +180,12 @@ rte_pktmbuf_pool_create(const char *name, unsigned n,
rte_pktmbuf_pool_init, _priv, rte_pktmbuf_init, NULL,
socket_id, 0);
#endif
+#else
+ return rte_mempool_create(name, n, elt_size,
+ cache_size, sizeof(struct rte_pktmbuf_pool_private),
+ rte_pktmbuf_pool_init, _priv, rte_pktmbuf_init, NULL,
+ socket_id, 0);
+#endif
}
/* do some sanity checks on a mbuf: panic if it fails */
diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
index a32c89e..a27eef9 100644
--- a/lib/librte_mempool/Makefile
+++ b/lib/librte_mempool/Makefile
@@ -42,8 +42,10 @@ LIBABIVER := 1
# all source are stored in SRCS-y
SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool.c
+ifeq ($(CONFIG_RTE_NEXT_ABI),y)
SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_handler.c
SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_mempool_default.c
+endif
ifeq ($(CONFIG_RTE_LIBRTE_XEN_DOM0),y)
SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += rte_dom0_mempool.c
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index 7342a7f..e77ef47 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -59,7 +59,10 @@
#include
#include "rte_mempool.h"
+#ifdef RTE_NEXT_ABI
#include "rte_mempool_handler.h"
+#endif
+
TAILQ_HEAD(rte_mempool_list, rte_tailq_entry);
@@ -150,7 +153,11 @@ mempool_add_elem(struct rte_mempool *mp, void *obj,
uint32_t obj_idx,
obj_init(mp, obj_init_arg, obj, obj_idx);
/* enqueue in ring */
+#ifdef RTE_NEXT_ABI
rte_mempool_ext_put_bulk(mp, , 1);
+#else
+ rte_ring_mp_enqueue_bulk(mp->ring, , 1);
+#endif
}
uint32_t
@@ -420,6 +427,7 @@ rte_mempool_create(const char *name, unsigned n, unsigned
elt_size,
MEMPOOL_PG_SHIFT_MAX);
}
+#ifdef RTE_NEXT_ABI
/*
* Common mempool create function.
* Create the mempool over already allocated chunk of memory.
@@ -711,6 +719,229 @@ rte_mempool_xmem_create(const char *name, unsigned n,
unsigned elt_size,
return mp;
}
+#else
+/*
+ * Create the mempool over already allocated chunk of memory.
+ * That external memory buffer can consists of physically disjoint pages.
+ * Setting vaddr to NULL, makes mempool to fallback to original behaviour
+ * and allocate space for mempool and it's elements as one big chunk of
+ * physically continuos memory.
+ * */
+struct rte_mempool *
+rte_mempool_xmem_create(const char *name,
