RE: [PATCH v2 2/2] util: Add phys_port_name support on virPCIGetNetName

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laine, 
Sorry for the mistake in the split of the commit, and thank for fix it 😊. 
I review you change and it is better. 
I tested it on Mellanox card with kernel 5.9 and it work good with SR-IOV legacy and SR-IOV switchdev.
You have my LGTM for it. 



> -----Original Message-----
> From: Laine Stump <laine@xxxxxxxxxx>
> Sent: Monday, January 25, 2021 7:23 AM
> To: libvir-list@xxxxxxxxxx
> Cc: Moshe Levi <moshele@xxxxxxxxxx>; Adrian Chiris <adrianc@xxxxxxxxxx>
> Subject: Re: [PATCH v2 2/2] util: Add phys_port_name support on
> virPCIGetNetName
> 
> External email: Use caution opening links or attachments
> 
> 
> 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;
> 





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux