[v2,04/15] node: switch IPv4 metadata to dynamic mbuf field
Checks
Commit Message
The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
It is moved to a dynamic field in order to allow removal of udata64.
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
lib/librte_node/ip4_lookup.c | 7 +++++++
lib/librte_node/ip4_rewrite.c | 10 ++++++++++
lib/librte_node/node_private.h | 12 ++++++++++--
3 files changed, 27 insertions(+), 2 deletions(-)
Comments
Hi Thomas,
On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> It is moved to a dynamic field in order to allow removal of udata64.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> lib/librte_node/ip4_lookup.c | 7 +++++++
> lib/librte_node/ip4_rewrite.c | 10 ++++++++++
> lib/librte_node/node_private.h | 12 ++++++++++--
> 3 files changed, 27 insertions(+), 2 deletions(-)
<...>
> --- a/lib/librte_node/node_private.h
> +++ b/lib/librte_node/node_private.h
> @@ -8,6 +8,7 @@
> #include <rte_common.h>
> #include <rte_log.h>
> #include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>
> extern int rte_node_logtype;
> #define NODE_LOG(level, node_name, ...) \
> @@ -21,7 +22,6 @@ extern int rte_node_logtype;
> #define node_dbg(node_name, ...) NODE_LOG(DEBUG, node_name, __VA_ARGS__)
>
> /**
> - *
> * Node mbuf private data to store next hop, ttl and checksum.
> */
> struct node_mbuf_priv1 {
> @@ -37,6 +37,13 @@ struct node_mbuf_priv1 {
> };
> };
>
> +static const struct rte_mbuf_dynfield node_mbuf_priv1_dynfield_desc = {
> + .name = "rte_node_dynfield_priv1",
> + .size = sizeof(struct node_mbuf_priv1 *),
> + .align = __alignof__(struct node_mbuf_priv1 *),
> +};
> +extern int node_mbuf_priv1_dynfield_offset;
> +
It should be "struct node_mbuf_priv1", not "struct node_mbuf_priv1 *"
27/10/2020 10:32, Olivier Matz:
> On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> > +static const struct rte_mbuf_dynfield node_mbuf_priv1_dynfield_desc = {
> > + .name = "rte_node_dynfield_priv1",
> > + .size = sizeof(struct node_mbuf_priv1 *),
> > + .align = __alignof__(struct node_mbuf_priv1 *),
> > +};
> > +extern int node_mbuf_priv1_dynfield_offset;
> > +
>
> It should be "struct node_mbuf_priv1", not "struct node_mbuf_priv1 *"
Yes, will fix in v3.
On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> It is moved to a dynamic field in order to allow removal of udata64.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> lib/librte_node/ip4_lookup.c | 7 +++++++
> lib/librte_node/ip4_rewrite.c | 10 ++++++++++
> lib/librte_node/node_private.h | 12 ++++++++++--
> 3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_node/ip4_lookup.c b/lib/librte_node/ip4_lookup.c
> index 8835aab9dd..c2f6d653f9 100644
> --- a/lib/librte_node/ip4_lookup.c
> +++ b/lib/librte_node/ip4_lookup.c
> @@ -21,6 +21,8 @@
>
> #include "node_private.h"
>
> +int node_mbuf_priv1_dynfield_offset;
> +
This change doesn't work in secondary as it is process local memory.
> #define IPV4_L3FWD_LPM_MAX_RULES 1024
> #define IPV4_L3FWD_LPM_NUMBER_TBL8S (1 << 8)
>
> @@ -178,6 +180,11 @@ ip4_lookup_node_init(const struct rte_graph *graph, struct rte_node *node)
> RTE_SET_USED(node);
>
> if (!init_once) {
> + node_mbuf_priv1_dynfield_offset = rte_mbuf_dynfield_register(
> + &node_mbuf_priv1_dynfield_desc);
> + if (node_mbuf_priv1_dynfield_offset < 0)
> + return -rte_errno;
> +
> /* Setup LPM tables for all sockets */
> RTE_LCORE_FOREACH(lcore_id)
> {
> diff --git a/lib/librte_node/ip4_rewrite.c b/lib/librte_node/ip4_rewrite.c
> index bb7f671b5c..1c4e51968c 100644
> --- a/lib/librte_node/ip4_rewrite.c
> +++ b/lib/librte_node/ip4_rewrite.c
> @@ -248,9 +248,19 @@ ip4_rewrite_node_process(struct rte_graph *graph, struct rte_node *node,
> static int
> ip4_rewrite_node_init(const struct rte_graph *graph, struct rte_node *node)
> {
> + static bool init_once;
>
> RTE_SET_USED(graph);
> RTE_SET_USED(node);
> +
> + if (!init_once) {
> + node_mbuf_priv1_dynfield_offset = rte_mbuf_dynfield_register(
> + &node_mbuf_priv1_dynfield_desc);
> + if (node_mbuf_priv1_dynfield_offset < 0)
> + return -rte_errno;
> + init_once = true;
> + }
> +
> node_dbg("ip4_rewrite", "Initialized ip4_rewrite node initialized");
>
> return 0;
> diff --git a/lib/librte_node/node_private.h b/lib/librte_node/node_private.h
> index ab7941c12b..359b40faed 100644
> --- a/lib/librte_node/node_private.h
> +++ b/lib/librte_node/node_private.h
> @@ -8,6 +8,7 @@
> #include <rte_common.h>
> #include <rte_log.h>
> #include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>
> extern int rte_node_logtype;
> #define NODE_LOG(level, node_name, ...) \
> @@ -21,7 +22,6 @@ extern int rte_node_logtype;
> #define node_dbg(node_name, ...) NODE_LOG(DEBUG, node_name, __VA_ARGS__)
>
> /**
> - *
> * Node mbuf private data to store next hop, ttl and checksum.
> */
> struct node_mbuf_priv1 {
> @@ -37,6 +37,13 @@ struct node_mbuf_priv1 {
> };
> };
>
> +static const struct rte_mbuf_dynfield node_mbuf_priv1_dynfield_desc = {
> + .name = "rte_node_dynfield_priv1",
> + .size = sizeof(struct node_mbuf_priv1 *),
> + .align = __alignof__(struct node_mbuf_priv1 *),
> +};
> +extern int node_mbuf_priv1_dynfield_offset;
> +
> /**
> * Node mbuf private area 2.
> */
> @@ -60,7 +67,8 @@ struct node_mbuf_priv2 {
> static __rte_always_inline struct node_mbuf_priv1 *
> node_mbuf_priv1(struct rte_mbuf *m)
> {
> - return (struct node_mbuf_priv1 *)&m->udata64;
> + return RTE_MBUF_DYNFIELD(m,
> + node_mbuf_priv1_dynfield_offset, struct node_mbuf_priv1 *);
There is a performance regression of ~1.4% in our platform (Octeontx2) because
of this change.
> }
>
> /**
> --
> 2.28.0
>
27/10/2020 15:23, Nithin Dabilpuram:
> On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> > The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> > It is moved to a dynamic field in order to allow removal of udata64.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> > --- a/lib/librte_node/ip4_lookup.c
> > +++ b/lib/librte_node/ip4_lookup.c
> > +int node_mbuf_priv1_dynfield_offset;
> > +
>
> This change doesn't work in secondary as it is process local memory.
Yes that's an issue.
Can we copy the value when starting a secondary process?
[...]
> > static __rte_always_inline struct node_mbuf_priv1 *
> > node_mbuf_priv1(struct rte_mbuf *m)
> > {
> > - return (struct node_mbuf_priv1 *)&m->udata64;
> > + return RTE_MBUF_DYNFIELD(m,
> > + node_mbuf_priv1_dynfield_offset, struct node_mbuf_priv1 *);
>
> There is a performance regression of ~1.4% in our platform (Octeontx2) because
> of this change.
Yes that's the price to pay for a more extensible DPDK.
It is much cheaper than not having room for new features,
or having features conflicting on the same mbuf field like here.
On Tue, Oct 27, 2020 at 03:33:35PM +0100, Thomas Monjalon wrote:
> 27/10/2020 15:23, Nithin Dabilpuram:
> > On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> > > The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> > > It is moved to a dynamic field in order to allow removal of udata64.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> > > --- a/lib/librte_node/ip4_lookup.c
> > > +++ b/lib/librte_node/ip4_lookup.c
> > > +int node_mbuf_priv1_dynfield_offset;
> > > +
> >
> > This change doesn't work in secondary as it is process local memory.
>
> Yes that's an issue.
> Can we copy the value when starting a secondary process?
Currently there is no call back which will be called only in secondary.
Can you move this value to node->ctx 8'th byte offset ? Node context is of size
16 bytes and should be sufficient.
Currently first 8 B of node->ctx is used to store that socket's lpm pointer.
>
> [...]
> > > static __rte_always_inline struct node_mbuf_priv1 *
> > > node_mbuf_priv1(struct rte_mbuf *m)
> > > {
> > > - return (struct node_mbuf_priv1 *)&m->udata64;
> > > + return RTE_MBUF_DYNFIELD(m,
> > > + node_mbuf_priv1_dynfield_offset, struct node_mbuf_priv1 *);
> >
> > There is a performance regression of ~1.4% in our platform (Octeontx2) because
> > of this change.
>
> Yes that's the price to pay for a more extensible DPDK.
> It is much cheaper than not having room for new features,
> or having features conflicting on the same mbuf field like here.
>
>
27/10/2020 16:33, Nithin Dabilpuram:
> On Tue, Oct 27, 2020 at 03:33:35PM +0100, Thomas Monjalon wrote:
> > 27/10/2020 15:23, Nithin Dabilpuram:
> > > On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> > > > The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> > > > It is moved to a dynamic field in order to allow removal of udata64.
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > ---
> > > > --- a/lib/librte_node/ip4_lookup.c
> > > > +++ b/lib/librte_node/ip4_lookup.c
> > > > +int node_mbuf_priv1_dynfield_offset;
> > > > +
> > >
> > > This change doesn't work in secondary as it is process local memory.
> >
> > Yes that's an issue.
> > Can we copy the value when starting a secondary process?
>
> Currently there is no call back which will be called only in secondary.
>
> Can you move this value to node->ctx 8'th byte offset ? Node context is of size
> 16 bytes and should be sufficient.
> Currently first 8 B of node->ctx is used to store that socket's lpm pointer.
Please would you be able to do such patch?
On Tue, Oct 27, 2020 at 04:57:16PM +0100, Thomas Monjalon wrote:
> 27/10/2020 16:33, Nithin Dabilpuram:
> > On Tue, Oct 27, 2020 at 03:33:35PM +0100, Thomas Monjalon wrote:
> > > 27/10/2020 15:23, Nithin Dabilpuram:
> > > > On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> > > > > The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> > > > > It is moved to a dynamic field in order to allow removal of udata64.
> > > > >
> > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > ---
> > > > > --- a/lib/librte_node/ip4_lookup.c
> > > > > +++ b/lib/librte_node/ip4_lookup.c
> > > > > +int node_mbuf_priv1_dynfield_offset;
> > > > > +
> > > >
> > > > This change doesn't work in secondary as it is process local memory.
> > >
> > > Yes that's an issue.
> > > Can we copy the value when starting a secondary process?
> >
> > Currently there is no call back which will be called only in secondary.
> >
> > Can you move this value to node->ctx 8'th byte offset ? Node context is of size
> > 16 bytes and should be sufficient.
> > Currently first 8 B of node->ctx is used to store that socket's lpm pointer.
>
> Please would you be able to do such patch?
Ack, will send a patch on this thread so that you can include in next version.
>
>
>
27/10/2020 17:16, Nithin Dabilpuram:
> On Tue, Oct 27, 2020 at 04:57:16PM +0100, Thomas Monjalon wrote:
> > 27/10/2020 16:33, Nithin Dabilpuram:
> > > On Tue, Oct 27, 2020 at 03:33:35PM +0100, Thomas Monjalon wrote:
> > > > 27/10/2020 15:23, Nithin Dabilpuram:
> > > > > On Mon, Oct 26, 2020 at 11:20:02PM +0100, Thomas Monjalon wrote:
> > > > > > The node_mbuf_priv1 was stored in the deprecated mbuf field udata64.
> > > > > > It is moved to a dynamic field in order to allow removal of udata64.
> > > > > >
> > > > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > > > ---
> > > > > > --- a/lib/librte_node/ip4_lookup.c
> > > > > > +++ b/lib/librte_node/ip4_lookup.c
> > > > > > +int node_mbuf_priv1_dynfield_offset;
> > > > > > +
> > > > >
> > > > > This change doesn't work in secondary as it is process local memory.
> > > >
> > > > Yes that's an issue.
> > > > Can we copy the value when starting a secondary process?
> > >
> > > Currently there is no call back which will be called only in secondary.
> > >
> > > Can you move this value to node->ctx 8'th byte offset ? Node context is of size
> > > 16 bytes and should be sufficient.
> > > Currently first 8 B of node->ctx is used to store that socket's lpm pointer.
> >
> > Please would you be able to do such patch?
>
> Ack, will send a patch on this thread so that you can include in next version.
Thanks
@@ -21,6 +21,8 @@
#include "node_private.h"
+int node_mbuf_priv1_dynfield_offset;
+
#define IPV4_L3FWD_LPM_MAX_RULES 1024
#define IPV4_L3FWD_LPM_NUMBER_TBL8S (1 << 8)
@@ -178,6 +180,11 @@ ip4_lookup_node_init(const struct rte_graph *graph, struct rte_node *node)
RTE_SET_USED(node);
if (!init_once) {
+ node_mbuf_priv1_dynfield_offset = rte_mbuf_dynfield_register(
+ &node_mbuf_priv1_dynfield_desc);
+ if (node_mbuf_priv1_dynfield_offset < 0)
+ return -rte_errno;
+
/* Setup LPM tables for all sockets */
RTE_LCORE_FOREACH(lcore_id)
{
@@ -248,9 +248,19 @@ ip4_rewrite_node_process(struct rte_graph *graph, struct rte_node *node,
static int
ip4_rewrite_node_init(const struct rte_graph *graph, struct rte_node *node)
{
+ static bool init_once;
RTE_SET_USED(graph);
RTE_SET_USED(node);
+
+ if (!init_once) {
+ node_mbuf_priv1_dynfield_offset = rte_mbuf_dynfield_register(
+ &node_mbuf_priv1_dynfield_desc);
+ if (node_mbuf_priv1_dynfield_offset < 0)
+ return -rte_errno;
+ init_once = true;
+ }
+
node_dbg("ip4_rewrite", "Initialized ip4_rewrite node initialized");
return 0;
@@ -8,6 +8,7 @@
#include <rte_common.h>
#include <rte_log.h>
#include <rte_mbuf.h>
+#include <rte_mbuf_dyn.h>
extern int rte_node_logtype;
#define NODE_LOG(level, node_name, ...) \
@@ -21,7 +22,6 @@ extern int rte_node_logtype;
#define node_dbg(node_name, ...) NODE_LOG(DEBUG, node_name, __VA_ARGS__)
/**
- *
* Node mbuf private data to store next hop, ttl and checksum.
*/
struct node_mbuf_priv1 {
@@ -37,6 +37,13 @@ struct node_mbuf_priv1 {
};
};
+static const struct rte_mbuf_dynfield node_mbuf_priv1_dynfield_desc = {
+ .name = "rte_node_dynfield_priv1",
+ .size = sizeof(struct node_mbuf_priv1 *),
+ .align = __alignof__(struct node_mbuf_priv1 *),
+};
+extern int node_mbuf_priv1_dynfield_offset;
+
/**
* Node mbuf private area 2.
*/
@@ -60,7 +67,8 @@ struct node_mbuf_priv2 {
static __rte_always_inline struct node_mbuf_priv1 *
node_mbuf_priv1(struct rte_mbuf *m)
{
- return (struct node_mbuf_priv1 *)&m->udata64;
+ return RTE_MBUF_DYNFIELD(m,
+ node_mbuf_priv1_dynfield_offset, struct node_mbuf_priv1 *);
}
/**