[RFC 0/7] Introduce FreeBSD macros for SAFE iteration

[RFC 0/7] Introduce FreeBSD macros for SAFE iteration

[Stephen Hemminger]

This series adds common macros for safe iteration over lists.
It is a subset copy of the macros from FreeBSD that are
missing from the Linux header sys/queue.h

Chose this over several other options:
  - let each driver define their own as needed.
    One Intel driver got it wrong, others will as well.
  - rename all the queue macros to RTE_XXX variants.
    Seems like useless renaming and confusion.
  - Several distros have libbsd package with the correct macros.
    But adding yet another dependency to DPDK would be annoying
    for something this basic.

There are more macros in FreeBSD header that could be useful,
but we can add those later as needed here.

Stephen Hemminger (7):
  eal: add queue macro extensions from FreeBSD
  net/qede: fix use after free
  bus/fslmc: fix use after free
  net/bnxt: fix use after free
  net/iavf: replace local version of TAILQ_FOREACH_SAFE
  vhost: replace open coded TAILQ_FOREACH_SAFE
  raw/ifpga: use EAL version of TAILQ_FOREACH_SAFE

 drivers/bus/fslmc/portal/dpaa2_hw_dpio.c |   5 +-
 drivers/net/bnxt/bnxt_filter.c           |   8 +-
 drivers/net/iavf/iavf_vchnl.c            |   8 +-
 drivers/net/qede/qede_ethdev.h           |   3 +-
 drivers/net/qede/qede_filter.c           |  13 +-
 drivers/raw/ifpga/base/opae_osdep.h      |   1 +
 lib/eal/include/meson.build              |   3 +-
 lib/eal/include/rte_queue.h              | 174 +++++++++++++++++++++++
 lib/vhost/socket.c                       |  11 +-
 9 files changed, 193 insertions(+), 33 deletions(-)
 create mode 100644 lib/eal/include/rte_queue.h

-- 
2.45.2

[RFC 1/7] eal: add queue macro extensions from FreeBSD

[Stephen Hemminger]

The Linux version of sys/queue.h is frozen at an older version
and is missing the _SAFE macro variants. Several drivers started
introducing the own workarounds for this. Should be handled in EAL.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/meson.build |   3 +-
 lib/eal/include/rte_queue.h | 174 ++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 lib/eal/include/rte_queue.h

diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
index d903577caa..75bf09381f 100644
--- a/lib/eal/include/meson.build
+++ b/lib/eal/include/meson.build
@@ -19,8 +19,8 @@ headers += files(
         'rte_eal.h',
         'rte_eal_memconfig.h',
         'rte_eal_trace.h',
-        'rte_errno.h',
         'rte_epoll.h',
+        'rte_errno.h',
         'rte_fbarray.h',
         'rte_hexdump.h',
         'rte_hypervisor.h',
@@ -38,6 +38,7 @@ headers += files(
         'rte_pci_dev_features.h',
         'rte_per_lcore.h',
         'rte_pflock.h',
+        'rte_queue.h',
         'rte_random.h',
         'rte_reciprocal.h',
         'rte_seqcount.h',
diff --git a/lib/eal/include/rte_queue.h b/lib/eal/include/rte_queue.h
new file mode 100644
index 0000000000..7ec807b15d
--- /dev/null
+++ b/lib/eal/include/rte_queue.h
@@ -0,0 +1,174 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ *
+ * Original Copyright (c) 1991, 1993
+ *	The Regents of the University of California.  All rights reserved.
+ */
+
+#ifndef _RTE_QUEUE_H_
+#define _RTE_QUEUE_H_
+
+/**
+ * @file
+ *  Defines macro's that exist in the FreeBSD version of queue.h
+ *  which are missing in other versions.
+ */
+
+#include <sys/queue.h>
+
+/*
+ * This file defines four types of data structures: singly-linked lists,
+ * singly-linked tail queues, lists and tail queues.
+ *
+ * Below is a summary of implemented functions where:
+ *  o  means the macro exists in original version
+ *  +  means the macro is added here
+ *  -  means the macro is not available
+ *  s  means the macro is available but is slow (runs in O(n) time)
+ *
+ *				SLIST	LIST	STAILQ	TAILQ
+ * _HEAD			o	o	o	o
+ * _HEAD_INITIALIZER		o	o	o	o
+ * _ENTRY			o	o	o	o
+ * _INIT			o	o	o	o
+ * _EMPTY			o	o	o	o
+ * _FIRST			o	o	o	o
+ * _NEXT			o	o	o	o
+ * _FOREACH			o	o	o	o
+ * _FOREACH_FROM		+	+	+	+
+ * _FOREACH_SAFE		+	+	+	+
+ * _FOREACH_FROM_SAFE		+	+	+	+
+ * _FOREACH_REVERSE		-	-	-	o
+ * _FOREACH_REVERSE_FROM	-	-	-	+
+ * _FOREACH_REVERSE_SAFE	-	-	-       +
+ * _FOREACH_REVERSE_FROM_SAFE	-	-	-	+
+ * _INSERT_HEAD			o	o	o	o
+ * _INSERT_BEFORE		-	o	-	o
+ * _INSERT_AFTER		o	o	o	o
+ * _INSERT_TAIL			-	-	o	o
+ * _CONCAT			s	s	o	o
+ * _REMOVE_AFTER		o	-	o	-
+ * _REMOVE_HEAD			o	o	o	o
+ * _REMOVE			s	o	s	o
+ *
+ */
+
+
+/*
+ * Singly-linked List declarations.
+ */
+#ifndef SLIST_FOREACH_FROM
+#define	SLIST_FOREACH_FROM(var, head, field)				\
+	for ((var) = ((var) ? (var) : SLIST_FIRST((head)));		\
+	    (var);							\
+	    (var) = SLIST_NEXT((var), field))
+#endif
+
+#ifndef SLIST_FOREACH_SAFE
+#define	SLIST_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = SLIST_FIRST((head));				\
+	    (var) && ((tvar) = SLIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+#ifndef SLIST_FOREACH_FROM_SAFE
+#define	SLIST_FOREACH_FROM_SAFE(var, head, field, tvar)			\
+	for ((var) = ((var) ? (var) : SLIST_FIRST((head)));		\
+	    (var) && ((tvar) = SLIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+
+/*
+ * Singly-linked Tail queue declarations.
+ */
+#ifndef STAILQ_FOREACH_FROM
+#define	STAILQ_FOREACH_FROM(var, head, field)				\
+	for ((var) = ((var) ? (var) : STAILQ_FIRST((head)));		\
+	   (var);							\
+	   (var) = STAILQ_NEXT((var), field))
+#endif
+
+#ifndef STAILQ_FOREACH_SAFE
+#define	STAILQ_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = STAILQ_FIRST((head));				\
+	    (var) && ((tvar) = STAILQ_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+#ifndef STAILQ_FOREACH_FROM_SAFE
+#define	STAILQ_FOREACH_FROM_SAFE(var, head, field, tvar)		\
+	for ((var) = ((var) ? (var) : STAILQ_FIRST((head)));		\
+	    (var) && ((tvar) = STAILQ_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+/*
+ * List declarations.
+ */
+#ifndef LIST_FOREACH_FROM
+#define	LIST_FOREACH_FROM(var, head, field)				\
+	for ((var) = ((var) ? (var) : LIST_FIRST((head)));		\
+	    (var);							\
+	    (var) = LIST_NEXT((var), field))
+#endif
+
+#ifndef LIST_FOREACH_SAFE
+#define	LIST_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = LIST_FIRST((head));				\
+	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+#ifndef LIST_FOREACH_FROM_SAFE
+#define	LIST_FOREACH_FROM_SAFE(var, head, field, tvar)			\
+	for ((var) = ((var) ? (var) : LIST_FIRST((head)));		\
+	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+/*
+ * Tail queue declarations.
+ */
+#ifndef TAILQ_FOREACH_FROM
+#define TAILQ_FOREACH_FROM(var, head, field)				\
+	for ((var) = ((var) ? (var) : TAILQ_FIRST((head)));		\
+	    (var);							\
+	    (var) = TAILQ_NEXT((var), field))
+#endif
+
+#ifndef TAILQ_FOREACH_SAFE
+#define TAILQ_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = TAILQ_FIRST((head));				\
+	    (var) && ((tvar) = TAILQ_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+#ifndef TAILQ_FOREACH_FROM_SAFE
+#define TAILQ_FOREACH_FROM_SAFE(var, head, field, tvar)			\
+	for ((var) = ((var) ? (var) : TAILQ_FIRST((head)));		\
+	    (var) && ((tvar) = TAILQ_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
+#ifndef TAILQ_FOREACH_REVERSE_FROM
+#define TAILQ_FOREACH_REVERSE_FROM(var, head, headname, field)		\
+	for ((var) = ((var) ? (var) : TAILQ_LAST((head), headname));	\
+	    (var);							\
+	    (var) = TAILQ_PREV((var), headname, field))
+#endif
+
+#ifndef TAILQ_FOREACH_REVERSE_SAFE
+#define TAILQ_FOREACH_REVERSE_SAFE(var, head, headname, field, tvar)	\
+	for ((var) = TAILQ_LAST((head), headname);			\
+	    (var) && ((tvar) = TAILQ_PREV((var), headname, field), 1);	\
+	    (var) = (tvar))
+#endif
+
+#ifndef TAILQ_FOREACH_REVERSE_FROM_SAFE
+#define TAILQ_FOREACH_REVERSE_FROM_SAFE(var, head, headname, field, tvar)\
+	for ((var) = ((var) ? (var) : TAILQ_LAST((head), headname));	\
+	    (var) && ((tvar) = TAILQ_PREV((var), headname, field), 1);	\
+	    (var) = (tvar))
+#endif
+
+#endif /* !_RTE_QUEUE_H_ */
-- 
2.45.2

[RFC 2/7] net/qede: fix use after free

[Stephen Hemminger]

The loop cleaning up flowdir resources was using SLIST_FOREACH
but the inner loop would call rte_free. Found by building with
address sanitizer undefined check.

Also remove needless initialization, and null check.

Fixes: f5765f66f9bb ("net/qede: refactor flow director into generic aRFS")
Cc: shahed.shaikh@cavium.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/qede/qede_ethdev.h |  3 +--
 drivers/net/qede/qede_filter.c | 13 +++++--------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/net/qede/qede_ethdev.h b/drivers/net/qede/qede_ethdev.h
index 3015c18504..b502658425 100644
--- a/drivers/net/qede/qede_ethdev.h
+++ b/drivers/net/qede/qede_ethdev.h
@@ -8,8 +8,7 @@
 #ifndef _QEDE_ETHDEV_H_
 #define _QEDE_ETHDEV_H_
 
-#include <sys/queue.h>
-
+#include <rte_queue.h>
 #include <rte_ether.h>
 #include <ethdev_driver.h>
 #include <ethdev_pci.h>
diff --git a/drivers/net/qede/qede_filter.c b/drivers/net/qede/qede_filter.c
index 14fb4338e9..c3f74c89d9 100644
--- a/drivers/net/qede/qede_filter.c
+++ b/drivers/net/qede/qede_filter.c
@@ -154,15 +154,12 @@ int qede_check_fdir_support(struct rte_eth_dev *eth_dev)
 void qede_fdir_dealloc_resc(struct rte_eth_dev *eth_dev)
 {
 	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
-	struct qede_arfs_entry *tmp = NULL;
+	struct qede_arfs_entry *tmp, *tmp2;
 
-	SLIST_FOREACH(tmp, &qdev->arfs_info.arfs_list_head, list) {
-		if (tmp) {
-			rte_memzone_free(tmp->mz);
-			SLIST_REMOVE(&qdev->arfs_info.arfs_list_head, tmp,
-				     qede_arfs_entry, list);
-			rte_free(tmp);
-		}
+	SLIST_FOREACH_SAFE(tmp, &qdev->arfs_info.arfs_list_head, list, tmp2) {
+		rte_memzone_free(tmp->mz);
+		SLIST_REMOVE(&qdev->arfs_info.arfs_list_head, tmp, qede_arfs_entry, list);
+		rte_free(tmp);
 	}
 }
 
-- 
2.45.2

[RFC 3/7] bus/fslmc: fix use after free

[Stephen Hemminger]

The cleanup loop would deference the dpio_dev after freeing.
Use TAILQ_FOREACH_SAFE to fix that.
Found by building with sanitizer undefined flag.

Fixes: e55d0494ab98 ("bus/fslmc: support secondary process")
Cc: shreyansh.jain@nxp.com
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/fslmc/portal/dpaa2_hw_dpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c b/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c
index 2dfcf7a498..6ae15c2054 100644
--- a/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c
+++ b/drivers/bus/fslmc/portal/dpaa2_hw_dpio.c
@@ -15,7 +15,6 @@
 #include <signal.h>
 #include <pthread.h>
 #include <sys/types.h>
-#include <sys/queue.h>
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/mman.h>
@@ -27,6 +26,7 @@
 #include <ethdev_driver.h>
 #include <rte_malloc.h>
 #include <rte_memcpy.h>
+#include <rte_queue.h>
 #include <rte_string_fns.h>
 #include <rte_cycles.h>
 #include <rte_kvargs.h>
@@ -403,6 +403,7 @@ dpaa2_create_dpio_device(int vdev_fd,
 	struct rte_dpaa2_device *obj)
 {
 	struct dpaa2_dpio_dev *dpio_dev = NULL;
+	struct dpaa2_dpio_dev *dpio_tmp;
 	struct vfio_region_info reg_info = { .argsz = sizeof(reg_info)};
 	struct qbman_swp_desc p_des;
 	struct dpio_attr attr;
@@ -588,7 +589,7 @@ dpaa2_create_dpio_device(int vdev_fd,
 	rte_free(dpio_dev);
 
 	/* For each element in the list, cleanup */
-	TAILQ_FOREACH(dpio_dev, &dpio_dev_list, next) {
+	TAILQ_FOREACH_SAFE(dpio_dev, &dpio_dev_list, next, dpio_tmp) {
 		if (dpio_dev->dpio) {
 			dpio_disable(dpio_dev->dpio, CMD_PRI_LOW,
 				dpio_dev->token);
-- 
2.45.2

[RFC 4/7] net/bnxt: fix use after free

[Stephen Hemminger]

The filter cleanup loop was using STAILQ_FOREACH and rte_free
and would dereference the filter after free.

Found by build with -Dbsanitize=address,undefined

Fixes: e8fe0e067b68 ("net/bnxt: fix allocation of PF info struct")
Cc: ajit.khaparde@broadcom.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/bnxt/bnxt_filter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_filter.c b/drivers/net/bnxt/bnxt_filter.c
index 7b90ba651f..f083f3aa94 100644
--- a/drivers/net/bnxt/bnxt_filter.c
+++ b/drivers/net/bnxt/bnxt_filter.c
@@ -3,14 +3,12 @@
  * All rights reserved.
  */
 
-#include <sys/queue.h>
-
 #include <rte_byteorder.h>
 #include <rte_log.h>
 #include <rte_malloc.h>
 #include <rte_flow.h>
 #include <rte_flow_driver.h>
-#include <rte_tailq.h>
+#include <rte_queue.h>
 
 #include "bnxt.h"
 #include "bnxt_filter.h"
@@ -151,7 +149,9 @@ void bnxt_free_filter_mem(struct bnxt *bp)
 	bp->filter_info = NULL;
 
 	for (i = 0; i < bp->pf->max_vfs; i++) {
-		STAILQ_FOREACH(filter, &bp->pf->vf_info[i].filter, next) {
+		struct bnxt_filter_info *tmp;
+
+		STAILQ_FOREACH_SAFE(filter, &bp->pf->vf_info[i].filter, next, tmp) {
 			rte_free(filter);
 			STAILQ_REMOVE(&bp->pf->vf_info[i].filter, filter,
 				      bnxt_filter_info, next);
-- 
2.45.2

Re: [RFC 4/7] net/bnxt: fix use after free

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On Mon, Jan 27, 2025 at 10:08 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The filter cleanup loop was using STAILQ_FOREACH and rte_free
> and would dereference the filter after free.
>
> Found by build with -Dbsanitize=address,undefined
>
> Fixes: e8fe0e067b68 ("net/bnxt: fix allocation of PF info struct")
> Cc: ajit.khaparde@broadcom.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  drivers/net/bnxt/bnxt_filter.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_filter.c b/drivers/net/bnxt/bnxt_filter.c
> index 7b90ba651f..f083f3aa94 100644
> --- a/drivers/net/bnxt/bnxt_filter.c
> +++ b/drivers/net/bnxt/bnxt_filter.c
> @@ -3,14 +3,12 @@
>   * All rights reserved.
>   */
>
> -#include <sys/queue.h>
> -
>  #include <rte_byteorder.h>
>  #include <rte_log.h>
>  #include <rte_malloc.h>
>  #include <rte_flow.h>
>  #include <rte_flow_driver.h>
> -#include <rte_tailq.h>
> +#include <rte_queue.h>
>
>  #include "bnxt.h"
>  #include "bnxt_filter.h"
> @@ -151,7 +149,9 @@ void bnxt_free_filter_mem(struct bnxt *bp)
>         bp->filter_info = NULL;
>
>         for (i = 0; i < bp->pf->max_vfs; i++) {
> -               STAILQ_FOREACH(filter, &bp->pf->vf_info[i].filter, next) {
> +               struct bnxt_filter_info *tmp;
> +
> +               STAILQ_FOREACH_SAFE(filter, &bp->pf->vf_info[i].filter, next, tmp) {
>                         rte_free(filter);
>                         STAILQ_REMOVE(&bp->pf->vf_info[i].filter, filter,
>                                       bnxt_filter_info, next);
> --
> 2.45.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

[RFC 5/7] net/iavf: replace local version of TAILQ_FOREACH_SAFE

[Stephen Hemminger]

Now in EAL as rte_queue.h

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/iavf/iavf_vchnl.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 065ab3594c..c9684ae189 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -13,6 +13,7 @@
 #include <rte_byteorder.h>
 #include <rte_common.h>
 #include <rte_os_shim.h>
+#include <rte_queue.h>
 
 #include <rte_debug.h>
 #include <rte_alarm.h>
@@ -52,13 +53,6 @@ static struct iavf_event_handler event_handler = {
 	.fd = {-1, -1},
 };
 
-#ifndef TAILQ_FOREACH_SAFE
-#define TAILQ_FOREACH_SAFE(var, head, field, tvar) \
-	for ((var) = TAILQ_FIRST((head)); \
-		(var) && ((tvar) = TAILQ_NEXT((var), field), 1); \
-	(var) = (tvar))
-#endif
-
 static uint32_t
 iavf_dev_event_handle(void *param __rte_unused)
 {
-- 
2.45.2

[RFC 6/7] vhost: replace open coded TAILQ_FOREACH_SAFE

[Stephen Hemminger]

Proper macro is now in EAL rte_queue.h use it instead.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/vhost/socket.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index 05a7e5902f..625ed08b7c 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -10,10 +10,10 @@
 #include <string.h>
 #include <sys/socket.h>
 #include <sys/un.h>
-#include <sys/queue.h>
 #include <errno.h>
 #include <fcntl.h>
 
+#include <rte_queue.h>
 #include <rte_thread.h>
 #include <rte_log.h>
 
@@ -458,14 +458,7 @@ vhost_user_client_reconnect(void *arg __rte_unused)
 	while (1) {
 		pthread_mutex_lock(&reconn_list.mutex);
 
-		/*
-		 * An equal implementation of TAILQ_FOREACH_SAFE,
-		 * which does not exist on all platforms.
-		 */
-		for (reconn = TAILQ_FIRST(&reconn_list.head);
-		     reconn != NULL; reconn = next) {
-			next = TAILQ_NEXT(reconn, next);
-
+		TAILQ_FOREACH_SAFE(reconn, &reconn_list.head, next, next) {
 			ret = vhost_user_connect_nonblock(reconn->vsocket->path, reconn->fd,
 						(struct sockaddr *)&reconn->un,
 						sizeof(reconn->un));
-- 
2.45.2

[RFC 7/7] raw/ifpga: use EAL version of TAILQ_FOREACH_SAFE

[Stephen Hemminger]

Prefer the EAL version over local version of macro.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/raw/ifpga/base/opae_osdep.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/raw/ifpga/base/opae_osdep.h b/drivers/raw/ifpga/base/opae_osdep.h
index e35a21c80e..b483d00a54 100644
--- a/drivers/raw/ifpga/base/opae_osdep.h
+++ b/drivers/raw/ifpga/base/opae_osdep.h
@@ -11,6 +11,7 @@
 
 #ifdef RTE_LIB_EAL
 #include "osdep_rte/osdep_generic.h"
+#include <rte_queue.h>
 #else
 #include "osdep_raw/osdep_generic.h"
 #endif
-- 
2.45.2

Re: [RFC 0/7] Introduce FreeBSD macros for SAFE iteration

[Bruce Richardson]

On Mon, Jan 27, 2025 at 10:03:54AM -0800, Stephen Hemminger wrote:
> This series adds common macros for safe iteration over lists.
> It is a subset copy of the macros from FreeBSD that are
> missing from the Linux header sys/queue.h
> 
> Chose this over several other options:
>   - let each driver define their own as needed.
>     One Intel driver got it wrong, others will as well.
>   - rename all the queue macros to RTE_XXX variants.
>     Seems like useless renaming and confusion.
>   - Several distros have libbsd package with the correct macros.
>     But adding yet another dependency to DPDK would be annoying
>     for something this basic.
> 

Actually, I wouldn't be that quick to eliminate the last option. It may
give us some additional options for simplification. For example, the
strlcpy and strlcat functions are in libbsd too, and if we had that as
mandatory dependency, perhaps we could remove some extra code there too?

/Bruce

Re: [RFC 0/7] Introduce FreeBSD macros for SAFE iteration

[Stephen Hemminger]

On Mon, 27 Jan 2025 18:16:18 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Mon, Jan 27, 2025 at 10:03:54AM -0800, Stephen Hemminger wrote:
> > This series adds common macros for safe iteration over lists.
> > It is a subset copy of the macros from FreeBSD that are
> > missing from the Linux header sys/queue.h
> > 
> > Chose this over several other options:
> >   - let each driver define their own as needed.
> >     One Intel driver got it wrong, others will as well.
> >   - rename all the queue macros to RTE_XXX variants.
> >     Seems like useless renaming and confusion.
> >   - Several distros have libbsd package with the correct macros.
> >     But adding yet another dependency to DPDK would be annoying
> >     for something this basic.
> >   
> 
> Actually, I wouldn't be that quick to eliminate the last option. It may
> give us some additional options for simplification. For example, the
> strlcpy and strlcat functions are in libbsd too, and if we had that as
> mandatory dependency, perhaps we could remove some extra code there too?
> 
> /Bruce
> 

I would be ok with using libbsd but only if we didn't have to keep a parallel
copy for all the other compiler and OS variants. And would it be global or
a per-driver dependency?

RE: [RFC 0/7] Introduce FreeBSD macros for SAFE iteration

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 27 January 2025 19.44
> 
> On Mon, 27 Jan 2025 18:16:18 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Mon, Jan 27, 2025 at 10:03:54AM -0800, Stephen Hemminger wrote:
> > > This series adds common macros for safe iteration over lists.
> > > It is a subset copy of the macros from FreeBSD that are
> > > missing from the Linux header sys/queue.h
> > >
> > > Chose this over several other options:
> > >   - let each driver define their own as needed.
> > >     One Intel driver got it wrong, others will as well.
> > >   - rename all the queue macros to RTE_XXX variants.
> > >     Seems like useless renaming and confusion.
> > >   - Several distros have libbsd package with the correct macros.
> > >     But adding yet another dependency to DPDK would be annoying
> > >     for something this basic.
> > >
> >
> > Actually, I wouldn't be that quick to eliminate the last option. It
> may
> > give us some additional options for simplification. For example, the
> > strlcpy and strlcat functions are in libbsd too, and if we had that
> as
> > mandatory dependency, perhaps we could remove some extra code there
> too?
> >
> > /Bruce
> >
> 
> I would be ok with using libbsd but only if we didn't have to keep a
> parallel
> copy for all the other compiler and OS variants. And would it be global
> or
> a per-driver dependency?

+1 to providing our own implementations of relevant libbsd features in the DPDK EAL, rather than depending on the entire libbsd (and libbsd-dev for development). Providing these features as part of a "utilities library" (which is currently integrated into the EAL) is better for non-Unix environments.

Furthermore, libbsd has plenty of stuff we don't need:
https://manpages.debian.org/testing/libbsd-dev/index.html

Re: [RFC 0/7] Introduce FreeBSD macros for SAFE iteration

[Stephen Hemminger]

On Mon, 27 Jan 2025 20:29:55 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Monday, 27 January 2025 19.44
> > 
> > On Mon, 27 Jan 2025 18:16:18 +0000
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >   
> > > On Mon, Jan 27, 2025 at 10:03:54AM -0800, Stephen Hemminger wrote:  
> > > > This series adds common macros for safe iteration over lists.
> > > > It is a subset copy of the macros from FreeBSD that are
> > > > missing from the Linux header sys/queue.h
> > > >
> > > > Chose this over several other options:
> > > >   - let each driver define their own as needed.
> > > >     One Intel driver got it wrong, others will as well.
> > > >   - rename all the queue macros to RTE_XXX variants.
> > > >     Seems like useless renaming and confusion.
> > > >   - Several distros have libbsd package with the correct macros.
> > > >     But adding yet another dependency to DPDK would be annoying
> > > >     for something this basic.
> > > >  
> > >
> > > Actually, I wouldn't be that quick to eliminate the last option. It  
> > may  
> > > give us some additional options for simplification. For example, the
> > > strlcpy and strlcat functions are in libbsd too, and if we had that  
> > as  
> > > mandatory dependency, perhaps we could remove some extra code there  
> > too?  
> > >
> > > /Bruce
> > >  
> > 
> > I would be ok with using libbsd but only if we didn't have to keep a
> > parallel
> > copy for all the other compiler and OS variants. And would it be global
> > or
> > a per-driver dependency?  
> 
> +1 to providing our own implementations of relevant libbsd features in the DPDK EAL, rather than depending on the entire libbsd (and libbsd-dev for development). Providing these features as part of a "utilities library" (which is currently integrated into the EAL) is better for non-Unix environments.
> 
> Furthermore, libbsd has plenty of stuff we don't need:
> https://manpages.debian.org/testing/libbsd-dev/index.html

The red-black tries in libbsd are very useful. In one product we used it as a way
to manage LPM rules, since the current DPDK model is O(N^2) and works terribly in
a router with 3M routes.