RE: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Morten Brørup]

> From: Feifei Wang [mailto:feifei.wa...@arm.com]
> Sent: Tuesday, 5 December 2023 04.13
> 
> 在 2023/12/4 15:41, Morten Brørup 写道:
> >> From: Feifei Wang [mailto:feifei.wa...@arm.com]
> >> Sent: Monday, 4 December 2023 03.39
> >>
> >> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) ==
> 1'
> >> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> >> mbuf's refcnt value should be 1. Thus we can simplify the code and
> >> remove the redundant part.
> >>
> >> Furthermore, according to [1], when the mbuf is stored inside the
> >> mempool, the m->refcnt value should be 1. And then it is detached
> >> from its parent for an indirect mbuf. Thus change the description of
> >> 'rte_pktmbuf_prefree_seg' function.
> >>
> >> [1]
> https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> >> olivier.m...@6wind.com/
> >>
> >> Suggested-by: Ruifeng Wang 
> >> Signed-off-by: Feifei Wang 
> >> ---
> >>   lib/mbuf/rte_mbuf.h | 22 +++---
> >>   1 file changed, 3 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> >> index 286b32b788..42e9b50d51 100644
> >> --- a/lib/mbuf/rte_mbuf.h
> >> +++ b/lib/mbuf/rte_mbuf.h
> >> @@ -1328,7 +1328,7 @@ static inline int
> >> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> >>*
> >>* This function does the same than a free, except that it does
> not
> >>* return the segment to its pool.
> >> - * It decreases the reference counter, and if it reaches 0, it is
> >> + * It decreases the reference counter, and if it reaches 1, it is
> > No, the original description is correct.
> > However, the reference counter is set to 1 when put back in the pool,
> as a shortcut so it isn't needed to be set back to 1 when allocated
> from the pool.
> 
> Ok.
> 
> for 'else if (__rte_mbuf_refcnt_update(m, -1) == 0)' case, it is easy
> to
> understand.
> 
> but for '(likely(rte_mbuf_refcnt_read(m) == 1))' case, I think this
> will
> create misleading. dpdk users maybe difficult to understand why the
> code
> can not match the function description.
> 
> Maybe we need some explanation here?

Agree. It is quite counterintuitive (but a clever optimization!) that the 
reference counter is 1 instead of 0 when free.

Something like:

static __rte_always_inline struct rte_mbuf *
rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
{
__rte_mbuf_sanity_check(m, 0);

if (likely(rte_mbuf_refcnt_read(m) == 1)) {
+   /* This branch is a performance optimized variant of the branch 
below.
+* If the reference counter would reach 0 when decrementing it,
+* do not decrement it to 0 and then initialize it to 1;
+* just leave it at 1, thereby avoiding writing to memory.
+*/

if (!RTE_MBUF_DIRECT(m)) {
rte_pktmbuf_detach(m);
if (RTE_MBUF_HAS_EXTBUF(m) &&
RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
__rte_pktmbuf_pinned_extbuf_decref(m))
return NULL;
}

if (m->next != NULL)
m->next = NULL;
if (m->nb_segs != 1)
m->nb_segs = 1;
+   /* No need to initialize the reference counter; it is already 
1. */

return m;

} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {

if (!RTE_MBUF_DIRECT(m)) {
rte_pktmbuf_detach(m);
if (RTE_MBUF_HAS_EXTBUF(m) &&
RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
__rte_pktmbuf_pinned_extbuf_decref(m))
return NULL;
}

if (m->next != NULL)
m->next = NULL;
if (m->nb_segs != 1)
m->nb_segs = 1;
+   /* Initialize the reference counter to 1, so
+* incrementing it is unnecessary when allocating the mbuf.
+*/
rte_mbuf_refcnt_set(m, 1);

return m;
}
return NULL;
}


Alternatively, add a function to do the initialization work:

static __rte_always_inline struct rte_mbuf *
rte_pktmbuf_prefree_seg_last_ref(struct rte_mbuf *m, const bool init_refcnt)
{
if (!RTE_MBUF_DIRECT(m)) {
rte_pktmbuf_detach(m);
if (RTE_MBUF_HAS_EXTBUF(m) &&
RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
__rte_pktmbuf_pinned_extbuf_decref(m))
return NULL;
}

if (m->next != NULL)
m->next = NULL;
if (m->nb_segs != 1)
m->nb_segs = 1;

+   /* The reference counter must be initialized to 1 when the mbuf is free,
+* so incrementing to 1 is unnecessary when allocating the mbuf.
+*/
if (init_refc

Re: [PATCH 1/3] vhost: robustify virtqueue access lock asserts

[Maxime Coquelin]

s/robustify/enhance/ ?

On 10/23/23 11:55, David Marchand wrote:

A simple comment in vhost_user_msg_handler() is not that robust.

Add a lock_all_qps property to message handlers so that their
implementation can add a build check and assert a vq is locked.

Signed-off-by: David Marchand 
---
  lib/vhost/vhost_user.c | 110 +++--
  1 file changed, 51 insertions(+), 59 deletions(-)



I can reword the commit title while applying:

Reviewed-by: Maxime Coquelin 

Thanks,
Maxime

mbuf init questions

[Morten Brørup]

Why is m->nb_segs initialized in rte_pktmbuf_prefree_seg()?

It's part of the m->rearm_data, and will be initialized on RX descriptor rearm 
anyway.


Slightly related:

When built without RTE_IOVA_IN_MBUF, m->next lives in the first cache line.

Theoretically, it would improve performance to initialize m->next together with 
the RX descriptor rearm, when writing to the first cache line anyway, instead 
of in rte_pktmbuf_prefree_seg(), which requires another write to the first 
cache line. (In the case where m->next is non-NULL and m->refcnt is 1.)



Med venlig hilsen / Kind regards,
-Morten Brørup

Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath

[Maxime Coquelin]

On 10/23/23 11:55, David Marchand wrote:

Now that a r/w lock is used, the access_ok field should only be updated
under a write lock.

Since the datapath code only takes a read lock on the virtqueue to check
access_ok, this lock must be released and a write lock taken before
calling vring_translate().

Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
---
  lib/vhost/virtio_net.c | 60 +++---
  1 file changed, 44 insertions(+), 16 deletions(-)



Reviewed-by: Maxime Coquelin 

[PATCH v4 1/3] node: support to add next node to ethdev Rx node

[Rakesh Kudurumalla]

By default all packets received on ethdev_rx node
is forwarded to pkt_cls node.This patch provides
library support to add a new node as next node to
ethdev_rx node and forward packet to new node from
rx node.

Signed-off-by: Rakesh Kudurumalla 
---
v4: Resolve compilation issues

 lib/node/ethdev_ctrl.c  | 45 +
 lib/node/rte_node_eth_api.h | 19 
 lib/node/version.map|  1 +
 3 files changed, 65 insertions(+)

diff --git a/lib/node/ethdev_ctrl.c b/lib/node/ethdev_ctrl.c
index d564b80e37..0faf4c19f2 100644
--- a/lib/node/ethdev_ctrl.c
+++ b/lib/node/ethdev_ctrl.c
@@ -3,6 +3,7 @@
  */
 
 #include 
+#include 
 
 #include 
 #include 
@@ -129,3 +130,47 @@ rte_node_eth_config(struct rte_node_ethdev_config *conf, 
uint16_t nb_confs,
ctrl.nb_graphs = nb_graphs;
return 0;
 }
+
+int
+rte_node_ethdev_rx_next_update(rte_node_t id, const char *edge_name)
+{
+   struct ethdev_rx_node_main *data;
+   ethdev_rx_node_elem_t *elem;
+   char **next_nodes;
+   int rc = -EINVAL;
+   uint32_t count;
+   uint16_t i = 0;
+
+   if (edge_name == NULL)
+   return rc;
+
+   count = rte_node_edge_get(id, NULL);
+
+   if (count == RTE_NODE_ID_INVALID)
+   return rc;
+
+   next_nodes = malloc(count);
+   if (next_nodes == NULL)
+   return -ENOMEM;
+
+   count = rte_node_edge_get(id, next_nodes);
+
+   while (next_nodes[i] != NULL) {
+   if (strcmp(edge_name, next_nodes[i]) == 0) {
+   data = ethdev_rx_get_node_data_get();
+   elem = data->head;
+   while (elem->next != data->head) {
+   if (elem->nid == id) {
+   elem->ctx.cls_next = i;
+   rc = 0;
+   goto exit;
+   }
+   elem = elem->next;
+   }
+   }
+   i++;
+   }
+exit:
+   free(next_nodes);
+   return rc;
+}
diff --git a/lib/node/rte_node_eth_api.h b/lib/node/rte_node_eth_api.h
index eaae50772d..b082a5bec1 100644
--- a/lib/node/rte_node_eth_api.h
+++ b/lib/node/rte_node_eth_api.h
@@ -23,6 +23,7 @@ extern "C" {
 #include 
 #include 
 #include 
+#include 
 
 /**
  * Port config for ethdev_rx and ethdev_tx node.
@@ -57,6 +58,24 @@ struct rte_node_ethdev_config {
  */
 int rte_node_eth_config(struct rte_node_ethdev_config *cfg,
uint16_t cnt, uint16_t nb_graphs);
+
+/**
+ * Update ethdev rx next node.
+ *
+ * @param id
+ *   Node id whose edge is to be updated.
+ * @param edge_name
+ *   Name of the next node.
+ *
+ * @return
+ *   RTE_EDGE_ID_INVALID if id is invalid
+ *   ENINVAL: Either of input parameters are invalid
+ *   ENOMEM: If memory allocation failed
+ *   0 on successful initialization.
+ */
+__rte_experimental
+int rte_node_ethdev_rx_next_update(rte_node_t id, const char *edge_name);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/node/version.map b/lib/node/version.map
index 99ffcdd414..07abc3a79f 100644
--- a/lib/node/version.map
+++ b/lib/node/version.map
@@ -16,6 +16,7 @@ EXPERIMENTAL {
rte_node_ip6_route_add;
 
# added in 23.11
+   rte_node_ethdev_rx_next_update;
rte_node_ip4_reassembly_configure;
rte_node_udp4_dst_port_add;
rte_node_udp4_usr_node_add;
-- 
2.25.1

[PATCH v4 2/3] app/graph: add ethdev forward command

[Rakesh Kudurumalla]

Adds a txport to forward packet for every rxport

Mapping will be used to forward packets to txport
received on rxport

Following commands are exposed:
- ethdev forward  "

Signed-off-by: Rakesh Kudurumalla 
---
 app/graph/cli.c |  1 +
 app/graph/ethdev.c  | 62 +
 app/graph/ethdev.h  |  1 +
 app/graph/ethdev_priv.h |  8 ++
 4 files changed, 72 insertions(+)

diff --git a/app/graph/cli.c b/app/graph/cli.c
index 30b12312d6..76f5b8e670 100644
--- a/app/graph/cli.c
+++ b/app/graph/cli.c
@@ -32,6 +32,7 @@ cmdline_parse_ctx_t modules_ctx[] = {
(cmdline_parse_inst_t *)ðdev_prom_mode_cmd_ctx,
(cmdline_parse_inst_t *)ðdev_ip4_cmd_ctx,
(cmdline_parse_inst_t *)ðdev_ip6_cmd_ctx,
+   (cmdline_parse_inst_t *)ðdev_forward_cmd_ctx,
(cmdline_parse_inst_t *)ðdev_cmd_ctx,
(cmdline_parse_inst_t *)ðdev_help_cmd_ctx,
(cmdline_parse_inst_t *)ðdev_rx_cmd_ctx,
diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
index c9b09168c1..bceee659a2 100644
--- a/app/graph/ethdev.c
+++ b/app/graph/ethdev.c
@@ -38,6 +38,10 @@ cmd_ethdev_ip4_addr_help[] = "ethdev  ip4 addr 
add  netmask config.tx_port_id = portid_tx;
+   rc = 0;
+   }
+
+   return rc;
+}
+
+static void
+cli_ethdev_forward(void *parsed_result, __rte_unused struct cmdline *cl, void 
*data __rte_unused)
+{
+   struct ethdev_fwd_cmd_tokens *res = parsed_result;
+   int rc = -EINVAL;
+
+   rc = ethdev_forward_config(res->tx_dev, res->rx_dev);
+   if (rc < 0)
+   printf(MSG_CMD_FAIL, res->cmd);
+}
+
+cmdline_parse_token_string_t ethdev_l2_cmd =
+   TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens, cmd, "ethdev");
+cmdline_parse_token_string_t ethdev_fwd_cmd =
+   TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens, fwd, "forward");
+cmdline_parse_token_string_t ethdev_tx_device =
+   TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens, tx_dev, NULL);
+cmdline_parse_token_string_t ethdev_rx_device =
+   TOKEN_STRING_INITIALIZER(struct ethdev_fwd_cmd_tokens, rx_dev, NULL);
+
+cmdline_parse_inst_t ethdev_forward_cmd_ctx = {
+   .f = cli_ethdev_forward,
+   .data = NULL,
+   .help_str = cmd_ethdev_forward_help,
+   .tokens = {
+  (void *)ðdev_l2_cmd,
+  (void *)ðdev_fwd_cmd,
+  (void *)ðdev_tx_device,
+  (void *)ðdev_rx_device,
+  NULL,
+   },
+};
diff --git a/app/graph/ethdev.h b/app/graph/ethdev.h
index 94d3247a2c..836052046b 100644
--- a/app/graph/ethdev.h
+++ b/app/graph/ethdev.h
@@ -15,6 +15,7 @@ extern cmdline_parse_inst_t ethdev_mtu_cmd_ctx;
 extern cmdline_parse_inst_t ethdev_prom_mode_cmd_ctx;
 extern cmdline_parse_inst_t ethdev_ip4_cmd_ctx;
 extern cmdline_parse_inst_t ethdev_ip6_cmd_ctx;
+extern cmdline_parse_inst_t ethdev_forward_cmd_ctx;
 extern cmdline_parse_inst_t ethdev_cmd_ctx;
 extern cmdline_parse_inst_t ethdev_help_cmd_ctx;
 
diff --git a/app/graph/ethdev_priv.h b/app/graph/ethdev_priv.h
index f231f3f3e1..e5e5fbc9ae 100644
--- a/app/graph/ethdev_priv.h
+++ b/app/graph/ethdev_priv.h
@@ -61,6 +61,13 @@ struct ethdev_ip6_cmd_tokens {
cmdline_fixed_string_t mask;
 };
 
+struct ethdev_fwd_cmd_tokens {
+   cmdline_fixed_string_t cmd;
+   cmdline_fixed_string_t fwd;
+   cmdline_fixed_string_t tx_dev;
+   cmdline_fixed_string_t rx_dev;
+};
+
 struct ethdev_cmd_tokens {
cmdline_fixed_string_t cmd;
cmdline_fixed_string_t dev;
@@ -98,6 +105,7 @@ struct ethdev_config {
uint32_t queue_size;
} tx;
 
+   uint16_t tx_port_id;
int promiscuous;
uint32_t mtu;
 };
-- 
2.25.1

[PATCH v4 3/3] app/graph: implement port forward usecase

[Rakesh Kudurumalla]

Added portforward usecase.In this usecase
packets received Rx port is forwarded to
respective Tx port.

Signed-off-by: Rakesh Kudurumalla 
---
 app/graph/ethdev.c   |  12 ++
 app/graph/ethdev.h   |   1 +
 app/graph/examples/l2fwd.cli |  41 +
 app/graph/examples/l2fwd_pcap.cli|  37 +
 app/graph/graph.c|   8 +-
 app/graph/l2fwd.c| 148 +++
 app/graph/l2fwd.h|  11 ++
 app/graph/meson.build|   1 +
 app/graph/module_api.h   |   1 +
 doc/guides/tools/graph.rst   |  27 
 doc/guides/tools/img/graph-usecase-l2fwd.svg |  84 +++
 11 files changed, 370 insertions(+), 1 deletion(-)
 create mode 100644 app/graph/examples/l2fwd.cli
 create mode 100644 app/graph/examples/l2fwd_pcap.cli
 create mode 100644 app/graph/l2fwd.c
 create mode 100644 app/graph/l2fwd.h
 create mode 100644 doc/guides/tools/img/graph-usecase-l2fwd.svg

diff --git a/app/graph/ethdev.c b/app/graph/ethdev.c
index bceee659a2..d048a6 100644
--- a/app/graph/ethdev.c
+++ b/app/graph/ethdev.c
@@ -77,6 +77,18 @@ ethdev_port_by_id(uint16_t port_id)
return NULL;
 }
 
+int16_t
+find_txport_by_rxport(uint16_t portid_rx)
+{
+   int portid = -EINVAL;
+   struct ethdev *port;
+   port = ethdev_port_by_id(portid_rx);
+   if (port)
+   portid = port->config.tx_port_id;
+
+   return portid;
+}
+
 void *
 ethdev_mempool_list_by_portid(uint16_t portid)
 {
diff --git a/app/graph/ethdev.h b/app/graph/ethdev.h
index 836052046b..946e14d801 100644
--- a/app/graph/ethdev.h
+++ b/app/graph/ethdev.h
@@ -33,6 +33,7 @@ extern uint32_t enabled_port_mask;
 
 void ethdev_start(void);
 void ethdev_stop(void);
+int16_t find_txport_by_rxport(uint16_t portid_rx);
 void *ethdev_mempool_list_by_portid(uint16_t portid);
 int16_t ethdev_portid_by_ip4(uint32_t ip, uint32_t mask);
 int16_t ethdev_portid_by_ip6(uint8_t *ip, uint8_t *mask);
diff --git a/app/graph/examples/l2fwd.cli b/app/graph/examples/l2fwd.cli
new file mode 100644
index 00..af24a5836a
--- /dev/null
+++ b/app/graph/examples/l2fwd.cli
@@ -0,0 +1,41 @@
+; SPDX-License-Identifier: BSD-3-Clause
+; Copyright(c) 2023 Marvell.
+
+;
+; Graph configuration for given usecase
+;
+graph l2fwd coremask 0xff bsz 32 tmo 10 model default pcap_enable 1 
num_pcap_pkts 10 pcap_file /tmp/output.pcap
+
+;
+; Mempools to be attached with ethdev
+;
+mempool mempool0 size 8192 buffers 4000 cache 256 numa 0
+
+;
+; DPDK devices and configuration.
+;
+; Note: Customize the parameters below to match your setup.
+;
+ethdev 0002:01:00.1 rxq 1 txq 8 mempool0
+ethdev 0002:01:00.4 rxq 1 txq 8 mempool0
+ethdev 0002:01:00.6 rxq 1 txq 8 mempool0
+ethdev 0002:02:00.0 rxq 1 txq 8 mempool0
+
+;
+; L2 mac forwarding rules
+;
+ethdev forward 0002:01:00.4 0002:02:00.0
+ethdev forward 0002:01:00.1 0002:01:00.6
+
+;
+; Port-Queue-Core mapping for ethdev_rx node
+;
+ethdev_rx map port 0002:02:00.0 queue 0 core 1
+ethdev_rx map port 0002:01:00.6 queue 0 core 2
+
+;
+; Graph start command to create graph.
+;
+; Note: No more command should come after this.
+;
+graph start
diff --git a/app/graph/examples/l2fwd_pcap.cli 
b/app/graph/examples/l2fwd_pcap.cli
new file mode 100644
index 00..718347f568
--- /dev/null
+++ b/app/graph/examples/l2fwd_pcap.cli
@@ -0,0 +1,37 @@
+; SPDX-License-Identifier: BSD-3-Clause
+; Copyright(c) 2023 Marvell.
+
+;
+; Graph configuration for given usecase
+;
+graph l2fwd coremask 0xff bsz 32 tmo 10 model default pcap_enable 1 
num_pcap_pkts 10 pcap_file /tmp/output.pcap
+
+;
+; Mempools to be attached with ethdev
+;
+mempool mempool0 size 8192 buffers 4000 cache 256 numa 0
+
+;
+; DPDK devices and configuration.
+;
+; Note: Customize the parameters below to match your setup.
+;
+ethdev net_pcap0 rxq 1 txq 8 mempool0
+ethdev net_pcap1 rxq 1 txq 8 mempool0
+
+;
+; L2 mac forwarding rules
+;
+ethdev forward net_pcap1 net_pcap0
+
+;
+; Port-Queue-Core mapping for ethdev_rx node
+;
+ethdev_rx map port net_pcap0 queue 0 core 1
+
+;
+; Graph start command to create graph.
+;
+; Note: No more command should come after this.
+;
+graph start
diff --git a/app/graph/graph.c b/app/graph/graph.c
index a65723a196..4e0441f1a7 100644
--- a/app/graph/graph.c
+++ b/app/graph/graph.c
@@ -24,7 +24,7 @@ cmd_graph_help[] = "graph  bsz  tmo  
coremask  "
   "model  pcap_enable <0 | 1> 
num_pcap_pkts "
   "pcap_file ";
 
-static const char * const supported_usecases[] = {"l3fwd"};
+static const char * const supported_usecases[] = {"l3fwd", "l2fwd"};
 struct graph_config graph_config;
 bool graph_started;
 
@@ -273,6 +273,12 @@ cli_graph_start(__rte_unused void *parsed_result, 
__rte_unused struct cmdline *c
break;
}
}
+   if (!strcmp(graph_c

RE: [PATCH] raw/cnxk_gpio: switch to dynamic logging

[Jerin Jacob Kollanukkaran]

> -Original Message-
> From: Tomasz Duszynski 
> Sent: Saturday, November 4, 2023 2:40 AM
> To: dev@dpdk.org; Jakub Palider ; Tomasz Duszynski
> 
> Cc: Jerin Jacob Kollanukkaran ;
> step...@networkplumber.org
> Subject: [PATCH] raw/cnxk_gpio: switch to dynamic logging
> 
> Dynamically allocated log type is a standard approach among all drivers.
> Switch to it.
> 
> Signed-off-by: Tomasz Duszynski 


Applied to dpdk-next-net-mrvl/for-main. Thanks


> ---
>  drivers/raw/cnxk_gpio/cnxk_gpio.c  | 22 --
>  drivers/raw/cnxk_gpio/cnxk_gpio.h  |  7 +++
>  drivers/raw/cnxk_gpio/cnxk_gpio_selftest.c | 19 ---
>  3 files changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/raw/cnxk_gpio/cnxk_gpio.c
> b/drivers/raw/cnxk_gpio/cnxk_gpio.c
> index 29c2506726..ebc914afcf 100644
> --- a/drivers/raw/cnxk_gpio/cnxk_gpio.c
> +++ b/drivers/raw/cnxk_gpio/cnxk_gpio.c
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> 
>  #include 
> @@ -213,13 +214,13 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip
> *gpiochip, char *allowlist)
>   errno = 0;
>   val = strtol(token, NULL, 10);
>   if (errno) {
> - RTE_LOG(ERR, PMD, "failed to parse %s\n", token);
> + CNXK_GPIO_LOG(ERR, "failed to parse %s", token);
>   ret = -errno;
>   goto out;
>   }
> 
>   if (val < 0 || val >= gpiochip->num_gpios) {
> - RTE_LOG(ERR, PMD, "gpio%d out of 0-%d range\n",
> val,
> + CNXK_GPIO_LOG(ERR, "gpio%d out of 0-%d range",
> val,
>   gpiochip->num_gpios - 1);
>   ret = -EINVAL;
>   goto out;
> @@ -229,7 +230,7 @@ cnxk_gpio_parse_allowlist(struct cnxk_gpiochip
> *gpiochip, char *allowlist)
>   if (list[i] != val)
>   continue;
> 
> - RTE_LOG(WARNING, PMD, "gpio%d already
> allowed\n", val);
> + CNXK_GPIO_LOG(WARNING, "gpio%d already
> allowed", val);
>   break;
>   }
>   if (i == queue)
> @@ -396,7 +397,7 @@ cnxk_gpio_queue_setup(struct rte_rawdev *dev,
> uint16_t queue_id,
>   return ret;
>   }
>   } else {
> - RTE_LOG(WARNING, PMD, "using existing gpio%d\n", gpio-
> >num);
> + CNXK_GPIO_LOG(WARNING, "using existing gpio%d", gpio-
> >num);
>   }
> 
>   gpiochip->gpios[num] = gpio;
> @@ -645,7 +646,7 @@ cnxk_gpio_process_buf(struct cnxk_gpio *gpio, struct
> rte_rawdev_buf *rbuf)
> 
>   /* get rid of last response if any */
>   if (gpio->rsp) {
> - RTE_LOG(WARNING, PMD, "previous response got
> overwritten\n");
> + CNXK_GPIO_LOG(WARNING, "previous response got
> overwritten");
>   rte_free(gpio->rsp);
>   }
>   gpio->rsp = rsp;
> @@ -739,7 +740,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>   cnxk_gpio_format_name(name, sizeof(name));
>   rawdev = rte_rawdev_pmd_allocate(name, sizeof(*gpiochip),
> rte_socket_id());
>   if (!rawdev) {
> - RTE_LOG(ERR, PMD, "failed to allocate %s rawdev\n", name);
> + CNXK_GPIO_LOG(ERR, "failed to allocate %s rawdev", name);
>   return -ENOMEM;
>   }
> 
> @@ -768,7 +769,7 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>   snprintf(buf, sizeof(buf), "%s/gpiochip%d/base",
> CNXK_GPIO_CLASS_PATH, gpiochip->num);
>   ret = cnxk_gpio_read_attr_int(buf, &gpiochip->base);
>   if (ret) {
> - RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
> + CNXK_GPIO_LOG(ERR, "failed to read %s", buf);
>   goto out;
>   }
> 
> @@ -776,20 +777,20 @@ cnxk_gpio_probe(struct rte_vdev_device *dev)
>   snprintf(buf, sizeof(buf), "%s/gpiochip%d/ngpio",
> CNXK_GPIO_CLASS_PATH, gpiochip->num);
>   ret = cnxk_gpio_read_attr_int(buf, &gpiochip->num_gpios);
>   if (ret) {
> - RTE_LOG(ERR, PMD, "failed to read %s\n", buf);
> + CNXK_GPIO_LOG(ERR, "failed to read %s", buf);
>   goto out;
>   }
>   gpiochip->num_queues = gpiochip->num_gpios;
> 
>   ret = cnxk_gpio_parse_allowlist(gpiochip, params->allowlist);
>   if (ret) {
> - RTE_LOG(ERR, PMD, "failed to parse allowed gpios\n");
> + CNXK_GPIO_LOG(ERR, "failed to parse allowed gpios");
>   goto out;
>   }
> 
>   gpiochip->gpios = rte_calloc(NULL, gpiochip->num_gpios, sizeof(struct
> cnxk_gpio *), 0);
>   if (!gpiochip->gpios) {
> - RTE_LOG(ERR, PMD, "failed to allocate gpios memory\n");
> + CNXK_GPIO_LOG(ERR, "failed to allocate gpios memory");
>   ret = -ENOMEM;
>   goto out;
>   }
> @@ -849,3 +850,4 @@ RTE_PMD_REGISTER_VDEV(cnxk_gpio, cn

Re: [dpdk-dev] [PATCH] net/af_xdp: fix memzone leak in error path

[Ferruh Yigit]

On 12/5/2023 1:23 AM, wangyunjian wrote:
> 
> 
>> -Original Message-
>> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com]
>> Sent: Monday, December 4, 2023 10:10 PM
>> To: wangyunjian ; dev@dpdk.org
>> Cc: ciara.lof...@intel.com; qi.z.zh...@intel.com; xudingke
>> ; Lilijun (Jerry) ;
>> sta...@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/af_xdp: fix memzone leak in error path
>>
>> On 12/1/2023 8:03 AM, Yunjian Wang wrote:
>>> In xdp_umem_configure() allocated memzone for the 'umem', we should
>>> free it when xsk_umem__create() call fails, otherwise it will lead to
>>> memory zone leak.
>>>
>>> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
>>> Cc: sta...@dpdk.org
>>>
>>> Signed-off-by: Yunjian Wang 
>>> ---
>>>  drivers/net/af_xdp/rte_eth_af_xdp.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> index 2a20a6960c..2a1fdafb3c 100644
>>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> @@ -1229,6 +1229,7 @@ xsk_umem_info *xdp_umem_configure(struct
>>> pmd_internals *internals,
>>>
>>> if (ret) {
>>> AF_XDP_LOG(ERR, "Failed to create umem\n");
>>> +   rte_memzone_free(mz);
>>>
>>
>> Doesn't 'xdp_umem_destroy()', in the label 'err', already free it?
> 
> In this case, 'mz' is not assigned to 'umem->mz'. Therefore,
> the'xdp_umem_destroy()' does not free 'mz'.
> 
> 

True.
What do you think to move 'umem->mz = mz;' assignment after 'mz == NULL'
check? So related code can be grouped together.

[PATCH 1/2] common/cnxk: support to dump debug info to file

[Rakesh Kudurumalla]

This patch dumps contents of receviced packet descriptor from CQ
for debug to file

Signed-off-by: Rakesh Kudurumalla 
---
 drivers/common/cnxk/roc_cpt.h   |  2 +-
 drivers/common/cnxk/roc_cpt_debug.c | 56 +
 drivers/common/cnxk/roc_nix.h   |  2 +-
 drivers/common/cnxk/roc_nix_debug.c |  3 +-
 4 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/common/cnxk/roc_cpt.h b/drivers/common/cnxk/roc_cpt.h
index 787bccb27d..523a5964a3 100644
--- a/drivers/common/cnxk/roc_cpt.h
+++ b/drivers/common/cnxk/roc_cpt.h
@@ -191,7 +191,7 @@ void __roc_api roc_cpt_iq_enable(struct roc_cpt_lf *lf);
 int __roc_api roc_cpt_lmtline_init(struct roc_cpt *roc_cpt,
   struct roc_cpt_lmtline *lmtline, int lf_id);
 
-void __roc_api roc_cpt_parse_hdr_dump(const struct cpt_parse_hdr_s *cpth);
+void __roc_api roc_cpt_parse_hdr_dump(FILE *file, const struct cpt_parse_hdr_s 
*cpth);
 int __roc_api roc_cpt_ctx_write(struct roc_cpt_lf *lf, void *sa_dptr,
void *sa_cptr, uint16_t sa_len);
 
diff --git a/drivers/common/cnxk/roc_cpt_debug.c 
b/drivers/common/cnxk/roc_cpt_debug.c
index dce3638507..8e69b0a0e5 100644
--- a/drivers/common/cnxk/roc_cpt_debug.c
+++ b/drivers/common/cnxk/roc_cpt_debug.c
@@ -5,38 +5,48 @@
 #include "roc_api.h"
 #include "roc_priv.h"
 
+#define cpt_dump(file, fmt, ...) do {  
 \
+   if ((file) == NULL) 
\
+   plt_dump(fmt, ##__VA_ARGS__);   
\
+   else
\
+   fprintf(file, fmt "\n", ##__VA_ARGS__); 
\
+} while (0)
+
 void
-roc_cpt_parse_hdr_dump(const struct cpt_parse_hdr_s *cpth)
+roc_cpt_parse_hdr_dump(FILE *file, const struct cpt_parse_hdr_s *cpth)
 {
struct cpt_frag_info_s *frag_info;
uint32_t offset;
uint64_t *slot;
 
-   plt_print("CPT_PARSE \t0x%p:", cpth);
+   cpt_dump(file, "CPT_PARSE \t0x%p:", cpth);
 
/* W0 */
-   plt_print("W0: cookie \t0x%x\t\tmatch_id \t0x%04x\t\terr_sum \t%u \t",
- cpth->w0.cookie, cpth->w0.match_id, cpth->w0.err_sum);
-   plt_print("W0: reas_sts \t0x%x\t\tet_owr \t%u\t\tpkt_fmt \t%u \t",
+   cpt_dump(file, "W0: cookie \t0x%x\t\tmatch_id \t0x%04x \t",
+ cpth->w0.cookie, cpth->w0.match_id);
+   cpt_dump(file, "W0: err_sum \t%u \t", cpth->w0.err_sum);
+   cpt_dump(file, "W0: reas_sts \t0x%x\t\tet_owr \t%u\t\tpkt_fmt \t%u \t",
  cpth->w0.reas_sts, cpth->w0.et_owr, cpth->w0.pkt_fmt);
-   plt_print("W0: pad_len \t%u\t\tnum_frags \t%u\t\tpkt_out \t%u \t",
+   cpt_dump(file, "W0: pad_len \t%u\t\tnum_frags \t%u\t\tpkt_out \t%u \t",
  cpth->w0.pad_len, cpth->w0.num_frags, cpth->w0.pkt_out);
 
/* W1 */
-   plt_print("W1: wqe_ptr \t0x%016lx\t", plt_be_to_cpu_64(cpth->wqe_ptr));
+   cpt_dump(file, "W1: wqe_ptr \t0x%016lx\t",
+   plt_be_to_cpu_64(cpth->wqe_ptr));
 
/* W2 */
-   plt_print("W2: frag_age \t0x%x\t\torig_pf_func \t0x%04x",
+   cpt_dump(file, "W2: frag_age \t0x%x\t\torig_pf_func \t0x%04x",
  cpth->w2.frag_age, cpth->w2.orig_pf_func);
-   plt_print("W2: il3_off \t0x%x\t\tfi_pad \t0x%x\t\tfi_offset \t0x%x \t",
- cpth->w2.il3_off, cpth->w2.fi_pad, cpth->w2.fi_offset);
+   cpt_dump(file, "W2: il3_off \t0x%x\t\tfi_pad \t0x%x \t",
+ cpth->w2.il3_off, cpth->w2.fi_pad);
+   cpt_dump(file, "W2: fi_offset \t0x%x \t", cpth->w2.fi_offset);
 
/* W3 */
-   plt_print("W3: hw_ccode \t0x%x\t\tuc_ccode \t0x%x\t\tspi \t0x%08x",
+   cpt_dump(file, "W3: hw_ccode \t0x%x\t\tuc_ccode \t0x%x\t\tspi \t0x%08x",
  cpth->w3.hw_ccode, cpth->w3.uc_ccode, cpth->w3.spi);
 
/* W4 */
-   plt_print("W4: esn \t%" PRIx64 " \t OR frag1_wqe_ptr \t0x%" PRIx64,
+   cpt_dump(file, "W4: esn \t%" PRIx64 " \t OR frag1_wqe_ptr \t0x%" PRIx64,
  cpth->esn, plt_be_to_cpu_64(cpth->frag1_wqe_ptr));
 
/* offset of 0 implies 256B, otherwise it implies offset*8B */
@@ -44,24 +54,24 @@ roc_cpt_parse_hdr_dump(const struct cpt_parse_hdr_s *cpth)
offset = (((offset - 1) & 0x1f) + 1) * 8;
frag_info = PLT_PTR_ADD(cpth, offset);
 
-   plt_print("CPT Fraginfo \t0x%p:", frag_info);
+   cpt_dump(file, "CPT Fraginfo \t0x%p:", frag_info);
 
/* W0 */
-   plt_print("W0: f0.info \t0x%x", frag_info->w0.f0.info);
-   plt_print("W0: f1.info \t0x%x", frag_info->w0.f1.info);
-   plt_print("W0: f2.info \t0x%x", frag_info->w0.f2.info);
-   plt_print("W0: f3.info \t0x%x", frag_info->w0.f3.info);
+   cpt_dump(file, "W0: f0.info \t0x%x", frag_info->w0.f0.info);
+   cpt_dump(file, "W0: f1.info \t0x%x", frag_info->w0.f1.info);
+   cpt_dump(fi

[PATCH v2 1/5] vhost: fix virtqueue access check in datapath

[David Marchand]

Now that a r/w lock is used, the access_ok field should only be updated
under a write lock.

Since the datapath code only takes a read lock on the virtqueue to check
access_ok, this lock must be released and a write lock taken before
calling vring_translate().

Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
Reviewed-by: Maxime Coquelin 
---
 lib/vhost/virtio_net.c | 60 +++---
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 8af20f1487..d00f4b03aa 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1696,6 +1696,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
return pkt_idx;
 }
 
+static void
+virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
+{
+   rte_rwlock_write_lock(&vq->access_lock);
+   vhost_user_iotlb_rd_lock(vq);
+   if (!vq->access_ok)
+   vring_translate(dev, vq);
+   vhost_user_iotlb_rd_unlock(vq);
+   rte_rwlock_write_unlock(&vq->access_lock);
+}
+
 static __rte_always_inline uint32_t
 virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
struct rte_mbuf **pkts, uint32_t count)
@@ -1710,9 +1721,13 @@ virtio_dev_rx(struct virtio_net *dev, struct 
vhost_virtqueue *vq,
 
vhost_user_iotlb_rd_lock(vq);
 
-   if (unlikely(!vq->access_ok))
-   if (unlikely(vring_translate(dev, vq) < 0))
-   goto out;
+   if (unlikely(!vq->access_ok)) {
+   vhost_user_iotlb_rd_unlock(vq);
+   rte_rwlock_read_unlock(&vq->access_lock);
+
+   virtio_dev_vring_translate(dev, vq);
+   goto out_no_unlock;
+   }
 
count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
if (count == 0)
@@ -1731,6 +1746,7 @@ virtio_dev_rx(struct virtio_net *dev, struct 
vhost_virtqueue *vq,
 out_access_unlock:
rte_rwlock_read_unlock(&vq->access_lock);
 
+out_no_unlock:
return nb_tx;
 }
 
@@ -2528,9 +2544,13 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, 
struct vhost_virtqueue *vq,
 
vhost_user_iotlb_rd_lock(vq);
 
-   if (unlikely(!vq->access_ok))
-   if (unlikely(vring_translate(dev, vq) < 0))
-   goto out;
+   if (unlikely(!vq->access_ok)) {
+   vhost_user_iotlb_rd_unlock(vq);
+   rte_rwlock_read_unlock(&vq->access_lock);
+
+   virtio_dev_vring_translate(dev, vq);
+   goto out_no_unlock;
+   }
 
count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
if (count == 0)
@@ -2551,6 +2571,7 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct 
vhost_virtqueue *vq,
 out_access_unlock:
rte_rwlock_write_unlock(&vq->access_lock);
 
+out_no_unlock:
return nb_tx;
 }
 
@@ -3581,11 +3602,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 
vhost_user_iotlb_rd_lock(vq);
 
-   if (unlikely(!vq->access_ok))
-   if (unlikely(vring_translate(dev, vq) < 0)) {
-   count = 0;
-   goto out;
-   }
+   if (unlikely(!vq->access_ok)) {
+   vhost_user_iotlb_rd_unlock(vq);
+   rte_rwlock_read_unlock(&vq->access_lock);
+
+   virtio_dev_vring_translate(dev, vq);
+   goto out_no_unlock;
+   }
 
/*
 * Construct a RARP broadcast packet, and inject it to the "pkts"
@@ -3646,6 +3669,7 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
if (unlikely(rarp_mbuf != NULL))
count += 1;
 
+out_no_unlock:
return count;
 }
 
@@ -4196,11 +4220,14 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t 
queue_id,
 
vhost_user_iotlb_rd_lock(vq);
 
-   if (unlikely(vq->access_ok == 0))
-   if (unlikely(vring_translate(dev, vq) < 0)) {
-   count = 0;
-   goto out;
-   }
+   if (unlikely(vq->access_ok == 0)) {
+   vhost_user_iotlb_rd_unlock(vq);
+   rte_rwlock_read_unlock(&vq->access_lock);
+
+   virtio_dev_vring_translate(dev, vq);
+   count = 0;
+   goto out_no_unlock;
+   }
 
/*
 * Construct a RARP broadcast packet, and inject it to the "pkts"
@@ -4266,5 +4293,6 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t 
queue_id,
if (unlikely(rarp_mbuf != NULL))
count += 1;
 
+out_no_unlock:
return count;
 }
-- 
2.42.0

[PATCH 2/2] net/cnxk: dump Rx descriptor info to file

[Rakesh Kudurumalla]

Add support for eth_rx_descriptor_dump for cn9k and cn10k.
This patch dumps contents of receviced packet descriptor from CQ
for debug to file

Signed-off-by: Rakesh Kudurumalla 
---
 drivers/net/cnxk/cn10k_ethdev.c | 67 +
 drivers/net/cnxk/cn9k_ethdev.c  | 53 ++
 2 files changed, 120 insertions(+)

diff --git a/drivers/net/cnxk/cn10k_ethdev.c b/drivers/net/cnxk/cn10k_ethdev.c
index 4a4e97287c..7eee9b1da8 100644
--- a/drivers/net/cnxk/cn10k_ethdev.c
+++ b/drivers/net/cnxk/cn10k_ethdev.c
@@ -656,6 +656,72 @@ cn10k_nix_reassembly_conf_set(struct rte_eth_dev *eth_dev,
return rc;
 }
 
+static int
+cn10k_nix_rx_avail_get(struct cn10k_eth_rxq *rxq)
+{
+   uint32_t qmask = rxq->qmask;
+   uint64_t reg, head, tail;
+   int available;
+
+   /* Use LDADDA version to avoid reorder */
+   reg = roc_atomic64_add_sync(rxq->wdata, rxq->cq_status);
+   /* CQ_OP_STATUS operation error */
+   if (reg & BIT_ULL(NIX_CQ_OP_STAT_OP_ERR) ||
+   reg & BIT_ULL(NIX_CQ_OP_STAT_CQ_ERR))
+   return 0;
+   tail = reg & 0xF;
+   head = (reg >> 20) & 0xF;
+   if (tail < head)
+   available = tail - head + qmask + 1;
+   else
+   available = tail - head;
+
+   return available;
+}
+
+static int
+cn10k_rx_descriptor_dump(const struct rte_eth_dev *eth_dev, uint16_t qid,
+uint16_t offset, uint16_t num, FILE *file)
+{
+   struct cn10k_eth_rxq *rxq = eth_dev->data->rx_queues[qid];
+   const uint64_t data_off = rxq->data_off;
+   const uint32_t qmask = rxq->qmask;
+   const uintptr_t desc = rxq->desc;
+   struct cpt_parse_hdr_s *cpth;
+   uint32_t head = rxq->head;
+   struct nix_cqe_hdr_s *cq;
+   uint16_t count = 0;
+   int availble_pkts;
+   uint64_t cq_w1;
+
+   availble_pkts = cn10k_nix_rx_avail_get(rxq);
+
+   if ((offset + num - 1) >= availble_pkts) {
+   plt_err("Invalid BD num=%u\n", num);
+   return -EINVAL;
+   }
+
+   while (count < num) {
+   cq = (struct nix_cqe_hdr_s *)(desc + CQE_SZ(head) +
+ count + offset);
+   cq_w1 = *((const uint64_t *)cq + 1);
+   if (cq_w1 & BIT(11)) {
+   rte_iova_t buff = *((rte_iova_t *)((uint64_t *)cq + 9));
+   struct rte_mbuf *mbuf =
+   (struct rte_mbuf *)(buff - data_off);
+   cpth = (struct cpt_parse_hdr_s *)
+   ((uintptr_t)mbuf + (uint16_t)data_off);
+   roc_cpt_parse_hdr_dump(file, cpth);
+   } else {
+   roc_nix_cqe_dump(file, cq);
+   }
+
+   count++;
+   head &= qmask;
+   }
+   return 0;
+}
+
 static int
 cn10k_nix_tm_mark_vlan_dei(struct rte_eth_dev *eth_dev, int mark_green,
   int mark_yellow, int mark_red,
@@ -794,6 +860,7 @@ nix_eth_dev_ops_override(void)
cn10k_nix_reassembly_capability_get;
cnxk_eth_dev_ops.ip_reassembly_conf_get = cn10k_nix_reassembly_conf_get;
cnxk_eth_dev_ops.ip_reassembly_conf_set = cn10k_nix_reassembly_conf_set;
+   cnxk_eth_dev_ops.eth_rx_descriptor_dump = cn10k_rx_descriptor_dump;
 }
 
 /* Update platform specific tm ops */
diff --git a/drivers/net/cnxk/cn9k_ethdev.c b/drivers/net/cnxk/cn9k_ethdev.c
index bae4dda5e2..e88631a02e 100644
--- a/drivers/net/cnxk/cn9k_ethdev.c
+++ b/drivers/net/cnxk/cn9k_ethdev.c
@@ -664,6 +664,58 @@ cn9k_nix_tm_mark_ip_dscp(struct rte_eth_dev *eth_dev, int 
mark_green,
return rc;
 }
 
+static int
+cn9k_nix_rx_avail_get(struct cn9k_eth_rxq *rxq)
+{
+   uint32_t qmask = rxq->qmask;
+   uint64_t reg, head, tail;
+   int available;
+
+   /* Use LDADDA version to avoid reorder */
+   reg = roc_atomic64_add_sync(rxq->wdata, rxq->cq_status);
+   /* CQ_OP_STATUS operation error */
+   if (reg & BIT_ULL(NIX_CQ_OP_STAT_OP_ERR) ||
+   reg & BIT_ULL(NIX_CQ_OP_STAT_CQ_ERR))
+   return 0;
+   tail = reg & 0xF;
+   head = (reg >> 20) & 0xF;
+   if (tail < head)
+   available = tail - head + qmask + 1;
+   else
+   available = tail - head;
+
+   return available;
+}
+
+static int
+cn9k_rx_descriptor_dump(const struct rte_eth_dev *eth_dev, uint16_t qid,
+uint16_t offset, uint16_t num, FILE *file)
+{
+   struct cn9k_eth_rxq *rxq = eth_dev->data->rx_queues[qid];
+   const uint32_t qmask = rxq->qmask;
+   const uintptr_t desc = rxq->desc;
+   uint32_t head = rxq->head;
+   struct nix_cqe_hdr_s *cq;
+   uint16_t count = 0;
+   int availble_pkts;
+
+   availble_pkts = cn9k_nix_rx_avail_get(rxq);
+
+   if ((offset + num - 1) >= availble_pkts) {
+   plt_err("Invali

[PATCH v2 4/5] vhost: annotate virtqueue access checks

[David Marchand]

Modifying vq->access_ok should be done with a write lock taken.
Annotate vring_translate() and vring_invalidate().

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
---
Changes since v1:
- moved fixes in separate patches,

---
 lib/vhost/vhost.h  | 7 +--
 lib/vhost/vhost_user.c | 8 
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index f8624fba3d..6767246656 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -295,7 +295,8 @@ struct vhost_virtqueue {
 #define VIRTIO_UNINITIALIZED_EVENTFD   (-2)
 
boolenabled;
-   boolaccess_ok;
+   /* Protected by vq->access_lock */
+   boolaccess_ok __rte_guarded_var;
boolready;
 
rte_rwlock_taccess_lock;
@@ -875,11 +876,13 @@ void *vhost_alloc_copy_ind_table(struct virtio_net *dev,
uint64_t desc_addr, uint64_t desc_len)
__rte_shared_locks_required(&vq->iotlb_lock);
 int vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
+   __rte_exclusive_locks_required(&vq->access_lock)
__rte_shared_locks_required(&vq->iotlb_lock);
 uint64_t translate_log_addr(struct virtio_net *dev, struct vhost_virtqueue *vq,
uint64_t log_addr)
__rte_shared_locks_required(&vq->iotlb_lock);
-void vring_invalidate(struct virtio_net *dev, struct vhost_virtqueue *vq);
+void vring_invalidate(struct virtio_net *dev, struct vhost_virtqueue *vq)
+   __rte_exclusive_locks_required(&vq->access_lock);
 
 static __rte_always_inline uint64_t
 vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index a323ce5fbf..651ea5854b 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -787,6 +787,8 @@ translate_ring_addresses(struct virtio_net **pdev, struct 
vhost_virtqueue **pvq)
dev = *pdev;
vq = *pvq;
 
+   vq_assert_lock(dev, vq);
+
if (vq->ring_addrs.flags & (1 << VHOST_VRING_F_LOG)) {
vq->log_guest_addr =
log_addr_to_gpa(dev, vq);
@@ -924,6 +926,9 @@ vhost_user_set_vring_addr(struct virtio_net **pdev,
/* addr->index refers to the queue index. The txq 1, rxq is 0. */
vq = dev->virtqueue[ctx->msg.payload.addr.index];
 
+   /* vhost_user_lock_all_queue_pairs locked all qps */
+   vq_assert_lock(dev, vq);
+
access_ok = vq->access_ok;
 
/*
@@ -1436,6 +1441,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
continue;
 
if (vq->desc || vq->avail || vq->used) {
+   /* vhost_user_lock_all_queue_pairs locked all qps */
+   vq_assert_lock(dev, vq);
+
/*
 * If the memory table got updated, the ring addresses
 * need to be translated again as virtual addresses have
-- 
2.42.0

[PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup

[David Marchand]

vring_translate and vring_invalidate change the vq access_ok field.
The access_ok field should only be updated under a (write) lock.

Fixes: a9120db8b98b ("vhost: add VDUSE device startup")
Fixes: ad67c65efda1 ("vhost: add VDUSE device stop")
Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
---
Changes since v1:
- moved fix out of patch 3,

---
 lib/vhost/vduse.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 080b58f7de..e198eeef64 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -196,6 +196,7 @@ vduse_vring_setup(struct virtio_net *dev, unsigned int 
index)
vq->size * sizeof(struct batch_copy_elem),
RTE_CACHE_LINE_SIZE, 0);
 
+   rte_rwlock_write_lock(&vq->access_lock);
vhost_user_iotlb_rd_lock(vq);
if (vring_translate(dev, vq))
VHOST_LOG_CONFIG(dev->ifname, ERR, "Failed to translate vring 
%d addresses\n",
@@ -206,6 +207,7 @@ vduse_vring_setup(struct virtio_net *dev, unsigned int 
index)
"Failed to disable guest notifications on vring 
%d\n",
index);
vhost_user_iotlb_rd_unlock(vq);
+   rte_rwlock_write_unlock(&vq->access_lock);
 
vq_efd.index = index;
vq_efd.fd = vq->kickfd;
@@ -259,7 +261,9 @@ vduse_vring_cleanup(struct virtio_net *dev, unsigned int 
index)
close(vq->kickfd);
vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
 
+   rte_rwlock_write_lock(&vq->access_lock);
vring_invalidate(dev, vq);
+   rte_rwlock_write_unlock(&vq->access_lock);
 
rte_free(vq->batch_copy_elems);
vq->batch_copy_elems = NULL;
-- 
2.42.0

[PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup

[David Marchand]

Calling vring_invalidate must be done with a (write) lock taken on the
virtqueue.

Fixes: 72d002b3ebda ("vhost: fix vring address handling during live migration")
Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
---
Changes since v1:
- moved fix out of patch 3,

---
 lib/vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index e36312181a..a323ce5fbf 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2198,7 +2198,9 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 
vhost_user_iotlb_flush_all(dev);
 
+   rte_rwlock_write_lock(&vq->access_lock);
vring_invalidate(dev, vq);
+   rte_rwlock_write_unlock(&vq->access_lock);
 
return RTE_VHOST_MSG_RESULT_REPLY;
 }
-- 
2.42.0

[PATCH v2 5/5] vhost: enhance virtqueue access lock asserts

[David Marchand]

A simple comment in vhost_user_msg_handler() is not that robust.

Add a lock_all_qps property to message handlers so that their
implementation can add a build check and assert a vq is locked.

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
Reviewed-by: Maxime Coquelin 
---
Changes since v1:
- moved this patch as the last of the series,

---
 lib/vhost/vhost_user.c | 114 +++--
 1 file changed, 53 insertions(+), 61 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 651ea5854b..e8020ccfd3 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -56,14 +56,24 @@
 #define INFLIGHT_ALIGNMENT 64
 #define INFLIGHT_VERSION   0x1
 
-typedef struct vhost_message_handler {
+typedef const struct vhost_message_handler {
const char *description;
int (*callback)(struct virtio_net **pdev, struct vhu_msg_context *ctx,
int main_fd);
bool accepts_fd;
+   bool lock_all_qps;
 } vhost_message_handler_t;
 static vhost_message_handler_t vhost_message_handlers[];
 
+/* vhost_user_msg_handler() locks all qps based on a handler's lock_all_qps.
+ * Later, a handler may need to ensure the vq has been locked (for example,
+ * when calling lock annotated helpers).
+ */
+#define VHOST_USER_ASSERT_LOCK(dev, vq, id) do { \
+   RTE_BUILD_BUG_ON(!vhost_message_handlers[id].lock_all_qps); \
+   vq_assert_lock(dev, vq); \
+} while (0)
+
 static int send_vhost_reply(struct virtio_net *dev, int sockfd, struct 
vhu_msg_context *ctx);
 static int read_vhost_message(struct virtio_net *dev, int sockfd, struct 
vhu_msg_context *ctx);
 
@@ -400,7 +410,7 @@ vhost_user_set_features(struct virtio_net **pdev,
cleanup_vq(vq, 1);
cleanup_vq_inflight(dev, vq);
/* vhost_user_lock_all_queue_pairs locked all qps */
-   vq_assert_lock(dev, vq);
+   VHOST_USER_ASSERT_LOCK(dev, vq, 
VHOST_USER_SET_FEATURES);
rte_rwlock_write_unlock(&vq->access_lock);
free_vq(dev, vq);
}
@@ -927,7 +937,7 @@ vhost_user_set_vring_addr(struct virtio_net **pdev,
vq = dev->virtqueue[ctx->msg.payload.addr.index];
 
/* vhost_user_lock_all_queue_pairs locked all qps */
-   vq_assert_lock(dev, vq);
+   VHOST_USER_ASSERT_LOCK(dev, vq, VHOST_USER_SET_VRING_ADDR);
 
access_ok = vq->access_ok;
 
@@ -1442,7 +1452,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
 
if (vq->desc || vq->avail || vq->used) {
/* vhost_user_lock_all_queue_pairs locked all qps */
-   vq_assert_lock(dev, vq);
+   VHOST_USER_ASSERT_LOCK(dev, vq, 
VHOST_USER_SET_MEM_TABLE);
 
/*
 * If the memory table got updated, the ring addresses
@@ -2234,7 +2244,7 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
vq = dev->virtqueue[index];
if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
/* vhost_user_lock_all_queue_pairs locked all qps */
-   vq_assert_lock(dev, vq);
+   VHOST_USER_ASSERT_LOCK(dev, vq, VHOST_USER_SET_VRING_ENABLE);
if (enable && vq->async && vq->async->pkts_inflight_n) {
VHOST_LOG_CONFIG(dev->ifname, ERR,
"failed to enable vring. Inflight packets must 
be completed first\n");
@@ -2839,41 +2849,43 @@ vhost_user_set_status(struct virtio_net **pdev,
 }
 
 #define VHOST_MESSAGE_HANDLERS \
-VHOST_MESSAGE_HANDLER(VHOST_USER_NONE, NULL, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_FEATURES, vhost_user_get_features, false) 
\
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_FEATURES, vhost_user_set_features, false) 
\
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_OWNER, vhost_user_set_owner, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_RESET_OWNER, vhost_user_reset_owner, false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_MEM_TABLE, vhost_user_set_mem_table, 
true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_BASE, vhost_user_set_log_base, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_FD, vhost_user_set_log_fd, true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_NUM, vhost_user_set_vring_num, 
false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ADDR, vhost_user_set_vring_addr, 
false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_BASE, vhost_user_set_vring_base, 
false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_VRING_BASE, vhost_user_get_vring_base, 
false) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_KICK, vhost_user_set_vring_kick, 
true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_CALL, vhost_user_set_vring_call, 
true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ERR, vhost_user_set_vring_err, 
true) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_PROTOCOL_FEATURES, 
vhost_user_get_protocol_features, false) \
-VHOST_MESSAGE_HANDLER(

Re: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Bruce Richardson]

On Tue, Dec 05, 2023 at 09:04:08AM +0100, Morten Brørup wrote:
> > From: Feifei Wang [mailto:feifei.wa...@arm.com]
> > Sent: Tuesday, 5 December 2023 04.13
> > 
> > 在 2023/12/4 15:41, Morten Brørup 写道:
> > >> From: Feifei Wang [mailto:feifei.wa...@arm.com]
> > >> Sent: Monday, 4 December 2023 03.39
> > >>
> > >> For 'rte_pktmbuf_prefree_seg' function, 'rte_mbuf_refcnt_read(m) ==
> > 1'
> > >> and '__rte_mbuf_refcnt_update(m, -1) == 0' are the same cases where
> > >> mbuf's refcnt value should be 1. Thus we can simplify the code and
> > >> remove the redundant part.
> > >>
> > >> Furthermore, according to [1], when the mbuf is stored inside the
> > >> mempool, the m->refcnt value should be 1. And then it is detached
> > >> from its parent for an indirect mbuf. Thus change the description of
> > >> 'rte_pktmbuf_prefree_seg' function.
> > >>
> > >> [1]
> > https://patches.dpdk.org/project/dpdk/patch/20170404162807.20157-4-
> > >> olivier.m...@6wind.com/
> > >>
> > >> Suggested-by: Ruifeng Wang 
> > >> Signed-off-by: Feifei Wang 
> > >> ---
> > >>   lib/mbuf/rte_mbuf.h | 22 +++---
> > >>   1 file changed, 3 insertions(+), 19 deletions(-)
> > >>
> > >> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > >> index 286b32b788..42e9b50d51 100644
> > >> --- a/lib/mbuf/rte_mbuf.h
> > >> +++ b/lib/mbuf/rte_mbuf.h
> > >> @@ -1328,7 +1328,7 @@ static inline int
> > >> __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> > >>*
> > >>* This function does the same than a free, except that it does
> > not
> > >>* return the segment to its pool.
> > >> - * It decreases the reference counter, and if it reaches 0, it is
> > >> + * It decreases the reference counter, and if it reaches 1, it is
> > > No, the original description is correct.
> > > However, the reference counter is set to 1 when put back in the pool,
> > as a shortcut so it isn't needed to be set back to 1 when allocated
> > from the pool.
> > 
> > Ok.
> > 
> > for 'else if (__rte_mbuf_refcnt_update(m, -1) == 0)' case, it is easy
> > to
> > understand.
> > 
> > but for '(likely(rte_mbuf_refcnt_read(m) == 1))' case, I think this
> > will
> > create misleading. dpdk users maybe difficult to understand why the
> > code
> > can not match the function description.
> > 
> > Maybe we need some explanation here?
> 
> Agree. It is quite counterintuitive (but a clever optimization!) that the 
> reference counter is 1 instead of 0 when free.
> 
> Something like:
> 
> static __rte_always_inline struct rte_mbuf *
> rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
> {
>   __rte_mbuf_sanity_check(m, 0);
> 
>   if (likely(rte_mbuf_refcnt_read(m) == 1)) {
> + /* This branch is a performance optimized variant of the branch 
> below.
> +  * If the reference counter would reach 0 when decrementing it,
> +  * do not decrement it to 0 and then initialize it to 1;
> +  * just leave it at 1, thereby avoiding writing to memory.
> +  */
>
Even more than avoiding writing to memory, we know that with a refcount of
1, this thread is the only thread using this mbuf, so we don't need to use
atomic operations at all. The decrement we avoid is an especially expensive
one because of the atomic aspect of it.

/Bruce

Re: [dpdk-dev] [PATCH] common/cnxk: add ROC API to get MKEX capability

[Jerin Jacob]

On Wed, Nov 8, 2023 at 9:03 AM  wrote:
>
> From: Satheesh Paul 
>
> Added ROC API to get MKEX capability.
>
> Signed-off-by: Satheesh Paul 
> Reviewed-by: Kiran Kumar K 

Applied to dpdk-next-net-mrvl/for-main. Thanks


> ---
>  drivers/common/cnxk/roc_npc.c   | 24 
>  drivers/common/cnxk/roc_npc.h   |  1 +
>  drivers/common/cnxk/version.map |  1 +
>  3 files changed, 26 insertions(+)
>
> diff --git a/drivers/common/cnxk/roc_npc.c b/drivers/common/cnxk/roc_npc.c
> index a0d88c0743..1958b3089d 100644
> --- a/drivers/common/cnxk/roc_npc.c
> +++ b/drivers/common/cnxk/roc_npc.c
> @@ -235,6 +235,30 @@ roc_npc_profile_name_get(struct roc_npc *roc_npc)
> return (char *)npc->profile_name;
>  }
>
> +int
> +roc_npc_kex_capa_get(struct roc_nix *roc_nix, uint64_t *kex_capability)
> +{
> +   struct nix *nix = roc_nix_to_nix_priv(roc_nix);
> +   struct npc npc;
> +   int rc = 0;
> +
> +   memset(&npc, 0, sizeof(npc));
> +
> +   npc.mbox = (&nix->dev)->mbox;
> +
> +   rc = npc_mcam_fetch_kex_cfg(&npc);
> +   if (rc)
> +   return rc;
> +
> +   rc = npc_mcam_fetch_hw_cap(&npc, &npc.hash_extract_cap);
> +   if (rc)
> +   return rc;
> +
> +   *kex_capability = npc_get_kex_capability(&npc);
> +
> +   return 0;
> +}
> +
>  int
>  roc_npc_init(struct roc_npc *roc_npc)
>  {
> diff --git a/drivers/common/cnxk/roc_npc.h b/drivers/common/cnxk/roc_npc.h
> index b71ddd1578..459fa33de9 100644
> --- a/drivers/common/cnxk/roc_npc.h
> +++ b/drivers/common/cnxk/roc_npc.h
> @@ -399,6 +399,7 @@ struct roc_npc {
>  int __roc_api roc_npc_init(struct roc_npc *roc_npc);
>  int __roc_api roc_npc_fini(struct roc_npc *roc_npc);
>  const char *__roc_api roc_npc_profile_name_get(struct roc_npc *roc_npc);
> +int __roc_api roc_npc_kex_capa_get(struct roc_nix *roc_nix, uint64_t 
> *kex_capability);
>
>  struct roc_npc_flow *__roc_api roc_npc_flow_create(struct roc_npc *roc_npc,
>const struct roc_npc_attr 
> *attr,
> diff --git a/drivers/common/cnxk/version.map b/drivers/common/cnxk/version.map
> index aa884a8fe2..1c7c65c51a 100644
> --- a/drivers/common/cnxk/version.map
> +++ b/drivers/common/cnxk/version.map
> @@ -438,6 +438,7 @@ INTERNAL {
> roc_npc_flow_parse;
> roc_npc_get_low_priority_mcam;
> roc_npc_init;
> +   roc_npc_kex_capa_get;
> roc_npc_mark_actions_get;
> roc_npc_mark_actions_sub_return;
> roc_npc_vtag_actions_get;
> --
> 2.39.2
>

Re: mbuf init questions

[Bruce Richardson]

On Tue, Dec 05, 2023 at 10:07:56AM +0100, Morten Brørup wrote:
> Why is m->nb_segs initialized in rte_pktmbuf_prefree_seg()?
> 
> It's part of the m->rearm_data, and will be initialized on RX descriptor 
> rearm anyway.
> 
Presumably this is to have a sane default for apps that allocate buffers
directly using mempool_get functions.

Overall, we probably need to document clearly for mbufs allocated using
mempool apis rather than mbuf_alloc what fields are valid and what need to
be initialized.

/Bruce

Re: [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup

[Maxime Coquelin]

On 12/5/23 10:45, David Marchand wrote:

vring_translate and vring_invalidate change the vq access_ok field.
The access_ok field should only be updated under a (write) lock.

Fixes: a9120db8b98b ("vhost: add VDUSE device startup")
Fixes: ad67c65efda1 ("vhost: add VDUSE device stop")
Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
---
Changes since v1:
- moved fix out of patch 3,

---
  lib/vhost/vduse.c | 4 
  1 file changed, 4 insertions(+)



Reviewed-by: Maxime Coquelin 

Thanks,
Maxime

Re: [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup

[Maxime Coquelin]

On 12/5/23 10:45, David Marchand wrote:

Calling vring_invalidate must be done with a (write) lock taken on the
virtqueue.

Fixes: 72d002b3ebda ("vhost: fix vring address handling during live migration")
Cc: sta...@dpdk.org

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
---
Changes since v1:
- moved fix out of patch 3,

---
  lib/vhost/vhost_user.c | 2 ++
  1 file changed, 2 insertions(+)


Reviewed-by: Maxime Coquelin 

Thanks,
Maxime

[PATCH 1/3] common/cnxk: optimize ethdev teardown time

[Rakesh Kudurumalla]

API mbox_alloc_msg_npa_aq_enq() mbox is called if SQ needs
to be updated from mbox response else mbox call to kernel
is bypassed reducing the time taken to complete
roc_nix_tm_sq_aura_fc() function.This reduces ethdev
teardown time by 20%.

Signed-off-by: Rakesh Kudurumalla 
---
 drivers/common/cnxk/roc_nix_tm_ops.c | 29 ++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/common/cnxk/roc_nix_tm_ops.c 
b/drivers/common/cnxk/roc_nix_tm_ops.c
index e1cef7a670..2c53472047 100644
--- a/drivers/common/cnxk/roc_nix_tm_ops.c
+++ b/drivers/common/cnxk/roc_nix_tm_ops.c
@@ -51,25 +51,26 @@ roc_nix_tm_sq_aura_fc(struct roc_nix_sq *sq, bool enable)
goto exit;
 
/* Read back npa aura ctx */
-   req = mbox_alloc_msg_npa_aq_enq(mbox);
-   if (req == NULL) {
-   rc = -ENOSPC;
-   goto exit;
-   }
+   if (enable) {
+   req = mbox_alloc_msg_npa_aq_enq(mbox);
+   if (req == NULL) {
+   rc = -ENOSPC;
+   goto exit;
+   }
 
-   req->aura_id = roc_npa_aura_handle_to_aura(aura_handle);
-   req->ctype = NPA_AQ_CTYPE_AURA;
-   req->op = NPA_AQ_INSTOP_READ;
+   req->aura_id = roc_npa_aura_handle_to_aura(aura_handle);
+   req->ctype = NPA_AQ_CTYPE_AURA;
+   req->op = NPA_AQ_INSTOP_READ;
 
-   rc = mbox_process_msg(mbox, (void *)&rsp);
-   if (rc)
-   goto exit;
+   rc = mbox_process_msg(mbox, (void *)&rsp);
+   if (rc)
+   goto exit;
 
-   /* Init when enabled as there might be no triggers */
-   if (enable)
+   /* Init when enabled as there might be no triggers */
*(volatile uint64_t *)sq->fc = rsp->aura.count;
-   else
+   } else {
*(volatile uint64_t *)sq->fc = sq->aura_sqb_bufs;
+   }
/* Sync write barrier */
plt_wmb();
rc = 0;
-- 
2.25.1

[PATCH 2/3] common/cnxk: added new API to enable disable SQ

[Rakesh Kudurumalla]

Added a new roc API to disable SQB aura FC
and update SQ state to disabled state in TX queue
stop.The same SQ status is verified during sq flush
to enable or disable SQB aura FC during ethdev
teardown.This fix reduces teardown time by 90%.

Signed-off-by: Rakesh Kudurumalla 
---
 drivers/common/cnxk/roc_nix.h   |  2 ++
 drivers/common/cnxk/roc_nix_queue.c | 15 +++
 drivers/common/cnxk/roc_nix_tm.c| 20 
 drivers/common/cnxk/version.map |  1 +
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/common/cnxk/roc_nix.h b/drivers/common/cnxk/roc_nix.h
index a96cf73757..82997c38ce 100644
--- a/drivers/common/cnxk/roc_nix.h
+++ b/drivers/common/cnxk/roc_nix.h
@@ -401,6 +401,7 @@ struct roc_nix_sq {
void *sqe_mem;
void *fc;
uint8_t tc;
+   bool enable;
 };
 
 struct roc_nix_link_info {
@@ -952,6 +953,7 @@ void __roc_api roc_nix_cq_head_tail_get(struct roc_nix 
*roc_nix, uint16_t qid,
uint32_t *head, uint32_t *tail);
 int __roc_api roc_nix_sq_init(struct roc_nix *roc_nix, struct roc_nix_sq *sq);
 int __roc_api roc_nix_sq_fini(struct roc_nix_sq *sq);
+int __roc_api roc_nix_sq_ena_dis(struct roc_nix_sq *sq, bool enable);
 void __roc_api roc_nix_sq_head_tail_get(struct roc_nix *roc_nix, uint16_t qid,
uint32_t *head, uint32_t *tail);
 
diff --git a/drivers/common/cnxk/roc_nix_queue.c 
b/drivers/common/cnxk/roc_nix_queue.c
index f96d5c3a96..ae4e0ea40c 100644
--- a/drivers/common/cnxk/roc_nix_queue.c
+++ b/drivers/common/cnxk/roc_nix_queue.c
@@ -92,6 +92,20 @@ nix_rq_ena_dis(struct dev *dev, struct roc_nix_rq *rq, bool 
enable)
return rc;
 }
 
+int
+roc_nix_sq_ena_dis(struct roc_nix_sq *sq, bool enable)
+{
+   int rc = 0;
+
+   rc = roc_nix_tm_sq_aura_fc(sq, enable);
+   if (rc)
+   goto done;
+
+   sq->enable = enable;
+done:
+   return rc;
+}
+
 int
 roc_nix_rq_ena_dis(struct roc_nix_rq *rq, bool enable)
 {
@@ -1409,6 +1423,7 @@ roc_nix_sq_init(struct roc_nix *roc_nix, struct 
roc_nix_sq *sq)
}
mbox_put(mbox);
 
+   sq->enable = true;
nix->sqs[qid] = sq;
sq->io_addr = nix->base + NIX_LF_OP_SENDX(0);
/* Evenly distribute LMT slot for each sq */
diff --git a/drivers/common/cnxk/roc_nix_tm.c b/drivers/common/cnxk/roc_nix_tm.c
index ece88b5e99..6a61e448a1 100644
--- a/drivers/common/cnxk/roc_nix_tm.c
+++ b/drivers/common/cnxk/roc_nix_tm.c
@@ -887,10 +887,12 @@ nix_tm_sq_flush_pre(struct roc_nix_sq *sq)
if (!sq)
continue;
 
-   rc = roc_nix_tm_sq_aura_fc(sq, false);
-   if (rc) {
-   plt_err("Failed to disable sqb aura fc, rc=%d", rc);
-   goto cleanup;
+   if (sq->enable) {
+   rc = roc_nix_tm_sq_aura_fc(sq, false);
+   if (rc) {
+   plt_err("Failed to disable sqb aura fc, rc=%d", 
rc);
+   goto cleanup;
+   }
}
 
/* Wait for sq entries to be flushed */
@@ -997,10 +999,12 @@ nix_tm_sq_flush_post(struct roc_nix_sq *sq)
once = true;
}
 
-   rc = roc_nix_tm_sq_aura_fc(s_sq, true);
-   if (rc) {
-   plt_err("Failed to enable sqb aura fc, rc=%d", rc);
-   return rc;
+   if (s_sq->enable) {
+   rc = roc_nix_tm_sq_aura_fc(s_sq, true);
+   if (rc) {
+   plt_err("Failed to enable sqb aura fc, rc=%d", 
rc);
+   return rc;
+   }
}
}
 
diff --git a/drivers/common/cnxk/version.map b/drivers/common/cnxk/version.map
index aa884a8fe2..4907b62013 100644
--- a/drivers/common/cnxk/version.map
+++ b/drivers/common/cnxk/version.map
@@ -339,6 +339,7 @@ INTERNAL {
roc_nix_rx_queue_intr_disable;
roc_nix_rx_queue_intr_enable;
roc_nix_sq_dump;
+   roc_nix_sq_ena_dis;
roc_nix_sq_fini;
roc_nix_sq_head_tail_get;
roc_nix_sq_init;
-- 
2.25.1

[PATCH 3/3] net/cnxk: reduce Tx queue release time

[Rakesh Kudurumalla]

Invoked newly added roc API to disable
SQB aura FC during TX queue start and TX queue
stop. This fix reduces ethdev teardown time

Signed-off-by: Rakesh Kudurumalla 
---
 drivers/net/cnxk/cnxk_ethdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/cnxk/cnxk_ethdev.c b/drivers/net/cnxk/cnxk_ethdev.c
index 5e11bbb017..2372a4e793 100644
--- a/drivers/net/cnxk/cnxk_ethdev.c
+++ b/drivers/net/cnxk/cnxk_ethdev.c
@@ -1521,7 +1521,7 @@ cnxk_nix_tx_queue_start(struct rte_eth_dev *eth_dev, 
uint16_t qid)
if (data->tx_queue_state[qid] == RTE_ETH_QUEUE_STATE_STARTED)
return 0;
 
-   rc = roc_nix_tm_sq_aura_fc(sq, true);
+   rc = roc_nix_sq_ena_dis(sq, true);
if (rc) {
plt_err("Failed to enable sq aura fc, txq=%u, rc=%d", qid, rc);
goto done;
@@ -1543,7 +1543,7 @@ cnxk_nix_tx_queue_stop(struct rte_eth_dev *eth_dev, 
uint16_t qid)
if (data->tx_queue_state[qid] == RTE_ETH_QUEUE_STATE_STOPPED)
return 0;
 
-   rc = roc_nix_tm_sq_aura_fc(sq, false);
+   rc = roc_nix_sq_ena_dis(sq, false);
if (rc) {
plt_err("Failed to disable sqb aura fc, txq=%u, rc=%d", qid,
rc);
-- 
2.25.1

RE: mbuf init questions

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richard...@intel.com]
> Sent: Tuesday, 5 December 2023 10.56
> 
> On Tue, Dec 05, 2023 at 10:07:56AM +0100, Morten Brørup wrote:
> > Why is m->nb_segs initialized in rte_pktmbuf_prefree_seg()?
> >
> > It's part of the m->rearm_data, and will be initialized on RX
> descriptor rearm anyway.
> >
> Presumably this is to have a sane default for apps that allocate
> buffers
> directly using mempool_get functions.

Ahh, yes... the great bypassing of the mbuf APIs!

> 
> Overall, we probably need to document clearly for mbufs allocated using
> mempool apis rather than mbuf_alloc what fields are valid and what need
> to
> be initialized.

Good point. Since it has become customary - and generally accepted practice - 
to bypass the mbuf APIs and use the mempool APIs directly, we should document 
how to use the mempool APIs in an mbuf API context.

Spaghetti code, but everyone loves the performance benefits by doing it. 
Including myself. ;-)

RE: [v2] net/af_xdp: enable a sock path alongside use_cni

[Koikkara Reeny, Shibin]

Hi Maryam,

Apologies for making it confusing.

As per the afxdp-plugins-for-kubernetes code it looks like the udsPodPath or 
udsSockDir[1] and udsPodSock[1] are constant and not changing. Only the 
interface name is changing and we are already passing the interface name 
through the command line. So I was suggesting we can write a logic to create 
sock path from these logic.

If I am wrong please correct me, Isn’t that the logic 
afxdp-plugins-for-kubernetes doing? [2]
This is only a suggestions.

[1] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/constants/constants.go#L84
[2] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/deviceplugin/poolManager_test.go#L99

I had added 2 small comment earlier in the code.


Regards,
Shibin
From: Maryam Tahhan 
Sent: Monday, December 4, 2023 6:41 PM
To: Koikkara Reeny, Shibin ; 
ferruh.yi...@amd.com; step...@networkplumber.org; lihuis...@huawei.com; 
fengcheng...@huawei.com; liuyongl...@huawei.com
Cc: dev@dpdk.org
Subject: Re: [v2] net/af_xdp: enable a sock path alongside use_cni

Hi Shibin

I'm not really sure what you are suggesting, is to make an assumption on the 
path part where the socket resides (aka hard code it) and then try to build the 
full UDS path in DPDK?

Yes the plugin is using constants ATM for certain parts of the UDS path, but 
that's not say that it's something that won't become configurable later on. 
Someone may not want to use "/tmp/afxdp_dp/" as the base directory. Then we'd 
have to change DPDK's implementation again. These are not really things that 
are configured by hand and are generated by initialization scripts (typically). 
I would rather build this with the idea that things can change in the future 
without having to change the DPDK implementation again.
BR
Maryam

On 04/12/2023 17:18, Koikkara Reeny, Shibin wrote:

Hi Maryam,



Apologies for asking this question bit late.

The UDS sock name will be afxdp.sock only and addition director is created 
between the sock name and the uds filepath (/tmp/afxdp_dp//afxdp.sock).



As per the command " --vdev net_af_xdp0,iface=,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock"

We are already passing the interface name(iface= . So can't we 
create the uds_path inside the program uds_path="/tmp/afxdp_dp/"+ iface + 
"afxdp.sock"





If you check the code afxdp-plugins-for-kubernetes constants.go [1] they still 
have the constants and also they are using these constants to create the path 
[2]



[1] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/constants/constants.go#L84

[2] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/deviceplugin/poolManager_test.go#L78



If we are able to create path in the program then user won't have to pass along 
argument value.



Regards,

Shibin



-Original Message-

From: Maryam Tahhan 

Sent: Monday, December 4, 2023 10:31 AM

To: ferruh.yi...@amd.com; 
step...@networkplumber.org;

lihuis...@huawei.com; 
fengcheng...@huawei.com;

liuyongl...@huawei.com; Koikkara Reeny, Shibin



Cc: dev@dpdk.org; Tahhan, Maryam 


Subject: [v2] net/af_xdp: enable a sock path alongside use_cni



With the original 'use_cni' implementation, (using a hardcoded socket rather

than a configurable one), if a single pod is requesting multiple net devices

and these devices are from different pools, then the container attempts to

mount all the netdev UDSes in the pod as /tmp/afxdp.sock. Which means

that at best only 1 netdev will handshake correctly with the AF_XDP DP. This

patch addresses this by making the socket parameter configurable alongside

the 'use_cni' param.

Tested with the AF_XDP DP CNI PR 81.



v2:

* Rename sock_path to uds_path.

* Update documentation to reflect when CAP_BPF is needed.

* Fix testpmd arguments in the provided example for Pods.

* Use AF_XDP API to update the xskmap entry.



Signed-off-by: Maryam Tahhan 

---

 doc/guides/howto/af_xdp_cni.rst | 24 ++-

 drivers/net/af_xdp/rte_eth_af_xdp.c | 62 ++---

 2 files changed, 54 insertions(+), 32 deletions(-)



diff --git a/doc/guides/howto/af_xdp_cni.rst

b/doc/guides/howto/af_xdp_cni.rst index a1a6d5b99c..7829526b40 100644

--- a/doc/guides/howto/af_xdp_cni.rst

+++ b/doc/guides/howto/af_xdp_cni.rst

@@ -38,9 +38,10 @@ The XSKMAP is a BPF map of AF_XDP sockets (XSK).

 The client can then proceed with creating an AF_XDP socket  and inserting

that socket into the XSKMAP pointed to by the descriptor.



-The EAL vdev argument ``use_cni`` is used to indicate that the user wishes -

to run the PMD in unprivileged mode and to receive the XSKMAP file

descriptor -from the CNI.

+The EAL v

RE: [PATCH] bus/vdev: fix devargs memory leak

[Ye, MingjinX]

Hi Burakov,

The patch has been sent for your review for quite some time. Can you please 
help to take a look and give some feedback?  Your prompt response will be 
appreciated a lot and very helpful for our next step moving forward.



-Mingjin

> -Original Message-
> From: Ye, MingjinX 
> Sent: 2023年11月17日 18:29
> To: Ye, MingjinX ; Burakov, Anatoly
> ; dev@dpdk.org
> Cc: Yang, Qiming ; Zhou, YidingX
> ; sta...@dpdk.org; Ling, WeiX
> 
> Subject: RE: [PATCH] bus/vdev: fix devargs memory leak
> 
> Hi Burakov,
> 
> can you please take a look at this patch.
> 
> Thanks,
> Mingjin
> 
> > > -Original Message-
> > > From: Ling, WeiX 
> > > Sent: Tuesday, September 12, 2023 5:08 PM
> > > To: Ye, MingjinX ; dev@dpdk.org
> > > Cc: Yang, Qiming ; Zhou, YidingX
> > > ; Ye, MingjinX ;
> > > sta...@dpdk.org; Burakov, Anatoly 
> > > Subject: RE: [PATCH] bus/vdev: fix devargs memory leak
> > >
> > > > -Original Message-
> > > > From: Mingjin Ye 
> > > > Sent: Friday, September 1, 2023 3:24 PM
> > > > To: dev@dpdk.org
> > > > Cc: Yang, Qiming ; Zhou, YidingX
> > > > ; Ye, MingjinX ;
> > > > sta...@dpdk.org; Burakov, Anatoly 
> > > > Subject: [PATCH] bus/vdev: fix devargs memory leak
> > > >
> > > > When a device is created by a secondary process, an empty devargs
> > > > is temporarily generated and bound to it. This causes the device
> > > > to not be associated with the correct devargs, and the empty
> > > > devargs are not released when the resource is freed.
> > > >
> > > > This patch fixes the issue by matching the devargs when inserting
> > > > a device in secondary process.
> > > >
> > > > Fixes: dda987315ca2 ("vdev: make virtual bus use its device
> > > > struct")
> > > > Fixes: a16040453968 ("eal: extract vdev infra")
> > > > Cc: sta...@dpdk.org
> > > >
> > > > Signed-off-by: Mingjin Ye 
> > > > ---
> > >
> > > Tested-by: Wei Ling 

[PATCH] net/iavf: fix no polling mode switch

[Mingjin Ye]

PMD does not switch to no polling mode when the PF triggers a reset event
or the watchdog detects a reset event. In this scenario, data path will
access the freed resources and cause a core dump.

This patch fixes this issue by automatically switching modes on VF reset.

Fixes: 5b3124a0a6ef ("net/iavf: support no polling when link down")
Cc: sta...@dpdk.org

Signed-off-by: Mingjin Ye 
---
 drivers/net/iavf/iavf.h|  1 +
 drivers/net/iavf/iavf_ethdev.c | 27 +++
 drivers/net/iavf/iavf_vchnl.c  | 24 ++--
 3 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
index 10868f2c30..18d39ee652 100644
--- a/drivers/net/iavf/iavf.h
+++ b/drivers/net/iavf/iavf.h
@@ -512,4 +512,5 @@ int iavf_flow_sub_check(struct iavf_adapter *adapter,
 void iavf_dev_watchdog_enable(struct iavf_adapter *adapter);
 void iavf_dev_watchdog_disable(struct iavf_adapter *adapter);
 int iavf_handle_hw_reset(struct rte_eth_dev *dev);
+void iavf_set_no_poll(struct iavf_adapter *adapter, bool link_change);
 #endif /* _IAVF_ETHDEV_H_ */
diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index d1edb0dd5c..0952998304 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -296,6 +296,7 @@ iavf_dev_watchdog(void *cb_arg)
PMD_DRV_LOG(INFO, "VF \"%s\" reset has completed",
adapter->vf.eth_dev->data->name);
adapter->vf.vf_reset = false;
+   iavf_set_no_poll(adapter, false);
}
/* If not in reset then poll vfr_inprogress register for VFLR event */
} else {
@@ -308,6 +309,7 @@ iavf_dev_watchdog(void *cb_arg)
 
/* enter reset state with VFLR event */
adapter->vf.vf_reset = true;
+   iavf_set_no_poll(adapter, false);
adapter->vf.link_up = false;
 
iavf_dev_event_post(adapter->vf.eth_dev, 
RTE_ETH_EVENT_INTR_RESET,
@@ -2916,8 +2918,10 @@ iavf_dev_close(struct rte_eth_dev *dev)
 * effect.
 */
 out:
-   if (vf->vf_reset && !rte_pci_set_bus_master(pci_dev, true))
+   if (vf->vf_reset && !rte_pci_set_bus_master(pci_dev, true)) {
vf->vf_reset = false;
+   iavf_set_no_poll(adapter, false);
+   }
 
/* disable watchdog */
iavf_dev_watchdog_disable(adapter);
@@ -2948,6 +2952,8 @@ static int
 iavf_dev_reset(struct rte_eth_dev *dev)
 {
int ret;
+   struct iavf_adapter *adapter =
+   IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
struct iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(dev->data->dev_private);
struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 
@@ -2962,6 +2968,7 @@ iavf_dev_reset(struct rte_eth_dev *dev)
return ret;
}
vf->vf_reset = false;
+   iavf_set_no_poll(adapter, false);
 
PMD_DRV_LOG(DEBUG, "Start dev_reset ...\n");
ret = iavf_dev_uninit(dev);
@@ -2977,10 +2984,13 @@ iavf_dev_reset(struct rte_eth_dev *dev)
 int
 iavf_handle_hw_reset(struct rte_eth_dev *dev)
 {
+   struct iavf_adapter *adapter =
+   IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
int ret;
 
vf->in_reset_recovery = true;
+   iavf_set_no_poll(adapter, false);
 
ret = iavf_dev_reset(dev);
if (ret)
@@ -2998,16 +3008,25 @@ iavf_handle_hw_reset(struct rte_eth_dev *dev)
if (ret)
goto error;
dev->data->dev_started = 1;
-
-   vf->in_reset_recovery = false;
-   return 0;
+   goto exit;
 
 error:
PMD_DRV_LOG(DEBUG, "RESET recover with error code=%d\n", ret);
+exit:
vf->in_reset_recovery = false;
+   iavf_set_no_poll(adapter, false);
return ret;
 }
 
+void
+iavf_set_no_poll(struct iavf_adapter *adapter, bool link_change)
+{
+   struct iavf_info *vf = &adapter->vf;
+
+   adapter->no_poll = (link_change & !vf->link_up) ||
+   vf->vf_reset || vf->in_reset_recovery;
+}
+
 static int
 iavf_dcf_cap_check_handler(__rte_unused const char *key,
   const char *value, __rte_unused void *opaque)
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 0a3e1d082c..d30f57 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -273,20 +273,18 @@ iavf_read_msg_from_pf(struct iavf_adapter *adapter, 
uint16_t buf_len,
iavf_dev_watchdog_enable(adapter);
}
if (adapter->devargs.no_poll_on_link_down) {
-   if (vf->link_up && adapter->no_poll) {
-   adapter->no_poll = false;
-

Re: 0001-vhost-optimize-vhost-user-get-protocol-features

[yuanzhiyuan0...@outlook.com]

From 4cf72842a07b2270876939fd2bb2367efaad95f4 Mon Sep 17 00:00:00 2001
From: Yuan Zhiyuan 
Date: Fri, 1 Dec 2023 11:27:51 +
Subject: [PATCH] vhost: optimize vhost user get protocol features

variable features is unused in vhost_user_get_protocol_features.

Signed-off-by: Yuan Zhiyuan 
---
 lib/vhost/vhost_user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index e36312181a..3e737eaf12 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2243,9 +2243,8 @@ vhost_user_get_protocol_features(struct virtio_net **pdev,
  int main_fd __rte_unused)
 {
  struct virtio_net *dev = *pdev;
- uint64_t features, protocol_features;
+ uint64_t protocol_features;
 
- rte_vhost_driver_get_features(dev->ifname, &features);
  rte_vhost_driver_get_protocol_features(dev->ifname, &protocol_features);
 
  ctx->msg.payload.u64 = protocol_features;
-- 
2.34.1

Re: [PATCH v2 4/5] vhost: annotate virtqueue access checks

[Maxime Coquelin]

On 12/5/23 10:45, David Marchand wrote:

Modifying vq->access_ok should be done with a write lock taken.
Annotate vring_translate() and vring_invalidate().

Signed-off-by: David Marchand 
Acked-by: Eelco Chaudron 
---
Changes since v1:
- moved fixes in separate patches,

---
  lib/vhost/vhost.h  | 7 +--
  lib/vhost/vhost_user.c | 8 
  2 files changed, 13 insertions(+), 2 deletions(-)



Reviewed-by: Maxime Coquelin 

Thanks,
Maxime

Re: [PATCH 1/1] ml/cnxk: exclude caching run stats from xstats

[Jerin Jacob]

On Tue, Nov 28, 2023 at 10:17 PM Srikanth Yalavarthi
 wrote:
>
> From: Anup Prabhu 
>
> Exclude the hardware and firmware latency of model data
> caching run from xstats calculation.
>
> Fixes: 9cfad6c334f2 ("ml/cnxk: update device and model xstats functions")
>
> Signed-off-by: Anup Prabhu 
> Acked-by: Srikanth Yalavarthi 


Updated the git commit as follows and applied to
dpdk-next-net-mrvl/for-main. Thanks


ml/cnxk: fix xstats calculation

Exclude the hardware and firmware latency of model data
caching run from xstats calculation.

Fixes: 9cfad6c334f2 ("ml/cnxk: update device and model xstats functions")
Cc: sta...@dpdk.org

Signed-off-by: Anup Prabhu 
Acked-by: Srikanth Yalavarthi 

> ---
>  drivers/ml/cnxk/cn10k_ml_ops.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/ml/cnxk/cn10k_ml_ops.c b/drivers/ml/cnxk/cn10k_ml_ops.c
> index 7f7e5efceac..53700387335 100644
> --- a/drivers/ml/cnxk/cn10k_ml_ops.c
> +++ b/drivers/ml/cnxk/cn10k_ml_ops.c
> @@ -288,6 +288,7 @@ cn10k_ml_model_xstat_get(struct cnxk_ml_dev *cnxk_mldev, 
> struct cnxk_ml_layer *l
>  static int
>  cn10k_ml_cache_model_data(struct cnxk_ml_dev *cnxk_mldev, struct 
> cnxk_ml_layer *layer)
>  {
> +   struct cn10k_ml_layer_xstats *xstats;
> char str[RTE_MEMZONE_NAMESIZE];
> const struct plt_memzone *mz;
> uint64_t isize = 0;
> @@ -309,6 +310,16 @@ cn10k_ml_cache_model_data(struct cnxk_ml_dev 
> *cnxk_mldev, struct cnxk_ml_layer *
>   PLT_PTR_ADD(mz->addr, isize), 1);
> plt_memzone_free(mz);
>
> +   /* Reset sync xstats. */
> +   xstats = layer->glow.sync_xstats;
> +   xstats->hw_latency_tot = 0;
> +   xstats->hw_latency_min = UINT64_MAX;
> +   xstats->hw_latency_max = 0;
> +   xstats->fw_latency_tot = 0;
> +   xstats->fw_latency_min = UINT64_MAX;
> +   xstats->fw_latency_max = 0;
> +   xstats->dequeued_count = 0;
> +
> return ret;
>  }
>
> --
> 2.42.0
>

Re: [v2] net/af_xdp: enable a sock path alongside use_cni

[Maryam Tahhan]

Hi Shibin

As I've already explained in my previous email, they are constant ATM, 
however they will become configurable. I am implementing the Operator 
and it will make a lot of these "fixed" params configurable. My 
recommendation is not to try to generate the path in DPDK - as it's 
likely to be different in different k8s environments.


As I've also mentioned, the current patch means that I don't need to 
come back in 2 months and update DPDK to support n paths for the UDS 
(aka future-proofing).


Additionally - this is a side discussion as far as this patch goes. *The 
point of this patch is to fix the broken UDS behavior *and it has been 
tested in a full deployment scenario. @Shibin if you strongly feel that 
there's a better approach, then please go ahead, implement it, test it 
in a full deployment scenario and push it for review.


In general, allowing the AF_XDP params to be configurable rather than 
fixing/hardcoding anything in DPDK decouples the AF_XDP DP from DPDK so 
we don't have to keep coming back to make changes.


BR
Maryam


On 05/12/2023 10:29, Koikkara Reeny, Shibin wrote:


Hi Maryam,

Apologies for making it confusing.

As per the afxdp-plugins-for-kubernetes code it looks like the 
udsPodPath or udsSockDir[1] and udsPodSock[1] are constant and not 
changing. Only the interface name is changing and we are already 
passing the interface name through the command line. So I was 
suggesting we can write a logic to create sock path from these logic.


If I am wrong please correct me, Isn’t that the logic 
afxdp-plugins-for-kubernetes doing? [2]


This is only a suggestions.

[1] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/constants/constants.go#L84


[2] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/deviceplugin/poolManager_test.go#L99


I had added 2 small comment earlier in the code.

Regards,

Shibin

*From:*Maryam Tahhan 
*Sent:* Monday, December 4, 2023 6:41 PM
*To:* Koikkara Reeny, Shibin ; 
ferruh.yi...@amd.com; step...@networkplumber.org; 
lihuis...@huawei.com; fengcheng...@huawei.com; liuyongl...@huawei.com

*Cc:* dev@dpdk.org
*Subject:* Re: [v2] net/af_xdp: enable a sock path alongside use_cni

Hi Shibin

I'm not really sure what you are suggesting, is to make an assumption 
on the path part where the socket resides (aka hard code it) and then 
try to build the full UDS path in DPDK?


Yes the plugin is using constants ATM for certain parts of the UDS 
path, but that's not say that it's something that won't become 
configurable later on. Someone may not want to use "/tmp/afxdp_dp/" as 
the base directory. Then we'd have to change DPDK's implementation 
again. These are not really things that are configured by hand and are 
generated by initialization scripts (typically). I would rather build 
this with the idea that things can change in the future without having 
to change the DPDK implementation again.


BR
Maryam

On 04/12/2023 17:18, Koikkara Reeny, Shibin wrote:

Hi Maryam,

Apologies for asking this question bit late.

The UDS sock name will be afxdp.sock only and addition director is created 
between the sock name and the uds filepath (/tmp/afxdp_dp//afxdp.sock).

As per the command " --vdev net_af_xdp0,iface=,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock"

We are already passing the interface name(iface= . So can't we create the 
uds_path inside the program uds_path="/tmp/afxdp_dp/"+ iface + "afxdp.sock"

If you check the code afxdp-plugins-for-kubernetes constants.go [1] they 
still have the constants and also they are using these constants to create the 
path [2]

[1]https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/constants/constants.go#L84  



[2]https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/deviceplugin/poolManager_test.go#L78

If we are able to create path in the program then user won't have to pass 
along argument value.

Regards,

Shibin

-Original Message-

From: Maryam Tahhan  

Sent: Monday, December 4, 2023 10:31 AM

To:ferruh.yi...@amd.com;step...@networkplumber.org;

lihuis...@huawei.com;fengcheng...@huawei.com;

liuyongl...@huawei.com; Koikkara Reeny, Shibin

  


Cc:dev@dpdk.org; Tahhan, Maryam  


Subject: [v2] net/af_xdp: enable a sock path alongside use_cni

With the original 'use_cni' implementation, (using a hardcoded socket 
rather

than a configurable one), if a single pod is requesting multiple net 
devices

and these devices are from different pools, then the container attempts 
to

mount all the netdev UDSes in the pod as /tmp/afxdp.sock. Which means

that at best only 1 netdev will handshake correctly with the AF_XDP DP. 
This

patch addresses this by making the socket parameter configurab

Re: [v2] net/af_xdp: enable a sock path alongside use_cni

[Maryam Tahhan]

On 04/12/2023 17:18, Koikkara Reeny, Shibin wrote:

  Prerequisites
@@ -223,8 +224,7 @@ Howto run dpdk-testpmd with CNI plugin:
   securityContext:
capabilities:
   add:
-   - CAP_NET_RAW
-   - CAP_BPF
+   - NET_RAW

Need to update the 1.3. Prerequisites.



Sorry, what are you referring to?





   resources:
 requests:
   hugepages-2Mi: 2Gi
@@ -239,14 +239,20 @@ Howto run dpdk-testpmd with CNI plugin:

.. _pod.yaml:https://github.com/intel/afxdp-plugins-for-
kubernetes/blob/v0.0.2/test/e2e/pod-1c1d.yaml

+.. note::
+
+   For Kernel versions older than 5.19 `CAP_BPF` is also required in
+   the container capabilities stanza.
+
  * Run DPDK with a command like the following:

.. code-block:: console

   kubectl exec -i  --container  -- \
-   //dpdk-testpmd -l 0,1 --no-pci \
-   --vdev=net_af_xdp0,use_cni=1,iface= \
-   -- --no-mlockall --in-memory
+   //dpdk-testpmd -l 0-2 --no-pci --main-lcore=2 \
+   --vdev net_af_xdp0,iface=,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock
\
+   --vdev net_af_xdp1,iface=e,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock

There is a typo " iface=e


Ack

[PATCH] buildtools/dpdk-cmdline-gen: fix code gen for IP addresses

[Bruce Richardson]

The C code generated for the tokens for matching IP addresses in
commandlines was missing the "static" prefix present in the output for
the other data-types.

Fixes: 3791e9ed ("buildtools: add a tool to generate cmdline boilerplate")
Cc: sta...@dpdk.org

Reported-by: Sunil Kumar Kori 
Signed-off-by: Bruce Richardson 
---
 buildtools/dpdk-cmdline-gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index 49b03bee4a..bf1253d949 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -71,7 +71,7 @@ def process_command(lineno, tokens, comment):
 elif t_type in ["IP", "IP_ADDR", "IPADDR"]:
 result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
 initializers.append(
-f"cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n"
+f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok 
=\n"
 f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result, 
{t_name});"
 )
 elif t_type.startswith("(") and t_type.endswith(")"):
-- 
2.40.1

[Bug 1333] DMA Perf test unable to parse multiple DMA devices from configuration file

[bugzilla]

https://bugs.dpdk.org/show_bug.cgi?id=1333

Bug ID: 1333
   Summary: DMA Perf test unable to parse multiple DMA devices
from configuration file
   Product: DPDK
   Version: 23.11
  Hardware: x86
OS: Linux
Status: UNCONFIRMED
  Severity: normal
  Priority: Normal
 Component: other
  Assignee: dev@dpdk.org
  Reporter: thiyagaraja...@amd.com
  Target Milestone: ---

Description: running dpdk-perf-test for DMA on Intel (Intel(R) Xeon(R) Platinum
8471N) for multiple devices and queues combination.

Sample Config File: `[DPDK root folder]/app/test-dma-perf/config.ini`
Current Value: `lcore_dma=lcore10@:00:04.2, lcore11@:00:04.3`
Modified value:
`lcore_dma=lcore10@:00:04.2-q0,lcore12@:00:04.2-q1,lcore13@:00:04.2-q2,lcore14@:00:04.2-q3,lcore15@:00:04.2-q4,lcore16@:00:04.2-q5,lcore17@:00:04.2-q6,lcore18@:00:04.2-q7,lcore19@:00:04.4-q0,lcore20@:00:04.4-q1,lcore21@:00:04.4-q2,lcore22@:00:04.4-q3,lcore23@:00:04.4-q4,lcore24@:00:04.4-q5,lcore25@:00:04.4-q6,lcore26@:00:04.4-q7`


Expected behavior:
All the devices are initialized and DMA Perf test run on 16 dma devices.
Observed behavior:
dma Perf test runs only on first 7 Devices.

-- 
You are receiving this mail because:
You are the assignee for the bug.

RE: [v2] net/af_xdp: enable a sock path alongside use_cni

[Koikkara Reeny, Shibin]

From: Maryam Tahhan 
Sent: Tuesday, December 5, 2023 11:31 AM
To: Koikkara Reeny, Shibin ; 
ferruh.yi...@amd.com; step...@networkplumber.org; lihuis...@huawei.com; 
fengcheng...@huawei.com; liuyongl...@huawei.com
Cc: dev@dpdk.org
Subject: Re: [v2] net/af_xdp: enable a sock path alongside use_cni

On 04/12/2023 17:18, Koikkara Reeny, Shibin wrote:

 Prerequisites

@@ -223,8 +224,7 @@ Howto run dpdk-testpmd with CNI plugin:

  securityContext:

   capabilities:

  add:

-   - CAP_NET_RAW

-   - CAP_BPF

+   - NET_RAW

Need to update the 1.3. Prerequisites.



Sorry, what are you referring to?



You are removing the CAP_NET_RAW and CAP_BPF. So you will need to update the 
doc section 1.3 Prerequisites.[1]

[1] https://doc.dpdk.org/guides/howto/af_xdp_cni.html







  resources:

requests:

  hugepages-2Mi: 2Gi

@@ -239,14 +239,20 @@ Howto run dpdk-testpmd with CNI plugin:



   .. _pod.yaml: https://github.com/intel/afxdp-plugins-for-

kubernetes/blob/v0.0.2/test/e2e/pod-1c1d.yaml



+.. note::

+

+   For Kernel versions older than 5.19 `CAP_BPF` is also required in

+   the container capabilities stanza.

+

 * Run DPDK with a command like the following:



   .. code-block:: console



  kubectl exec -i  --container  -- \

-   //dpdk-testpmd -l 0,1 --no-pci \

-   --vdev=net_af_xdp0,use_cni=1,iface= \

-   -- --no-mlockall --in-memory

+   //dpdk-testpmd -l 0-2 --no-pci --main-lcore=2 \

+   --vdev net_af_xdp0,iface=,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock

\

+   --vdev net_af_xdp1,iface=e,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock

There is a typo " iface=e

VMXNET3 MTU

[Morten Brørup]

Hi Jochen,

The VMXNET3 driver supports two different max MTU sizes, depending on VMware 
guest version:
https://elixir.bootlin.com/dpdk/latest/source/drivers/net/vmxnet3/vmxnet3_ethdev.c#L1650

I think the reported max MTU size should reflect this too, e.g.:
https://elixir.bootlin.com/dpdk/latest/source/drivers/net/vmxnet3/vmxnet3_ethdev.c#L1564

- dev_info->max_mtu = VMXNET3_MAX_MTU;
+ dev_info->max_mtu = VMXNET3_VERSION_GE_6(hw) ? VMXNET3_V6_MAX_MTU : 
VMXNET3_MAX_MTU;


Med venlig hilsen / Kind regards,
-Morten Brørup

[dpdk-dev] [PATCH v2] drivers: add represented port flow item for cnxk

[psatheesh]

From: Satheesh Paul 

Adding support for represented port flow item for cnxk device.

Signed-off-by: Kiran Kumar K 
Signed-off-by: Satheesh Paul 
---
v2:
* Fix issue with flow key alg for cnxk.

 doc/guides/nics/features/cnxk.ini|   1 +
 doc/guides/nics/features/cnxk_vf.ini |   1 +
 drivers/common/cnxk/roc_npc.c|  61 ---
 drivers/common/cnxk/roc_npc.h|  13 +++-
 drivers/common/cnxk/roc_npc_mcam.c   |  68 -
 drivers/common/cnxk/roc_npc_parse.c  |  11 +++
 drivers/common/cnxk/roc_npc_priv.h   |   1 +
 drivers/net/cnxk/cnxk_flow.c | 106 ++-
 8 files changed, 166 insertions(+), 96 deletions(-)

diff --git a/doc/guides/nics/features/cnxk.ini 
b/doc/guides/nics/features/cnxk.ini
index ac7de9a0f0..1c7d804aab 100644
--- a/doc/guides/nics/features/cnxk.ini
+++ b/doc/guides/nics/features/cnxk.ini
@@ -72,6 +72,7 @@ mark = Y
 mpls = Y
 nvgre= Y
 raw  = Y
+represented_port = Y
 sctp = Y
 tcp  = Y
 tx_queue = Y
diff --git a/doc/guides/nics/features/cnxk_vf.ini 
b/doc/guides/nics/features/cnxk_vf.ini
index b03e8b35c3..96bfd86ac8 100644
--- a/doc/guides/nics/features/cnxk_vf.ini
+++ b/doc/guides/nics/features/cnxk_vf.ini
@@ -63,6 +63,7 @@ mark = Y
 mpls = Y
 nvgre= Y
 raw  = Y
+represented_port = Y
 sctp = Y
 tcp  = Y
 tx_queue = Y
diff --git a/drivers/common/cnxk/roc_npc.c b/drivers/common/cnxk/roc_npc.c
index 1958b3089d..3712f1920c 100644
--- a/drivers/common/cnxk/roc_npc.c
+++ b/drivers/common/cnxk/roc_npc.c
@@ -522,6 +522,8 @@ npc_parse_actions(struct roc_npc *roc_npc, const struct 
roc_npc_attr *attr,
flow->ctr_id = NPC_COUNTER_NONE;
flow->mtr_id = ROC_NIX_MTR_ID_INVALID;
pf_func = npc->pf_func;
+   if (flow->has_rep)
+   pf_func = flow->rep_pf_func;
 
for (; actions->type != ROC_NPC_ACTION_TYPE_END; actions++) {
switch (actions->type) {
@@ -817,10 +819,14 @@ npc_parse_pattern(struct npc *npc, const struct 
roc_npc_item_info pattern[],
  struct roc_npc_flow *flow, struct npc_parse_state *pst)
 {
npc_parse_stage_func_t parse_stage_funcs[] = {
-   npc_parse_meta_items, npc_parse_mark_item, npc_parse_pre_l2, 
npc_parse_cpt_hdr,
-   npc_parse_higig2_hdr, npc_parse_tx_queue,  npc_parse_la, 
npc_parse_lb,
-   npc_parse_lc, npc_parse_ld,npc_parse_le, 
npc_parse_lf,
-   npc_parse_lg, npc_parse_lh,
+   npc_parse_meta_items, npc_parse_port_representor_id,
+   npc_parse_mark_item,  npc_parse_pre_l2,
+   npc_parse_cpt_hdr,npc_parse_higig2_hdr,
+   npc_parse_tx_queue,   npc_parse_la,
+   npc_parse_lb, npc_parse_lc,
+   npc_parse_ld, npc_parse_le,
+   npc_parse_lf, npc_parse_lg,
+   npc_parse_lh,
};
uint8_t layer = 0;
int key_offset;
@@ -1059,15 +1065,20 @@ npc_rss_action_program(struct roc_npc *roc_npc,
   struct roc_npc_flow *flow)
 {
const struct roc_npc_action_rss *rss;
+   struct roc_npc *npc = roc_npc;
uint32_t rss_grp;
uint8_t alg_idx;
int rc;
 
+   if (flow->has_rep) {
+   npc = roc_npc->rep_npc;
+   npc->flowkey_cfg_state = roc_npc->flowkey_cfg_state;
+   }
+
for (; actions->type != ROC_NPC_ACTION_TYPE_END; actions++) {
if (actions->type == ROC_NPC_ACTION_TYPE_RSS) {
rss = (const struct roc_npc_action_rss *)actions->conf;
-   rc = npc_rss_action_configure(roc_npc, rss, &alg_idx,
- &rss_grp, flow->mcam_id);
+   rc = npc_rss_action_configure(npc, rss, &alg_idx, 
&rss_grp, flow->mcam_id);
if (rc)
return rc;
 
@@ -1090,7 +1101,7 @@ npc_vtag_cfg_delete(struct roc_npc *roc_npc, struct 
roc_npc_flow *flow)
struct roc_nix *roc_nix = roc_npc->roc_nix;
struct nix_vtag_config *vtag_cfg;
struct nix_vtag_config_rsp *rsp;
-   struct mbox *mbox;
+   struct mbox *mbox, *ombox;
struct nix *nix;
int rc = 0;
 
@@ -1100,7 +,10 @@ npc_vtag_cfg_delete(struct roc_npc *roc_npc, struct 
roc_npc_flow *flow)
} tx_vtag_action;
 
nix = roc_nix_to_nix_priv(roc_nix);
-   mbox = mbox_get((&nix->dev)->mbox);
+   ombox = (&nix->dev)->mbox;
+   if (flow->has_rep)
+   ombox = flow->rep_mbox;
+   mbox = mbox_get(ombox);
 
tx_vtag_action.reg = flow->vtag_action;
vtag_cfg = mbox_alloc_msg_nix_vtag_cfg(mbox);
@@ -1351,6 +1365,8 @@ npc_vtag_action_program(struct roc

RE: [v2] net/af_xdp: enable a sock path alongside use_cni

[Koikkara Reeny, Shibin]

Hi Maryam,

I was suggesting with reference to the constant ATM and the implementation in 
afxdp_plugin. If you think they will also be changing in the future then please 
go with what you think will be the best.

Regards,
Shibin

From: Maryam Tahhan 
Sent: Tuesday, December 5, 2023 11:29 AM
To: Koikkara Reeny, Shibin ; 
ferruh.yi...@amd.com; step...@networkplumber.org; lihuis...@huawei.com; 
fengcheng...@huawei.com; liuyongl...@huawei.com
Cc: dev@dpdk.org
Subject: Re: [v2] net/af_xdp: enable a sock path alongside use_cni

Hi Shibin

As I've already explained in my previous email, they are constant ATM, however 
they will become configurable. I am implementing the Operator and it will make 
a lot of these "fixed" params configurable. My recommendation is not to try to 
generate the path in DPDK - as it's likely to be different in different k8s 
environments.

As I've also mentioned, the current patch means that I don't need to come back 
in 2 months and update DPDK to support n paths for the UDS (aka 
future-proofing).

Additionally - this is a side discussion as far as this patch goes. The point 
of this patch is to fix the broken UDS behavior and it has been tested in a 
full deployment scenario. @Shibin if you strongly feel that there's a better 
approach, then please go ahead, implement it, test it in a full deployment 
scenario and push it for review.

In general, allowing the AF_XDP params to be configurable rather than 
fixing/hardcoding anything in DPDK decouples the AF_XDP DP from DPDK so we 
don't have to keep coming back to make changes.

BR
Maryam


On 05/12/2023 10:29, Koikkara Reeny, Shibin wrote:
Hi Maryam,

Apologies for making it confusing.

As per the afxdp-plugins-for-kubernetes code it looks like the udsPodPath or 
udsSockDir[1] and udsPodSock[1] are constant and not changing. Only the 
interface name is changing and we are already passing the interface name 
through the command line. So I was suggesting we can write a logic to create 
sock path from these logic.

If I am wrong please correct me, Isn’t that the logic 
afxdp-plugins-for-kubernetes doing? [2]
This is only a suggestions.

[1] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/constants/constants.go#L84
[2] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/deviceplugin/poolManager_test.go#L99

I had added 2 small comment earlier in the code.


Regards,
Shibin
From: Maryam Tahhan 
Sent: Monday, December 4, 2023 6:41 PM
To: Koikkara Reeny, Shibin 
; 
ferruh.yi...@amd.com; 
step...@networkplumber.org; 
lihuis...@huawei.com; 
fengcheng...@huawei.com; 
liuyongl...@huawei.com
Cc: dev@dpdk.org
Subject: Re: [v2] net/af_xdp: enable a sock path alongside use_cni

Hi Shibin

I'm not really sure what you are suggesting, is to make an assumption on the 
path part where the socket resides (aka hard code it) and then try to build the 
full UDS path in DPDK?

Yes the plugin is using constants ATM for certain parts of the UDS path, but 
that's not say that it's something that won't become configurable later on. 
Someone may not want to use "/tmp/afxdp_dp/" as the base directory. Then we'd 
have to change DPDK's implementation again. These are not really things that 
are configured by hand and are generated by initialization scripts (typically). 
I would rather build this with the idea that things can change in the future 
without having to change the DPDK implementation again.
BR
Maryam

On 04/12/2023 17:18, Koikkara Reeny, Shibin wrote:

Hi Maryam,



Apologies for asking this question bit late.

The UDS sock name will be afxdp.sock only and addition director is created 
between the sock name and the uds filepath (/tmp/afxdp_dp//afxdp.sock).



As per the command " --vdev net_af_xdp0,iface=,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock"

We are already passing the interface name(iface= . So can't we 
create the uds_path inside the program uds_path="/tmp/afxdp_dp/"+ iface + 
"afxdp.sock"





If you check the code afxdp-plugins-for-kubernetes constants.go [1] they still 
have the constants and also they are using these constants to create the path 
[2]



[1] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/constants/constants.go#L84

[2] 
https://github.com/intel/afxdp-plugins-for-kubernetes/blob/main/internal/deviceplugin/poolManager_test.go#L78



If we are able to create path in the program then user won't have to pass along 
argument value.



Regards,

Shibin



-Original Message-

From: Maryam Tahhan 

Sent: Monday, December 4, 2023 10:31 AM

To: ferruh.yi...@amd.com; 
step...@networkplumber.org;

lihuis...@huawei.com

[PATCH v2] net/af_xdp: fix memzone leak in error path

[Yunjian Wang]

In xdp_umem_configure() allocated memzone for the 'umem', we should
free it when xsk_umem__create() call fails, otherwise it will lead to
memory zone leak. To fix it move 'umem->mz = mz;' assignment after
'mz == NULL' check.

Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
Cc: sta...@dpdk.org

Signed-off-by: Yunjian Wang 
---
v2: update code suggested by Ferruh Yigit
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c 
b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 353c8688ec..9f0f751d4a 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -1235,6 +1235,7 @@ xsk_umem_info *xdp_umem_configure(struct pmd_internals 
*internals,
goto err;
}
 
+   umem->mz = mz;
ret = xsk_umem__create(&umem->umem, mz->addr,
   ETH_AF_XDP_NUM_BUFFERS * ETH_AF_XDP_FRAME_SIZE,
   &rxq->fq, &rxq->cq,
@@ -1244,7 +1245,6 @@ xsk_umem_info *xdp_umem_configure(struct pmd_internals 
*internals,
AF_XDP_LOG(ERR, "Failed to create umem\n");
goto err;
}
-   umem->mz = mz;
 
return umem;
 
-- 
2.33.0

RE: [dpdk-dev] [PATCH] net/af_xdp: fix memzone leak in error path

[wangyunjian]

> -Original Message-
> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com]
> Sent: Tuesday, December 5, 2023 5:42 PM
> To: wangyunjian ; dev@dpdk.org
> Cc: ciara.lof...@intel.com; qi.z.zh...@intel.com; xudingke
> ; Lilijun (Jerry) ;
> sta...@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/af_xdp: fix memzone leak in error path
> 
> On 12/5/2023 1:23 AM, wangyunjian wrote:
> >
> >
> >> -Original Message-
> >> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com]
> >> Sent: Monday, December 4, 2023 10:10 PM
> >> To: wangyunjian ; dev@dpdk.org
> >> Cc: ciara.lof...@intel.com; qi.z.zh...@intel.com; xudingke
> >> ; Lilijun (Jerry) ;
> >> sta...@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] net/af_xdp: fix memzone leak in error
> >> path
> >>
> >> On 12/1/2023 8:03 AM, Yunjian Wang wrote:
> >>> In xdp_umem_configure() allocated memzone for the 'umem', we should
> >>> free it when xsk_umem__create() call fails, otherwise it will lead
> >>> to memory zone leak.
> >>>
> >>> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
> >>> Cc: sta...@dpdk.org
> >>>
> >>> Signed-off-by: Yunjian Wang 
> >>> ---
> >>>  drivers/net/af_xdp/rte_eth_af_xdp.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >>> index 2a20a6960c..2a1fdafb3c 100644
> >>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> >>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> >>> @@ -1229,6 +1229,7 @@ xsk_umem_info *xdp_umem_configure(struct
> >>> pmd_internals *internals,
> >>>
> >>>   if (ret) {
> >>>   AF_XDP_LOG(ERR, "Failed to create umem\n");
> >>> + rte_memzone_free(mz);
> >>>
> >>
> >> Doesn't 'xdp_umem_destroy()', in the label 'err', already free it?
> >
> > In this case, 'mz' is not assigned to 'umem->mz'. Therefore,
> > the'xdp_umem_destroy()' does not free 'mz'.
> >
> >
> 
> True.
> What do you think to move 'umem->mz = mz;' assignment after 'mz == NULL'
> check? So related code can be grouped together.

OK, I update the patch in v2. Thanks.
https://patchwork.dpdk.org/project/dpdk/patch/ab06b0b804b4b27ea8444381ea14a98a3c3231bb.1701778603.git.wangyunj...@huawei.com/
> 
> 
> 

RE: [PATCH v2] net/af_xdp: fix memzone leak in error path

[Loftus, Ciara]

> 
> In xdp_umem_configure() allocated memzone for the 'umem', we should
> free it when xsk_umem__create() call fails, otherwise it will lead to
> memory zone leak. To fix it move 'umem->mz = mz;' assignment after
> 'mz == NULL' check.
> 
> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Yunjian Wang 
> ---
> v2: update code suggested by Ferruh Yigit
> ---
>  drivers/net/af_xdp/rte_eth_af_xdp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 353c8688ec..9f0f751d4a 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -1235,6 +1235,7 @@ xsk_umem_info *xdp_umem_configure(struct
> pmd_internals *internals,
>   goto err;
>   }
> 
> + umem->mz = mz;
>   ret = xsk_umem__create(&umem->umem, mz->addr,
>  ETH_AF_XDP_NUM_BUFFERS *
> ETH_AF_XDP_FRAME_SIZE,
>  &rxq->fq, &rxq->cq,
> @@ -1244,7 +1245,6 @@ xsk_umem_info *xdp_umem_configure(struct
> pmd_internals *internals,
>   AF_XDP_LOG(ERR, "Failed to create umem\n");
>   goto err;
>   }
> - umem->mz = mz;
> 
>   return umem;
> 
> --
> 2.33.0

Thank you for the patch.

Acked-by: Ciara Loftus 

RE: [v2] net/af_xdp: enable a sock path alongside use_cni

[Loftus, Ciara]

> -Original Message-
> From: Maryam Tahhan 
> Sent: Monday, December 4, 2023 10:31 AM
> To: ferruh.yi...@amd.com; step...@networkplumber.org;
> lihuis...@huawei.com; fengcheng...@huawei.com;
> liuyongl...@huawei.com; Koikkara Reeny, Shibin
> 
> Cc: dev@dpdk.org; Tahhan, Maryam 
> Subject: [v2] net/af_xdp: enable a sock path alongside use_cni
> 
> With the original 'use_cni' implementation, (using a
> hardcoded socket rather than a configurable one),
> if a single pod is requesting multiple net devices
> and these devices are from different pools, then
> the container attempts to mount all the netdev UDSes
> in the pod as /tmp/afxdp.sock. Which means that at best
> only 1 netdev will handshake correctly with the AF_XDP
> DP. This patch addresses this by making the socket
> parameter configurable alongside the 'use_cni' param.
> Tested with the AF_XDP DP CNI PR 81.
> 
> v2:
> * Rename sock_path to uds_path.
> * Update documentation to reflect when CAP_BPF is needed.
> * Fix testpmd arguments in the provided example for Pods.
> * Use AF_XDP API to update the xskmap entry.
> 
> Signed-off-by: Maryam Tahhan 
> ---
>  doc/guides/howto/af_xdp_cni.rst | 24 ++-
>  drivers/net/af_xdp/rte_eth_af_xdp.c | 62 ++---
>  2 files changed, 54 insertions(+), 32 deletions(-)
> 
> diff --git a/doc/guides/howto/af_xdp_cni.rst
> b/doc/guides/howto/af_xdp_cni.rst
> index a1a6d5b99c..7829526b40 100644
> --- a/doc/guides/howto/af_xdp_cni.rst
> +++ b/doc/guides/howto/af_xdp_cni.rst
> @@ -38,9 +38,10 @@ The XSKMAP is a BPF map of AF_XDP sockets (XSK).
>  The client can then proceed with creating an AF_XDP socket
>  and inserting that socket into the XSKMAP pointed to by the descriptor.
> 
> -The EAL vdev argument ``use_cni`` is used to indicate that the user wishes
> -to run the PMD in unprivileged mode and to receive the XSKMAP file
> descriptor
> -from the CNI.
> +The EAL vdev arguments ``use_cni`` and ``uds_path`` are used to indicate that
> +the user wishes to run the PMD in unprivileged mode and to receive the
> XSKMAP
> +file descriptor from the CNI.
> +
>  When this flag is set,
>  the ``XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD`` libbpf flag
>  should be used when creating the socket
> @@ -49,7 +50,7 @@ Instead the loading is handled by the CNI.
> 
>  .. note::
> 
> -   The Unix Domain Socket file path appear in the end user is
> "/tmp/afxdp.sock".
> +   The Unix Domain Socket file path appears to the end user at
> "/tmp/afxdp_dp//afxdp.sock".
> 
> 
>  Prerequisites
> @@ -223,8 +224,7 @@ Howto run dpdk-testpmd with CNI plugin:
>   securityContext:
>capabilities:
>   add:
> -   - CAP_NET_RAW
> -   - CAP_BPF
> +   - NET_RAW
>   resources:
> requests:
>   hugepages-2Mi: 2Gi
> @@ -239,14 +239,20 @@ Howto run dpdk-testpmd with CNI plugin:
> 
>.. _pod.yaml: https://github.com/intel/afxdp-plugins-for-
> kubernetes/blob/v0.0.2/test/e2e/pod-1c1d.yaml
> 
> +.. note::
> +
> +   For Kernel versions older than 5.19 `CAP_BPF` is also required in
> +   the container capabilities stanza.
> +
>  * Run DPDK with a command like the following:
> 
>.. code-block:: console
> 
>   kubectl exec -i  --container  -- \
> -   //dpdk-testpmd -l 0,1 --no-pci \
> -   --vdev=net_af_xdp0,use_cni=1,iface= \
> -   -- --no-mlockall --in-memory
> +   //dpdk-testpmd -l 0-2 --no-pci --main-lcore=2 \
> +   --vdev net_af_xdp0,iface= name>,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock \
> +   --vdev net_af_xdp1,iface=e name>,use_cni=1,uds_path=/tmp/afxdp_dp//afxdp.sock \
> +   -- -i --a --nb-cores=2 --rxq=1 --txq=1 --forward-mode=macswap;
> 
>  For further reference please use the `e2e`_ test case in `AF_XDP Plugin for
> Kubernetes`_
> 
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 353c8688ec..505ed6cf1e 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -88,7 +88,6 @@ RTE_LOG_REGISTER_DEFAULT(af_xdp_logtype, NOTICE);
>  #define UDS_MAX_CMD_LEN  64
>  #define UDS_MAX_CMD_RESP 128
>  #define UDS_XSK_MAP_FD_MSG   "/xsk_map_fd"
> -#define UDS_SOCK "/tmp/afxdp.sock"
>  #define UDS_CONNECT_MSG  "/connect"
>  #define UDS_HOST_OK_MSG  "/host_ok"
>  #define UDS_HOST_NAK_MSG "/host_nak"
> @@ -171,6 +170,7 @@ struct pmd_internals {
>   bool custom_prog_configured;
>   bool force_copy;
>   bool use_cni;
> + char uds_path[PATH_MAX];
>   struct bpf_map *map;
> 
>   struct rte_ether_addr eth_addr;
> @@ -191,6 +191,7 @@ struct pmd_process_private {
>  #define ETH_AF_XDP_BUDGET_ARG"busy_budget"
>  #define ETH_AF_XDP_FORCE_COPY_ARG"force_copy"
>  #define ETH_AF_XDP_USE_CNI_ARG   "use_cni"
> +

Re: [v2] net/af_xdp: enable a sock path alongside use_cni

[Maryam Tahhan]

On 05/12/2023 13:43, Loftus, Ciara wrote:

also be provided\n",

Thanks for the patch Maryam.
Do we really need the use_cni devarg anymore if we must also always pair it 
with a uds_path string?
I am in favour of removing it if both yourself and Shibin think that makes 
sense too.

Ciara


Hey Ciara

I'm happy to remove the use_cni arg in favour of just the uds_path

BR
Maryam

RE: [v2] net/af_xdp: enable a sock path alongside use_cni

[Koikkara Reeny, Shibin]

Hi Ciara,

I agree.

Regards,
Shibin

From: Maryam Tahhan 
Sent: Tuesday, December 5, 2023 2:38 PM
To: Loftus, Ciara ; ferruh.yi...@amd.com; 
step...@networkplumber.org; lihuis...@huawei.com; fengcheng...@huawei.com; 
liuyongl...@huawei.com; Koikkara Reeny, Shibin 
Cc: dev@dpdk.org
Subject: Re: [v2] net/af_xdp: enable a sock path alongside use_cni

On 05/12/2023 13:43, Loftus, Ciara wrote:

also be provided\n",

Thanks for the patch Maryam.

Do we really need the use_cni devarg anymore if we must also always pair it 
with a uds_path string?

I am in favour of removing it if both yourself and Shibin think that makes 
sense too.



Ciara

Hey Ciara

I'm happy to remove the use_cni arg in favour of just the uds_path

BR
Maryam

[PATCH 0/3] enhancements for dpdk-cmdline-gen script

[Bruce Richardson]

This set contains some small enhancements for the cmdline generation
script introduced in the last release. Specifically:

* Add support for commands with an optional variable parameter. This
  is needed to support command pairs like testpmd's "start tx_first"
  and "start tx_first 128" (to send 128 packets rather than 32).

* Improve IP address handling support. We make the "IP" type correspond
  to the cmdline lib IP type which supports IPv4 and v6. Then we add
  explicit support for IPv4 addresses and IPv6 addresses only via
  new type names.


Bruce Richardson (3):
  buildtools/dpdk-cmdline-gen: support optional parameters
  buildtools/dpdk-cmdline-gen: fix IP address initializer
  buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types

 buildtools/dpdk-cmdline-gen.py| 21 -
 doc/guides/prog_guide/cmdline.rst | 17 +
 2 files changed, 37 insertions(+), 1 deletion(-)

--
2.40.1

[PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters

[Bruce Richardson]

Sometimes a user may want to have a command which takes an optional
parameter. For commands with an optional constant string, this is no
issue, as it can be configured as two separate commands, e.g.

start
start tx_first.

However, if we want to have a variable parameter on the command, we hit
issues, because variable tokens do not form part of the generated
command names used for function and result-struct naming. To avoid
duplicate name issues, we add a special syntax to allow the user to
indicate that a particular variable parameter should be included in the
name. Any leading variable names starting with a "__" will be included
in command naming.

This then allows us to have:

start
start tx_first
start tx_first __n

without hitting any naming conflicts.

Signed-off-by: Bruce Richardson 
---
 buildtools/dpdk-cmdline-gen.py|  9 -
 doc/guides/prog_guide/cmdline.rst | 13 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index bf1253d949..faee4ffca7 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -40,7 +40,12 @@ def process_command(lineno, tokens, comment):
 name_tokens = []
 for t in tokens:
 if t.startswith("<"):
-break
+# stop processing the name building at a variable token,
+# UNLESS the token name starts with "__"
+t_type, t_name = t[1:].split(">")
+if not t_name.startswith("__"):
+break
+t = t_name[2:]   # strip off the leading '__'
 name_tokens.append(t)
 name = "_".join(name_tokens)
 
@@ -51,6 +56,8 @@ def process_command(lineno, tokens, comment):
 if t.startswith("<"):
 t_type, t_name = t[1:].split(">")
 t_val = "NULL"
+if t_name.startswith("__"):
+t_name = t_name[2:]
 else:
 t_type = "STRING"
 t_name = t
diff --git a/doc/guides/prog_guide/cmdline.rst 
b/doc/guides/prog_guide/cmdline.rst
index b804d7a328..fc32d727dc 100644
--- a/doc/guides/prog_guide/cmdline.rst
+++ b/doc/guides/prog_guide/cmdline.rst
@@ -155,6 +155,19 @@ To get at the results structure for each command above,
 the ``parsed_result`` parameter should be cast to ``struct cmd_quit_result``
 or ``struct cmd_show_port_stats_result`` respectively.
 
+.. note::
+
+  In some cases, the user may want to have an optional variable parameter at 
the end of a command.
+  Such a variable parameter would not normally be included in the 
 string,
+  leading to duplicate definition errors.
+  To work around this,
+  any variable token with a name prefixed by ``'__'`` will be included in the 
cmdname string,
+  with the prefix removed.
+  Using this, it is possible to have commands, such as:
+  ``start tx_first`` and ``start tx_first __n``, without them 
conflicting.
+  The resulting code generated will expect functions called 
``cmd_start_tx_first_parsed``
+  and ``cmd_start_tx_first_n_parsed`` respectively.
+
 Integrating with the Application
 
 
-- 
2.40.1

[PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address initializer

[Bruce Richardson]

The IP address type should be generic for both IPv4 and IPv6 and so use
the cmdline lib's TOKEN_IPADDR_INITIALIZER rather than
TOKEN_IPV4_INITIALIZER.

Fixes: 3791e9ed ("buildtools: add a tool to generate cmdline boilerplate")
Cc: sta...@dpdk.org

Signed-off-by: Bruce Richardson 
---
 buildtools/dpdk-cmdline-gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index faee4ffca7..8b4f22ca24 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -79,7 +79,7 @@ def process_command(lineno, tokens, comment):
 result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
 initializers.append(
 f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok 
=\n"
-f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result, 
{t_name});"
+f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result, 
{t_name});"
 )
 elif t_type.startswith("(") and t_type.endswith(")"):
 result_struct.append(f"\tcmdline_fixed_string_t {t_name};")
-- 
2.40.1

[PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types

[Bruce Richardson]

Add support for generating cmdline lib code to just match IPv4 addresses
or IPv6 addresses, rather than IP addresses in general.

Signed-off-by: Bruce Richardson 
---
 buildtools/dpdk-cmdline-gen.py| 12 
 doc/guides/prog_guide/cmdline.rst |  4 
 2 files changed, 16 insertions(+)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index 8b4f22ca24..7dadded783 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -81,6 +81,18 @@ def process_command(lineno, tokens, comment):
 f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok 
=\n"
 f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result, 
{t_name});"
 )
+elif t_type in ["IPV4", "IPv4", "IPV4_ADDR"]:
+result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
+initializers.append(
+f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok 
=\n"
+f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result, 
{t_name});"
+)
+elif t_type in ["IPV6", "IPv6", "IPV6_ADDR"]:
+result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
+initializers.append(
+f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok 
=\n"
+f"\tTOKEN_IPV6_INITIALIZER(struct cmd_{name}_result, 
{t_name});"
+)
 elif t_type.startswith("(") and t_type.endswith(")"):
 result_struct.append(f"\tcmdline_fixed_string_t {t_name};")
 t_val = f'"{t_type[1:-1].replace(",","#")}"'
diff --git a/doc/guides/prog_guide/cmdline.rst 
b/doc/guides/prog_guide/cmdline.rst
index fc32d727dc..f62f17f1aa 100644
--- a/doc/guides/prog_guide/cmdline.rst
+++ b/doc/guides/prog_guide/cmdline.rst
@@ -70,6 +70,10 @@ The format of the list file must be:
 
   * ``src_ip``
 
+  * ``dst_ip4``
+
+  * ``dst_ip6``
+
 * Variable fields, which take their values from a list of options,
   have the comma-separated option list placed in braces, rather than a the 
type name.
   For example,
-- 
2.40.1

Re: [v2] net/af_xdp: enable a sock path alongside use_cni

[Stephen Hemminger]

On Mon,  4 Dec 2023 05:31:01 -0500
Maryam Tahhan  wrote:

> With the original 'use_cni' implementation, (using a
> hardcoded socket rather than a configurable one),
> if a single pod is requesting multiple net devices
> and these devices are from different pools, then
> the container attempts to mount all the netdev UDSes
> in the pod as /tmp/afxdp.sock. Which means that at best
> only 1 netdev will handshake correctly with the AF_XDP
> DP. This patch addresses this by making the socket
> parameter configurable alongside the 'use_cni' param.
> Tested with the AF_XDP DP CNI PR 81.
> 
> v2:
> * Rename sock_path to uds_path.
> * Update documentation to reflect when CAP_BPF is needed.
> * Fix testpmd arguments in the provided example for Pods.
> * Use AF_XDP API to update the xskmap entry.
> 
> Signed-off-by: Maryam Tahhan 

Why does XDP PMD not use abstract socket path?
Having actual file visible in file system can cause permission
and leftover file issues that are not present with abstract path.

If you use abstract path then when last reference is gone (ie server exits)
the path is removed. With regular paths, the file gets stuck in the
file system and has to be cleaned up.

Re: [PATCH v9 21/21] dts: test suites docstring update

[Jeremy Spewock]

Reviewed-by: Jeremy Spewock 

On Mon, Dec 4, 2023 at 5:24 AM Juraj Linkeš 
wrote:

> Format according to the Google format and PEP257, with slight
> deviations.
>
> Signed-off-by: Juraj Linkeš 
> ---
>  dts/tests/TestSuite_hello_world.py | 16 +---
>  dts/tests/TestSuite_os_udp.py  | 20 ++
>  dts/tests/TestSuite_smoke_tests.py | 61 --
>  3 files changed, 72 insertions(+), 25 deletions(-)
>
> diff --git a/dts/tests/TestSuite_hello_world.py
> b/dts/tests/TestSuite_hello_world.py
> index 768ba1cfa8..fd7ff1534d 100644
> --- a/dts/tests/TestSuite_hello_world.py
> +++ b/dts/tests/TestSuite_hello_world.py
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2010-2014 Intel Corporation
>
> -"""
> +"""The DPDK hello world app test suite.
> +
>  Run the helloworld example app and verify it prints a message for each
> used core.
>  No other EAL parameters apart from cores are used.
>  """
> @@ -15,22 +16,25 @@
>
>
>  class TestHelloWorld(TestSuite):
> +"""DPDK hello world app test suite."""
> +
>  def set_up_suite(self) -> None:
> -"""
> +"""Set up the test suite.
> +
>  Setup:
>  Build the app we're about to test - helloworld.
>  """
>  self.app_helloworld_path =
> self.sut_node.build_dpdk_app("helloworld")
>
>  def test_hello_world_single_core(self) -> None:
> -"""
> +"""Single core test case.
> +
>  Steps:
>  Run the helloworld app on the first usable logical core.
>  Verify:
>  The app prints a message from the used core:
>  "hello from core "
>  """
> -
>  # get the first usable core
>  lcore_amount = LogicalCoreCount(1, 1, 1)
>  lcores = LogicalCoreCountFilter(self.sut_node.lcores,
> lcore_amount).filter()
> @@ -42,14 +46,14 @@ def test_hello_world_single_core(self) -> None:
>  )
>
>  def test_hello_world_all_cores(self) -> None:
> -"""
> +"""All cores test case.
> +
>  Steps:
>  Run the helloworld app on all usable logical cores.
>  Verify:
>  The app prints a message from all used cores:
>  "hello from core "
>  """
> -
>  # get the maximum logical core number
>  eal_para = self.sut_node.create_eal_parameters(
>  lcore_filter_specifier=LogicalCoreList(self.sut_node.lcores)
> diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_udp.py
> index bf6b93deb5..2cf29d37bb 100644
> --- a/dts/tests/TestSuite_os_udp.py
> +++ b/dts/tests/TestSuite_os_udp.py
> @@ -1,7 +1,8 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2023 PANTHEON.tech s.r.o.
>
> -"""
> +"""Basic IPv4 OS routing test suite.
> +
>  Configure SUT node to route traffic from if1 to if2.
>  Send a packet to the SUT node, verify it comes back on the second port on
> the TG node.
>  """
> @@ -13,24 +14,26 @@
>
>
>  class TestOSUdp(TestSuite):
> +"""IPv4 UDP OS routing test suite."""
> +
>  def set_up_suite(self) -> None:
> -"""
> +"""Set up the test suite.
> +
>  Setup:
> -Configure SUT ports and SUT to route traffic from if1 to if2.
> +Bind the SUT ports to the OS driver, configure the ports and
> configure the SUT
> +to route traffic from if1 to if2.
>  """
> -
> -# This test uses kernel drivers
>  self.sut_node.bind_ports_to_driver(for_dpdk=False)
>  self.configure_testbed_ipv4()
>
>  def test_os_udp(self) -> None:
> -"""
> +"""Basic UDP IPv4 traffic test case.
> +
>  Steps:
>  Send a UDP packet.
>  Verify:
>  The packet with proper addresses arrives at the other TG port.
>  """
> -
>  packet = Ether() / IP() / UDP()
>
>  received_packets = self.send_packet_and_capture(packet)
> @@ -40,7 +43,8 @@ def test_os_udp(self) -> None:
>  self.verify_packets(expected_packet, received_packets)
>
>  def tear_down_suite(self) -> None:
> -"""
> +"""Tear down the test suite.
> +
>  Teardown:
>  Remove the SUT port configuration configured in setup.
>  """
> diff --git a/dts/tests/TestSuite_smoke_tests.py
> b/dts/tests/TestSuite_smoke_tests.py
> index 8958f58dac..5e2bac14bd 100644
> --- a/dts/tests/TestSuite_smoke_tests.py
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -1,6 +1,17 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2023 University of New Hampshire
>
> +"""Smoke test suite.
> +
> +Smoke tests are a class of tests which are used for validating a minimal
> set of important features.
> +These are the most important features without which (or when they're
> faulty) the software wouldn't
> +work properly. Thus, if any failure occurs while testing these features,
> +there isn't that much of a reason to continue testing, as the software is
> 

Re: [PATCH v1] mbuf: remove the redundant code for mbuf prefree

[Stephen Hemminger]

On Mon, 4 Dec 2023 11:07:08 +
Konstantin Ananyev  wrote:

> > > 2.25.1  
> > 
> > NAK.
> > 
> > This patch is not race safe.   
> 
> +1, It is a bad idea.

The patch does raise a couple of issues that could be addressed by
rearranging. There is duplicate code, and there are no comments
to explain the rationale.

Maybe something like the following (untested):

diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 286b32b788a5..b43c055fbe3f 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -1342,42 +1342,32 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 {
__rte_mbuf_sanity_check(m, 0);
 
-   if (likely(rte_mbuf_refcnt_read(m) == 1)) {
-
-   if (!RTE_MBUF_DIRECT(m)) {
-   rte_pktmbuf_detach(m);
-   if (RTE_MBUF_HAS_EXTBUF(m) &&
-   RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
-   __rte_pktmbuf_pinned_extbuf_decref(m))
-   return NULL;
-   }
-
-   if (m->next != NULL)
-   m->next = NULL;
-   if (m->nb_segs != 1)
-   m->nb_segs = 1;
-
-   return m;
-
-   } else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
-
-   if (!RTE_MBUF_DIRECT(m)) {
-   rte_pktmbuf_detach(m);
-   if (RTE_MBUF_HAS_EXTBUF(m) &&
-   RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
-   __rte_pktmbuf_pinned_extbuf_decref(m))
-   return NULL;
-   }
-
-   if (m->next != NULL)
-   m->next = NULL;
-   if (m->nb_segs != 1)
-   m->nb_segs = 1;
+   if (likely(rte_mbuf_refcnt_read(m) != 1) ) {
+   /* If this is the only reference to the mbuf it can just
+* be setup for reuse without modifying reference count.
+*/
+   } else if (unlikely(__rte_mbuf_refcnt_update(m, -1) == 0)) {
+   /* This was last reference reset to 1 for recycling/free. */
rte_mbuf_refcnt_set(m, 1);
+   } else {
+   /* mbuf is still in use */
+   return NULL;
+   }
 
-   return m;
+   if (!RTE_MBUF_DIRECT(m)) {
+   rte_pktmbuf_detach(m);
+   if (RTE_MBUF_HAS_EXTBUF(m) &&
+   RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
+   __rte_pktmbuf_pinned_extbuf_decref(m))
+
+   return NULL;
}
-   return NULL;
+
+   if (m->next != NULL)
+   m->next = NULL;
+   if (m->nb_segs != 1)
+   m->nb_segs = 1;
+   return m;
 }
 
 /**

[PATCH] raw/ntb: refactor to support NTB on 5th Gen Intel Xeon

[Junfeng Guo]

Refactor to support for 5th Gen Intel Xeon Scalable processors. Note that
NTB devices within the 3rd, 4th and 5th Gen Intel Xeon Scalable processors
share the same device id, and compliant to PCIe 4.0 spec.
It is more reasonable to distinguish NTB devices by their PCIe generation.

Signed-off-by: Junfeng Guo 
---
 doc/guides/rawdevs/ntb.rst |   7 +-
 doc/guides/rel_notes/release_24_03.rst |   4 +
 drivers/raw/ntb/ntb.c  |   8 +-
 drivers/raw/ntb/ntb.h  |   5 +-
 drivers/raw/ntb/ntb_hw_intel.c | 182 +++--
 drivers/raw/ntb/ntb_hw_intel.h |  90 ++--
 usertools/dpdk-devbind.py  |   8 +-
 7 files changed, 149 insertions(+), 155 deletions(-)

diff --git a/doc/guides/rawdevs/ntb.rst b/doc/guides/rawdevs/ntb.rst
index f8befc6594..ee452af4fd 100644
--- a/doc/guides/rawdevs/ntb.rst
+++ b/doc/guides/rawdevs/ntb.rst
@@ -153,6 +153,7 @@ Limitation
 
 This PMD is only supported on Intel Xeon Platforms:
 
-- 4th Generation Intel® Xeon® Scalable Processors.
-- 3rd Generation Intel® Xeon® Scalable Processors.
-- 2nd Generation Intel® Xeon® Scalable Processors.
+- 5th Generation Intel® Xeon® Scalable Processors. (NTB GEN4 device id: 0x347E)
+- 4th Generation Intel® Xeon® Scalable Processors. (NTB GEN4 device id: 0x347E)
+- 3rd Generation Intel® Xeon® Scalable Processors. (NTB GEN4 device id: 0x347E)
+- 2nd Generation Intel® Xeon® Scalable Processors. (NTB GEN3 device id: 0x201C)
diff --git a/doc/guides/rel_notes/release_24_03.rst 
b/doc/guides/rel_notes/release_24_03.rst
index bd84875d4f..a43f64b199 100644
--- a/doc/guides/rel_notes/release_24_03.rst
+++ b/doc/guides/rel_notes/release_24_03.rst
@@ -55,6 +55,10 @@ New Features
  Also, make sure to start the actual text at the margin.
  ===
 
+* **Added support for Intel NTB with 5th Gen Intel Xeon Scalable processors..**
+
+Added NTB device (4th generation) support for the 5th Gen Intel Xeon Platforms.
+
 * **Improved support of RSS hash algorithm.**
 
   * Added new function ``rte_eth_find_rss_algo`` to get RSS hash
diff --git a/drivers/raw/ntb/ntb.c b/drivers/raw/ntb/ntb.c
index 0ed4c14592..2a163eff60 100644
--- a/drivers/raw/ntb/ntb.c
+++ b/drivers/raw/ntb/ntb.c
@@ -24,8 +24,8 @@
 #include "ntb.h"
 
 static const struct rte_pci_id pci_id_ntb_map[] = {
-   { RTE_PCI_DEVICE(NTB_INTEL_VENDOR_ID, NTB_INTEL_DEV_ID_B2B_SKX) },
-   { RTE_PCI_DEVICE(NTB_INTEL_VENDOR_ID, NTB_INTEL_DEV_ID_B2B_ICX) },
+   { RTE_PCI_DEVICE(NTB_INTEL_VENDOR_ID, NTB_INTEL_DEV_ID_B2B_GEN3) },
+   { RTE_PCI_DEVICE(NTB_INTEL_VENDOR_ID, NTB_INTEL_DEV_ID_B2B_GEN4) },
{ .vendor_id = 0, /* sentinel */ },
 };
 
@@ -1378,8 +1378,8 @@ ntb_init_hw(struct rte_rawdev *dev, struct rte_pci_device 
*pci_dev)
hw->link_width = NTB_WIDTH_NONE;
 
switch (pci_dev->id.device_id) {
-   case NTB_INTEL_DEV_ID_B2B_SKX:
-   case NTB_INTEL_DEV_ID_B2B_ICX:
+   case NTB_INTEL_DEV_ID_B2B_GEN3:
+   case NTB_INTEL_DEV_ID_B2B_GEN4:
hw->ntb_ops = &intel_ntb_ops;
break;
default:
diff --git a/drivers/raw/ntb/ntb.h b/drivers/raw/ntb/ntb.h
index a30a6b60c9..d2e6d566ed 100644
--- a/drivers/raw/ntb/ntb.h
+++ b/drivers/raw/ntb/ntb.h
@@ -17,9 +17,8 @@ extern int ntb_logtype;
 #define NTB_INTEL_VENDOR_ID 0x8086
 
 /* Device IDs */
-#define NTB_INTEL_DEV_ID_B2B_SKX0x201C
-#define NTB_INTEL_DEV_ID_B2B_ICX0x347E
-#define NTB_INTEL_DEV_ID_B2B_SPR0x347E
+#define NTB_INTEL_DEV_ID_B2B_GEN3   0x201C
+#define NTB_INTEL_DEV_ID_B2B_GEN4   0x347E
 
 /* Reserved to app to use. */
 #define NTB_SPAD_USER   "spad_user_"
diff --git a/drivers/raw/ntb/ntb_hw_intel.c b/drivers/raw/ntb/ntb_hw_intel.c
index 9b4465176a..3e2094437f 100644
--- a/drivers/raw/ntb/ntb_hw_intel.c
+++ b/drivers/raw/ntb/ntb_hw_intel.c
@@ -25,25 +25,6 @@ static enum xeon_ntb_bar intel_ntb_bar[] = {
XEON_NTB_BAR45,
 };
 
-static inline int
-is_gen3_ntb(const struct ntb_hw *hw)
-{
-   if (hw->pci_dev->id.device_id == NTB_INTEL_DEV_ID_B2B_SKX)
-   return 1;
-
-   return 0;
-}
-
-static inline int
-is_gen4_ntb(const struct ntb_hw *hw)
-{
-   if (hw->pci_dev->id.device_id == NTB_INTEL_DEV_ID_B2B_ICX ||
-   hw->pci_dev->id.device_id == NTB_INTEL_DEV_ID_B2B_SPR)
-   return 1;
-
-   return 0;
-}
-
 static int
 intel_ntb3_check_ppd(struct ntb_hw *hw)
 {
@@ -51,35 +32,36 @@ intel_ntb3_check_ppd(struct ntb_hw *hw)
int ret;
 
ret = rte_pci_read_config(hw->pci_dev, ®_val,
- sizeof(reg_val), XEON_PPD_OFFSET);
+ sizeof(reg_val), XEON_NTB3_PPD_OFFSET);
if (ret < 0) {
NTB_LOG(ERR, "Cannot get NTB PPD (PCIe port definition).");
return -EIO;
}
 
/* Check connection topo type. Only support B2B. */
-   switch (reg_val & XEON_PPD_CONN_MASK)

RE: [PATCH v1] net/memif: fix segfault with Tx burst larger than 255

[Joyce Kong]

> -Original Message-
> From: Stephen Hemminger 
> Sent: Wednesday, December 6, 2023 2:34 AM
> To: Joyce Kong 
> Cc: Jakub Grajciar ; Morten Brørup
> ; Ruifeng Wang ;
> dev@dpdk.org; nd ; sta...@dpdk.org; Liangxing Wang
> 
> Subject: Re: [PATCH v1] net/memif: fix segfault with Tx burst larger than 255
> 
> On Tue,  5 Dec 2023 04:05:24 +
> Joyce Kong  wrote:
> 
> > There will be a segfault when tx burst size is larger than 256. This
> > is because eth_memif_tx uses an index i which is uint8_t to count
> > transmitted nb_pkts. Extend i to uint16_t, the same size as nb_pkts.
> >
> > Fixes: b5613c8f9d0a ("net/memif: add a Tx fast path")
> > Cc: sta...@dpdk.org
> >
> > Reported-by: Liangxing Wang 
> > Signed-off-by: Joyce Kong 
> > Reviewed-by: Ruifeng Wang 
> > ---
> 
> I wonder if other drivers have same bug?

I don't think this is a common bug.
This is a special case as the bug is introduced for whether choosing the memif 
Tx fast path. 

> 
> Reviewed-by: Stephen Hemminger 

[PATCH] net/iavf: support Tx LLDP on scalar and AVX512

[Zhichao Zeng]

This patch adds a testpmd command "set tx lldp on" which will register
an mbuf dynflag at the application level, currently only supported on.

The IAVF will fills the SWTCH_UPLINK bit in the Tx context descriptor
based on the mbuf dynflag to send the LLDP packets.

For avx512, need to close the Tx port first, then "set tx lldp on",
and reopen the port to select correct path.

Signed-off-by: Zhichao Zeng 
---
 doc/guides/rel_notes/release_24_03.rst  |  3 ++
 drivers/net/iavf/iavf_rxtx.c| 10 
 drivers/net/iavf/iavf_rxtx.h|  3 ++
 drivers/net/iavf/iavf_rxtx_vec_avx512.c | 17 +++
 drivers/net/iavf/iavf_rxtx_vec_common.h |  6 +++
 drivers/net/iavf/iavf_testpmd.c | 68 +
 drivers/net/iavf/meson.build|  3 ++
 drivers/net/iavf/rte_pmd_iavf.h |  2 +
 8 files changed, 112 insertions(+)
 create mode 100644 drivers/net/iavf/iavf_testpmd.c

diff --git a/doc/guides/rel_notes/release_24_03.rst 
b/doc/guides/rel_notes/release_24_03.rst
index e9c9717706..46576f62fe 100644
--- a/doc/guides/rel_notes/release_24_03.rst
+++ b/doc/guides/rel_notes/release_24_03.rst
@@ -55,6 +55,9 @@ New Features
  Also, make sure to start the actual text at the margin.
  ===
 
+* **Updated Intel iavf driver.**
+
+  * Added support for Tx LLDP packet on scalar and avx512.
 
 Removed Items
 -
diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index f19aa14646..a588b769a8 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -2427,6 +2427,8 @@ iavf_calc_context_desc(uint64_t flags, uint8_t vlan_flag)
if (flags & RTE_MBUF_F_TX_VLAN &&
vlan_flag & IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
return 1;
+   if (rte_mbuf_dynflag_lookup(IAVF_TX_LLDP_DYNFLAG, NULL) > 0)
+   return 1;
return 0;
 }
 
@@ -2445,6 +2447,9 @@ iavf_fill_ctx_desc_cmd_field(volatile uint64_t *field, 
struct rte_mbuf *m,
cmd |= IAVF_TX_CTX_DESC_IL2TAG2
<< IAVF_TXD_CTX_QW1_CMD_SHIFT;
}
+   if (rte_mbuf_dynflag_lookup(IAVF_TX_LLDP_DYNFLAG, NULL) > 0)
+   cmd |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
+   << IAVF_TXD_CTX_QW1_CMD_SHIFT;
 
*field |= cmd;
 }
@@ -4039,6 +4044,11 @@ iavf_set_tx_function(struct rte_eth_dev *dev)
dev->tx_pkt_prepare = iavf_prep_pkts;
PMD_DRV_LOG(DEBUG, "Using AVX512 OFFLOAD Vector 
Tx (port %d).",
dev->data->port_id);
+   } else if (check_ret == IAVF_VECTOR_CTX_PATH) {
+   dev->tx_pkt_burst = 
iavf_xmit_pkts_vec_avx512_ctx;
+   dev->tx_pkt_prepare = iavf_prep_pkts;
+   PMD_DRV_LOG(DEBUG, "Using AVX512 CONTEXT Vector 
Tx (port %d).",
+   dev->data->port_id);
} else {
dev->tx_pkt_burst = 
iavf_xmit_pkts_vec_avx512_ctx_offload;
dev->tx_pkt_prepare = iavf_prep_pkts;
diff --git a/drivers/net/iavf/iavf_rxtx.h b/drivers/net/iavf/iavf_rxtx.h
index f432f9d956..ee52864915 100644
--- a/drivers/net/iavf/iavf_rxtx.h
+++ b/drivers/net/iavf/iavf_rxtx.h
@@ -66,6 +66,7 @@
 #define IAVF_VECTOR_PATH 0
 #define IAVF_VECTOR_OFFLOAD_PATH 1
 #define IAVF_VECTOR_CTX_OFFLOAD_PATH 2
+#define IAVF_VECTOR_CTX_PATH 3
 
 #define DEFAULT_TX_RS_THRESH 32
 #define DEFAULT_TX_FREE_THRESH   32
@@ -752,6 +753,8 @@ uint16_t iavf_xmit_pkts_vec_avx512_offload(void *tx_queue,
   uint16_t nb_pkts);
 uint16_t iavf_xmit_pkts_vec_avx512_ctx_offload(void *tx_queue, struct rte_mbuf 
**tx_pkts,
  uint16_t nb_pkts);
+uint16_t iavf_xmit_pkts_vec_avx512_ctx(void *tx_queue, struct rte_mbuf 
**tx_pkts,
+ uint16_t nb_pkts);
 int iavf_txq_vec_setup_avx512(struct iavf_tx_queue *txq);
 
 uint8_t iavf_proto_xtr_type_to_rxdid(uint8_t xtr_type);
diff --git a/drivers/net/iavf/iavf_rxtx_vec_avx512.c 
b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
index 7a7df6d258..dc3354f331 100644
--- a/drivers/net/iavf/iavf_rxtx_vec_avx512.c
+++ b/drivers/net/iavf/iavf_rxtx_vec_avx512.c
@@ -2,6 +2,7 @@
  * Copyright(c) 2020 Intel Corporation
  */
 
+#include "rte_pmd_iavf.h"
 #include "iavf_rxtx_vec_common.h"
 
 #include 
@@ -2206,6 +2207,9 @@ ctx_vtx1(volatile struct iavf_tx_desc *txdp, struct 
rte_mbuf *pkt,
low_ctx_qw |= (uint64_t)pkt->vlan_tci << 
IAVF_TXD_CTX_QW0_L2TAG2_PARAM;
}
}
+   if (rte_mbuf_dynflag_lookup(IAVF_TX_LLDP_DYNFLAG, NULL) > 0)
+   high_ctx_qw |= IAVF_TX_CTX_DESC_SWTCH_UPLINK
+   << IAVF_TXD_CTX_QW1_CMD_SHIFT;
uint64_t high_data_qw = (IAVF_TX_DESC_DTYPE_DAT

RE: [EXT] [PATCH] buildtools/dpdk-cmdline-gen: fix code gen for IP addresses

[Sunil Kumar Kori]

> -Original Message-
> From: Bruce Richardson 
> Sent: Tuesday, December 5, 2023 5:01 PM
> To: dev@dpdk.org
> Cc: Bruce Richardson ; sta...@dpdk.org;
> Sunil Kumar Kori 
> Subject: [EXT] [PATCH] buildtools/dpdk-cmdline-gen: fix code gen for IP
> addresses
> 
> External Email
> 
> --
> The C code generated for the tokens for matching IP addresses in
> commandlines was missing the "static" prefix present in the output for the
> other data-types.
> 
> Fixes: 3791e9ed ("buildtools: add a tool to generate cmdline
> boilerplate")
> Cc: sta...@dpdk.org
> 
> Reported-by: Sunil Kumar Kori 
> Signed-off-by: Bruce Richardson 
> ---
>  buildtools/dpdk-cmdline-gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-
> gen.py index 49b03bee4a..bf1253d949 100755
> --- a/buildtools/dpdk-cmdline-gen.py
> +++ b/buildtools/dpdk-cmdline-gen.py
> @@ -71,7 +71,7 @@ def process_command(lineno, tokens, comment):
>  elif t_type in ["IP", "IP_ADDR", "IPADDR"]:
>  result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
>  initializers.append(
> -f"cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n"
> +f"static cmdline_parse_token_ipaddr_t
> cmd_{name}_{t_name}_tok =\n"
>  f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
>  )
>  elif t_type.startswith("(") and t_type.endswith(")"):
> --
Acked-by: Sunil Kumar Kori 

> 2.40.1

RE: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters

[Sunil Kumar Kori]

> -Original Message-
> From: Bruce Richardson 
> Sent: Tuesday, December 5, 2023 8:21 PM
> To: dev@dpdk.org
> Cc: Sunil Kumar Kori ; david.march...@redhat.com;
> Bruce Richardson 
> Subject: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional
> parameters
> 
> External Email
> 
> --
> Sometimes a user may want to have a command which takes an optional
> parameter. For commands with an optional constant string, this is no issue,
> as it can be configured as two separate commands, e.g.
> 
> start
> start tx_first.
> 
> However, if we want to have a variable parameter on the command, we hit
> issues, because variable tokens do not form part of the generated
> command names used for function and result-struct naming. To avoid
> duplicate name issues, we add a special syntax to allow the user to
> indicate that a particular variable parameter should be included in the
> name. Any leading variable names starting with a "__" will be included in
> command naming.
> 
> This then allows us to have:
> 
> start
> start tx_first
> start tx_first __n
> 
> without hitting any naming conflicts.
> 
It's a good option provided to user to choose name as per the need. I have 
another thought that how about providing flexibility to skip a token too along 
with implemented support.
Consider following example:
1. ethdev dev mtu  size
2. ethdev dev promiscuous <(on,off)>enable

So tokens and functions should be as cmd_ethdev_mtu_parsed() and cmd_ethdev_ 
promiscuous_parsed().
Here token dev  should be skipped. If it make sense, then please add 
this support too. 

> Signed-off-by: Bruce Richardson 
> ---
>  buildtools/dpdk-cmdline-gen.py|  9 -
>  doc/guides/prog_guide/cmdline.rst | 13 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> --
> 2.40.1

RE: [EXT] [PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address initializer

[Sunil Kumar Kori]

> -Original Message-
> From: Bruce Richardson 
> Sent: Tuesday, December 5, 2023 8:21 PM
> To: dev@dpdk.org
> Cc: Sunil Kumar Kori ; david.march...@redhat.com;
> Bruce Richardson ; sta...@dpdk.org
> Subject: [EXT] [PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address
> initializer
> 
> External Email
> 
> --
> The IP address type should be generic for both IPv4 and IPv6 and so use the
> cmdline lib's TOKEN_IPADDR_INITIALIZER rather than
> TOKEN_IPV4_INITIALIZER.
> 
> Fixes: 3791e9ed ("buildtools: add a tool to generate cmdline
> boilerplate")
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Bruce Richardson 
> ---
>  buildtools/dpdk-cmdline-gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-
> gen.py index faee4ffca7..8b4f22ca24 100755
> --- a/buildtools/dpdk-cmdline-gen.py
> +++ b/buildtools/dpdk-cmdline-gen.py
> @@ -79,7 +79,7 @@ def process_command(lineno, tokens, comment):
>  result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
>  initializers.append(
>  f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok
> =\n"
> -f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
> +f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
>  )
>  elif t_type.startswith("(") and t_type.endswith(")"):
>  result_struct.append(f"\tcmdline_fixed_string_t {t_name};")

Acked-by: Sunil Kumar Kori 
> --
> 2.40.1

RE: [EXT] [PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types

[Sunil Kumar Kori]

> -Original Message-
> From: Bruce Richardson 
> Sent: Tuesday, December 5, 2023 8:21 PM
> To: dev@dpdk.org
> Cc: Sunil Kumar Kori ; david.march...@redhat.com;
> Bruce Richardson 
> Subject: [EXT] [PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4
> and v6 types
> 
> External Email
> 
> --
> Add support for generating cmdline lib code to just match IPv4 addresses
> or IPv6 addresses, rather than IP addresses in general.
> 
> Signed-off-by: Bruce Richardson 
> ---
>  buildtools/dpdk-cmdline-gen.py| 12 
>  doc/guides/prog_guide/cmdline.rst |  4 
>  2 files changed, 16 insertions(+)
> 
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-
> gen.py index 8b4f22ca24..7dadded783 100755
> --- a/buildtools/dpdk-cmdline-gen.py
> +++ b/buildtools/dpdk-cmdline-gen.py
> @@ -81,6 +81,18 @@ def process_command(lineno, tokens, comment):
>  f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok
> =\n"
>  f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
>  )
> +elif t_type in ["IPV4", "IPv4", "IPV4_ADDR"]:
> +result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
> +initializers.append(
> +f"static cmdline_parse_token_ipaddr_t
> cmd_{name}_{t_name}_tok =\n"
> +f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
> +)
> +elif t_type in ["IPV6", "IPv6", "IPV6_ADDR"]:
> +result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
> +initializers.append(
> +f"static cmdline_parse_token_ipaddr_t
> cmd_{name}_{t_name}_tok =\n"
> +f"\tTOKEN_IPV6_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
> +)
>  elif t_type.startswith("(") and t_type.endswith(")"):
>  result_struct.append(f"\tcmdline_fixed_string_t {t_name};")
>  t_val = f'"{t_type[1:-1].replace(",","#")}"'
> diff --git a/doc/guides/prog_guide/cmdline.rst
> b/doc/guides/prog_guide/cmdline.rst
> index fc32d727dc..f62f17f1aa 100644
> --- a/doc/guides/prog_guide/cmdline.rst
> +++ b/doc/guides/prog_guide/cmdline.rst
> @@ -70,6 +70,10 @@ The format of the list file must be:
> 
>* ``src_ip``
> 
> +  * ``dst_ip4``
> +
> +  * ``dst_ip6``
> +
>  * Variable fields, which take their values from a list of options,
>have the comma-separated option list placed in braces, rather than a the
> type name.
>For example,

Acked-by: Sunil Kumar Kori 
> --
> 2.40.1