[PATCH v02] net/af_packet: add rollover and defrag options

[PATCH v02] net/af_packet: add rollover and defrag options

[Gur Stavi]

net_af_packet PMD multi "queue" support relies on Linux FANOUT capability.
Linux FANOUT is a SW based load balancer that is similar to HW RSS which
is more common for DPDK PMDs. Instead of multiple HW descriptor queues,
AF PACKET uses multiple sockets.
HW RSS will typically drop a packet if its selected RX queue is empty.
However, Linux FANOUT, as a SW load balancer, can be configured to avoid
this packet drop by rolling over to the next socket.
This rollover functionality was ALWAYS enabled in net_af_packet. It is
surrounded by ifdef, but only to allow compilation on ancient Linux
versions that did not have it.

Since DPDK applications are usually designed for HW based PMDs, this
rollover functionality, which the developers are likely unaware of, could
be confusing.

Another option that is part of Linux FANOUT is DEFRAG that instructs Linux
to compose complete IP packet out of fragments before delivering it to the
PACKET socket. Again, this behavior typically does not exist for HW based
PMDs and may confuse users.

This patch adds 2 options to control these features:
rollover=[0|1],defrag=[0|1]
For backward compatibility both features are enabled by default even
though most users will probably want both of them disabled.

Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
---
v2:
* Improve error handling for parsing of arg strings. Add a wrapper around
  strtoul to replace atoi.
* Fix checkpatch indentation error due to copy-paste.

v1: https://mails.dpdk.org/archives/dev/2024-October/307451.html
---
 doc/guides/nics/af_packet.rst             |  4 ++
 drivers/net/af_packet/rte_eth_af_packet.c | 77 ++++++++++++++++++-----
 2 files changed, 65 insertions(+), 16 deletions(-)

diff --git a/doc/guides/nics/af_packet.rst b/doc/guides/nics/af_packet.rst
index 66b977e1a2..4bc748b50b 100644
--- a/doc/guides/nics/af_packet.rst
+++ b/doc/guides/nics/af_packet.rst
@@ -29,6 +29,8 @@ Some of these, in turn, will be used to configure the PACKET_MMAP settings.
 *   ``framesz`` - PACKET_MMAP frame size (optional, default 2048B; Note: multiple
     of 16B);
 *   ``framecnt`` - PACKET_MMAP frame count (optional, default 512).
+*   ``rollover`` - disable(0)/enable(1) PACKET_FANOUT_ROLLOVER (optional, default 1).
+*   ``defrag`` - disable(0)/enable(1) PACKET_FANOUT_FLAG_DEFRAG (optional, default 1).

 Because this implementation is based on PACKET_MMAP, and PACKET_MMAP has its
 own pre-requisites, it should be noted that the inner workings of PACKET_MMAP
@@ -47,6 +49,8 @@ For the full details behind PACKET_MMAP's structures and settings, consider
 reading the `PACKET_MMAP documentation in the Kernel
 <https://www.kernel.org/doc/Documentation/networking/packet_mmap.txt>`_.

+For details of ``rollover`` anb ``defrag`` refer to the *packet(7)* man page.
+
 Prerequisites
 -------------

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 68870dd3e2..1a043e8398 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -36,6 +36,8 @@
 #define ETH_AF_PACKET_FRAMESIZE_ARG	"framesz"
 #define ETH_AF_PACKET_FRAMECOUNT_ARG	"framecnt"
 #define ETH_AF_PACKET_QDISC_BYPASS_ARG	"qdisc_bypass"
+#define ETH_AF_PACKET_ROLLOVER		"rollover"
+#define ETH_AF_PACKET_DEFRAG		"defrag"

 #define DFLT_FRAME_SIZE		(1 << 11)
 #define DFLT_FRAME_COUNT	(1 << 9)
@@ -91,6 +93,8 @@ static const char *valid_arguments[] = {
 	ETH_AF_PACKET_FRAMESIZE_ARG,
 	ETH_AF_PACKET_FRAMECOUNT_ARG,
 	ETH_AF_PACKET_QDISC_BYPASS_ARG,
+	ETH_AF_PACKET_ROLLOVER,
+	ETH_AF_PACKET_DEFRAG,
 	NULL
 };

@@ -659,6 +663,20 @@ open_packet_iface(const char *key __rte_unused,
 	return 0;
 }

+static int
+uint_arg_parse(const char *arg_str, unsigned int *arg_val)
+{
+	unsigned long val;
+	char *endptr;
+
+	val = strtoul(arg_str, &endptr, 10);
+	if (*arg_str == '\0' || *endptr != '\0')
+		return -EINVAL;
+
+	*arg_val = val;
+	return 0;
+}
+
 static int
 rte_pmd_init_internals(struct rte_vdev_device *dev,
                        const int sockfd,
@@ -676,6 +694,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	const unsigned int numa_node = dev->device.numa_node;
 	struct rte_eth_dev_data *data = NULL;
 	struct rte_kvargs_pair *pair = NULL;
+	const char *ifname = NULL;
 	struct ifreq ifr;
 	size_t ifnamelen;
 	unsigned k_idx;
@@ -686,6 +705,8 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	int rc, tpver, discard;
 	int qsockfd = -1;
 	unsigned int i, q, rdsize;
+	unsigned int rollover = 1;
+	unsigned int defrag = 1;
 #if defined(PACKET_FANOUT)
 	int fanout_arg;
 #endif
@@ -693,9 +714,30 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	for (k_idx = 0; k_idx < kvlist->count; k_idx++) {
 		pair = &kvlist->pairs[k_idx];
 		if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL)
-			break;
+			ifname = pair->value;
+		if (strcmp(pair->key, ETH_AF_PACKET_ROLLOVER) == 0) {
+			rc = uint_arg_parse(pair->value, &rollover);
+			if (rc || rollover > 1) {
+				PMD_LOG(ERR,
+					"%s: invalid rollover value: %s",
+					name, pair->value);
+				return -1;
+			}
+			continue;
+		}
+		if (strcmp(pair->key, ETH_AF_PACKET_DEFRAG) == 0) {
+			rc = uint_arg_parse(pair->value, &defrag);
+			if (rc || defrag > 1) {
+				PMD_LOG(ERR,
+					"%s: invalid defrag value: %s",
+					name, pair->value);
+				return -1;
+			}
+			continue;
+		}
 	}
-	if (pair == NULL) {
+
+	if (ifname == NULL) {
 		PMD_LOG(ERR,
 			"%s: no interface specified for AF_PACKET ethdev",
 		        name);
@@ -738,21 +780,21 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	req->tp_frame_size = framesize;
 	req->tp_frame_nr = framecnt;

-	ifnamelen = strlen(pair->value);
+	ifnamelen = strlen(ifname);
 	if (ifnamelen < sizeof(ifr.ifr_name)) {
-		memcpy(ifr.ifr_name, pair->value, ifnamelen);
+		memcpy(ifr.ifr_name, ifname, ifnamelen);
 		ifr.ifr_name[ifnamelen] = '\0';
 	} else {
 		PMD_LOG(ERR,
 			"%s: I/F name too long (%s)",
-			name, pair->value);
+			name, ifname);
 		goto free_internals;
 	}
 	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
 		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
 		goto free_internals;
 	}
-	(*internals)->if_name = strdup(pair->value);
+	(*internals)->if_name = strdup(ifname);
 	if ((*internals)->if_name == NULL)
 		goto free_internals;
 	(*internals)->if_index = ifr.ifr_ifindex;
@@ -770,9 +812,12 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,

 #if defined(PACKET_FANOUT)
 	fanout_arg = (getpid() ^ (*internals)->if_index) & 0xffff;
-	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG) << 16;
+	fanout_arg |= PACKET_FANOUT_HASH << 16;
+	if (defrag)
+		fanout_arg |= PACKET_FANOUT_FLAG_DEFRAG << 16;
 #if defined(PACKET_FANOUT_FLAG_ROLLOVER)
-	fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
+	if (rollover)
+		fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
 #endif
 #endif

@@ -792,7 +837,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not set PACKET_VERSION on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}

@@ -802,7 +847,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not set PACKET_LOSS on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}

@@ -813,7 +858,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 			if (rc == -1) {
 				PMD_LOG_ERRNO(ERR,
 					"%s: could not set PACKET_QDISC_BYPASS on AF_PACKET socket for %s",
-					name, pair->value);
+					name, ifname);
 				goto error;
 			}
 #endif
@@ -823,7 +868,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not set PACKET_RX_RING on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}

@@ -831,7 +876,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not set PACKET_TX_RING on AF_PACKET "
-				"socket for %s", name, pair->value);
+				"socket for %s", name, ifname);
 			goto error;
 		}

@@ -844,7 +889,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rx_queue->map == MAP_FAILED) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: call to mmap failed on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}

@@ -881,7 +926,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not bind AF_PACKET socket to %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}

@@ -891,7 +936,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG_ERRNO(ERR,
 				"%s: could not set PACKET_FANOUT on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 #endif

base-commit: 98613d32e3dac58d685f4f236cf8cc9733abaaf3
--
2.40.1

Re: [PATCH v02] net/af_packet: add rollover and defrag options

[Ferruh Yigit]

On 10/31/2024 1:50 PM, Gur Stavi wrote:
> net_af_packet PMD multi "queue" support relies on Linux FANOUT capability.
> Linux FANOUT is a SW based load balancer that is similar to HW RSS which
> is more common for DPDK PMDs. Instead of multiple HW descriptor queues,
> AF PACKET uses multiple sockets.
> HW RSS will typically drop a packet if its selected RX queue is empty.
> However, Linux FANOUT, as a SW load balancer, can be configured to avoid
> this packet drop by rolling over to the next socket.
> This rollover functionality was ALWAYS enabled in net_af_packet. It is
> surrounded by ifdef, but only to allow compilation on ancient Linux
> versions that did not have it.
> 
> Since DPDK applications are usually designed for HW based PMDs, this
> rollover functionality, which the developers are likely unaware of, could
> be confusing.
> 
> Another option that is part of Linux FANOUT is DEFRAG that instructs Linux
> to compose complete IP packet out of fragments before delivering it to the
> PACKET socket. Again, this behavior typically does not exist for HW based
> PMDs and may confuse users.
> 
> This patch adds 2 options to control these features:
> rollover=[0|1],defrag=[0|1]
> For backward compatibility both features are enabled by default even
> though most users will probably want both of them disabled.
> 
> Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
> ---
> v2:
> * Improve error handling for parsing of arg strings. Add a wrapper around
>   strtoul to replace atoi.
> * Fix checkpatch indentation error due to copy-paste.
> 
> v1: https://mails.dpdk.org/archives/dev/2024-October/307451.html
> ---
>  doc/guides/nics/af_packet.rst             |  4 ++
>  drivers/net/af_packet/rte_eth_af_packet.c | 77 ++++++++++++++++++-----
>  2 files changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/doc/guides/nics/af_packet.rst b/doc/guides/nics/af_packet.rst
> index 66b977e1a2..4bc748b50b 100644
> --- a/doc/guides/nics/af_packet.rst
> +++ b/doc/guides/nics/af_packet.rst
> @@ -29,6 +29,8 @@ Some of these, in turn, will be used to configure the PACKET_MMAP settings.
>  *   ``framesz`` - PACKET_MMAP frame size (optional, default 2048B; Note: multiple
>      of 16B);
>  *   ``framecnt`` - PACKET_MMAP frame count (optional, default 512).
> +*   ``rollover`` - disable(0)/enable(1) PACKET_FANOUT_ROLLOVER (optional, default 1).
> +*   ``defrag`` - disable(0)/enable(1) PACKET_FANOUT_FLAG_DEFRAG (optional, default 1).
> 

+1 to introduce both options, thanks.
Do you have any practical usecase to disable them?


>  Because this implementation is based on PACKET_MMAP, and PACKET_MMAP has its
>  own pre-requisites, it should be noted that the inner workings of PACKET_MMAP
> @@ -47,6 +49,8 @@ For the full details behind PACKET_MMAP's structures and settings, consider
>  reading the `PACKET_MMAP documentation in the Kernel
>  <https://www.kernel.org/doc/Documentation/networking/packet_mmap.txt>`_.
> 
> +For details of ``rollover`` anb ``defrag`` refer to the *packet(7)* man page.
> +
>  Prerequisites
>  -------------
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 68870dd3e2..1a043e8398 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -36,6 +36,8 @@
>  #define ETH_AF_PACKET_FRAMESIZE_ARG	"framesz"
>  #define ETH_AF_PACKET_FRAMECOUNT_ARG	"framecnt"
>  #define ETH_AF_PACKET_QDISC_BYPASS_ARG	"qdisc_bypass"
> +#define ETH_AF_PACKET_ROLLOVER		"rollover"
> +#define ETH_AF_PACKET_DEFRAG		"defrag"
> 

Can you please add new arguments to 'RTE_PMD_REGISTER_PARAM_STRING'
macro below?

>  #define DFLT_FRAME_SIZE		(1 << 11)
>  #define DFLT_FRAME_COUNT	(1 << 9)
> @@ -91,6 +93,8 @@ static const char *valid_arguments[] = {
>  	ETH_AF_PACKET_FRAMESIZE_ARG,
>  	ETH_AF_PACKET_FRAMECOUNT_ARG,
>  	ETH_AF_PACKET_QDISC_BYPASS_ARG,
> +	ETH_AF_PACKET_ROLLOVER,
> +	ETH_AF_PACKET_DEFRAG,
>  	NULL
>  };
> 
> @@ -659,6 +663,20 @@ open_packet_iface(const char *key __rte_unused,
>  	return 0;
>  }
> 
> +static int
> +uint_arg_parse(const char *arg_str, unsigned int *arg_val)
> +{
> +	unsigned long val;
> +	char *endptr;
> +
> +	val = strtoul(arg_str, &endptr, 10);
> +	if (*arg_str == '\0' || *endptr != '\0')
> +		return -EINVAL;
> +
> +	*arg_val = val;
> +	return 0;
> +}
> +
>  static int
>  rte_pmd_init_internals(struct rte_vdev_device *dev,
>                         const int sockfd,
> @@ -676,6 +694,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  	const unsigned int numa_node = dev->device.numa_node;
>  	struct rte_eth_dev_data *data = NULL;
>  	struct rte_kvargs_pair *pair = NULL;
> +	const char *ifname = NULL;
>  	struct ifreq ifr;
>  	size_t ifnamelen;
>  	unsigned k_idx;
> @@ -686,6 +705,8 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  	int rc, tpver, discard;
>  	int qsockfd = -1;
>  	unsigned int i, q, rdsize;
> +	unsigned int rollover = 1;
> +	unsigned int defrag = 1;
>  #if defined(PACKET_FANOUT)
>  	int fanout_arg;
>  #endif
> @@ -693,9 +714,30 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  	for (k_idx = 0; k_idx < kvlist->count; k_idx++) {
>  		pair = &kvlist->pairs[k_idx];
>  		if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL)
> -			break;
> +			ifname = pair->value;
> +		if (strcmp(pair->key, ETH_AF_PACKET_ROLLOVER) == 0) {
> +			rc = uint_arg_parse(pair->value, &rollover);
> +			if (rc || rollover > 1) {
> +				PMD_LOG(ERR,
> +					"%s: invalid rollover value: %s",
> +					name, pair->value);
> +				return -1;
> +			}
> +			continue;
> +		}
> +		if (strcmp(pair->key, ETH_AF_PACKET_DEFRAG) == 0) {
> +			rc = uint_arg_parse(pair->value, &defrag);
> +			if (rc || defrag > 1) {
> +				PMD_LOG(ERR,
> +					"%s: invalid defrag value: %s",
> +					name, pair->value);
> +				return -1;
> +			}
> +			continue;
> +		}
>

Instead of adding parameter parsing here, they are already in
'rte_eth_from_packet()' function, better to add there,

BUT, before adding more paramter, can you please fix the parameter
parsing in the af_packet PMD? This is in my list for a long time now.

Instead of accessing to 'kvlist' internals (kvlist->count,
kvlist->pairs[]), and use 'strstr' (or 'strcmp'), there is already
existing 'rte_kvargs_process()' API for it, you can find multiple
samples in other drivers.


>  	}
> -	if (pair == NULL) {
> +
> +	if (ifname == NULL) {
>  		PMD_LOG(ERR,
>  			"%s: no interface specified for AF_PACKET ethdev",
>  		        name);
> @@ -738,21 +780,21 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  	req->tp_frame_size = framesize;
>  	req->tp_frame_nr = framecnt;
> 
> -	ifnamelen = strlen(pair->value);
> +	ifnamelen = strlen(ifname);
>  	if (ifnamelen < sizeof(ifr.ifr_name)) {
> -		memcpy(ifr.ifr_name, pair->value, ifnamelen);
> +		memcpy(ifr.ifr_name, ifname, ifnamelen);
>  		ifr.ifr_name[ifnamelen] = '\0';
>  	} else {
>  		PMD_LOG(ERR,
>  			"%s: I/F name too long (%s)",
> -			name, pair->value);
> +			name, ifname);
>  		goto free_internals;
>  	}
>  	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
>  		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
>  		goto free_internals;
>  	}
> -	(*internals)->if_name = strdup(pair->value);
> +	(*internals)->if_name = strdup(ifname);
>  	if ((*internals)->if_name == NULL)
>  		goto free_internals;
>  	(*internals)->if_index = ifr.ifr_ifindex;
> @@ -770,9 +812,12 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> 
>  #if defined(PACKET_FANOUT)
>  	fanout_arg = (getpid() ^ (*internals)->if_index) & 0xffff;
> -	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG) << 16;
> +	fanout_arg |= PACKET_FANOUT_HASH << 16;
> +	if (defrag)
> +		fanout_arg |= PACKET_FANOUT_FLAG_DEFRAG << 16;
>  #if defined(PACKET_FANOUT_FLAG_ROLLOVER)
> -	fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
> +	if (rollover)
> +		fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
>  #endif
>  #endif
> 
> @@ -792,7 +837,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rc == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not set PACKET_VERSION on AF_PACKET socket for %s",
> -				name, pair->value);
> +				name, ifname);
>  			goto error;
>  		}
> 
> @@ -802,7 +847,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rc == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not set PACKET_LOSS on AF_PACKET socket for %s",
> -				name, pair->value);
> +				name, ifname);
>  			goto error;
>  		}
> 
> @@ -813,7 +858,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  			if (rc == -1) {
>  				PMD_LOG_ERRNO(ERR,
>  					"%s: could not set PACKET_QDISC_BYPASS on AF_PACKET socket for %s",
> -					name, pair->value);
> +					name, ifname);
>  				goto error;
>  			}
>  #endif
> @@ -823,7 +868,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rc == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not set PACKET_RX_RING on AF_PACKET socket for %s",
> -				name, pair->value);
> +				name, ifname);
>  			goto error;
>  		}
> 
> @@ -831,7 +876,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rc == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not set PACKET_TX_RING on AF_PACKET "
> -				"socket for %s", name, pair->value);
> +				"socket for %s", name, ifname);
>  			goto error;
>  		}
> 
> @@ -844,7 +889,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rx_queue->map == MAP_FAILED) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: call to mmap failed on AF_PACKET socket for %s",
> -				name, pair->value);
> +				name, ifname);
>  			goto error;
>  		}
> 
> @@ -881,7 +926,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rc == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not bind AF_PACKET socket to %s",
> -				name, pair->value);
> +				name, ifname);
>  			goto error;
>  		}
> 
> @@ -891,7 +936,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		if (rc == -1) {
>  			PMD_LOG_ERRNO(ERR,
>  				"%s: could not set PACKET_FANOUT on AF_PACKET socket for %s",
> -				name, pair->value);
> +				name, ifname);
>  			goto error;
>  		}
>  #endif
> 
> base-commit: 98613d32e3dac58d685f4f236cf8cc9733abaaf3
> --
> 2.40.1
> 

RE: [PATCH v02] net/af_packet: add rollover and defrag options

[Gur Stavi]

> > net_af_packet PMD multi "queue" support relies on Linux FANOUT
> capability.
> > Linux FANOUT is a SW based load balancer that is similar to HW RSS
> which
> > is more common for DPDK PMDs. Instead of multiple HW descriptor queues,
> > AF PACKET uses multiple sockets.
> > HW RSS will typically drop a packet if its selected RX queue is empty.
> > However, Linux FANOUT, as a SW load balancer, can be configured to
> avoid
> > this packet drop by rolling over to the next socket.
> > This rollover functionality was ALWAYS enabled in net_af_packet. It is
> > surrounded by ifdef, but only to allow compilation on ancient Linux
> > versions that did not have it.
> >
> > Since DPDK applications are usually designed for HW based PMDs, this
> > rollover functionality, which the developers are likely unaware of,
> could
> > be confusing.
> >
> > Another option that is part of Linux FANOUT is DEFRAG that instructs
> Linux
> > to compose complete IP packet out of fragments before delivering it to
> the
> > PACKET socket. Again, this behavior typically does not exist for HW
> based
> > PMDs and may confuse users.
> >
> > This patch adds 2 options to control these features:
> > rollover=[0|1],defrag=[0|1]
> > For backward compatibility both features are enabled by default even
> > though most users will probably want both of them disabled.
> >
> > Signed-off-by: Gur Stavi <gur.stavi@huawei.com>
> > ---
> > v2:
> > * Improve error handling for parsing of arg strings. Add a wrapper
> around
> >   strtoul to replace atoi.
> > * Fix checkpatch indentation error due to copy-paste.
> >
> > v1: https://mails.dpdk.org/archives/dev/2024-October/307451.html
> > ---
> >  doc/guides/nics/af_packet.rst             |  4 ++
> >  drivers/net/af_packet/rte_eth_af_packet.c | 77 ++++++++++++++++++-----
> >  2 files changed, 65 insertions(+), 16 deletions(-)
> >
> > diff --git a/doc/guides/nics/af_packet.rst
> b/doc/guides/nics/af_packet.rst
> > index 66b977e1a2..4bc748b50b 100644
> > --- a/doc/guides/nics/af_packet.rst
> > +++ b/doc/guides/nics/af_packet.rst
> > @@ -29,6 +29,8 @@ Some of these, in turn, will be used to configure the
> PACKET_MMAP settings.
> >  *   ``framesz`` - PACKET_MMAP frame size (optional, default 2048B;
> Note: multiple
> >      of 16B);
> >  *   ``framecnt`` - PACKET_MMAP frame count (optional, default 512).
> > +*   ``rollover`` - disable(0)/enable(1) PACKET_FANOUT_ROLLOVER
> (optional, default 1).
> > +*   ``defrag`` - disable(0)/enable(1) PACKET_FANOUT_FLAG_DEFRAG
> (optional, default 1).
> >
> 
> +1 to introduce both options, thanks.
> Do you have any practical usecase to disable them?

Not a very specific use case, but generic though experiments:
A DPDK should assume that all packets of a UDP flow will arrive to a
specific queue/core (or be dropped). It will maintain a
non-synchronized SW state for that flow only accessed by the designated
core. Having rollover deliver a flow packet to another queue/core could
cause various buggy behaviors over this state.

IP fragmentation is strongly discouraged. An app may want to explicitly
Ignore/detect fragments to help system maintainers identify offenders and
Fix the configuration that leads to fragmentation. Having Linux "secretly"
handling fragmentation can prevent this.

> 
> 
> >  Because this implementation is based on PACKET_MMAP, and PACKET_MMAP
> has its
> >  own pre-requisites, it should be noted that the inner workings of
> PACKET_MMAP
> > @@ -47,6 +49,8 @@ For the full details behind PACKET_MMAP's structures
> and settings, consider
> >  reading the `PACKET_MMAP documentation in the Kernel
> >
> <https://www.kernel.org/doc/Documentation/networking/packet_mmap.txt>`_.
> >
> > +For details of ``rollover`` anb ``defrag`` refer to the *packet(7)*
> man page.
> > +
> >  Prerequisites
> >  -------------
> >
> > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> b/drivers/net/af_packet/rte_eth_af_packet.c
> > index 68870dd3e2..1a043e8398 100644
> > --- a/drivers/net/af_packet/rte_eth_af_packet.c
> > +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> > @@ -36,6 +36,8 @@
> >  #define ETH_AF_PACKET_FRAMESIZE_ARG	"framesz"
> >  #define ETH_AF_PACKET_FRAMECOUNT_ARG	"framecnt"
> >  #define ETH_AF_PACKET_QDISC_BYPASS_ARG	"qdisc_bypass"
> > +#define ETH_AF_PACKET_ROLLOVER		"rollover"
> > +#define ETH_AF_PACKET_DEFRAG		"defrag"
> >
> 
> Can you please add new arguments to 'RTE_PMD_REGISTER_PARAM_STRING'
> macro below?
> 
> >  #define DFLT_FRAME_SIZE		(1 << 11)
> >  #define DFLT_FRAME_COUNT	(1 << 9)
> > @@ -91,6 +93,8 @@ static const char *valid_arguments[] = {
> >  	ETH_AF_PACKET_FRAMESIZE_ARG,
> >  	ETH_AF_PACKET_FRAMECOUNT_ARG,
> >  	ETH_AF_PACKET_QDISC_BYPASS_ARG,
> > +	ETH_AF_PACKET_ROLLOVER,
> > +	ETH_AF_PACKET_DEFRAG,
> >  	NULL
> >  };
> >
> > @@ -659,6 +663,20 @@ open_packet_iface(const char *key __rte_unused,
> >  	return 0;
> >  }
> >
> > +static int
> > +uint_arg_parse(const char *arg_str, unsigned int *arg_val)
> > +{
> > +	unsigned long val;
> > +	char *endptr;
> > +
> > +	val = strtoul(arg_str, &endptr, 10);
> > +	if (*arg_str == '\0' || *endptr != '\0')
> > +		return -EINVAL;
> > +
> > +	*arg_val = val;
> > +	return 0;
> > +}
> > +
> >  static int
> >  rte_pmd_init_internals(struct rte_vdev_device *dev,
> >                         const int sockfd,
> > @@ -676,6 +694,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  	const unsigned int numa_node = dev->device.numa_node;
> >  	struct rte_eth_dev_data *data = NULL;
> >  	struct rte_kvargs_pair *pair = NULL;
> > +	const char *ifname = NULL;
> >  	struct ifreq ifr;
> >  	size_t ifnamelen;
> >  	unsigned k_idx;
> > @@ -686,6 +705,8 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  	int rc, tpver, discard;
> >  	int qsockfd = -1;
> >  	unsigned int i, q, rdsize;
> > +	unsigned int rollover = 1;
> > +	unsigned int defrag = 1;
> >  #if defined(PACKET_FANOUT)
> >  	int fanout_arg;
> >  #endif
> > @@ -693,9 +714,30 @@ rte_pmd_init_internals(struct rte_vdev_device
> *dev,
> >  	for (k_idx = 0; k_idx < kvlist->count; k_idx++) {
> >  		pair = &kvlist->pairs[k_idx];
> >  		if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL)
> > -			break;
> > +			ifname = pair->value;
> > +		if (strcmp(pair->key, ETH_AF_PACKET_ROLLOVER) == 0) {
> > +			rc = uint_arg_parse(pair->value, &rollover);
> > +			if (rc || rollover > 1) {
> > +				PMD_LOG(ERR,
> > +					"%s: invalid rollover value: %s",
> > +					name, pair->value);
> > +				return -1;
> > +			}
> > +			continue;
> > +		}
> > +		if (strcmp(pair->key, ETH_AF_PACKET_DEFRAG) == 0) {
> > +			rc = uint_arg_parse(pair->value, &defrag);
> > +			if (rc || defrag > 1) {
> > +				PMD_LOG(ERR,
> > +					"%s: invalid defrag value: %s",
> > +					name, pair->value);
> > +				return -1;
> > +			}
> > +			continue;
> > +		}
> >
> 
> Instead of adding parameter parsing here, they are already in
> 'rte_eth_from_packet()' function, better to add there,
> 
> BUT, before adding more paramter, can you please fix the parameter
> parsing in the af_packet PMD? This is in my list for a long time now.
> 
> Instead of accessing to 'kvlist' internals (kvlist->count,
> kvlist->pairs[]), and use 'strstr' (or 'strcmp'), there is already
> existing 'rte_kvargs_process()' API for it, you can find multiple
> samples in other drivers.
> 
> 
> >  	}
> > -	if (pair == NULL) {
> > +
> > +	if (ifname == NULL) {
> >  		PMD_LOG(ERR,
> >  			"%s: no interface specified for AF_PACKET ethdev",
> >  		        name);
> > @@ -738,21 +780,21 @@ rte_pmd_init_internals(struct rte_vdev_device
> *dev,
> >  	req->tp_frame_size = framesize;
> >  	req->tp_frame_nr = framecnt;
> >
> > -	ifnamelen = strlen(pair->value);
> > +	ifnamelen = strlen(ifname);
> >  	if (ifnamelen < sizeof(ifr.ifr_name)) {
> > -		memcpy(ifr.ifr_name, pair->value, ifnamelen);
> > +		memcpy(ifr.ifr_name, ifname, ifnamelen);
> >  		ifr.ifr_name[ifnamelen] = '\0';
> >  	} else {
> >  		PMD_LOG(ERR,
> >  			"%s: I/F name too long (%s)",
> > -			name, pair->value);
> > +			name, ifname);
> >  		goto free_internals;
> >  	}
> >  	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
> >  		PMD_LOG_ERRNO(ERR, "%s: ioctl failed (SIOCGIFINDEX)", name);
> >  		goto free_internals;
> >  	}
> > -	(*internals)->if_name = strdup(pair->value);
> > +	(*internals)->if_name = strdup(ifname);
> >  	if ((*internals)->if_name == NULL)
> >  		goto free_internals;
> >  	(*internals)->if_index = ifr.ifr_ifindex;
> > @@ -770,9 +812,12 @@ rte_pmd_init_internals(struct rte_vdev_device
> *dev,
> >
> >  #if defined(PACKET_FANOUT)
> >  	fanout_arg = (getpid() ^ (*internals)->if_index) & 0xffff;
> > -	fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG) <<
> 16;
> > +	fanout_arg |= PACKET_FANOUT_HASH << 16;
> > +	if (defrag)
> > +		fanout_arg |= PACKET_FANOUT_FLAG_DEFRAG << 16;
> >  #if defined(PACKET_FANOUT_FLAG_ROLLOVER)
> > -	fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
> > +	if (rollover)
> > +		fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16;
> >  #endif
> >  #endif
> >
> > @@ -792,7 +837,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rc == -1) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: could not set PACKET_VERSION on AF_PACKET
> socket for %s",
> > -				name, pair->value);
> > +				name, ifname);
> >  			goto error;
> >  		}
> >
> > @@ -802,7 +847,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rc == -1) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: could not set PACKET_LOSS on AF_PACKET socket
> for %s",
> > -				name, pair->value);
> > +				name, ifname);
> >  			goto error;
> >  		}
> >
> > @@ -813,7 +858,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  			if (rc == -1) {
> >  				PMD_LOG_ERRNO(ERR,
> >  					"%s: could not set PACKET_QDISC_BYPASS on
> AF_PACKET socket for %s",
> > -					name, pair->value);
> > +					name, ifname);
> >  				goto error;
> >  			}
> >  #endif
> > @@ -823,7 +868,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rc == -1) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: could not set PACKET_RX_RING on AF_PACKET
> socket for %s",
> > -				name, pair->value);
> > +				name, ifname);
> >  			goto error;
> >  		}
> >
> > @@ -831,7 +876,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rc == -1) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: could not set PACKET_TX_RING on AF_PACKET "
> > -				"socket for %s", name, pair->value);
> > +				"socket for %s", name, ifname);
> >  			goto error;
> >  		}
> >
> > @@ -844,7 +889,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rx_queue->map == MAP_FAILED) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: call to mmap failed on AF_PACKET socket for
> %s",
> > -				name, pair->value);
> > +				name, ifname);
> >  			goto error;
> >  		}
> >
> > @@ -881,7 +926,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rc == -1) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: could not bind AF_PACKET socket to %s",
> > -				name, pair->value);
> > +				name, ifname);
> >  			goto error;
> >  		}
> >
> > @@ -891,7 +936,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
> >  		if (rc == -1) {
> >  			PMD_LOG_ERRNO(ERR,
> >  				"%s: could not set PACKET_FANOUT on AF_PACKET
> socket for %s",
> > -				name, pair->value);
> > +				name, ifname);
> >  			goto error;
> >  		}
> >  #endif
> >
> > base-commit: 98613d32e3dac58d685f4f236cf8cc9733abaaf3
> > --
> > 2.40.1
> >