On 1/21/21 7:15 AM, Moshe Levi wrote:
From: Dmytro Linkin <dlinkin@xxxxxxxxxx>
Current virPCIGetNetName() logic is to get net device name by checking
it's phys_port_id, if caller provide it, or by it's index (eg, by it's
position at sysfs net directory). This approach worked fine up until
linux kernel version 5.8, where NVIDIA Mellanox driver implemented
linking of VFs' representors to PF PCI address [1]
This mean
that device's sysfs net directory will hold multiple net devices. Ex.:
$ ls '/sys/bus/pci/devices/0000:82:00.0/net'
ens1f0 eth0 eth1
In switchdev mode the PF and the VF represntors support phys_port_name
In that case there is only a single netdev on the PCI device which is
the PF. The other net devices are the VF representors which we want
to exclude. whereas in the case of using phys_port_id, there could be
multiple PFs, and so we have to match the phys_port_id of the VF
virPCIGetNetName() will try to get PF name by it's index - 0. The
problem here is that the PF nedev entry may not be the first.
To fix that, for switch devices, we introduce a new logic to select the
PF uplink netdev according to the content of phys_port_name. Extend
virPCIGetNetName() logic work in following sequence:
- filter by phys_port_id, if it's provided, (multi PF)
or
- filter by phys_port_name if exist (one PF)
- get net device by it's index (position) in sysfs net directory.
[1] - https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
commit/?id=123f0f53dd64b67e34142485fe866a8a581f12f1
Co-Authored-by: Moshe Levi <moshele@xxxxxxxxxx>
Signed-off-by: Dmytro Linkin <dlinkin@xxxxxxxxxx>
Reviewed-by: Adrian Chiris <adrianc@xxxxxxxxxx>
---
src/util/virnetdev.c | 7 +++----
src/util/virpci.c | 38 +++++++++++++++++++++++++++++++++++++-
src/util/virpci.h | 5 +++++
3 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index d41b967d6a..2485718b48 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1263,7 +1263,7 @@ virNetDevGetVirtualFunctions(const char *pfname,
}
if (virPCIGetNetName(pci_sysfs_device_link, 0,
- pfPhysPortID, NULL, &((*vfname)[i])) < 0) {
+ pfPhysPortID, &((*vfname)[i])) < 0) {
This change disappears once the incorrect chunks are removed from patch 1.
goto cleanup;
}
@@ -1358,8 +1358,7 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
return -1;
if (virPCIGetNetName(physfn_sysfs_path, 0,
- vfPhysPortID,
- VIR_PF_PHYS_PORT_NAME_REGEX, pfname) < 0) {
+ vfPhysPortID, pfname) < 0) {
Likewise.
return -1;
}
@@ -1422,7 +1421,7 @@ virNetDevPFGetVF(const char *pfname, int vf, char **vfname)
* isn't bound to a netdev driver, it won't have a netdev name,
* and vfname will be NULL).
*/
- return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, NULL, vfname);
+ return virPCIGetNetName(virtfnSysfsPath, 0, pfPhysPortID, vfname);
Likewise
}
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 50fd5ef7ea..e20fb9e10b 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2469,7 +2469,7 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr,
* @idx: used to choose which netdev when there are several
(side note: I was spelunking the code trying to figure out when this idx
is actually set and used, and it seems like the answer might be "never".
I should probably look into cleaning it up)
* (ignored if physPortID is set)
* @physPortID: match this string in the netdev's phys_port_id
- * (or NULL to ignore and use idx instead)
+ * (or NULL to ignore and use phys_port_name or idx instead)
* @netname: used to return the name of the netdev
* (set to NULL (but returns success) if there is no netdev)
*
@@ -2483,6 +2483,7 @@ virPCIGetNetName(const char *device_link_sysfs_path,
{
g_autofree char *pcidev_sysfs_net_path = NULL;
g_autofree char *firstEntryName = NULL;
+ g_autofree char *thisPhysPortName = NULL;
g_autoptr(DIR) dir = NULL;
struct dirent *entry = NULL;
size_t i = 0;
@@ -2522,7 +2523,42 @@ virPCIGetNetName(const char *device_link_sysfs_path,
continue;
}
+
} else {
+ /* Most switch devices use phys_port_name instead of
+ * phys_port_id.
+ * NOTE: VFs' representors net devices can be linked to PF's PCI
+ * device, which mean that there'll be multiple net devices
+ * instances and to get a proper net device need to match on
+ * specific regex.
+ * To get PF netdev, for ex., used following regex:
+ * "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
+ * or to get exact VF's netdev next regex is used:
+ * "pf0vf1$"
+ */
+ if (virNetDevGetPhysPortName(entry->d_name, &thisPhysPortName) < 0)
+ return -1;
+
+ if (thisPhysPortName) {
+ /* if this one doesn't match, keep looking */
+ if (!virStringMatch(thisPhysPortName, VIR_PF_PHYS_PORT_NAME_REGEX)) {
+ VIR_FREE(thisPhysPortName);
When I started using g_autofree and explicitly called VIR_FREE() on a
g_autofree pointer so I could re-use it, I was told this is a bad
practice and to avoid it. We can easily avoid it in this case by
declaring thisPhysPortName inside the else clause where it is used (just
before the line above us ^^^ that calls virNetDevGetPhysPortName()) -
that way it will be autofreed each time through the loop.
+ /* Save the first entry we find to use as a failsafe
+ * in case we fail to match on regex.
+ */
+ if (!firstEntryName)
+ firstEntryName = g_strdup(entry->d_name);
All this stuff about setting firstEntryName is pointless in the else
clause of "if (physPortID)": we will either get to the bottom of the
while() loop and return without using firstEntryName, or we will
continue back to the top of the loop until we run out of entries, drop
out of the loop, and get to:
if (!physPortID)
return 0;
Since we've already established that physPortID == NULL, that's going to
take us out of this function before we can use firstEntryName.
+
+ continue;
+ }
+ } else {
+ /* Save the first entry we find to use as a failsafe in case
+ * phys_port_name is not supported.
+ */
+ if (!firstEntryName)
+ firstEntryName = g_strdup(entry->d_name);
+ continue;
The else clause here is what will be taken for any sort of hardware that
has only a single netdev listed in the "net" directory of the PCI device
(so pretty much everything except a new generation Mellanox card,
right?). I would have thought that in this case we should treat it as
below (just increment i and continue)
(BTW, if one of the netdevs in a PCI device's "net" directory has a
phys_port_name, is it *always* the case that all the other netdevs
listed in that directory also have a phys_port_name? (ie if the PF has a
phys_port_name, will the VF representors always have a phys_port_name,
and vice versa?) I'm assuming the answer is "yes".
+ }
if (i++ < idx)
continue;
After thinking about this a bit, I've come up with the attached diff
(based on your patches - you can just add it to your local directory
with "git am -3 0001-blah.patch"). Does that look right to you? If it
does (and it works!) then I can squash it in and push. If you're not
quite happy with it, then you can send another iteration. I promise I'll
be much faster about responding now!! (be sure to Cc me so I notice it
though, and maybe even ping me in IRC.)
}
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 43828b0a8a..9e89ede1d5 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -55,6 +55,11 @@ struct _virZPCIDeviceAddress {
#define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
+/* Represents format of PF's phys_port_name in switchdev mode:
+ * 'p%u' or 'p%us%u'. New line checked since value is readed from sysfs file.
+ */
+# define VIR_PF_PHYS_PORT_NAME_REGEX "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
There should be no space between # and define here. I'm trying to
remember if it's the cppi package that finds that, or something else.
(that's also in my diff)
+
struct _virPCIDeviceAddress {
unsigned int domain;
unsigned int bus;
>From 66d10cb7508199a9a6ca3eef05a5375c8456967a Mon Sep 17 00:00:00 2001
From: Laine Stump <laine@xxxxxxxxxx>
Date: Mon, 25 Jan 2021 00:13:37 -0500
Subject: [PATCH] additional changes for phys_port_name patch
Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
src/util/virpci.c | 63 +++++++++++++++++++----------------------------
src/util/virpci.h | 2 +-
2 files changed, 26 insertions(+), 39 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index e20fb9e10b..c685927b60 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2467,7 +2467,7 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr,
* virPCIGetNetName:
* @device_link_sysfs_path: sysfs path to the PCI device
* @idx: used to choose which netdev when there are several
- * (ignored if physPortID is set)
+ * (ignored if physPortID is set or physPortName is available)
* @physPortID: match this string in the netdev's phys_port_id
* (or NULL to ignore and use phys_port_name or idx instead)
* @netname: used to return the name of the netdev
@@ -2483,7 +2483,6 @@ virPCIGetNetName(const char *device_link_sysfs_path,
{
g_autofree char *pcidev_sysfs_net_path = NULL;
g_autofree char *firstEntryName = NULL;
- g_autofree char *thisPhysPortName = NULL;
g_autoptr(DIR) dir = NULL;
struct dirent *entry = NULL;
size_t i = 0;
@@ -2502,6 +2501,14 @@ virPCIGetNetName(const char *device_link_sysfs_path,
}
while (virDirRead(dir, &entry, pcidev_sysfs_net_path) > 0) {
+ /* save the first entry we find to use as a failsafe
+ * in case we don't match the phys_port_id. This is
+ * needed because some NIC drivers (e.g. i40e)
+ * implement phys_port_id for PFs, but not for VFs
+ */
+ if (!firstEntryName)
+ firstEntryName = g_strdup(entry->d_name);
+
/* if the caller sent a physPortID, compare it to the
* physportID of this netdev. If not, look for entry[idx].
*/
@@ -2512,17 +2519,8 @@ virPCIGetNetName(const char *device_link_sysfs_path,
return -1;
/* if this one doesn't match, keep looking */
- if (STRNEQ_NULLABLE(physPortID, thisPhysPortID)) {
- /* save the first entry we find to use as a failsafe
- * in case we don't match the phys_port_id. This is
- * needed because some NIC drivers (e.g. i40e)
- * implement phys_port_id for PFs, but not for VFs
- */
- if (!firstEntryName)
- firstEntryName = g_strdup(entry->d_name);
-
+ if (STRNEQ_NULLABLE(physPortID, thisPhysPortID))
continue;
- }
} else {
/* Most switch devices use phys_port_name instead of
@@ -2536,44 +2534,34 @@ virPCIGetNetName(const char *device_link_sysfs_path,
* or to get exact VF's netdev next regex is used:
* "pf0vf1$"
*/
+ g_autofree char *thisPhysPortName = NULL;
+
if (virNetDevGetPhysPortName(entry->d_name, &thisPhysPortName) < 0)
return -1;
if (thisPhysPortName) {
- /* if this one doesn't match, keep looking */
- if (!virStringMatch(thisPhysPortName, VIR_PF_PHYS_PORT_NAME_REGEX)) {
- VIR_FREE(thisPhysPortName);
- /* Save the first entry we find to use as a failsafe
- * in case we fail to match on regex.
- */
- if (!firstEntryName)
- firstEntryName = g_strdup(entry->d_name);
+ /* if this one doesn't match, keep looking */
+ if (!virStringMatch(thisPhysPortName, VIR_PF_PHYS_PORT_NAME_REGEX))
continue;
- }
+
} else {
- /* Save the first entry we find to use as a failsafe in case
- * phys_port_name is not supported.
- */
- if (!firstEntryName)
- firstEntryName = g_strdup(entry->d_name);
- continue;
+
+ if (i++ < idx)
+ continue;
}
- if (i++ < idx)
- continue;
}
*netname = g_strdup(entry->d_name);
return 0;
}
- if (!physPortID)
- return 0;
-
if (firstEntryName) {
- /* we didn't match the provided phys_port_id, but this
- * is probably because phys_port_id isn't implemented
- * for this NIC driver, so just return the first
+ /* we didn't match the provided phys_port_id / find a
+ * phys_port_name matching VIR_PF_PHYS_PORT_NAME_REGEX / find
+ * as many net devices as the value of idx, but this is
+ * probably because phys_port_id / phys_port_name isn't
+ * implemented for this NIC driver, so just return the first
* (probably only) netname we found.
*/
*netname = g_steal_pointer(&firstEntryName);
@@ -2581,9 +2569,8 @@ virPCIGetNetName(const char *device_link_sysfs_path,
}
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Could not find network device with "
- "phys_port_id '%s' under PCI device at %s"),
- physPortID, device_link_sysfs_path);
+ _("Could not find any network device under PCI device at %s"),
+ device_link_sysfs_path);
return -1;
}
diff --git a/src/util/virpci.h b/src/util/virpci.h
index 9e89ede1d5..1a4bf1d751 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -58,7 +58,7 @@ struct _virZPCIDeviceAddress {
/* Represents format of PF's phys_port_name in switchdev mode:
* 'p%u' or 'p%us%u'. New line checked since value is readed from sysfs file.
*/
-# define VIR_PF_PHYS_PORT_NAME_REGEX "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
+#define VIR_PF_PHYS_PORT_NAME_REGEX "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
struct _virPCIDeviceAddress {
unsigned int domain;
--
2.29.2
