Skip to contentSkip to conversation

eal/hotplug: allow monitor to be setup by multiple places

PATCH Superseded Long Li 5 participants 9 messages Dec 1, 2020 – Jan 17, 2021 Network drivers

lore
Share & tools
lore

Long Li

author
#0 lore raw

PATCH 1/2 eal/hotplug: allow monitor to be setup by multiple places

From: Stephen Hemminger <[email protected]>

In some cases, a device or infrastructure may want to enable hotplug but application may also try and start hotplug as well. Therefore change the monitor_started from a boolean into a reference count.

---
 lib/librte_eal/linux/eal_dev.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
lib/librte_eal/linux/eal_dev.c +7 -8
index 5c0e752b2d..5fa679989e 100644
@@ -23,8 +23,11 @@
 
 #include "eal_private.h"
 
-static struct rte_intr_handle intr_handle = {.fd = -1 };
-static bool monitor_started;
+static struct rte_intr_handle intr_handle = {
+	.type = RTE_INTR_HANDLE_DEV_EVENT,
+	.fd = -1,
+};
+static uint32_t monitor_refcount;
 static bool hotplug_handle;
 
 #define EAL_UEV_MSG_LEN 4096
@@ -300,7 +303,7 @@ rte_dev_event_monitor_start(void)
 {
 	int ret;
 
-	if (monitor_started)
+	if (__atomic_fetch_add(&monitor_refcount, 1, __ATOMIC_RELAXED))
 		return 0;
 
 	ret = dev_uev_socket_fd_create();
@@ -309,7 +312,6 @@ rte_dev_event_monitor_start(void)
 		return -1;
 	}
 
-	intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT;
 	ret = rte_intr_callback_register(&intr_handle, dev_uev_handler, NULL);
 
 	if (ret) {
@@ -317,8 +319,6 @@ rte_dev_event_monitor_start(void)
 		return -1;
 	}
 
-	monitor_started = true;
-
 	return 0;
 }
 
@@ -327,7 +327,7 @@ rte_dev_event_monitor_stop(void)
 {
 	int ret;
 
-	if (!monitor_started)
+	if (__atomic_sub_fetch(&monitor_refcount, 1, __ATOMIC_RELAXED))
 		return 0;
 
 	ret = rte_intr_callback_unregister(&intr_handle, dev_uev_handler,
@@ -339,7 +339,6 @@ rte_dev_event_monitor_stop(void)
 
 	close(intr_handle.fd);
 	intr_handle.fd = -1;
-	monitor_started = false;
 
 	return 0;
 }
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 2/2 net/netvsc: support VF device hot add/remove

From: Long Li <[email protected]>

When a VF device is present, netvsc can send or receive packets over the VF device. The VF device driver communicates directly with the PCI device via the PF from the host hypervisor. This is faster than exchanging data with netvsp via vmbus, i.e. syntheic path.

In Azure and Hyper-v environments, VF device can be hot added or hot removed at anytime while guest VM is running. This patch improves netvsc to support VF device hot add/remove.

1. netvsc monitors all system hot add activities over the PCI bus. When it detects a VF device is added to the system and is managed under this netvsc device, it asks EAL to probe and start this VF device, then it attaches and switches data path to the VF device.

2. After a VF device is attached to netvsc, netvsc monitors this device on hot remove. When this VF device is hot removed, netvsc switches data path to synthetic, stops this VF device and removes it from EAL.

3. If any failure happens during a VF device hot remove or add, the netvsc falls back to synthetic path for all data traffic.

---
 drivers/net/netvsc/hn_ethdev.c | 162 ++++++++++++++++-
 drivers/net/netvsc/hn_nvs.c    |   4 +-
 drivers/net/netvsc/hn_nvs.h    |   2 +-
 drivers/net/netvsc/hn_rxtx.c   |  36 ++--
 drivers/net/netvsc/hn_var.h    |  52 ++++--
 drivers/net/netvsc/hn_vf.c     | 318 +++++++++++++++++++++++++++------
 6 files changed, 485 insertions(+), 89 deletions(-)
drivers/net/netvsc/hn_ethdev.c +155 -7
index 49f954305d..1c0a4b2985 100644
@@ -9,6 +9,10 @@
 #include <stdio.h>
 #include <errno.h>
 #include <unistd.h>
+#include <dirent.h>
+#include <net/if.h>
+#include <net/if_arp.h>
+#include <sys/ioctl.h>
 
 #include <rte_ethdev.h>
 #include <rte_memcpy.h>
@@ -27,6 +31,7 @@
 #include <rte_eal.h>
 #include <rte_dev.h>
 #include <rte_bus_vmbus.h>
+#include <rte_alarm.h>
 
 #include "hn_logs.h"
 #include "hn_var.h"
@@ -50,6 +55,9 @@
 #define NETVSC_ARG_TXBREAK "tx_copybreak"
 #define NETVSC_ARG_RX_EXTMBUF_ENABLE "rx_extmbuf_enable"
 
+/* The max number of retry when hot adding a VF device */
+#define NETVSC_MAX_HOTADD_RETRY 5
+
 struct hn_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
 	unsigned int offset;
@@ -542,6 +550,124 @@ static int hn_subchan_configure(struct hn_data *hv,
 	return err;
 }
 
+static void netvsc_hotplug_retry(void *args)
+{
+	int ret;
+	struct hn_data *hv = args;
+	struct rte_eth_dev *dev = &rte_eth_devices[hv->port_id];
+	struct rte_devargs *d = &hv->devargs;
+	char buf[256];
+
+	DIR *di;
+	struct dirent *dir;
+	struct ifreq req;
+	struct rte_ether_addr eth_addr;
+	int s;
+
+	PMD_DRV_LOG(DEBUG, "%s: retry count %d\n",
+		    __func__, hv->eal_hot_plug_retry);
+
+	if (hv->eal_hot_plug_retry++ > NETVSC_MAX_HOTADD_RETRY)
+		return;
+
+	snprintf(buf, sizeof(buf), "/sys/bus/pci/devices/%s/net", d->name);
+	di = opendir(buf);
+	if (!di) {
+		PMD_DRV_LOG(DEBUG, "%s: can't open directory %s, "
+			    "retrying in 1 second\n", __func__, buf);
+		goto retry;
+	}
+
+	while ((dir = readdir(di))) {
+		/* Skip . and .. directories */
+		if (!strcmp(dir->d_name, ".") || !strcmp(dir->d_name, ".."))
+			continue;
+
+		/* trying to get mac address if this is a network device*/
+		s = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
+		if (s == -1) {
+			PMD_DRV_LOG(ERR, "Failed to create socket errno %d\n",
+				    errno);
+			closedir(di);
+			return;
+		}
+		strlcpy(req.ifr_name, dir->d_name, sizeof(req.ifr_name));
+		ret = ioctl(s, SIOCGIFHWADDR, &req);
+		close(s);
+		if (ret == -1) {
+			PMD_DRV_LOG(ERR, "Failed to send SIOCGIFHWADDR for "
+				    "device %s\n", dir->d_name);
+			continue;
+		}
+		if (req.ifr_hwaddr.sa_family != ARPHRD_ETHER)
+			continue;
+		memcpy(eth_addr.addr_bytes, req.ifr_hwaddr.sa_data,
+		       RTE_DIM(eth_addr.addr_bytes));
+
+		if (rte_is_same_ether_addr(&eth_addr, dev->data->mac_addrs)) {
+			PMD_DRV_LOG(NOTICE, "Found matching MAC address, "
+				    "adding device %s network name %s\n",
+				    d->name, dir->d_name);
+			ret = rte_eal_hotplug_add(d->bus->name, d->name,
+						  d->args);
+			if (ret)
+				PMD_DRV_LOG(ERR,
+					    "Failed to add PCI device %s\n",
+					    d->name);
+		}
+		/* When the code reaches here, we either have already added
+		 * the device, or its MAC address did not match.
+		 */
+		closedir(di);
+		return;
+	}
+retry:
+	/* The device is still being initialized, retry after 1 second */
+	rte_eal_alarm_set(1000000, netvsc_hotplug_retry, hv);
+}
+
+static void
+netvsc_hotadd_callback(const char *device_name, enum rte_dev_event_type type,
+		       void *arg)
+{
+	struct hn_data *hv = arg;
+	struct rte_devargs *d = &hv->devargs;
+	int ret;
+
+	PMD_DRV_LOG(INFO, "Device notification type=%d device_name=%s\n",
+		    type, device_name);
+
+	switch (type) {
+	case RTE_DEV_EVENT_ADD:
+		/* if we already has a VF, don't check on hot add */
+		if (hv->vf_ctx.vf_state > vf_removed)
+			break;
+
+		ret = rte_devargs_parse(d, device_name);
+		if (ret) {
+			PMD_DRV_LOG(ERR,
+				    "devargs parsing failed ret=%d\n", ret);
+			return;
+		}
+
+		if (!strcmp(d->bus->name, "pci")) {
+			/* Start the process of figuring out if this
+			 * PCI device is a VF device
+			 */
+			hv->eal_hot_plug_retry = 0;
+			rte_eal_alarm_set(1000000, netvsc_hotplug_retry, hv);
+		}
+
+		/* We will switch to VF on RDNIS configure message
+		 * sent from VSP
+		 */
+
+		break;
+	default:
+		break;
+	}
+}
+
 static int hn_dev_configure(struct rte_eth_dev *dev)
 {
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
@@ -617,7 +743,7 @@ static int hn_dev_configure(struct rte_eth_dev *dev)
 		}
 	}
 
-	return hn_vf_configure(dev, dev_conf);
+	return hn_vf_configure_locked(dev, dev_conf);
 }
 
 static int hn_dev_stats_get(struct rte_eth_dev *dev,
@@ -827,6 +953,14 @@ hn_dev_start(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* Register to monitor hot plug events */
+	error = rte_dev_event_callback_register(NULL, netvsc_hotadd_callback,
+						hv);
+	if (error) {
+		PMD_DRV_LOG(ERR, "failed to register device event callback\n");
+		return error;
+	}
+
 	error = hn_rndis_set_rxfilter(hv,
 				      NDIS_PACKET_TYPE_BROADCAST |
 				      NDIS_PACKET_TYPE_ALL_MULTICAST |
@@ -853,6 +987,7 @@ hn_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 	dev->data->dev_started = 0;
 
+	rte_dev_event_callback_unregister(NULL, netvsc_hotadd_callback, hv);
 	hn_rndis_set_rxfilter(hv, 0);
 	return hn_vf_stop(dev);
 }
@@ -861,11 +996,14 @@ static int
 hn_dev_close(struct rte_eth_dev *dev)
 {
 	int ret;
+	struct hn_data *hv = dev->data->dev_private;
 
 	PMD_INIT_FUNC_TRACE();
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	rte_eal_alarm_cancel(netvsc_hotplug_retry, &hv->devargs);
+
 	ret = hn_vf_close(dev);
 	hn_dev_free_queues(dev);
 
@@ -990,7 +1128,10 @@ eth_hn_dev_init(struct rte_eth_dev *eth_dev)
 	hv->max_queues = 1;
 
 	rte_rwlock_init(&hv->vf_lock);
-	hv->vf_port = HN_INVALID_PORT;
+	hv->vf_ctx.vf_vsc_switched = false;
+	hv->vf_ctx.vf_vsp_reported = false;
+	hv->vf_ctx.vf_attached = false;
+	hv->vf_ctx.vf_state = vf_unknown;
 
 	err = hn_parse_args(eth_dev);
 	if (err)
@@ -1044,12 +1185,10 @@ eth_hn_dev_init(struct rte_eth_dev *eth_dev)
 	hv->max_queues = RTE_MIN(rxr_cnt, (unsigned int)max_chan);
 
 	/* If VF was reported but not added, do it now */
-	if (hv->vf_present && !hn_vf_attached(hv)) {
+	if (hv->vf_ctx.vf_vsp_reported && !hv->vf_ctx.vf_vsc_switched) {
 		PMD_INIT_LOG(DEBUG, "Adding VF device");
 
 		err = hn_vf_add(eth_dev, hv);
-		if (err)
-			hv->vf_present = 0;
 	}
 
 	return 0;
@@ -1095,15 +1234,23 @@ static int eth_hn_probe(struct rte_vmbus_driver *drv __rte_unused,
 
 	PMD_INIT_FUNC_TRACE();
 
+	ret = rte_dev_event_monitor_start();
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to start device event monitoring\n");
+		return ret;
+	}
+
 	eth_dev = eth_dev_vmbus_allocate(dev, sizeof(struct hn_data));
 	if (!eth_dev)
 		return -ENOMEM;
 
 	ret = eth_hn_dev_init(eth_dev);
-	if (ret)
+	if (ret) {
 		eth_dev_vmbus_release(eth_dev);
-	else
+		rte_dev_event_monitor_stop();
+	} else {
 		rte_eth_dev_probing_finish(eth_dev);
+	}
 
 	return ret;
 }
@@ -1124,6 +1271,7 @@ static int eth_hn_remove(struct rte_vmbus_device *dev)
 		return ret;
 
 	eth_dev_vmbus_release(eth_dev);
+	rte_dev_event_monitor_stop();
 	return 0;
 }
 
drivers/net/netvsc/hn_nvs.c +3 -1
index eeb82ab9ee..a0ee7d8bfa 100644
@@ -569,7 +569,7 @@ hn_nvs_alloc_subchans(struct hn_data *hv, uint32_t *nsubch)
 	return 0;
 }
 
-void
+int
 hn_nvs_set_datapath(struct hn_data *hv, uint32_t path)
 {
 	struct hn_nvs_datapath dp;
@@ -588,4 +588,6 @@ hn_nvs_set_datapath(struct hn_data *hv, uint32_t path)
 			    "send set datapath failed: %d",
 			    error);
 	}
+
+	return error;
 }
drivers/net/netvsc/hn_nvs.h +1 -1
index 015839e364..3766d2ee34 100644
@@ -212,7 +212,7 @@ int	hn_nvs_attach(struct hn_data *hv, unsigned int mtu);
 void	hn_nvs_detach(struct hn_data *hv);
 void	hn_nvs_ack_rxbuf(struct vmbus_channel *chan, uint64_t tid);
 int	hn_nvs_alloc_subchans(struct hn_data *hv, uint32_t *nsubch);
-void	hn_nvs_set_datapath(struct hn_data *hv, uint32_t path);
+int	hn_nvs_set_datapath(struct hn_data *hv, uint32_t path);
 void	hn_nvs_handle_vfassoc(struct rte_eth_dev *dev,
 			      const struct vmbus_chanpkt_hdr *hdr,
 			      const void *data);
drivers/net/netvsc/hn_rxtx.c +22 -14
index 015662fdb4..0f4ef0100b 100644
@@ -1492,16 +1492,20 @@ hn_xmit_pkts(void *ptxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		hn_process_events(hv, txq->queue_id, 0);
 
 	/* Transmit over VF if present and up */
-	rte_rwlock_read_lock(&hv->vf_lock);
-	vf_dev = hn_get_vf_dev(hv);
-	if (vf_dev && vf_dev->data->dev_started) {
-		void *sub_q = vf_dev->data->tx_queues[queue_id];
-
-		nb_tx = (*vf_dev->tx_pkt_burst)(sub_q, tx_pkts, nb_pkts);
+	if (hv->vf_ctx.vf_vsc_switched) {
+		rte_rwlock_read_lock(&hv->vf_lock);
+		vf_dev = hn_get_vf_dev(hv);
+		if (hv->vf_ctx.vf_vsc_switched && vf_dev &&
+		    vf_dev->data->dev_started) {
+			void *sub_q = vf_dev->data->tx_queues[queue_id];
+
+			nb_tx = (*vf_dev->tx_pkt_burst)
+					(sub_q, tx_pkts, nb_pkts);
+			rte_rwlock_read_unlock(&hv->vf_lock);
+			return nb_tx;
+		}
 		rte_rwlock_read_unlock(&hv->vf_lock);
-		return nb_tx;
 	}
-	rte_rwlock_read_unlock(&hv->vf_lock);
 
 	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
 		struct rte_mbuf *m = tx_pkts[nb_tx];
@@ -1614,13 +1618,17 @@ hn_recv_pkts(void *prxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					   (void **)rx_pkts, nb_pkts, NULL);
 
 	/* If VF is available, check that as well */
-	rte_rwlock_read_lock(&hv->vf_lock);
-	vf_dev = hn_get_vf_dev(hv);
-	if (vf_dev && vf_dev->data->dev_started)
-		nb_rcv += hn_recv_vf(vf_dev->data->port_id, rxq,
-				     rx_pkts + nb_rcv, nb_pkts - nb_rcv);
+	if (hv->vf_ctx.vf_vsc_switched) {
+		rte_rwlock_read_lock(&hv->vf_lock);
+		vf_dev = hn_get_vf_dev(hv);
+		if (hv->vf_ctx.vf_vsc_switched && vf_dev &&
+		    vf_dev->data->dev_started)
+			nb_rcv += hn_recv_vf(vf_dev->data->port_id, rxq,
+					     rx_pkts + nb_rcv,
+					     nb_pkts - nb_rcv);
 
-	rte_rwlock_read_unlock(&hv->vf_lock);
+		rte_rwlock_read_unlock(&hv->vf_lock);
+	}
 	return nb_rcv;
 }
 
drivers/net/netvsc/hn_var.h +36 -16

91 lines not on this page · raw message

drivers/net/netvsc/hn_vf.c +268 -50

405 lines not on this page · raw message

Signature · 1 line
2.25.1
author
#2 lore raw

PATCH v2 2/2 net/netvsc: support VF device hot add/remove

From: Long Li <[email protected]>

When a VF device is present, netvsc can send or receive packets over the VF device. The VF device driver communicates directly with the PCI device via the PF from the host hypervisor. This is faster than exchanging data with netvsp via vmbus, i.e. syntheic path.

In Azure and Hyper-v environments, VF device can be hot added or hot removed at anytime while guest VM is running. This patch improves netvsc to support VF device hot add/remove.

1. netvsc monitors all system hot add activities over the PCI bus. When it detects a VF device is added to the system and is managed under this netvsc device, it asks EAL to probe and start this VF device, then it attaches and switches data path to the VF device.

2. After a VF device is attached to netvsc, netvsc monitors this device on hot remove. When this VF device is hot removed, netvsc switches data path to synthetic, stops this VF device and removes it from EAL.

3. If any failure happens during a VF device hot remove or add, the netvsc falls back to synthetic path for all data traffic.

---
 drivers/net/netvsc/hn_ethdev.c | 166 ++++++++++++++++-
 drivers/net/netvsc/hn_nvs.c    |   4 +-
 drivers/net/netvsc/hn_nvs.h    |   2 +-
 drivers/net/netvsc/hn_rxtx.c   |  36 ++--
 drivers/net/netvsc/hn_var.h    |  52 ++++--
 drivers/net/netvsc/hn_vf.c     | 318 +++++++++++++++++++++++++++------
 6 files changed, 489 insertions(+), 89 deletions(-)
drivers/net/netvsc/hn_ethdev.c +159 -7
index 49f954305d..5a401b4b06 100644
@@ -9,6 +9,10 @@
 #include <stdio.h>
 #include <errno.h>
 #include <unistd.h>
+#include <dirent.h>
+#include <net/if.h>
+#include <net/if_arp.h>
+#include <sys/ioctl.h>
 
 #include <rte_ethdev.h>
 #include <rte_memcpy.h>
@@ -27,6 +31,7 @@
 #include <rte_eal.h>
 #include <rte_dev.h>
 #include <rte_bus_vmbus.h>
+#include <rte_alarm.h>
 
 #include "hn_logs.h"
 #include "hn_var.h"
@@ -50,6 +55,9 @@
 #define NETVSC_ARG_TXBREAK "tx_copybreak"
 #define NETVSC_ARG_RX_EXTMBUF_ENABLE "rx_extmbuf_enable"
 
+/* The max number of retry when hot adding a VF device */
+#define NETVSC_MAX_HOTADD_RETRY 10
+
 struct hn_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
 	unsigned int offset;
@@ -542,6 +550,128 @@ static int hn_subchan_configure(struct hn_data *hv,
 	return err;
 }
 
+static void netvsc_hotplug_retry(void *args)
+{
+	int ret;
+	struct hn_data *hv = args;
+	struct rte_eth_dev *dev = &rte_eth_devices[hv->port_id];
+	struct rte_devargs *d = &hv->devargs;
+	char buf[256];
+
+	DIR *di;
+	struct dirent *dir;
+	struct ifreq req;
+	struct rte_ether_addr eth_addr;
+	int s;
+
+	PMD_DRV_LOG(DEBUG, "%s: retry count %d\n",
+		    __func__, hv->eal_hot_plug_retry);
+
+	if (hv->eal_hot_plug_retry++ > NETVSC_MAX_HOTADD_RETRY)
+		return;
+
+	snprintf(buf, sizeof(buf), "/sys/bus/pci/devices/%s/net", d->name);
+	di = opendir(buf);
+	if (!di) {
+		PMD_DRV_LOG(DEBUG, "%s: can't open directory %s, "
+			    "retrying in 1 second\n", __func__, buf);
+		goto retry;
+	}
+
+	while ((dir = readdir(di))) {
+		/* Skip . and .. directories */
+		if (!strcmp(dir->d_name, ".") || !strcmp(dir->d_name, ".."))
+			continue;
+
+		/* trying to get mac address if this is a network device*/
+		s = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
+		if (s == -1) {
+			PMD_DRV_LOG(ERR, "Failed to create socket errno %d\n",
+				    errno);
+			break;
+		}
+		strlcpy(req.ifr_name, dir->d_name, sizeof(req.ifr_name));
+		ret = ioctl(s, SIOCGIFHWADDR, &req);
+		close(s);
+		if (ret == -1) {
+			PMD_DRV_LOG(ERR, "Failed to send SIOCGIFHWADDR for "
+				    "device %s\n", dir->d_name);
+			break;
+		}
+		if (req.ifr_hwaddr.sa_family != ARPHRD_ETHER) {
+			closedir(di);
+			return;
+		}
+		memcpy(eth_addr.addr_bytes, req.ifr_hwaddr.sa_data,
+		       RTE_DIM(eth_addr.addr_bytes));
+
+		if (rte_is_same_ether_addr(&eth_addr, dev->data->mac_addrs)) {
+			PMD_DRV_LOG(NOTICE, "Found matching MAC address, "
+				    "adding device %s network name %s\n",
+				    d->name, dir->d_name);
+			ret = rte_eal_hotplug_add(d->bus->name, d->name,
+						  d->args);
+			if (ret) {
+				PMD_DRV_LOG(ERR,
+					    "Failed to add PCI device %s\n",
+					    d->name);
+				break;
+			}
+		}
+		/* When the code reaches here, we either have already added
+		 * the device, or its MAC address did not match.
+		 */
+		closedir(di);
+		return;
+	}
+	closedir(di);
+retry:
+	/* The device is still being initialized, retry after 1 second */
+	rte_eal_alarm_set(1000000, netvsc_hotplug_retry, hv);
+}
+
+static void
+netvsc_hotadd_callback(const char *device_name, enum rte_dev_event_type type,
+		       void *arg)
+{
+	struct hn_data *hv = arg;
+	struct rte_devargs *d = &hv->devargs;
+	int ret;
+
+	PMD_DRV_LOG(INFO, "Device notification type=%d device_name=%s\n",
+		    type, device_name);
+
+	switch (type) {
+	case RTE_DEV_EVENT_ADD:
+		/* if we already has a VF, don't check on hot add */
+		if (hv->vf_ctx.vf_state > vf_removed)
+			break;
+
+		ret = rte_devargs_parse(d, device_name);
+		if (ret) {
+			PMD_DRV_LOG(ERR,
+				    "devargs parsing failed ret=%d\n", ret);
+			return;
+		}
+
+		if (!strcmp(d->bus->name, "pci")) {
+			/* Start the process of figuring out if this
+			 * PCI device is a VF device
+			 */
+			hv->eal_hot_plug_retry = 0;
+			rte_eal_alarm_set(1000000, netvsc_hotplug_retry, hv);
+		}
+
+		/* We will switch to VF on RDNIS configure message
+		 * sent from VSP
+		 */
+
+		break;
+	default:
+		break;
+	}
+}
+
 static int hn_dev_configure(struct rte_eth_dev *dev)
 {
 	struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
@@ -617,7 +747,7 @@ static int hn_dev_configure(struct rte_eth_dev *dev)
 		}
 	}
 
-	return hn_vf_configure(dev, dev_conf);
+	return hn_vf_configure_locked(dev, dev_conf);
 }
 
 static int hn_dev_stats_get(struct rte_eth_dev *dev,
@@ -827,6 +957,14 @@ hn_dev_start(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* Register to monitor hot plug events */
+	error = rte_dev_event_callback_register(NULL, netvsc_hotadd_callback,
+						hv);
+	if (error) {
+		PMD_DRV_LOG(ERR, "failed to register device event callback\n");
+		return error;
+	}
+
 	error = hn_rndis_set_rxfilter(hv,
 				      NDIS_PACKET_TYPE_BROADCAST |
 				      NDIS_PACKET_TYPE_ALL_MULTICAST |
@@ -853,6 +991,7 @@ hn_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 	dev->data->dev_started = 0;
 
+	rte_dev_event_callback_unregister(NULL, netvsc_hotadd_callback, hv);
 	hn_rndis_set_rxfilter(hv, 0);
 	return hn_vf_stop(dev);
 }
@@ -861,11 +1000,14 @@ static int
 hn_dev_close(struct rte_eth_dev *dev)
 {
 	int ret;
+	struct hn_data *hv = dev->data->dev_private;
 
 	PMD_INIT_FUNC_TRACE();
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	rte_eal_alarm_cancel(netvsc_hotplug_retry, &hv->devargs);
+
 	ret = hn_vf_close(dev);
 	hn_dev_free_queues(dev);
 
@@ -990,7 +1132,10 @@ eth_hn_dev_init(struct rte_eth_dev *eth_dev)
 	hv->max_queues = 1;
 
 	rte_rwlock_init(&hv->vf_lock);
-	hv->vf_port = HN_INVALID_PORT;
+	hv->vf_ctx.vf_vsc_switched = false;
+	hv->vf_ctx.vf_vsp_reported = false;
+	hv->vf_ctx.vf_attached = false;
+	hv->vf_ctx.vf_state = vf_unknown;
 
 	err = hn_parse_args(eth_dev);
 	if (err)
@@ -1044,12 +1189,10 @@ eth_hn_dev_init(struct rte_eth_dev *eth_dev)
 	hv->max_queues = RTE_MIN(rxr_cnt, (unsigned int)max_chan);
 
 	/* If VF was reported but not added, do it now */
-	if (hv->vf_present && !hn_vf_attached(hv)) {
+	if (hv->vf_ctx.vf_vsp_reported && !hv->vf_ctx.vf_vsc_switched) {
 		PMD_INIT_LOG(DEBUG, "Adding VF device");
 
 		err = hn_vf_add(eth_dev, hv);
-		if (err)
-			hv->vf_present = 0;
 	}
 
 	return 0;
@@ -1095,15 +1238,23 @@ static int eth_hn_probe(struct rte_vmbus_driver *drv __rte_unused,
 
 	PMD_INIT_FUNC_TRACE();
 
+	ret = rte_dev_event_monitor_start();
+	if (ret) {
+		PMD_DRV_LOG(ERR, "Failed to start device event monitoring\n");
+		return ret;
+	}
+
 	eth_dev = eth_dev_vmbus_allocate(dev, sizeof(struct hn_data));
 	if (!eth_dev)
 		return -ENOMEM;
 
 	ret = eth_hn_dev_init(eth_dev);
-	if (ret)
+	if (ret) {
 		eth_dev_vmbus_release(eth_dev);
-	else
+		rte_dev_event_monitor_stop();
+	} else {
 		rte_eth_dev_probing_finish(eth_dev);
+	}
 
 	return ret;
 }
@@ -1124,6 +1275,7 @@ static int eth_hn_remove(struct rte_vmbus_device *dev)
 		return ret;
 
 	eth_dev_vmbus_release(eth_dev);
+	rte_dev_event_monitor_stop();
 	return 0;
 }
 
drivers/net/netvsc/hn_nvs.c +3 -1
index eeb82ab9ee..a0ee7d8bfa 100644
@@ -569,7 +569,7 @@ hn_nvs_alloc_subchans(struct hn_data *hv, uint32_t *nsubch)
 	return 0;
 }
 
-void
+int
 hn_nvs_set_datapath(struct hn_data *hv, uint32_t path)
 {
 	struct hn_nvs_datapath dp;
@@ -588,4 +588,6 @@ hn_nvs_set_datapath(struct hn_data *hv, uint32_t path)
 			    "send set datapath failed: %d",
 			    error);
 	}
+
+	return error;
 }
drivers/net/netvsc/hn_nvs.h +1 -1
index 015839e364..3766d2ee34 100644
@@ -212,7 +212,7 @@ int	hn_nvs_attach(struct hn_data *hv, unsigned int mtu);
 void	hn_nvs_detach(struct hn_data *hv);
 void	hn_nvs_ack_rxbuf(struct vmbus_channel *chan, uint64_t tid);
 int	hn_nvs_alloc_subchans(struct hn_data *hv, uint32_t *nsubch);
-void	hn_nvs_set_datapath(struct hn_data *hv, uint32_t path);
+int	hn_nvs_set_datapath(struct hn_data *hv, uint32_t path);
 void	hn_nvs_handle_vfassoc(struct rte_eth_dev *dev,
 			      const struct vmbus_chanpkt_hdr *hdr,
 			      const void *data);
drivers/net/netvsc/hn_rxtx.c +22 -14
index 015662fdb4..0f4ef0100b 100644
@@ -1492,16 +1492,20 @@ hn_xmit_pkts(void *ptxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		hn_process_events(hv, txq->queue_id, 0);
 
 	/* Transmit over VF if present and up */
-	rte_rwlock_read_lock(&hv->vf_lock);
-	vf_dev = hn_get_vf_dev(hv);
-	if (vf_dev && vf_dev->data->dev_started) {
-		void *sub_q = vf_dev->data->tx_queues[queue_id];
-
-		nb_tx = (*vf_dev->tx_pkt_burst)(sub_q, tx_pkts, nb_pkts);
+	if (hv->vf_ctx.vf_vsc_switched) {
+		rte_rwlock_read_lock(&hv->vf_lock);
+		vf_dev = hn_get_vf_dev(hv);
+		if (hv->vf_ctx.vf_vsc_switched && vf_dev &&
+		    vf_dev->data->dev_started) {
+			void *sub_q = vf_dev->data->tx_queues[queue_id];
+
+			nb_tx = (*vf_dev->tx_pkt_burst)
+					(sub_q, tx_pkts, nb_pkts);
+			rte_rwlock_read_unlock(&hv->vf_lock);
+			return nb_tx;
+		}
 		rte_rwlock_read_unlock(&hv->vf_lock);
-		return nb_tx;
 	}
-	rte_rwlock_read_unlock(&hv->vf_lock);
 
 	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
 		struct rte_mbuf *m = tx_pkts[nb_tx];
@@ -1614,13 +1618,17 @@ hn_recv_pkts(void *prxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					   (void **)rx_pkts, nb_pkts, NULL);
 
 	/* If VF is available, check that as well */
-	rte_rwlock_read_lock(&hv->vf_lock);
-	vf_dev = hn_get_vf_dev(hv);
-	if (vf_dev && vf_dev->data->dev_started)
-		nb_rcv += hn_recv_vf(vf_dev->data->port_id, rxq,
-				     rx_pkts + nb_rcv, nb_pkts - nb_rcv);
+	if (hv->vf_ctx.vf_vsc_switched) {
+		rte_rwlock_read_lock(&hv->vf_lock);
+		vf_dev = hn_get_vf_dev(hv);
+		if (hv->vf_ctx.vf_vsc_switched && vf_dev &&
+		    vf_dev->data->dev_started)
+			nb_rcv += hn_recv_vf(vf_dev->data->port_id, rxq,
+					     rx_pkts + nb_rcv,
+					     nb_pkts - nb_rcv);
 
-	rte_rwlock_read_unlock(&hv->vf_lock);
+		rte_rwlock_read_unlock(&hv->vf_lock);
+	}
 	return nb_rcv;
 }
 
drivers/net/netvsc/hn_var.h +36 -16

91 lines not on this page · raw message

drivers/net/netvsc/hn_vf.c +268 -50

405 lines not on this page · raw message

Signature · 1 line
2.25.1
#3 lore raw

Re: [dpdk-dev] [PATCH v2 2/2] net/netvsc: support VF device hot add/remove

Quoted 9 lines · On Mon, 21 Dec 2020 13:33:22 -0800 Long Li <[email protected]> wrote:
	/* trying to get mac address if this is a network device*/
drivers/net/netvsc/hn_ethdev.c +8 -0
+		s = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
+		if (s == -1) {
+			PMD_DRV_LOG(ERR, "Failed to create socket errno %d\n",
+				    errno);
+			break;
+		}
+		strlcpy(req.ifr_name, dir->d_name, sizeof(req.ifr_name));
+		ret = ioctl(s, SIOCGIFHWADDR, &req);

drivers/net/netvsc/hn_ethdev.c static int hn_subchan_configure(struct hn_data *hv, · line 604

Quoted 10 lines
drivers/net/netvsc/hn_ethdev.c +10 -0
+		close(s);
+		if (ret == -1) {
+			PMD_DRV_LOG(ERR, "Failed to send SIOCGIFHWADDR for "
+				    "device %s\n", dir->d_name);
+			break;
+		}
+		if (req.ifr_hwaddr.sa_family != ARPHRD_ETHER) {
+			closedir(di);
+			return;
+		}

You have sysfs directory open at this point, would it be easier to continue with sysfs to find the mac address?

		snprintf(ifpath, sizeof(ifpath), "%s/address", dir->d_name);
		fd = openat(dirfd(di), ifpath, O_RDONLY);
		read(fd...)
#4 lore raw

Re: [dpdk-dev] [PATCH v2 2/2] net/netvsc: support VF device hot add/remove

Quoted 31 lines

Subject: Re: [dpdk-dev] [PATCH v2 2/2] net/netvsc: support VF device hot add/remove

drivers/net/netvsc/hn_ethdev.c static int hn_subchan_configure(struct hn_data *hv,

Quoted 4 lines · On Mon, 21 Dec 2020 13:33:22 -0800 Long Li <[email protected]> wrote:
	/* trying to get mac address if this is a network device*/
drivers/net/netvsc/hn_ethdev.c +3 -0
+		s = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
+		if (s == -1) {
+			PMD_DRV_LOG(ERR, "Failed to create socket

errno %d\n",

drivers/net/netvsc/hn_ethdev.c static int hn_subchan_configure(struct hn_data *hv,

Quoted 8 lines
drivers/net/netvsc/hn_ethdev.c +8 -0
+				    errno);
+			break;
+		}
+		strlcpy(req.ifr_name, dir->d_name, sizeof(req.ifr_name));
+		ret = ioctl(s, SIOCGIFHWADDR, &req);
+		close(s);
+		if (ret == -1) {
+			PMD_DRV_LOG(ERR, "Failed to send

SIOCGIFHWADDR for "

drivers/net/netvsc/hn_ethdev.c static int hn_subchan_configure(struct hn_data *hv, · line 604

Quoted 7 lines
drivers/net/netvsc/hn_ethdev.c +7 -0
+				    "device %s\n", dir->d_name);
+			break;
+		}
+		if (req.ifr_hwaddr.sa_family != ARPHRD_ETHER) {
+			closedir(di);
+			return;
+		}

You have sysfs directory open at this point, would it be easier to continue with sysfs to find the mac address?

Quoted 3 lines
		snprintf(ifpath, sizeof(ifpath), "%s/address", dir->d_name);
		fd = openat(dirfd(di), ifpath, O_RDONLY);
		read(fd...)

I think sending SIOCGIFHWADDR through socket is safer. This is the same as vdev_netvsc does. If the network hardware is not ethernet, reading the address directly from sysfs may require some additional parsing.

#5 lore raw
In reply to Long Li
Quoted 31 lines · Long Li

From: Stephen Hemminger <[email protected]>

In some cases, a device or infrastructure may want to enable hotplug but application may also try and start hotplug as well. Therefore change the monitor_started from a boolean into a reference count.

---
 lib/librte_eal/linux/eal_dev.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
lib/librte_eal/linux/eal_dev.c +5 -2
index 5c0e752b2d..5fa679989e 100644
@@ -23,8 +23,11 @@
 
 #include "eal_private.h"
 
-static struct rte_intr_handle intr_handle = {.fd = -1 };
-static bool monitor_started;
+static struct rte_intr_handle intr_handle = {
+	.type = RTE_INTR_HANDLE_DEV_EVENT,
+	.fd = -1,
+};
+static uint32_t monitor_refcount;
 static bool hotplug_handle;
 
 #define EAL_UEV_MSG_LEN 4096

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_start(void) · line 307

Quoted 6 lines
lib/librte_eal/linux/eal_dev.c +1 -1
@@ -300,7 +303,7 @@ rte_dev_event_monitor_start(void)
 {
 	int ret;
 
-	if (monitor_started)
+	if (__atomic_fetch_add(&monitor_refcount, 1, __ATOMIC_RELAXED))
 		return 0;

If dev_uev_socket_fd_create or rte_intr_callback_register fail, you'll have incremented the refcount but there won't be anything registered, and calls to rte_dev_event_monitor_start will do nothing. Will that be a problem?

In other words, it seems to me the semantics is changing from call rte_dev_event_monitor_start until it succeeds, to call rte_dev_event_monitor_start, and if it fails call rte_dev_event_monitor_stop and then rte_dev_event_monitor_start again

Quoted 19 lines
lib/librte_eal/linux/eal_dev.c +0 -3
 	ret = dev_uev_socket_fd_create();
@@ -309,7 +312,6 @@ rte_dev_event_monitor_start(void)
 		return -1;
 	}
 
-	intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT;
 	ret = rte_intr_callback_register(&intr_handle, dev_uev_handler, NULL);
 
 	if (ret) {
@@ -317,8 +319,6 @@ rte_dev_event_monitor_start(void)
 		return -1;
 	}
 
-	monitor_started = true;
-
 	return 0;
 }
 

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_stop(void) · line 331

Quoted 6 lines
lib/librte_eal/linux/eal_dev.c +1 -1
@@ -327,7 +327,7 @@ rte_dev_event_monitor_stop(void)
 {
 	int ret;
 
-	if (!monitor_started)
+	if (__atomic_sub_fetch(&monitor_refcount, 1, __ATOMIC_RELAXED))
 		return 0;

Same question in reverse - if rte_intr_callback_unregister fails, the refcount will have been decreased anyway, so nothing will close the file handle, right?

Quoted 9 lines
lib/librte_eal/linux/eal_dev.c +0 -1
 	ret = rte_intr_callback_unregister(&intr_handle, dev_uev_handler,
@@ -339,7 +339,6 @@ rte_dev_event_monitor_stop(void)
 
 	close(intr_handle.fd);
 	intr_handle.fd = -1;
-	monitor_started = false;
 
 	return 0;
 }
#6 lore raw
Quoted 73 lines

Subject: Re: [dpdk-dev] [PATCH 1/2] eal/hotplug: allow monitor to be setup by multiple places

Quoted 31 lines · Long Li

From: Stephen Hemminger <[email protected]>

In some cases, a device or infrastructure may want to enable hotplug but application may also try and start hotplug as well. Therefore change the monitor_started from a boolean into a reference count.

---
 lib/librte_eal/linux/eal_dev.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
(quoted diff) +0 -0

b/lib/librte_eal/linux/eal_dev.c index 5c0e752b2d..5fa679989e 100644

lib/librte_eal/linux/eal_dev.c +5 -1
@@ -23,8 +23,11 @@
 
 #include "eal_private.h"
 
-static struct rte_intr_handle intr_handle = {.fd = -1 }; -static bool
 monitor_started;
+static struct rte_intr_handle intr_handle = {
+	.type = RTE_INTR_HANDLE_DEV_EVENT,
+	.fd = -1,
+};
+static uint32_t monitor_refcount;
 static bool hotplug_handle;
 
 #define EAL_UEV_MSG_LEN 4096

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_start(void) { · line 305

Quoted 4 lines
lib/librte_eal/linux/eal_dev.c +1 -1
@@ -300,7 +303,7 @@ rte_dev_event_monitor_start(void)  {
 	int ret;
 
-	if (monitor_started)
+	if (__atomic_fetch_add(&monitor_refcount, 1,

__ATOMIC_RELAXED))

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_start(void) { · line 306

Quoted 1 line
lib/librte_eal/linux/eal_dev.c +0 -0
 		return 0;

If dev_uev_socket_fd_create or rte_intr_callback_register fail, you'll have incremented the refcount but there won't be anything registered, and calls to rte_dev_event_monitor_start will do nothing. Will that be a problem?

In other words, it seems to me the semantics is changing from call rte_dev_event_monitor_start until it succeeds, to call rte_dev_event_monitor_start, and if it fails call rte_dev_event_monitor_stop and then rte_dev_event_monitor_start again

Quoted 22 lines
lib/librte_eal/linux/eal_dev.c +0 -1
 	ret = dev_uev_socket_fd_create();
@@ -309,7 +312,6 @@ rte_dev_event_monitor_start(void)
 		return -1;
 	}
 
-	intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT;
 	ret = rte_intr_callback_register(&intr_handle, dev_uev_handler,
 NULL);
 
lib/librte_eal/linux/eal_dev.c +0 -2
 	if (ret) {
@@ -317,8 +319,6 @@ rte_dev_event_monitor_start(void)
 		return -1;
 	}
 
-	monitor_started = true;
-
 	return 0;
 }
 
@@ -327,7 +327,7 @@ rte_dev_event_monitor_stop(void)  {
 	int ret;
 
Quoted 7 lines

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_stop(void) { · line 329

Quoted 2 lines
lib/librte_eal/linux/eal_dev.c +1 -1
-	if (!monitor_started)
+	if (__atomic_sub_fetch(&monitor_refcount, 1,

__ATOMIC_RELAXED))

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_stop(void) { · line 330

Quoted 1 line
lib/librte_eal/linux/eal_dev.c +0 -0
 		return 0;

Same question in reverse - if rte_intr_callback_unregister fails, the refcount will have been decreased anyway, so nothing will close the file handle, right?

I'm sending v2 to address the comments.

Quoted 11 lines

lib/librte_eal/linux/eal_dev.c rte_dev_event_monitor_stop(void) · line 333

Quoted 2 lines
lib/librte_eal/linux/eal_dev.c +0 -0
 	ret = rte_intr_callback_unregister(&intr_handle, dev_uev_handler,

@@

Quoted 8 lines
lib/librte_eal/linux/eal_dev.c +0 -2
-339,7 +339,6 @@ rte_dev_event_monitor_stop(void)
 
 	close(intr_handle.fd);
 	intr_handle.fd = -1;
-	monitor_started = false;
 
 	return 0;
 }
author
#7 lore raw

PATCH v2 1/2 eal/hotplug: allow monitor to be setup by multiple places

In reply to Long Li

From: Long Li <[email protected]>

In some cases, a device or infrastructure may want to enable hotplug but application may also try and start hotplug as well. Therefore change the monitor_started from a boolean into a reference count.

---
 lib/librte_eal/linux/eal_dev.c | 56 ++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 16 deletions(-)
lib/librte_eal/linux/eal_dev.c +40 -16
index 5c0e752b2d..3b905e18f5 100644
@@ -23,8 +23,12 @@
 
 #include "eal_private.h"
 
-static struct rte_intr_handle intr_handle = {.fd = -1 };
-static bool monitor_started;
+static struct rte_intr_handle intr_handle = {
+	.type = RTE_INTR_HANDLE_DEV_EVENT,
+	.fd = -1,
+};
+static rte_rwlock_t monitor_lock = RTE_RWLOCK_INITIALIZER;
+static uint32_t monitor_refcount;
 static bool hotplug_handle;
 
 #define EAL_UEV_MSG_LEN 4096
@@ -298,50 +302,70 @@ dev_uev_handler(__rte_unused void *param)
 int
 rte_dev_event_monitor_start(void)
 {
-	int ret;
+	int ret = 0;
 
-	if (monitor_started)
-		return 0;
+	rte_rwlock_write_lock(&monitor_lock);
+
+	if (monitor_refcount) {
+		monitor_refcount++;
+		goto exit;
+	}
 
 	ret = dev_uev_socket_fd_create();
 	if (ret) {
 		RTE_LOG(ERR, EAL, "error create device event fd.\n");
-		return -1;
+		goto exit;
 	}
 
-	intr_handle.type = RTE_INTR_HANDLE_DEV_EVENT;
 	ret = rte_intr_callback_register(&intr_handle, dev_uev_handler, NULL);
 
 	if (ret) {
 		RTE_LOG(ERR, EAL, "fail to register uevent callback.\n");
-		return -1;
+		close(intr_handle.fd);
+		intr_handle.fd = -1;
+		goto exit;
 	}
 
-	monitor_started = true;
+	monitor_refcount++;
 
-	return 0;
+exit:
+	rte_rwlock_write_unlock(&monitor_lock);
+	return ret;
 }
 
 int
 rte_dev_event_monitor_stop(void)
 {
-	int ret;
+	int ret = 0;
 
-	if (!monitor_started)
-		return 0;
+	rte_rwlock_write_lock(&monitor_lock);
+
+	if (!monitor_refcount) {
+		RTE_LOG(ERR, EAL, "device event monitor already stopped\n");
+		goto exit;
+	}
+
+	if (monitor_refcount > 1) {
+		monitor_refcount--;
+		goto exit;
+	}
 
 	ret = rte_intr_callback_unregister(&intr_handle, dev_uev_handler,
 					   (void *)-1);
 	if (ret < 0) {
 		RTE_LOG(ERR, EAL, "fail to unregister uevent callback.\n");
-		return ret;
+		goto exit;
 	}
 
 	close(intr_handle.fd);
 	intr_handle.fd = -1;
-	monitor_started = false;
 
-	return 0;
+	monitor_refcount--;
+
+exit:
+	rte_rwlock_write_unlock(&monitor_lock);
+
+	return ret;
 }
 
 int
Signature · 1 line
2.25.1
#8 lore raw

21/12/2020 22:32, Long Li:

Quoted 7 lines

From: Long Li <[email protected]>

In some cases, a device or infrastructure may want to enable hotplug but application may also try and start hotplug as well. Therefore change the monitor_started from a boolean into a reference count.

Series applied, thanks.