[dpdk-dev] [PATCH] Fix loss of data stored in udata64 mbuf field

[dpdk-dev] [PATCH] Fix loss of data stored in udata64 mbuf field

[Michal Krawczyk]

DPDK v20.11 removed udata64 field, and changed type of the dynfield1 to
uint32_t.

Due to define:
  lib/common/pg_compat.h:#define udata64                 dynfield1[0]
the copy of udata64 was in fact copying only the first 32 bits.

Using udata64 as an address is ok, as it will point to the beginning of
the dynfield1 array.

Signed-off-by: Michal Krawczyk <mk@semihalf.com>
Reviewed-by: Igor Chauskin <igorch@amazon.com>
---
 lib/common/mbuf.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/common/mbuf.h b/lib/common/mbuf.h
index 2951454..07d97ad 100644
--- a/lib/common/mbuf.h
+++ b/lib/common/mbuf.h
@@ -30,7 +30,11 @@ pktmbuf_reset(struct rte_mbuf *m)
 {
 	union pktgen_data d;
 
-	d.udata = m->udata64;	/* Save the original value */
+	/* Save the original value. As udata64 can be either single field or the
+	 * first value of the 32-bit elements array, the whole memory chunk must
+	 * be converted directly to 64-bit.
+	 */
+	d.udata = *(uint64_t *)(&m->udata64);
 
 	rte_pktmbuf_reset(m);
 
-- 
2.25.1

Re: [dpdk-dev] [PATCH] Fix loss of data stored in udata64 mbuf field

[David Marchand]

On Thu, Feb 4, 2021 at 9:52 AM Michal Krawczyk <mk@semihalf.com> wrote:
>
> DPDK v20.11 removed udata64 field, and changed type of the dynfield1 to
> uint32_t.
>
> Due to define:
>   lib/common/pg_compat.h:#define udata64                 dynfield1[0]
> the copy of udata64 was in fact copying only the first 32 bits.

I did not look at the pktgen code, but directly accessing mbuf field
dynfieldX is likely a bug.

pktgen should register a dynfield for its own usage to avoid conflicts
with other parts of the dpdk.
Example of such a conversion to the new mechanism:
https://git.dpdk.org/dpdk/commit/?id=eb8258402b3f


-- 
David Marchand

Re: [dpdk-dev] [PATCH] Fix loss of data stored in udata64 mbuf field

[Michał Krawczyk]

czw., 4 lut 2021 o 10:01 David Marchand <david.marchand@redhat.com> napisał(a):
>
> On Thu, Feb 4, 2021 at 9:52 AM Michal Krawczyk <mk@semihalf.com> wrote:
> >
> > DPDK v20.11 removed udata64 field, and changed type of the dynfield1 to
> > uint32_t.
> >
> > Due to define:
> >   lib/common/pg_compat.h:#define udata64                 dynfield1[0]
> > the copy of udata64 was in fact copying only the first 32 bits.
>
> I did not look at the pktgen code, but directly accessing mbuf field
> dynfieldX is likely a bug.
>
> pktgen should register a dynfield for its own usage to avoid conflicts
> with other parts of the dpdk.
> Example of such a conversion to the new mechanism:
> https://git.dpdk.org/dpdk/commit/?id=eb8258402b3f
>

Hi David,

thanks for pointing this out. I wasn't aware of that, I'll rework my
fix to satisfy the dynfield requirements.

Best regards,
Michal

>
> --
> David Marchand
>

Re: [dpdk-dev] [PATCH] Fix loss of data stored in udata64 mbuf field

[Thomas Monjalon]

04/02/2021 10:17, Michał Krawczyk:
> czw., 4 lut 2021 o 10:01 David Marchand <david.marchand@redhat.com> napisał(a):
> >
> > On Thu, Feb 4, 2021 at 9:52 AM Michal Krawczyk <mk@semihalf.com> wrote:
> > >
> > > DPDK v20.11 removed udata64 field, and changed type of the dynfield1 to
> > > uint32_t.
> > >
> > > Due to define:
> > >   lib/common/pg_compat.h:#define udata64                 dynfield1[0]
> > > the copy of udata64 was in fact copying only the first 32 bits.
> >
> > I did not look at the pktgen code, but directly accessing mbuf field
> > dynfieldX is likely a bug.
> >
> > pktgen should register a dynfield for its own usage to avoid conflicts
> > with other parts of the dpdk.
> > Example of such a conversion to the new mechanism:
> > https://git.dpdk.org/dpdk/commit/?id=eb8258402b3f
> >
> 
> Hi David,
> 
> thanks for pointing this out. I wasn't aware of that, I'll rework my
> fix to satisfy the dynfield requirements.

Sorry for having to say that,
but you are supposed to read the release notes when upgrading.

[dpdk-dev] [PATCH v2] Fix loss of data stored in udata64 mbuf field

[Michal Krawczyk]

DPDK v20.11 removed mbuf's udata64 field, and changed type of the
dynfield1 to uint32_t.

Due to pktgen's define for DPDK v20.11+:
  lib/common/pg_compat.h:#define udata64                 dynfield1[0]
udata64 field was being mapped to the first entry of the dynfield1
array.

Because of that, accessing dynfield1 directly was causing 2 issues:
  * As dynfield1 is 32-bit array, using it interchangeably with the
    udata64 is causing only the first 32-bits of the intended 64-bits to
    be copied
  * dynfield1 may also be used by the other parts of the DPDK and it may
    cause conflicts

To fix that, the dynfield1 is no longer mapped to the udata64 as those
fields are uncompatible. Instead, the dynamic field API is used to get
the pktgen data offset from the dynfield1 array.

It was wrapped into an getter function, which could be reworked if
backward compatibility will be needed.

Signed-off-by: Michal Krawczyk <mk@semihalf.com>
Reviewed-by: Igor Chauskin <igorch@amazon.com>
---
v2:
* Converted pktgen to use mbuf's dynamic fields API, instead of accessing
  dynfield1 directly

 app/pktgen-main.c      | 16 ++++++++++++++++
 app/pktgen-pcap.c      |  2 +-
 app/pktgen.c           |  2 +-
 lib/common/mbuf.h      | 21 ++++++++++++++++++++-
 lib/common/pg_compat.h |  1 -
 5 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/app/pktgen-main.c b/app/pktgen-main.c
index 2d9d754..8dd31e9 100644
--- a/app/pktgen-main.c
+++ b/app/pktgen-main.c
@@ -29,6 +29,16 @@
 #include "pktgen-log.h"
 #include "cli-functions.h"
 
+/* Offset to the mbuf dynamic field holding pktgen data. */
+int pktgen_dynfield_offset = -1;
+
+/* Descriptor used for the mbuf dynamic field configuration. */
+static const struct rte_mbuf_dynfield pktgen_dynfield_desc = {
+	.name = "pktgen_dynfield_data",
+	.size = sizeof(union pktgen_data),
+	.align = __alignof__(union pktgen_data),
+};
+
 #ifdef GUI
 int pktgen_gui_main(int argc, char *argv[]);
 #endif
@@ -455,6 +465,12 @@ main(int argc, char **argv)
 	argc -= ret;
 	argv += ret;
 
+	/* Configure pktgen data which will be encapsulated in the mbuf. */
+	pktgen_dynfield_offset =
+		rte_mbuf_dynfield_register(&pktgen_dynfield_desc);
+	if (pktgen_dynfield_offset < 0)
+		rte_exit(EXIT_FAILURE, "Cannot register mbuf field\n");
+
 	if (pktgen_cli_create())
 		return -1;
 
diff --git a/app/pktgen-pcap.c b/app/pktgen-pcap.c
index 249d62c..6732919 100644
--- a/app/pktgen-pcap.c
+++ b/app/pktgen-pcap.c
@@ -258,7 +258,7 @@ pktgen_pcap_mbuf_ctor(struct rte_mempool *mp,
 	m->next		= NULL;
 
 	for (;; ) {
-		union pktgen_data *d = (union pktgen_data *)&m->udata64;
+		union pktgen_data *d = pktgen_data_field(m);
 
 		if ( (i & 0x3ff) == 0) {
 			scrn_printf(1, 1, "%c\b", "-\\|/"[(i >> 10) & 3]);
diff --git a/app/pktgen.c b/app/pktgen.c
index ef9f531..fd28606 100644
--- a/app/pktgen.c
+++ b/app/pktgen.c
@@ -927,7 +927,7 @@ pktgen_setup_cb(struct rte_mempool *mp,
 {
 	pkt_data_t *data = (pkt_data_t *)opaque;
 	struct rte_mbuf *m = (struct rte_mbuf *)obj;
-	union pktgen_data *d = (union pktgen_data *)&m->udata64;
+	union pktgen_data *d = pktgen_data_field(m);
 	port_info_t *info;
 	pkt_seq_t *pkt;
 	uint16_t qid, idx;
diff --git a/lib/common/mbuf.h b/lib/common/mbuf.h
index 2951454..88cc7b0 100644
--- a/lib/common/mbuf.h
+++ b/lib/common/mbuf.h
@@ -25,12 +25,31 @@ union pktgen_data {
 	};
 };
 
+extern int pktgen_dynfield_offset;
+
+/**
+ * Get pointer to the pktgen specific data encapsulated in the mbuf. Dynamic
+ * field API has to be used for this purpose, to avoid conflicts with other
+ * parts of the DPDK.
+ *
+ *  @param m
+ *    Pointer to the mbuf from which the pktgen data should be retrieved.
+ *  @return
+ *    Pointer to the pktgen specific data encapsulated in the mbuf.
+ */
+static inline union pktgen_data *
+pktgen_data_field(struct rte_mbuf *m)
+{
+	return RTE_MBUF_DYNFIELD(m, pktgen_dynfield_offset,
+		union pktgen_data *);
+}
+
 static inline void
 pktmbuf_reset(struct rte_mbuf *m)
 {
 	union pktgen_data d;
 
-	d.udata = m->udata64;	/* Save the original value */
+	d = *pktgen_data_field(m); /* Save the original value */
 
 	rte_pktmbuf_reset(m);
 
diff --git a/lib/common/pg_compat.h b/lib/common/pg_compat.h
index 2da1014..aba2863 100644
--- a/lib/common/pg_compat.h
+++ b/lib/common/pg_compat.h
@@ -33,7 +33,6 @@ extern "C" {
 
 #if RTE_VERSION >= RTE_VERSION_NUM(20,11,0,0)
 #define pg_get_initial_lcore    rte_get_main_lcore
-#define udata64                 dynfield1[0]
 #define PG_DEVTYPE_BLOCKED RTE_DEVTYPE_BLOCKED
 #define PG_DEVTYPE_ALLOWED RTE_DEVTYPE_ALLOWED
 #else
-- 
2.25.1