[dpdk-dev] [PATCH 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[dpdk-dev] [PATCH 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[Tiago Lam]

When setting the MTU, eth_dev_mtu_set() is called to validate the
provided MTU. As part of that, it calculates the useful area to store
data and compares it against the MTU, to guarantee that there's enough
space to store the data. It calculates that as:
    "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"

However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
sockaddr_ll) internally, meaning the useuful area of data above will
have sizeof(struct sockaddr_ll) decremented twice.

Instead, the useful area of data should be calculated as:
    "tp_frame_size - TPACKET2_HDRLEN"

This makes sure that there's enough useful area to fit the provided MTU
after excluding tpacket2_hdr and sockaddr_ll.

Fixes: cc68ac4 ("net/af_packet: support MTU change")

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 95a98c6..264cfc0 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -433,8 +433,7 @@ eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 	int ret;
 	int s;
 	unsigned int data_size = internals->req.tp_frame_size -
-				 TPACKET2_HDRLEN -
-				 sizeof(struct sockaddr_ll);
+				 TPACKET2_HDRLEN;
 
 	if (mtu > data_size)
 		return -EINVAL;
-- 
2.7.4

[dpdk-dev] [PATCH 2/3] net/af_packet: Move parse and validation of iface.

[Tiago Lam]

Instead of re-iterating through kvlist just to parse the
ETH_AF_PACKET_IFACE_ARG argument in rte_pmd_init_internals(), we now use
the already existing iteration in rte_eth_from_packet() to parse and
validate the ETH_AF_PACKET_IFACE_ARG argument.

This will be useful for a later commit, which needs to access the
interface name to get the underlying configured MTU.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 77 ++++++++++++++++---------------
 1 file changed, 41 insertions(+), 36 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 264cfc0..4e95dd7 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -540,15 +540,12 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		       unsigned int qdisc_bypass,
                        struct pmd_internals **internals,
                        struct rte_eth_dev **eth_dev,
-                       struct rte_kvargs *kvlist)
+                       const char *ifname)
 {
 	const char *name = rte_vdev_device_name(dev);
 	const unsigned int numa_node = dev->device.numa_node;
 	struct rte_eth_dev_data *data = NULL;
-	struct rte_kvargs_pair *pair = NULL;
 	struct ifreq ifr;
-	size_t ifnamelen;
-	unsigned k_idx;
 	struct sockaddr_ll sockaddr;
 	struct tpacket_req *req;
 	struct pkt_rx_queue *rx_queue;
@@ -560,18 +557,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	int fanout_arg;
 #endif
 
-	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;
-	}
-	if (pair == NULL) {
-		PMD_LOG(ERR,
-			"%s: no interface specified for AF_PACKET ethdev",
-		        name);
-		return -1;
-	}
-
 	PMD_LOG(INFO,
 		"%s: creating AF_PACKET-backed ethdev on numa socket %u",
 		name, numa_node);
@@ -593,23 +578,14 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	req->tp_frame_size = framesize;
 	req->tp_frame_nr = framecnt;
 
-	ifnamelen = strlen(pair->value);
-	if (ifnamelen < sizeof(ifr.ifr_name)) {
-		memcpy(ifr.ifr_name, pair->value, ifnamelen);
-		ifr.ifr_name[ifnamelen] = '\0';
-	} else {
-		PMD_LOG(ERR,
-			"%s: I/F name too long (%s)",
-			name, pair->value);
-		return -1;
-	}
+	memcpy(ifr.ifr_name, ifname, strlen(ifname));
 	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
 		PMD_LOG(ERR,
 			"%s: ioctl failed (SIOCGIFINDEX)",
 		        name);
 		return -1;
 	}
-	(*internals)->if_name = strdup(pair->value);
+	(*internals)->if_name = strdup(ifname);
 	if ((*internals)->if_name == NULL)
 		return -1;
 	(*internals)->if_index = ifr.ifr_ifindex;
@@ -651,7 +627,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_VERSION on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -661,7 +637,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_LOSS on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -671,7 +647,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_QDISC_BYPASS on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 #else
@@ -682,7 +658,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_RX_RING on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -690,7 +666,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_TX_RING on AF_PACKET "
-				"socket for %s", name, pair->value);
+				"socket for %s", name, ifname);
 			goto error;
 		}
 
@@ -703,7 +679,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rx_queue->map == MAP_FAILED) {
 			PMD_LOG(ERR,
 				"%s: call to mmap failed on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -740,7 +716,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not bind AF_PACKET socket to %s",
-			        name, pair->value);
+			        name, ifname);
 			goto error;
 		}
 
@@ -750,7 +726,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_FANOUT on AF_PACKET socket "
-				"for %s", name, pair->value);
+				"for %s", name, ifname);
 			goto error;
 		}
 #endif
@@ -816,6 +792,9 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	unsigned int framecount = DFLT_FRAME_COUNT;
 	unsigned int qpairs = 1;
 	unsigned int qdisc_bypass = 1;
+	struct ifreq ifr;
+	char *ifname = NULL;
+	size_t ifnamelen;
 
 	/* do some parameter checking */
 	if (*sockfd < 0)
@@ -877,6 +856,32 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 			}
 			continue;
 		}
+		if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL) {
+			ifname = pair->value;
+			if (strlen(ifname) == 0) {
+				RTE_LOG(ERR, PMD,
+					"%s: invalid iface value\n",
+					name);
+				return -1;
+			}
+
+			continue;
+		}
+	}
+
+	if (ifname == NULL) {
+		RTE_LOG(ERR, PMD,
+			"%s: no interface specified for AF_PACKET ethdev\n",
+		        name);
+		return -1;
+	}
+
+	ifnamelen = strlen(ifname);
+	if (ifnamelen >= sizeof(ifr.ifr_name)) {
+		RTE_LOG(ERR, PMD,
+			"%s: I/F name too long (%s)\n",
+			name, ifname);
+		return -1;
 	}
 
 	if (framesize > blocksize) {
@@ -904,7 +909,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 				   framesize, framecount,
 				   qdisc_bypass,
 				   &internals, &eth_dev,
-				   kvlist) < 0)
+				   ifname) < 0)
 		return -1;
 
 	eth_dev->rx_pkt_burst = eth_af_packet_rx;
-- 
2.7.4

[dpdk-dev] [PATCH 3/3] net/af_packet: Get 'framesz' from the iface's MTU.

[Tiago Lam]

Use the underlying MTU to calculate the framsize to be used for the mmap
RINGs. This is to make it more flexible on deployments with different
MTU requirements, instead of using a pre-defined value of 2048B.

If a 'framsz' option is provided, that value is used instead and the MTU
of the underlying interface is ignored.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 33 ++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 4e95dd7..45ee72b 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -788,7 +788,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	unsigned k_idx;
 	unsigned int blockcount;
 	unsigned int blocksize = DFLT_BLOCK_SIZE;
-	unsigned int framesize = DFLT_FRAME_SIZE;
+	unsigned int framesize = 0;
 	unsigned int framecount = DFLT_FRAME_COUNT;
 	unsigned int qpairs = 1;
 	unsigned int qdisc_bypass = 1;
@@ -877,17 +877,40 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	}
 
 	ifnamelen = strlen(ifname);
-	if (ifnamelen >= sizeof(ifr.ifr_name)) {
+	if (ifnamelen < sizeof(ifr.ifr_name)) {
+		memcpy(ifr.ifr_name, ifname, ifnamelen);
+		ifr.ifr_name[ifnamelen] = '\0';
+	} else {
 		RTE_LOG(ERR, PMD,
 			"%s: I/F name too long (%s)\n",
 			name, ifname);
 		return -1;
 	}
 
+	/*
+	 * Base framesize on the MTU of the underlying interface, if no
+	 * 'framesz' option is given
+	 */
+	if (!framesize) {
+		if (ioctl(*sockfd, SIOCGIFMTU, &ifr) == -1) {
+			RTE_LOG(ERR, PMD,
+				"%s: ioctl failed (SIOCGIFMTU)",
+				name);
+			framesize = DFLT_FRAME_SIZE;
+		} else {
+			framesize = ifr.ifr_mtu;
+			/*
+			 * Align to TPACKET_ALIGNMENT and add TPACKET2_HDRLEN, to
+			 * account for the extra needed space
+			 */
+			framesize = TPACKET_ALIGN(framesize + TPACKET2_HDRLEN);
+		}
+	}
+
 	if (framesize > blocksize) {
-		PMD_LOG(ERR,
-			"%s: AF_PACKET MMAP frame size exceeds block size!",
-		        name);
+		RTE_LOG(ERR, PMD,
+			"%s: AF_PACKET MMAP frame size (%u) exceeds block size (%u)!",
+		        name, framesize, blocksize);
 		return -1;
 	}
 
-- 
2.7.4

[dpdk-dev] [PATCH v2 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[Tiago Lam]

When setting the MTU, eth_dev_mtu_set() is called to validate the
provided MTU. As part of that, it calculates the useful area to store
data and compares it against the MTU, to guarantee that there's enough
space to store the data. It calculates that as:
    "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"

However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
sockaddr_ll) internally, meaning the useuful area of data above will
have sizeof(struct sockaddr_ll) decremented twice.

Instead, the useful area of data should be calculated as:
    "tp_frame_size - TPACKET2_HDRLEN"

This makes sure that there's enough useful area to fit the provided MTU
after excluding tpacket2_hdr and sockaddr_ll.

Fixes: cc68ac4 ("net/af_packet: support MTU change")

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---
 drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 95a98c6..264cfc0 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -433,8 +433,7 @@ eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 	int ret;
 	int s;
 	unsigned int data_size = internals->req.tp_frame_size -
-				 TPACKET2_HDRLEN -
-				 sizeof(struct sockaddr_ll);
+				 TPACKET2_HDRLEN;
 
 	if (mtu > data_size)
 		return -EINVAL;
-- 
2.7.4

[dpdk-dev] [PATCH v2 2/3] net/af_packet: move parse and validation of iface

[Tiago Lam]

Instead of re-iterating through kvlist just to parse the
ETH_AF_PACKET_IFACE_ARG argument in rte_pmd_init_internals(), we now use
the already existing iteration in rte_eth_from_packet() to parse and
validate the ETH_AF_PACKET_IFACE_ARG argument.

This will be useful for a later commit, which needs to access the
interface name to get the underlying configured MTU.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---

v2: Fix checkpatches.sh and check-git-log.sh warnings.

---
 drivers/net/af_packet/rte_eth_af_packet.c | 77 ++++++++++++++++---------------
 1 file changed, 41 insertions(+), 36 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 264cfc0..8d749a2 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -540,15 +540,12 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		       unsigned int qdisc_bypass,
                        struct pmd_internals **internals,
                        struct rte_eth_dev **eth_dev,
-                       struct rte_kvargs *kvlist)
+					   const char *ifname)
 {
 	const char *name = rte_vdev_device_name(dev);
 	const unsigned int numa_node = dev->device.numa_node;
 	struct rte_eth_dev_data *data = NULL;
-	struct rte_kvargs_pair *pair = NULL;
 	struct ifreq ifr;
-	size_t ifnamelen;
-	unsigned k_idx;
 	struct sockaddr_ll sockaddr;
 	struct tpacket_req *req;
 	struct pkt_rx_queue *rx_queue;
@@ -560,18 +557,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	int fanout_arg;
 #endif
 
-	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;
-	}
-	if (pair == NULL) {
-		PMD_LOG(ERR,
-			"%s: no interface specified for AF_PACKET ethdev",
-		        name);
-		return -1;
-	}
-
 	PMD_LOG(INFO,
 		"%s: creating AF_PACKET-backed ethdev on numa socket %u",
 		name, numa_node);
@@ -593,23 +578,14 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 	req->tp_frame_size = framesize;
 	req->tp_frame_nr = framecnt;
 
-	ifnamelen = strlen(pair->value);
-	if (ifnamelen < sizeof(ifr.ifr_name)) {
-		memcpy(ifr.ifr_name, pair->value, ifnamelen);
-		ifr.ifr_name[ifnamelen] = '\0';
-	} else {
-		PMD_LOG(ERR,
-			"%s: I/F name too long (%s)",
-			name, pair->value);
-		return -1;
-	}
+	memcpy(ifr.ifr_name, ifname, strlen(ifname));
 	if (ioctl(sockfd, SIOCGIFINDEX, &ifr) == -1) {
 		PMD_LOG(ERR,
 			"%s: ioctl failed (SIOCGIFINDEX)",
 		        name);
 		return -1;
 	}
-	(*internals)->if_name = strdup(pair->value);
+	(*internals)->if_name = strdup(ifname);
 	if ((*internals)->if_name == NULL)
 		return -1;
 	(*internals)->if_index = ifr.ifr_ifindex;
@@ -651,7 +627,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_VERSION on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -661,7 +637,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_LOSS on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -671,7 +647,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_QDISC_BYPASS on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 #else
@@ -682,7 +658,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_RX_RING on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -690,7 +666,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_TX_RING on AF_PACKET "
-				"socket for %s", name, pair->value);
+				"socket for %s", name, ifname);
 			goto error;
 		}
 
@@ -703,7 +679,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rx_queue->map == MAP_FAILED) {
 			PMD_LOG(ERR,
 				"%s: call to mmap failed on AF_PACKET socket for %s",
-				name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -740,7 +716,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not bind AF_PACKET socket to %s",
-			        name, pair->value);
+				name, ifname);
 			goto error;
 		}
 
@@ -750,7 +726,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
 		if (rc == -1) {
 			PMD_LOG(ERR,
 				"%s: could not set PACKET_FANOUT on AF_PACKET socket "
-				"for %s", name, pair->value);
+				"for %s", name, ifname);
 			goto error;
 		}
 #endif
@@ -816,6 +792,9 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	unsigned int framecount = DFLT_FRAME_COUNT;
 	unsigned int qpairs = 1;
 	unsigned int qdisc_bypass = 1;
+	struct ifreq ifr;
+	char *ifname = NULL;
+	size_t ifnamelen;
 
 	/* do some parameter checking */
 	if (*sockfd < 0)
@@ -877,6 +856,32 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 			}
 			continue;
 		}
+		if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL) {
+			ifname = pair->value;
+			if (strlen(ifname) == 0) {
+				RTE_LOG(ERR, PMD,
+					"%s: invalid iface value\n",
+					name);
+				return -1;
+			}
+
+			continue;
+		}
+	}
+
+	if (ifname == NULL) {
+		RTE_LOG(ERR, PMD,
+			"%s: no interface specified for AF_PACKET ethdev\n",
+			name);
+		return -1;
+	}
+
+	ifnamelen = strlen(ifname);
+	if (ifnamelen >= sizeof(ifr.ifr_name)) {
+		RTE_LOG(ERR, PMD,
+			"%s: I/F name too long (%s)\n",
+			name, ifname);
+		return -1;
 	}
 
 	if (framesize > blocksize) {
@@ -904,7 +909,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 				   framesize, framecount,
 				   qdisc_bypass,
 				   &internals, &eth_dev,
-				   kvlist) < 0)
+				   ifname) < 0)
 		return -1;
 
 	eth_dev->rx_pkt_burst = eth_af_packet_rx;
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 2/3] net/af_packet: move parse and validation of iface

[Ferruh Yigit]

On 11/20/2018 10:26 AM, Tiago Lam wrote:
> Instead of re-iterating through kvlist just to parse the
> ETH_AF_PACKET_IFACE_ARG argument in rte_pmd_init_internals(), we now use
> the already existing iteration in rte_eth_from_packet() to parse and
> validate the ETH_AF_PACKET_IFACE_ARG argument.

+1

> 
> This will be useful for a later commit, which needs to access the
> interface name to get the underlying configured MTU.
> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
> 
> v2: Fix checkpatches.sh and check-git-log.sh warnings.
> 
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 77 ++++++++++++++++---------------
>  1 file changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 264cfc0..8d749a2 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -540,15 +540,12 @@ rte_pmd_init_internals(struct rte_vdev_device *dev,
>  		       unsigned int qdisc_bypass,
>                         struct pmd_internals **internals,
>                         struct rte_eth_dev **eth_dev,
> -                       struct rte_kvargs *kvlist)
> +					   const char *ifname)

Please align other parameter formatting.

<...>

> @@ -877,6 +856,32 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
>  			}
>  			continue;
>  		}
> +		if (strstr(pair->key, ETH_AF_PACKET_IFACE_ARG) != NULL) {
> +			ifname = pair->value;
> +			if (strlen(ifname) == 0) {
> +				RTE_LOG(ERR, PMD,
> +					"%s: invalid iface value\n",
> +					name);
> +				return -1;
> +			}
> +
> +			continue;
> +		}

Indeed instead of accessing kvargs internal pair->values, this should call
rte_kvargs_process() but that is for another patch to fix all occurrences.

> +	}
> +
> +	if (ifname == NULL) {
> +		RTE_LOG(ERR, PMD,
> +			"%s: no interface specified for AF_PACKET ethdev\n",
> +			name);
> +		return -1;
> +	}
> +
> +	ifnamelen = strlen(ifname);

I am aware this is copy-paste but since update, perhaps we can use strnlen()
instead.

[dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Tiago Lam]

Use the underlying MTU to calculate the framsize to be used for the mmap
RINGs. This is to make it more flexible on deployments with different
MTU requirements, instead of using a pre-defined value of 2048B.

If a 'framsz' option is provided, that value is used instead and the MTU
of the underlying interface is ignored.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
---

v2: Fix checkpatches.sh and check-git-log.sh warnings.

---
 drivers/net/af_packet/rte_eth_af_packet.c | 33 ++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index 8d749a2..5549211 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -788,7 +788,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	unsigned k_idx;
 	unsigned int blockcount;
 	unsigned int blocksize = DFLT_BLOCK_SIZE;
-	unsigned int framesize = DFLT_FRAME_SIZE;
+	unsigned int framesize = 0;
 	unsigned int framecount = DFLT_FRAME_COUNT;
 	unsigned int qpairs = 1;
 	unsigned int qdisc_bypass = 1;
@@ -877,17 +877,40 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
 	}
 
 	ifnamelen = strlen(ifname);
-	if (ifnamelen >= sizeof(ifr.ifr_name)) {
+	if (ifnamelen < sizeof(ifr.ifr_name)) {
+		memcpy(ifr.ifr_name, ifname, ifnamelen);
+		ifr.ifr_name[ifnamelen] = '\0';
+	} else {
 		RTE_LOG(ERR, PMD,
 			"%s: I/F name too long (%s)\n",
 			name, ifname);
 		return -1;
 	}
 
+	/*
+	 * Base framesize on the MTU of the underlying interface, if no
+	 * 'framesz' option is given
+	 */
+	if (!framesize) {
+		if (ioctl(*sockfd, SIOCGIFMTU, &ifr) == -1) {
+			RTE_LOG(ERR, PMD,
+				"%s: ioctl failed (SIOCGIFMTU)",
+				name);
+			framesize = DFLT_FRAME_SIZE;
+		} else {
+			framesize = ifr.ifr_mtu;
+			/*
+			 * Align to TPACKET_ALIGNMENT and add TPACKET2_HDRLEN,
+			 * to account for the extra needed space
+			 */
+			framesize = TPACKET_ALIGN(framesize + TPACKET2_HDRLEN);
+		}
+	}
+
 	if (framesize > blocksize) {
-		PMD_LOG(ERR,
-			"%s: AF_PACKET MMAP frame size exceeds block size!",
-		        name);
+		RTE_LOG(ERR, PMD,
+			"%s: AF_PACKET MMAP frame size (%u) exceeds block size (%u)!",
+			name, framesize, blocksize);
 		return -1;
 	}
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Ferruh Yigit]

On 11/20/2018 10:26 AM, Tiago Lam wrote:
> Use the underlying MTU to calculate the framsize to be used for the mmap
> RINGs. This is to make it more flexible on deployments with different
> MTU requirements, instead of using a pre-defined value of 2048B.

This behavior change should be documented in af_packet documentation which is
missing unfortunately.
Would you able to introduce the initial/basic af_packet doc to at least to
document device argument? If not please let me know, I can work on it.

> 
> If a 'framsz' option is provided, that value is used instead and the MTU
> of the underlying interface is ignored.
> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
> 
> v2: Fix checkpatches.sh and check-git-log.sh warnings.

<...>

> @@ -877,17 +877,40 @@ rte_eth_from_packet(struct rte_vdev_device *dev,
>  	}
>  
>  	ifnamelen = strlen(ifname);
> -	if (ifnamelen >= sizeof(ifr.ifr_name)) {
> +	if (ifnamelen < sizeof(ifr.ifr_name)) {
> +		memcpy(ifr.ifr_name, ifname, ifnamelen);
> +		ifr.ifr_name[ifnamelen] = '\0';

This can be replaces with strlcpy().

> +	} else {
>  		RTE_LOG(ERR, PMD,
>  			"%s: I/F name too long (%s)\n",
>  			name, ifname);
>  		return -1;
>  	}
>  
> +	/*
> +	 * Base framesize on the MTU of the underlying interface, if no
> +	 * 'framesz' option is given
> +	 */
> +	if (!framesize) {
> +		if (ioctl(*sockfd, SIOCGIFMTU, &ifr) == -1) {
> +			RTE_LOG(ERR, PMD,
> +				"%s: ioctl failed (SIOCGIFMTU)",
> +				name);
> +			framesize = DFLT_FRAME_SIZE;

It may be good to add a log to say default frame size will be used, and perhaps
print out the default value.

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Ferruh Yigit]

On 11/27/2018 5:43 PM, Ferruh Yigit wrote:
> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>> Use the underlying MTU to calculate the framsize to be used for the mmap
>> RINGs. This is to make it more flexible on deployments with different
>> MTU requirements, instead of using a pre-defined value of 2048B.
> 
> This behavior change should be documented in af_packet documentation which is
> missing unfortunately.
> Would you able to introduce the initial/basic af_packet doc to at least to
> document device argument? If not please let me know, I can work on it.

Hi John W.,

Is there any chance you can work on an af_packet driver documentation on DPDK as
a maintainer of the PMD?

Thanks,
ferruh

> 
>>
>> If a 'framsz' option is provided, that value is used instead and the MTU
>> of the underlying interface is ignored.
>>
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>

<...>

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Lam, Tiago]

On 27/11/2018 17:43, Ferruh Yigit wrote:
> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>> Use the underlying MTU to calculate the framsize to be used for the mmap
>> RINGs. This is to make it more flexible on deployments with different
>> MTU requirements, instead of using a pre-defined value of 2048B.
> 
> This behavior change should be documented in af_packet documentation which is
> missing unfortunately.
> Would you able to introduce the initial/basic af_packet doc to at least to
> document device argument? If not please let me know, I can work on it.
> 

Thanks for the review, Ferruh.

Yeah, I don't mind cooking something up and submitting here for review;
I'll wait a couple of days for a reply from John W. before proceeding,
though.

But given there's no documentation for af_packet yet, do you prefer to
wait for that to be available, and apply it all together? Or could that
be applied later as part of another patch?

Tiago.

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Ferruh Yigit]

On 11/28/2018 1:12 PM, Lam, Tiago wrote:
> On 27/11/2018 17:43, Ferruh Yigit wrote:
>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>> RINGs. This is to make it more flexible on deployments with different
>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>
>> This behavior change should be documented in af_packet documentation which is
>> missing unfortunately.
>> Would you able to introduce the initial/basic af_packet doc to at least to
>> document device argument? If not please let me know, I can work on it.
>>
> 
> Thanks for the review, Ferruh.
> 
> Yeah, I don't mind cooking something up and submitting here for review;
> I'll wait a couple of days for a reply from John W. before proceeding,
> though.

Thanks, appreciated. Agreed to wait a little.

> 
> But given there's no documentation for af_packet yet, do you prefer to
> wait for that to be available, and apply it all together? Or could that
> be applied later as part of another patch?

Both are OK, depends on your availability.

I think it is better, to show the history, first patch as the documentation
patch for existing behavior and your patch updating framsz usage (3/3) to update
that document as well.

Thanks,
ferruh

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Lam, Tiago]

Hi Ferruh,

On 28/11/2018 13:33, Ferruh Yigit wrote:
> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>> RINGs. This is to make it more flexible on deployments with different
>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>
>>> This behavior change should be documented in af_packet documentation which is
>>> missing unfortunately.
>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>> document device argument? If not please let me know, I can work on it.
>>>
>>
>> Thanks for the review, Ferruh.
>>
>> Yeah, I don't mind cooking something up and submitting here for review;
>> I'll wait a couple of days for a reply from John W. before proceeding,
>> though.
> 
> Thanks, appreciated. Agreed to wait a little.
> 
>>
>> But given there's no documentation for af_packet yet, do you prefer to
>> wait for that to be available, and apply it all together? Or could that
>> be applied later as part of another patch?
> 
> Both are OK, depends on your availability.
> 
> I think it is better, to show the history, first patch as the documentation
> patch for existing behavior and your patch updating framsz usage (3/3) to update
> that document as well.

As agreed, I just sent a patch with an initial take on adding some docs
for af_packet. Once that's in I'll submit another revision of this
patchset, including an update to the documentation.

Just an aside, patch 1/3 of this series is a bugfix, it could go in
irrespective of the documentation, it seems.

Tiago.

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Ferruh Yigit]

On 12/17/2018 9:21 AM, Lam, Tiago wrote:
> Hi Ferruh,
> 
> On 28/11/2018 13:33, Ferruh Yigit wrote:
>> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>>> RINGs. This is to make it more flexible on deployments with different
>>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>>
>>>> This behavior change should be documented in af_packet documentation which is
>>>> missing unfortunately.
>>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>>> document device argument? If not please let me know, I can work on it.
>>>>
>>>
>>> Thanks for the review, Ferruh.
>>>
>>> Yeah, I don't mind cooking something up and submitting here for review;
>>> I'll wait a couple of days for a reply from John W. before proceeding,
>>> though.
>>
>> Thanks, appreciated. Agreed to wait a little.
>>
>>>
>>> But given there's no documentation for af_packet yet, do you prefer to
>>> wait for that to be available, and apply it all together? Or could that
>>> be applied later as part of another patch?
>>
>> Both are OK, depends on your availability.
>>
>> I think it is better, to show the history, first patch as the documentation
>> patch for existing behavior and your patch updating framsz usage (3/3) to update
>> that document as well.
> 
> As agreed, I just sent a patch with an initial take on adding some docs
> for af_packet. Once that's in I'll submit another revision of this
> patchset, including an update to the documentation.
> 
> Just an aside, patch 1/3 of this series is a bugfix, it could go in
> irrespective of the documentation, it seems.

Agreed. Doc patch is merged, I will get first patch and will wait for a new
version for next.

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Yigit, Ferruh]

On 11/28/2018 1:12 PM, Lam, Tiago wrote:
> On 27/11/2018 17:43, Ferruh Yigit wrote:
>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>> RINGs. This is to make it more flexible on deployments with different
>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>
>> This behavior change should be documented in af_packet documentation which is
>> missing unfortunately.
>> Would you able to introduce the initial/basic af_packet doc to at least to
>> document device argument? If not please let me know, I can work on it.
>>
> 
> Thanks for the review, Ferruh.
> 
> Yeah, I don't mind cooking something up and submitting here for review;
> I'll wait a couple of days for a reply from John W. before proceeding,
> though.
> 
> But given there's no documentation for af_packet yet, do you prefer to
> wait for that to be available, and apply it all together? Or could that
> be applied later as part of another patch?

Unfortunately Tiago was not able to work on this task anymore.
And since Tiago's af_packet doc update already merged, I was planning to
complete the task and applied the comments into his patchset but had a few
questions, sharing here in case there are more interested people on task, cc'ed
Ian & Kevin.

Code is not functioning correct when there are gaps in the block, I mean when
block size is not multiple of frame size. There can be some assumption in the
code that memory is continuous.

Also I am not sure the benefit of the using MTU for this case. There are a few
restrictions, block size should be multiple of page size, it is 4K by default.
When using MTU, 1500, for frame size, instead of 2048 Bytes hardcoded value,
still only 2 frames fit into blocksize and there is no benefit, (and it is not
functioning with current code as I mentioned above.)
So this can be required for the cases MTU is bigger than 2048, not sure if this
is the concern of the patch.
Also it can provide some memory optimization when MTU is 1024 bytes or less, so
this provides memory optimization more than flexibility on deployment.

Hi Ian, Kevin,

Are you aware of any use case of this patch in OvS?

Thanks,
ferruh

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Yigit, Ferruh]

On 2/18/2019 6:01 PM, Yigit, Ferruh wrote:
> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>> RINGs. This is to make it more flexible on deployments with different
>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>
>>> This behavior change should be documented in af_packet documentation which is
>>> missing unfortunately.
>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>> document device argument? If not please let me know, I can work on it.
>>>
>>
>> Thanks for the review, Ferruh.
>>
>> Yeah, I don't mind cooking something up and submitting here for review;
>> I'll wait a couple of days for a reply from John W. before proceeding,
>> though.
>>
>> But given there's no documentation for af_packet yet, do you prefer to
>> wait for that to be available, and apply it all together? Or could that
>> be applied later as part of another patch?
> 
> Unfortunately Tiago was not able to work on this task anymore.
> And since Tiago's af_packet doc update already merged, I was planning to
> complete the task and applied the comments into his patchset but had a few
> questions, sharing here in case there are more interested people on task, cc'ed
> Ian & Kevin.
> 
> Code is not functioning correct when there are gaps in the block, I mean when
> block size is not multiple of frame size. There can be some assumption in the
> code that memory is continuous.
> 
> Also I am not sure the benefit of the using MTU for this case. There are a few
> restrictions, block size should be multiple of page size, it is 4K by default.
> When using MTU, 1500, for frame size, instead of 2048 Bytes hardcoded value,
> still only 2 frames fit into blocksize and there is no benefit, (and it is not
> functioning with current code as I mentioned above.)
> So this can be required for the cases MTU is bigger than 2048, not sure if this
> is the concern of the patch.
> Also it can provide some memory optimization when MTU is 1024 bytes or less, so
> this provides memory optimization more than flexibility on deployment.
> 
> Hi Ian, Kevin,
> 
> Are you aware of any use case of this patch in OvS?

It seems there is no follow up to this patchset, I am updating them as not
applicable.

For references, patches mentioned are:
https://patches.dpdk.org/user/todo/dpdk/?series=2498

Re: [dpdk-dev] [PATCH v2 3/3] net/af_packet: get 'framesz' from the iface's MTU

[Yigit, Ferruh]

On 2/18/2019 6:01 PM, Yigit, Ferruh wrote:
> On 11/28/2018 1:12 PM, Lam, Tiago wrote:
>> On 27/11/2018 17:43, Ferruh Yigit wrote:
>>> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>>>> Use the underlying MTU to calculate the framsize to be used for the mmap
>>>> RINGs. This is to make it more flexible on deployments with different
>>>> MTU requirements, instead of using a pre-defined value of 2048B.
>>>
>>> This behavior change should be documented in af_packet documentation which is
>>> missing unfortunately.
>>> Would you able to introduce the initial/basic af_packet doc to at least to
>>> document device argument? If not please let me know, I can work on it.
>>>
>>
>> Thanks for the review, Ferruh.
>>
>> Yeah, I don't mind cooking something up and submitting here for review;
>> I'll wait a couple of days for a reply from John W. before proceeding,
>> though.
>>
>> But given there's no documentation for af_packet yet, do you prefer to
>> wait for that to be available, and apply it all together? Or could that
>> be applied later as part of another patch?
> 
> Unfortunately Tiago was not able to work on this task anymore.
> And since Tiago's af_packet doc update already merged, I was planning to
> complete the task and applied the comments into his patchset but had a few
> questions, sharing here in case there are more interested people on task, cc'ed
> Ian & Kevin.
> 
> Code is not functioning correct when there are gaps in the block, I mean when
> block size is not multiple of frame size. There can be some assumption in the
> code that memory is continuous.
> 
> Also I am not sure the benefit of the using MTU for this case. There are a few
> restrictions, block size should be multiple of page size, it is 4K by default.
> When using MTU, 1500, for frame size, instead of 2048 Bytes hardcoded value,
> still only 2 frames fit into blocksize and there is no benefit, (and it is not
> functioning with current code as I mentioned above.)
> So this can be required for the cases MTU is bigger than 2048, not sure if this
> is the concern of the patch.
> Also it can provide some memory optimization when MTU is 1024 bytes or less, so
> this provides memory optimization more than flexibility on deployment.
> 
> Hi Ian, Kevin,
> 
> Are you aware of any use case of this patch in OvS?

It seems there is no follow up to this patchset, I am updating them as not
applicable.

For references, patches mentioned are:
https://patches.dpdk.org/user/todo/dpdk/?series=2498

Re: [dpdk-dev] [PATCH v2 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[Kevin Traynor]

On 11/20/2018 10:26 AM, Tiago Lam wrote:
> When setting the MTU, eth_dev_mtu_set() is called to validate the
> provided MTU. As part of that, it calculates the useful area to store
> data and compares it against the MTU, to guarantee that there's enough
> space to store the data. It calculates that as:
>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
> 
> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
> sockaddr_ll) internally, meaning the useuful area of data above will
> have sizeof(struct sockaddr_ll) decremented twice.
> 
> Instead, the useful area of data should be calculated as:
>     "tp_frame_size - TPACKET2_HDRLEN"
> 
> This makes sure that there's enough useful area to fit the provided MTU
> after excluding tpacket2_hdr and sockaddr_ll.
> 
> Fixes: cc68ac4 ("net/af_packet: support MTU change")
> 

It should be for stable also?

> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> ---
>  drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
> index 95a98c6..264cfc0 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -433,8 +433,7 @@ eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>  	int ret;
>  	int s;
>  	unsigned int data_size = internals->req.tp_frame_size -
> -				 TPACKET2_HDRLEN -
> -				 sizeof(struct sockaddr_ll);
> +				 TPACKET2_HDRLEN;
>  
>  	if (mtu > data_size)
>  		return -EINVAL;
> 

Re: [dpdk-dev] [PATCH v2 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[Lam, Tiago]

On 20/11/2018 10:29, Kevin Traynor wrote:
> On 11/20/2018 10:26 AM, Tiago Lam wrote:
>> When setting the MTU, eth_dev_mtu_set() is called to validate the
>> provided MTU. As part of that, it calculates the useful area to store
>> data and compares it against the MTU, to guarantee that there's enough
>> space to store the data. It calculates that as:
>>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
>>
>> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
>> sockaddr_ll) internally, meaning the useuful area of data above will
>> have sizeof(struct sockaddr_ll) decremented twice.
>>
>> Instead, the useful area of data should be calculated as:
>>     "tp_frame_size - TPACKET2_HDRLEN"
>>
>> This makes sure that there's enough useful area to fit the provided MTU
>> after excluding tpacket2_hdr and sockaddr_ll.
>>
>> Fixes: cc68ac4 ("net/af_packet: support MTU change")
>>
> 
> It should be for stable also?

Indeed, thanks for pointing that out. I've missed the Cc there, but it
should be backported - I can track it down to 17.02. I'll make sure I
add the Cc if a new iteration is needed.

Tiago.

> 
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>> ---
>>  drivers/net/af_packet/rte_eth_af_packet.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
>> index 95a98c6..264cfc0 100644
>> --- a/drivers/net/af_packet/rte_eth_af_packet.c
>> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
>> @@ -433,8 +433,7 @@ eth_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
>>  	int ret;
>>  	int s;
>>  	unsigned int data_size = internals->req.tp_frame_size -
>> -				 TPACKET2_HDRLEN -
>> -				 sizeof(struct sockaddr_ll);
>> +				 TPACKET2_HDRLEN;
>>  
>>  	if (mtu > data_size)
>>  		return -EINVAL;
>>
> 

Re: [dpdk-dev] [PATCH v2 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[Ferruh Yigit]

On 11/20/2018 10:26 AM, Tiago Lam wrote:
> When setting the MTU, eth_dev_mtu_set() is called to validate the
> provided MTU. As part of that, it calculates the useful area to store
> data and compares it against the MTU, to guarantee that there's enough
> space to store the data. It calculates that as:
>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
> 
> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
> sockaddr_ll) internally, meaning the useuful area of data above will
> have sizeof(struct sockaddr_ll) decremented twice.

There are a few typos above.

> 
> Instead, the useful area of data should be calculated as:
>     "tp_frame_size - TPACKET2_HDRLEN"
> 
> This makes sure that there's enough useful area to fit the provided MTU
> after excluding tpacket2_hdr and sockaddr_ll.
> 
> Fixes: cc68ac4 ("net/af_packet: support MTU change")

The syntax is slightly different, can you please try following git alias:
alias.fixline=log -1 --abbrev=12 --format='Fixes: %h ("%s")%nCc: %ae'

> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>

Also can you please fix ./devtools/check-git-log.sh warnings on patches?

Re: [dpdk-dev] [PATCH v2 1/3] net/af_packet: set_mtu() decrements sockaddr twice

[Ferruh Yigit]

On 11/20/2018 10:26 AM, Tiago Lam wrote:
> When setting the MTU, eth_dev_mtu_set() is called to validate the
> provided MTU. As part of that, it calculates the useful area to store
> data and compares it against the MTU, to guarantee that there's enough
> space to store the data. It calculates that as:
>     "tp_frame_size - TPACKET2_HDRLEN - sizeof(struct sockaddr_ll)"
> 
> However, the TPACKET2_HDRLEN macro already increaments sizeof(struct
> sockaddr_ll) internally, meaning the useuful area of data above will
> have sizeof(struct sockaddr_ll) decremented twice.
> 
> Instead, the useful area of data should be calculated as:
>     "tp_frame_size - TPACKET2_HDRLEN"
> 
> This makes sure that there's enough useful area to fit the provided MTU
> after excluding tpacket2_hdr and sockaddr_ll.
> 
> Fixes: cc68ac4 ("net/af_packet: support MTU change")
> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/master, thanks.