[v2] pcap: support MTU set
Checks
Commit Message
Support rte_eth_dev_set_mtu for pcap driver when the
pcap device is convigured to point to a network interface.
This is rebased an consolidated from earlier version.
Added support for FreeBSD.
Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - fix FreeBSD part
drivers/net/pcap/pcap_ethdev.c | 49 ++++++++++++++++++++++++++-
drivers/net/pcap/pcap_osdep.h | 1 +
drivers/net/pcap/pcap_osdep_freebsd.c | 23 +++++++++++++
drivers/net/pcap/pcap_osdep_linux.c | 21 ++++++++++++
drivers/net/pcap/pcap_osdep_windows.c | 7 ++++
5 files changed, 100 insertions(+), 1 deletion(-)
Comments
On 7/4/2023 10:02 PM, Stephen Hemminger wrote:
> Support rte_eth_dev_set_mtu for pcap driver when the
> pcap device is convigured to point to a network interface.
>
> This is rebased an consolidated from earlier version.
> Added support for FreeBSD.
>
As far as I understand motivation is to make pcap PMD behave close the
physical NIC and able to test the application MTU feature.
If so, Ido's v4 was simpler, which doesn't distinguish if pcap backed by
physical interface or .pcap file.
What was wrong with that approach?
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v2 - fix FreeBSD part
>
> drivers/net/pcap/pcap_ethdev.c | 49 ++++++++++++++++++++++++++-
> drivers/net/pcap/pcap_osdep.h | 1 +
> drivers/net/pcap/pcap_osdep_freebsd.c | 23 +++++++++++++
> drivers/net/pcap/pcap_osdep_linux.c | 21 ++++++++++++
> drivers/net/pcap/pcap_osdep_windows.c | 7 ++++
> 5 files changed, 100 insertions(+), 1 deletion(-)
>
Better to update driver documentation and release notes too.
> diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
> index bfec0850456f..672f42d30d8e 100644
> --- a/drivers/net/pcap/pcap_ethdev.c
> +++ b/drivers/net/pcap/pcap_ethdev.c
> @@ -5,6 +5,7 @@
> */
>
> #include <stdlib.h>
> +#include <stdbool.h>
> #include <time.h>
>
> #include <pcap.h>
> @@ -495,8 +496,13 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> */
> ret = pcap_sendpacket(pcap,
> rte_pktmbuf_read(mbuf, 0, len, temp_data), len);
> - if (unlikely(ret != 0))
> + if (unlikely(ret != 0)) {
> + /* Oversize packet dropped due to MTU */
> + if (errno == EMSGSIZE)
> + continue;
This will leak mbuf, since loop continues application will assume driver
sent the packet and freed mbuf. Ido's version does it right.
> break;
> + }
> +
> num_tx++;
> tx_bytes += len;
> rte_pktmbuf_free(mbuf);
> @@ -808,6 +814,46 @@ eth_stats_reset(struct rte_eth_dev *dev)
> return 0;
> }
>
> +static int
> +eth_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
> +{
> + struct pmd_internals *internals = dev->data->dev_private;
> + unsigned int i;
> + bool is_supported = false;
> + int ret;
> +
> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> + struct pcap_rx_queue *queue = &internals->rx_queue[i];
> +
> + if ((strcmp(queue->type, ETH_PCAP_IFACE_ARG) == 0) ||
> + (strcmp(queue->type, ETH_PCAP_RX_IFACE_ARG) == 0) ||
> + (strcmp(queue->type, ETH_PCAP_RX_IFACE_IN_ARG) == 0)) {
> + ret = osdep_iface_mtu_set(queue->name, mtu);
> + if (ret < 0)
> + return ret;
> + is_supported = true;
> + }
If there are multiple devices (each pcap queue represents a device), and
one in the middle one fails to set MTU, there will be mixed MTU values
in devices.
> + }
> +
> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> + struct pcap_tx_queue *queue = &internals->tx_queue[i];
> +
> + if ((strcmp(queue->type, ETH_PCAP_IFACE_ARG) == 0) ||
Rx devices and Tx devices can be same device, causing duplicated MTU set
for same device, but it is hard to detect if they are same devices.
At least it is easier to detect when queue type is 'ETH_PCAP_IFACE_ARG',
what about set MTU once for this queue type?
On Wed, 5 Jul 2023 12:37:41 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> On 7/4/2023 10:02 PM, Stephen Hemminger wrote:
> > Support rte_eth_dev_set_mtu for pcap driver when the
> > pcap device is convigured to point to a network interface.
> >
> > This is rebased an consolidated from earlier version.
> > Added support for FreeBSD.
> >
>
> As far as I understand motivation is to make pcap PMD behave close the
> physical NIC and able to test the application MTU feature.
> If so, Ido's v4 was simpler, which doesn't distinguish if pcap backed by
> physical interface or .pcap file.
> What was wrong with that approach?
I started with Ido's patch, then:
- combined the two patches into one.
- fixed the error handling (propogate errno correctly)
- add missing freebsd support
Normally would just give feedback but patch was so old not sure if it was
stuck and unlikely to get merged.
I've suggested 2 ways to do it
1. Data path enforcement by pcap pmd [PATCH v4]
http://patches.dpdk.org/project/dpdk/patch/20220606162147.57218-1-ido@cgstowernetworks.com/
2. Control path only sets the underlying OS network interface MTU [PATCH v8]
http://patches.dpdk.org/project/dpdk/cover/20220620083944.51517-1-ido@cgstowernetworks.com/
It was pretty long ago - not sure which is in favor
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, 5 July 2023 18:19
> To: Ferruh Yigit <ferruh.yigit@amd.com>
> Cc: dev@dpdk.org; Ido Goshen <Ido@cgstowernetworks.com>
> Subject: Re: [PATCH v2] pcap: support MTU set
>
> On Wed, 5 Jul 2023 12:37:41 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> > On 7/4/2023 10:02 PM, Stephen Hemminger wrote:
> > > Support rte_eth_dev_set_mtu for pcap driver when the pcap device is
> > > convigured to point to a network interface.
> > >
> > > This is rebased an consolidated from earlier version.
> > > Added support for FreeBSD.
> > >
> >
> > As far as I understand motivation is to make pcap PMD behave close the
> > physical NIC and able to test the application MTU feature.
> > If so, Ido's v4 was simpler, which doesn't distinguish if pcap backed
> > by physical interface or .pcap file.
> > What was wrong with that approach?
>
> I started with Ido's patch, then:
> - combined the two patches into one.
> - fixed the error handling (propogate errno correctly)
> - add missing freebsd support
>
> Normally would just give feedback but patch was so old not sure if it was
> stuck and unlikely to get merged.
@@ -5,6 +5,7 @@
*/
#include <stdlib.h>
+#include <stdbool.h>
#include <time.h>
#include <pcap.h>
@@ -495,8 +496,13 @@ eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
*/
ret = pcap_sendpacket(pcap,
rte_pktmbuf_read(mbuf, 0, len, temp_data), len);
- if (unlikely(ret != 0))
+ if (unlikely(ret != 0)) {
+ /* Oversize packet dropped due to MTU */
+ if (errno == EMSGSIZE)
+ continue;
break;
+ }
+
num_tx++;
tx_bytes += len;
rte_pktmbuf_free(mbuf);
@@ -808,6 +814,46 @@ eth_stats_reset(struct rte_eth_dev *dev)
return 0;
}
+static int
+eth_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
+{
+ struct pmd_internals *internals = dev->data->dev_private;
+ unsigned int i;
+ bool is_supported = false;
+ int ret;
+
+ for (i = 0; i < dev->data->nb_rx_queues; i++) {
+ struct pcap_rx_queue *queue = &internals->rx_queue[i];
+
+ if ((strcmp(queue->type, ETH_PCAP_IFACE_ARG) == 0) ||
+ (strcmp(queue->type, ETH_PCAP_RX_IFACE_ARG) == 0) ||
+ (strcmp(queue->type, ETH_PCAP_RX_IFACE_IN_ARG) == 0)) {
+ ret = osdep_iface_mtu_set(queue->name, mtu);
+ if (ret < 0)
+ return ret;
+ is_supported = true;
+ }
+ }
+
+ for (i = 0; i < dev->data->nb_tx_queues; i++) {
+ struct pcap_tx_queue *queue = &internals->tx_queue[i];
+
+ if ((strcmp(queue->type, ETH_PCAP_IFACE_ARG) == 0) ||
+ (strcmp(queue->type, ETH_PCAP_TX_IFACE_ARG) == 0)) {
+ ret = osdep_iface_mtu_set(queue->name, mtu);
+ if (ret < 0)
+ return ret;
+ is_supported = true;
+ }
+ }
+
+ if (!is_supported)
+ return -ENOTSUP;
+
+ PMD_LOG(INFO, "MTU set %s %u\n", dev->device->name, mtu);
+ return 0;
+}
+
static inline void
infinite_rx_ring_free(struct rte_ring *pkts)
{
@@ -1005,6 +1051,7 @@ static const struct eth_dev_ops ops = {
.link_update = eth_link_update,
.stats_get = eth_stats_get,
.stats_reset = eth_stats_reset,
+ .mtu_set = eth_mtu_set,
};
static int
@@ -14,5 +14,6 @@ extern int eth_pcap_logtype;
int osdep_iface_index_get(const char *name);
int osdep_iface_mac_get(const char *name, struct rte_ether_addr *mac);
+int osdep_iface_mtu_set(const char *name, uint16_t mtu);
#endif
@@ -6,7 +6,9 @@
#include <net/if.h>
#include <net/if_dl.h>
+#include <sys/ioctl.h>
#include <sys/sysctl.h>
+#include <unistd.h>
#include <rte_malloc.h>
#include <rte_memcpy.h>
@@ -57,3 +59,24 @@ osdep_iface_mac_get(const char *if_name, struct rte_ether_addr *mac)
rte_free(buf);
return 0;
}
+
+int
+osdep_iface_mtu_set(const char *if_name, uint16_t mtu)
+{
+ struct ifreq ifr = { };
+ int if_fd = socket(AF_INET, SOCK_DGRAM, 0);
+
+ if (if_fd == -1)
+ return -errno;
+
+ strlcpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name));
+ ifr.ifr_mtu = mtu;
+ if (ioctl(if_fd, SIOCSIFMTU, &ifr)) {
+ PMD_LOG(ERR, "%s mtu set to %d failed\n", if_name, mtu);
+ close(if_fd);
+ return -errno;
+ }
+
+ close(if_fd);
+ return 0;
+}
@@ -40,3 +40,24 @@ osdep_iface_mac_get(const char *if_name, struct rte_ether_addr *mac)
close(if_fd);
return 0;
}
+
+int
+osdep_iface_mtu_set(const char *if_name, uint16_t mtu)
+{
+ struct ifreq ifr;
+ int if_fd = socket(AF_INET, SOCK_DGRAM, 0);
+
+ if (if_fd == -1)
+ return -errno;
+
+ rte_strscpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name));
+ ifr.ifr_mtu = mtu;
+ if (ioctl(if_fd, SIOCSIFMTU, &ifr)) {
+ PMD_LOG(ERR, "%s mtu set to %d failed\n", if_name, mtu);
+ close(if_fd);
+ return -errno;
+ }
+
+ close(if_fd);
+ return 0;
+}
@@ -116,3 +116,10 @@ osdep_iface_mac_get(const char *device_name, struct rte_ether_addr *mac)
free(info);
return ret;
}
+
+int
+osdep_iface_mtu_set(__rte_unused const char *device_name, __rte_unused uint16_t mtu)
+{
+ PMD_LOG(ERR, "mtu set not supported on Windows\n");
+ return -ENOTSUP;
+}