netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH mlx5-next v4 0/4] Dynamically assign MSI-X vectors count
@ 2021-01-24 13:11 Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-24 13:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Saeed Mahameed
  Cc: Leon Romanovsky, Jason Gunthorpe, Alexander Duyck,
	Jakub Kicinski, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

From: Leon Romanovsky <leonro@nvidia.com>

Changelog
v4:
 * Used sysfs_emit() instead of sprintf() in new sysfs entries.
 * Changed EXPORT_SYMBOL to be EXPORT_SYMBOL_GPL for pci_iov_virtfn_devfn().
 * Rewrote sysfs registration code to be driven by PF that wants to enable VF
   overlay instead of creating to all SR-IOV devices.
 * Grouped all such functionality under new "vfs_overlay" folder.
 * Combined two PCI patches into one.
v3: https://lore.kernel.org/linux-pci/20210117081548.1278992-1-leon@kernel.org
 * Renamed pci_set_msix_vec_count to be pci_vf_set_msix_vec_count.
 * Added VF msix_cap check to hide sysfs entry if device doesn't support msix.
 * Changed "-" to be ":" in the mlx5 patch to silence CI warnings about missing
   kdoc description.
 * Split differently error print in mlx5 driver to avoid checkpatch warning.
v2: https://lore.kernel.org/linux-pci/20210114103140.866141-1-leon@kernel.org
 * Patch 1:
  * Renamed vf_msix_vec sysfs knob to be sriov_vf_msix_count
  * Added PF and VF device locks during set MSI-X call to protect from parallel
    driver bind/unbind operations.
  * Removed extra checks when reading sriov_vf_msix, because users will
    be able to distinguish between supported/not supported by looking on
    sriov_vf_total_msix count.
  * Changed all occurrences of "numb" to be "count"
  * Changed returned error from EOPNOTSUPP to be EBUSY if user tries to set
    MSI-X count after driver already bound to the VF.
  * Added extra comment in pci_set_msix_vec_count() to emphasize that driver
    should not be bound.
 * Patch 2:
  * Changed vf_total_msix from int to be u32 and updated function signatures
    accordingly.
  * Improved patch title
v1: https://lore.kernel.org/linux-pci/20210110150727.1965295-1-leon@kernel.org
 * Improved wording and commit messages of first PCI patch
 * Added extra PCI patch to provide total number of MSI-X vectors
 * Prohibited read of vf_msix_vec sysfs file if driver doesn't support write
 * Removed extra function definition in pci.h
v0: https://lore.kernel.org/linux-pci/20210103082440.34994-1-leon@kernel.org

--------------------------------------------------------------------
Hi,

The number of MSI-X vectors is PCI property visible through lspci, that
field is read-only and configured by the device.

The static assignment of an amount of MSI-X vectors doesn't allow utilize
the newly created VF because it is not known to the device the future load
and configuration where that VF will be used.

The VFs are created on the hypervisor and forwarded to the VMs that have
different properties (for example number of CPUs).

To overcome the inefficiency in the spread of such MSI-X vectors, we
allow the kernel to instruct the device with the needed number of such
vectors, before VF is initialized and bounded to the driver.

Before this series:
[root@server ~]# lspci -vs 0000:08:00.2
08:00.2 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
....
        Capabilities: [9c] MSI-X: Enable- Count=12 Masked-

Configuration script:
1. Start fresh
echo 0 > /sys/bus/pci/devices/0000\:08\:00.0/sriov_numvfs
modprobe -q -r mlx5_ib mlx5_core
2. Ensure that driver doesn't run and it is safe to change MSI-X
echo 0 > /sys/bus/pci/devices/0000\:08\:00.0/sriov_drivers_autoprobe
3. Load driver for the PF
modprobe mlx5_core
4. Configure one of the VFs with new number
echo 2 > /sys/bus/pci/devices/0000\:08\:00.0/sriov_numvfs
echo 21 > /sys/bus/pci/devices/0000\:08\:00.2/vfs_overlay/sriov_vf_msix_count

After this series:
[root@server ~]# lspci -vs 0000:08:00.2
08:00.2 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
....
        Capabilities: [9c] MSI-X: Enable- Count=21 Masked-

Thanks

Leon Romanovsky (4):
  PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  net/mlx5: Add dynamic MSI-X capabilities bits
  net/mlx5: Dynamically assign MSI-X vectors count
  net/mlx5: Allow to the users to configure number of MSI-X vectors

 Documentation/ABI/testing/sysfs-bus-pci       |  32 ++++
 .../net/ethernet/mellanox/mlx5/core/main.c    |  16 ++
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |   6 +
 .../net/ethernet/mellanox/mlx5/core/pci_irq.c |  72 +++++++
 .../net/ethernet/mellanox/mlx5/core/sriov.c   |  59 +++++-
 drivers/pci/iov.c                             | 180 ++++++++++++++++++
 drivers/pci/msi.c                             |  47 +++++
 drivers/pci/pci.h                             |   4 +
 include/linux/mlx5/mlx5_ifc.h                 |  11 +-
 include/linux/pci.h                           |  10 +
 10 files changed, 434 insertions(+), 3 deletions(-)

--
2.29.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-24 13:11 [PATCH mlx5-next v4 0/4] Dynamically assign MSI-X vectors count Leon Romanovsky
@ 2021-01-24 13:11 ` Leon Romanovsky
  2021-01-24 16:47   ` Alexander Duyck
  2021-01-25 21:52   ` Jakub Kicinski
  2021-01-24 13:11 ` [PATCH mlx5-next v4 2/4] net/mlx5: Add dynamic MSI-X capabilities bits Leon Romanovsky
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-24 13:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Saeed Mahameed
  Cc: Leon Romanovsky, Jason Gunthorpe, Alexander Duyck,
	Jakub Kicinski, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

From: Leon Romanovsky <leonro@nvidia.com>

Extend PCI sysfs interface with a new callback that allows configure
the number of MSI-X vectors for specific SR-IO VF. This is needed
to optimize the performance of newly bound devices by allocating
the number of vectors based on the administrator knowledge of targeted VM.

This function is applicable for SR-IOV VF because such devices allocate
their MSI-X table before they will run on the VMs and HW can't guess the
right number of vectors, so the HW allocates them statically and equally.

1) The newly added /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
file will be seen for the VFs and it is writable as long as a driver is not
bounded to the VF.

The values accepted are:
 * > 0 - this will be number reported by the VF's MSI-X capability
 * < 0 - not valid
 * = 0 - will reset to the device default value

2) In order to make management easy, provide new read-only sysfs file that
returns a total number of possible to configure MSI-X vectors.

cat /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
  = 0 - feature is not supported
  > 0 - total number of MSI-X vectors to consume by the VFs

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 Documentation/ABI/testing/sysfs-bus-pci |  32 +++++
 drivers/pci/iov.c                       | 180 ++++++++++++++++++++++++
 drivers/pci/msi.c                       |  47 +++++++
 drivers/pci/pci.h                       |   4 +
 include/linux/pci.h                     |  10 ++
 5 files changed, 273 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 25c9c39770c6..4d206ade5331 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -375,3 +375,35 @@ Description:
 		The value comes from the PCI kernel device state and can be one
 		of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold".
 		The file is read only.
+
+What:		/sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
+Date:		January 2021
+Contact:	Leon Romanovsky <leonro@nvidia.com>
+Description:
+		This file is associated with the SR-IOV VFs.
+		It allows configuration of the number of MSI-X vectors for
+		the VF. This is needed to optimize performance of newly bound
+		devices by allocating the number of vectors based on the
+		administrator knowledge of targeted VM.
+
+		The values accepted are:
+		 * > 0 - this will be number reported by the VF's MSI-X
+			 capability
+		 * < 0 - not valid
+		 * = 0 - will reset to the device default value
+
+		The file is writable if the PF is bound to a driver that
+		set sriov_vf_total_msix > 0 and there is no driver bound
+		to the VF.
+
+What:		/sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
+Date:		January 2021
+Contact:	Leon Romanovsky <leonro@nvidia.com>
+Description:
+		This file is associated with the SR-IOV PFs.
+		It returns a total number of possible to configure MSI-X
+		vectors on the enabled VFs.
+
+		The values returned are:
+		 * > 0 - this will be total number possible to consume by VFs,
+		 * = 0 - feature is not supported
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 4afd4ee4f7f0..3e95f835eba5 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -31,6 +31,7 @@ int pci_iov_virtfn_devfn(struct pci_dev *dev, int vf_id)
 	return (dev->devfn + dev->sriov->offset +
 		dev->sriov->stride * vf_id) & 0xff;
 }
+EXPORT_SYMBOL_GPL(pci_iov_virtfn_devfn);

 /*
  * Per SR-IOV spec sec 3.3.10 and 3.3.11, First VF Offset and VF Stride may
@@ -157,6 +158,166 @@ int pci_iov_sysfs_link(struct pci_dev *dev,
 	return rc;
 }

+#ifdef CONFIG_PCI_MSI
+static ssize_t sriov_vf_msix_count_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	int count = pci_msix_vec_count(pdev);
+
+	if (count < 0)
+		return count;
+
+	return sysfs_emit(buf, "%d\n", count);
+}
+
+static ssize_t sriov_vf_msix_count_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct pci_dev *vf_dev = to_pci_dev(dev);
+	int val, ret;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = pci_vf_set_msix_vec_count(vf_dev, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_RW(sriov_vf_msix_count);
+
+static ssize_t sriov_vf_total_msix_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	return sysfs_emit(buf, "%u\n", pdev->sriov->vf_total_msix);
+}
+static DEVICE_ATTR_RO(sriov_vf_total_msix);
+#endif
+
+static struct attribute *sriov_pf_dev_attrs[] = {
+#ifdef CONFIG_PCI_MSI
+	&dev_attr_sriov_vf_total_msix.attr,
+#endif
+	NULL,
+};
+
+static struct attribute *sriov_vf_dev_attrs[] = {
+#ifdef CONFIG_PCI_MSI
+	&dev_attr_sriov_vf_msix_count.attr,
+#endif
+	NULL,
+};
+
+static umode_t sriov_pf_attrs_are_visible(struct kobject *kobj,
+					  struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pdev->msix_cap || !dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+static umode_t sriov_vf_attrs_are_visible(struct kobject *kobj,
+					  struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (!pdev->msix_cap || dev_is_pf(dev))
+		return 0;
+
+	return a->mode;
+}
+
+static const struct attribute_group sriov_pf_dev_attr_group = {
+	.attrs = sriov_pf_dev_attrs,
+	.is_visible = sriov_pf_attrs_are_visible,
+	.name = "vfs_overlay",
+};
+
+static const struct attribute_group sriov_vf_dev_attr_group = {
+	.attrs = sriov_vf_dev_attrs,
+	.is_visible = sriov_vf_attrs_are_visible,
+	.name = "vfs_overlay",
+};
+
+int pci_enable_vfs_overlay(struct pci_dev *dev)
+{
+	struct pci_dev *virtfn;
+	int id, ret;
+
+	if (!dev->is_physfn || !dev->sriov->num_VFs)
+		return 0;
+
+	ret = sysfs_create_group(&dev->dev.kobj, &sriov_pf_dev_attr_group);
+	if (ret)
+		return ret;
+
+	for (id = 0; id < dev->sriov->num_VFs; id++) {
+		virtfn = pci_get_domain_bus_and_slot(
+			pci_domain_nr(dev->bus), pci_iov_virtfn_bus(dev, id),
+			pci_iov_virtfn_devfn(dev, id));
+
+		if (!virtfn)
+			continue;
+
+		ret = sysfs_create_group(&virtfn->dev.kobj,
+					 &sriov_vf_dev_attr_group);
+		if (ret)
+			goto out;
+	}
+	return 0;
+
+out:
+	while (id--) {
+		virtfn = pci_get_domain_bus_and_slot(
+			pci_domain_nr(dev->bus), pci_iov_virtfn_bus(dev, id),
+			pci_iov_virtfn_devfn(dev, id));
+
+		if (!virtfn)
+			continue;
+
+		sysfs_remove_group(&virtfn->dev.kobj, &sriov_vf_dev_attr_group);
+	}
+	sysfs_remove_group(&dev->dev.kobj, &sriov_pf_dev_attr_group);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pci_enable_vfs_overlay);
+
+void pci_disable_vfs_overlay(struct pci_dev *dev)
+{
+	struct pci_dev *virtfn;
+	int id;
+
+	if (!dev->is_physfn || !dev->sriov->num_VFs)
+		return;
+
+	id = dev->sriov->num_VFs;
+	while (id--) {
+		virtfn = pci_get_domain_bus_and_slot(
+			pci_domain_nr(dev->bus), pci_iov_virtfn_bus(dev, id),
+			pci_iov_virtfn_devfn(dev, id));
+
+		if (!virtfn)
+			continue;
+
+		sysfs_remove_group(&virtfn->dev.kobj, &sriov_vf_dev_attr_group);
+	}
+	sysfs_remove_group(&dev->dev.kobj, &sriov_pf_dev_attr_group);
+}
+EXPORT_SYMBOL_GPL(pci_disable_vfs_overlay);
+
 int pci_iov_add_virtfn(struct pci_dev *dev, int id)
 {
 	int i;
@@ -596,6 +757,7 @@ static void sriov_disable(struct pci_dev *dev)
 		sysfs_remove_link(&dev->dev.kobj, "dep_link");

 	iov->num_VFs = 0;
+	iov->vf_total_msix = 0;
 	pci_iov_set_numvfs(dev, 0);
 }

@@ -1054,6 +1216,24 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);

+/**
+ * pci_sriov_set_vf_total_msix - set total number of MSI-X vectors for the VFs
+ * @dev: the PCI PF device
+ * @count: the total number of MSI-X vector to consume by the VFs
+ *
+ * Sets the number of MSI-X vectors that is possible to consume by the VFs.
+ * This interface is complimentary part of the pci_vf_set_msix_vec_count()
+ * that will be used to configure the required number on the VF.
+ */
+void pci_sriov_set_vf_total_msix(struct pci_dev *dev, u32 count)
+{
+	if (!dev->is_physfn)
+		return;
+
+	dev->sriov->vf_total_msix = count;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_set_vf_total_msix);
+
 /**
  * pci_sriov_configure_simple - helper to configure SR-IOV
  * @dev: the PCI device
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 3162f88fe940..1022fe9e6efd 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -991,6 +991,53 @@ int pci_msix_vec_count(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_msix_vec_count);

+/**
+ * pci_vf_set_msix_vec_count - change the reported number of MSI-X vectors
+ * This function is applicable for SR-IOV VF because such devices allocate
+ * their MSI-X table before they will run on the VMs and HW can't guess the
+ * right number of vectors, so the HW allocates them statically and equally.
+ * @dev: VF device that is going to be changed
+ * @count: amount of MSI-X vectors
+ **/
+int pci_vf_set_msix_vec_count(struct pci_dev *dev, int count)
+{
+	struct pci_dev *pdev = pci_physfn(dev);
+	int ret;
+
+	if (count < 0)
+		/*
+		 * We don't support negative numbers for now,
+		 * but maybe in the future it will make sense.
+		 */
+		return -EINVAL;
+
+	device_lock(&pdev->dev);
+	if (!pdev->driver) {
+		ret = -EOPNOTSUPP;
+		goto err_pdev;
+	}
+
+	device_lock(&dev->dev);
+	if (dev->driver) {
+		/*
+		 * Driver already probed this VF and configured itself
+		 * based on previously configured (or default) MSI-X vector
+		 * count. It is too late to change this field for this
+		 * specific VF.
+		 */
+		ret = -EBUSY;
+		goto err_dev;
+	}
+
+	ret = pdev->driver->sriov_set_msix_vec_count(dev, count);
+
+err_dev:
+	device_unlock(&dev->dev);
+err_pdev:
+	device_unlock(&pdev->dev);
+	return ret;
+}
+
 static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 			     int nvec, struct irq_affinity *affd, int flags)
 {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5c59365092fa..2bd6560d91e2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -183,6 +183,7 @@ extern unsigned int pci_pm_d3hot_delay;

 #ifdef CONFIG_PCI_MSI
 void pci_no_msi(void);
+int pci_vf_set_msix_vec_count(struct pci_dev *dev, int count);
 #else
 static inline void pci_no_msi(void) { }
 #endif
@@ -326,6 +327,9 @@ struct pci_sriov {
 	u16		subsystem_device; /* VF subsystem device */
 	resource_size_t	barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
 	bool		drivers_autoprobe; /* Auto probing of VFs by driver */
+	u32		vf_total_msix;  /* Total number of MSI-X vectors the VFs
+					 * can consume
+					 */
 };

 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b32126d26997..526ef67dabf6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -856,6 +856,8 @@ struct module;
  *		e.g. drivers/net/e100.c.
  * @sriov_configure: Optional driver callback to allow configuration of
  *		number of VFs to enable via sysfs "sriov_numvfs" file.
+ * @sriov_set_msix_vec_count: Driver callback to change number of MSI-X vectors
+ *              exposed by the sysfs "vf_msix_vec" entry.
  * @err_handler: See Documentation/PCI/pci-error-recovery.rst
  * @groups:	Sysfs attribute groups.
  * @driver:	Driver model structure.
@@ -871,6 +873,7 @@ struct pci_driver {
 	int  (*resume)(struct pci_dev *dev);	/* Device woken up */
 	void (*shutdown)(struct pci_dev *dev);
 	int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
+	int  (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */
 	const struct pci_error_handlers *err_handler;
 	const struct attribute_group **groups;
 	struct device_driver	driver;
@@ -2059,6 +2062,9 @@ void __iomem *pci_ioremap_wc_bar(struct pci_dev *pdev, int bar);
 int pci_iov_virtfn_bus(struct pci_dev *dev, int id);
 int pci_iov_virtfn_devfn(struct pci_dev *dev, int id);

+int pci_enable_vfs_overlay(struct pci_dev *dev);
+void pci_disable_vfs_overlay(struct pci_dev *dev);
+
 int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn);
 void pci_disable_sriov(struct pci_dev *dev);

@@ -2072,6 +2078,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev);
 int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
 resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
 void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
+void pci_sriov_set_vf_total_msix(struct pci_dev *dev, u32 count);

 /* Arch may override these (weak) */
 int pcibios_sriov_enable(struct pci_dev *pdev, u16 num_vfs);
@@ -2100,6 +2107,8 @@ static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id)
 }
 static inline void pci_iov_remove_virtfn(struct pci_dev *dev,
 					 int id) { }
+static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
+static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
 static inline void pci_disable_sriov(struct pci_dev *dev) { }
 static inline int pci_num_vf(struct pci_dev *dev) { return 0; }
 static inline int pci_vfs_assigned(struct pci_dev *dev)
@@ -2112,6 +2121,7 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev)
 static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno)
 { return 0; }
 static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { }
+static inline void pci_sriov_set_vf_total_msix(struct pci_dev *dev, u32 count) {}
 #endif

 #if defined(CONFIG_HOTPLUG_PCI) || defined(CONFIG_HOTPLUG_PCI_MODULE)
--
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH mlx5-next v4 2/4] net/mlx5: Add dynamic MSI-X capabilities bits
  2021-01-24 13:11 [PATCH mlx5-next v4 0/4] Dynamically assign MSI-X vectors count Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
@ 2021-01-24 13:11 ` Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 3/4] net/mlx5: Dynamically assign MSI-X vectors count Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 4/4] net/mlx5: Allow to the users to configure number of MSI-X vectors Leon Romanovsky
  3 siblings, 0 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-24 13:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Saeed Mahameed
  Cc: Leon Romanovsky, Jason Gunthorpe, Alexander Duyck,
	Jakub Kicinski, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

From: Leon Romanovsky <leonro@nvidia.com>

These new fields declare the number of MSI-X vectors that is
possible to allocate on the VF through PF configuration.

Value must be in range defined by min_dynamic_vf_msix_table_size
and max_dynamic_vf_msix_table_size.

The driver should continue to query its MSI-X table through PCI
configuration header.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/mlx5/mlx5_ifc.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index b96f99f1198e..31e6eac67f51 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -1657,7 +1657,16 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 	u8	   reserved_at_6e0[0x10];
 	u8	   sf_base_id[0x10];
 
-	u8	   reserved_at_700[0x80];
+	u8	   reserved_at_700[0x8];
+	u8	   num_total_dynamic_vf_msix[0x18];
+	u8	   reserved_at_720[0x14];
+	u8	   dynamic_msix_table_size[0xc];
+	u8	   reserved_at_740[0xc];
+	u8	   min_dynamic_vf_msix_table_size[0x4];
+	u8	   reserved_at_750[0x4];
+	u8	   max_dynamic_vf_msix_table_size[0xc];
+
+	u8	   reserved_at_760[0x20];
 	u8	   vhca_tunnel_commands[0x40];
 	u8	   reserved_at_7c0[0x40];
 };
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH mlx5-next v4 3/4] net/mlx5: Dynamically assign MSI-X vectors count
  2021-01-24 13:11 [PATCH mlx5-next v4 0/4] Dynamically assign MSI-X vectors count Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 2/4] net/mlx5: Add dynamic MSI-X capabilities bits Leon Romanovsky
@ 2021-01-24 13:11 ` Leon Romanovsky
  2021-01-24 13:11 ` [PATCH mlx5-next v4 4/4] net/mlx5: Allow to the users to configure number of MSI-X vectors Leon Romanovsky
  3 siblings, 0 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-24 13:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Saeed Mahameed
  Cc: Leon Romanovsky, Jason Gunthorpe, Alexander Duyck,
	Jakub Kicinski, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

From: Leon Romanovsky <leonro@nvidia.com>

The number of MSI-X vectors is PCI property visible through lspci, that
field is read-only and configured by the device. The static assignment
of an amount of MSI-X vectors doesn't allow utilize the newly created
VF because it is not known to the device the future load and configuration
where that VF will be used.

To overcome the inefficiency in the spread of such MSI-X vectors, we
allow the kernel to instruct the device with the needed number of such
vectors.

Such change immediately increases the amount of MSI-X vectors for the
system with @ VFs from 12 vectors per-VF, to be 32 vectors per-VF.

Before this patch:
[root@server ~]# lspci -vs 0000:08:00.2
08:00.2 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
....
	Capabilities: [9c] MSI-X: Enable- Count=12 Masked-

After this patch:
[root@server ~]# lspci -vs 0000:08:00.2
08:00.2 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
....
	Capabilities: [9c] MSI-X: Enable- Count=32 Masked-

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/main.c    |  4 ++
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |  5 ++
 .../net/ethernet/mellanox/mlx5/core/pci_irq.c | 72 +++++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/sriov.c   | 13 +++-
 4 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index ca6f2fc39ea0..79cfcc844156 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -567,6 +567,10 @@ static int handle_hca_cap(struct mlx5_core_dev *dev, void *set_ctx)
 	if (MLX5_CAP_GEN_MAX(dev, mkey_by_name))
 		MLX5_SET(cmd_hca_cap, set_hca_cap, mkey_by_name, 1);
 
+	if (MLX5_CAP_GEN_MAX(dev, num_total_dynamic_vf_msix))
+		MLX5_SET(cmd_hca_cap, set_hca_cap, num_total_dynamic_vf_msix,
+			 MLX5_CAP_GEN_MAX(dev, num_total_dynamic_vf_msix));
+
 	return set_caps(dev, set_ctx, MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE);
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
index 0a0302ce7144..5babb4434a87 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
@@ -172,6 +172,11 @@ int mlx5_irq_attach_nb(struct mlx5_irq_table *irq_table, int vecidx,
 		       struct notifier_block *nb);
 int mlx5_irq_detach_nb(struct mlx5_irq_table *irq_table, int vecidx,
 		       struct notifier_block *nb);
+
+int mlx5_set_msix_vec_count(struct mlx5_core_dev *dev, int devfn,
+			    int msix_vec_count);
+int mlx5_get_default_msix_vec_count(struct mlx5_core_dev *dev, int num_vfs);
+
 struct cpumask *
 mlx5_irq_get_affinity_mask(struct mlx5_irq_table *irq_table, int vecidx);
 struct cpu_rmap *mlx5_irq_get_rmap(struct mlx5_irq_table *table);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
index 6fd974920394..2a35888fcff0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c
@@ -55,6 +55,78 @@ static struct mlx5_irq *mlx5_irq_get(struct mlx5_core_dev *dev, int vecidx)
 	return &irq_table->irq[vecidx];
 }
 
+/**
+ * mlx5_get_default_msix_vec_count() - Get defaults of number of MSI-X vectors
+ * to be set
+ * @dev: PF to work on
+ * @num_vfs: Number of VFs was asked when SR-IOV was enabled
+ **/
+int mlx5_get_default_msix_vec_count(struct mlx5_core_dev *dev, int num_vfs)
+{
+	int num_vf_msix, min_msix, max_msix;
+
+	num_vf_msix = MLX5_CAP_GEN_MAX(dev, num_total_dynamic_vf_msix);
+	if (!num_vf_msix)
+		return 0;
+
+	min_msix = MLX5_CAP_GEN(dev, min_dynamic_vf_msix_table_size);
+	max_msix = MLX5_CAP_GEN(dev, max_dynamic_vf_msix_table_size);
+
+	/* Limit maximum number of MSI-X to leave some of them free in the
+	 * pool and ready to be assigned by the users without need to resize
+	 * other Vfs.
+	 */
+	return max(min(num_vf_msix / num_vfs, max_msix / 2), min_msix);
+}
+
+/**
+ * mlx5_set_msix_vec_count() - Set dynamically allocated MSI-X to the VF
+ * @dev: PF to work on
+ * @function_id: Internal PCI VF function id
+ * @msix_vec_count: Number of MSI-X to set
+ **/
+int mlx5_set_msix_vec_count(struct mlx5_core_dev *dev, int function_id,
+			    int msix_vec_count)
+{
+	int sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
+	int num_vf_msix, min_msix, max_msix;
+	void *hca_cap, *cap;
+	int ret;
+
+	num_vf_msix = MLX5_CAP_GEN_MAX(dev, num_total_dynamic_vf_msix);
+	if (!num_vf_msix)
+		return 0;
+
+	if (!MLX5_CAP_GEN(dev, vport_group_manager) || !mlx5_core_is_pf(dev))
+		return -EOPNOTSUPP;
+
+	min_msix = MLX5_CAP_GEN(dev, min_dynamic_vf_msix_table_size);
+	max_msix = MLX5_CAP_GEN(dev, max_dynamic_vf_msix_table_size);
+
+	if (msix_vec_count < min_msix)
+		return -EINVAL;
+
+	if (msix_vec_count > max_msix)
+		return -EOVERFLOW;
+
+	hca_cap = kzalloc(sz, GFP_KERNEL);
+	if (!hca_cap)
+		return -ENOMEM;
+
+	cap = MLX5_ADDR_OF(set_hca_cap_in, hca_cap, capability);
+	MLX5_SET(cmd_hca_cap, cap, dynamic_msix_table_size, msix_vec_count);
+
+	MLX5_SET(set_hca_cap_in, hca_cap, opcode, MLX5_CMD_OP_SET_HCA_CAP);
+	MLX5_SET(set_hca_cap_in, hca_cap, other_function, 1);
+	MLX5_SET(set_hca_cap_in, hca_cap, function_id, function_id);
+
+	MLX5_SET(set_hca_cap_in, hca_cap, op_mod,
+		 MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE << 1);
+	ret = mlx5_cmd_exec_in(dev, set_hca_cap, hca_cap);
+	kfree(hca_cap);
+	return ret;
+}
+
 int mlx5_irq_attach_nb(struct mlx5_irq_table *irq_table, int vecidx,
 		       struct notifier_block *nb)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
index 3094d20297a9..f0ec86a1c8a6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
@@ -71,8 +71,7 @@ static int sriov_restore_guids(struct mlx5_core_dev *dev, int vf)
 static int mlx5_device_enable_sriov(struct mlx5_core_dev *dev, int num_vfs)
 {
 	struct mlx5_core_sriov *sriov = &dev->priv.sriov;
-	int err;
-	int vf;
+	int err, vf, num_msix_count;
 
 	if (!MLX5_ESWITCH_MANAGER(dev))
 		goto enable_vfs_hca;
@@ -85,12 +84,22 @@ static int mlx5_device_enable_sriov(struct mlx5_core_dev *dev, int num_vfs)
 	}
 
 enable_vfs_hca:
+	num_msix_count = mlx5_get_default_msix_vec_count(dev, num_vfs);
 	for (vf = 0; vf < num_vfs; vf++) {
 		err = mlx5_core_enable_hca(dev, vf + 1);
 		if (err) {
 			mlx5_core_warn(dev, "failed to enable VF %d (%d)\n", vf, err);
 			continue;
 		}
+
+		err = mlx5_set_msix_vec_count(dev, vf + 1, num_msix_count);
+		if (err) {
+			mlx5_core_warn(dev,
+				       "failed to set MSI-X vector counts VF %d, err %d\n",
+				       vf, err);
+			continue;
+		}
+
 		sriov->vfs_ctx[vf].enabled = 1;
 		if (MLX5_CAP_GEN(dev, port_type) == MLX5_CAP_PORT_TYPE_IB) {
 			err = sriov_restore_guids(dev, vf);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH mlx5-next v4 4/4] net/mlx5: Allow to the users to configure number of MSI-X vectors
  2021-01-24 13:11 [PATCH mlx5-next v4 0/4] Dynamically assign MSI-X vectors count Leon Romanovsky
                   ` (2 preceding siblings ...)
  2021-01-24 13:11 ` [PATCH mlx5-next v4 3/4] net/mlx5: Dynamically assign MSI-X vectors count Leon Romanovsky
@ 2021-01-24 13:11 ` Leon Romanovsky
  3 siblings, 0 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-24 13:11 UTC (permalink / raw)
  To: Bjorn Helgaas, Saeed Mahameed
  Cc: Leon Romanovsky, Jason Gunthorpe, Alexander Duyck,
	Jakub Kicinski, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

From: Leon Romanovsky <leonro@nvidia.com>

Implement ability to configure MSI-X for the SR-IOV VFs.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/main.c    | 12 +++++
 .../ethernet/mellanox/mlx5/core/mlx5_core.h   |  1 +
 .../net/ethernet/mellanox/mlx5/core/sriov.c   | 46 +++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 79cfcc844156..228765c38cf8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1395,6 +1395,14 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_load_one;
 	}
 
+	err = pci_enable_vfs_overlay(pdev);
+	if (err) {
+		mlx5_core_err(dev,
+			      "pci_enable_vfs_overlay failed with error code %d\n",
+			      err);
+		goto err_vfs_overlay;
+	}
+
 	err = mlx5_crdump_enable(dev);
 	if (err)
 		dev_err(&pdev->dev, "mlx5_crdump_enable failed with error code %d\n", err);
@@ -1403,6 +1411,8 @@ static int init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	devlink_reload_enable(devlink);
 	return 0;
 
+err_vfs_overlay:
+	mlx5_unload_one(dev, true);
 err_load_one:
 	mlx5_pci_close(dev);
 pci_init_err:
@@ -1422,6 +1432,7 @@ static void remove_one(struct pci_dev *pdev)
 
 	devlink_reload_disable(devlink);
 	mlx5_crdump_disable(dev);
+	pci_disable_vfs_overlay(pdev);
 	mlx5_drain_health_wq(dev);
 	mlx5_unload_one(dev, true);
 	mlx5_pci_close(dev);
@@ -1650,6 +1661,7 @@ static struct pci_driver mlx5_core_driver = {
 	.shutdown	= shutdown,
 	.err_handler	= &mlx5_err_handler,
 	.sriov_configure   = mlx5_core_sriov_configure,
+	.sriov_set_msix_vec_count = mlx5_core_sriov_set_msix_vec_count,
 };
 
 static void mlx5_core_verify_params(void)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
index 5babb4434a87..8a2523d2d43a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mlx5_core.h
@@ -138,6 +138,7 @@ void mlx5_sriov_cleanup(struct mlx5_core_dev *dev);
 int mlx5_sriov_attach(struct mlx5_core_dev *dev);
 void mlx5_sriov_detach(struct mlx5_core_dev *dev);
 int mlx5_core_sriov_configure(struct pci_dev *dev, int num_vfs);
+int mlx5_core_sriov_set_msix_vec_count(struct pci_dev *vf, int msix_vec_count);
 int mlx5_core_enable_hca(struct mlx5_core_dev *dev, u16 func_id);
 int mlx5_core_disable_hca(struct mlx5_core_dev *dev, u16 func_id);
 int mlx5_create_scheduling_element_cmd(struct mlx5_core_dev *dev, u8 hierarchy,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
index f0ec86a1c8a6..252aa44ffbe3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
@@ -144,6 +144,7 @@ mlx5_device_disable_sriov(struct mlx5_core_dev *dev, int num_vfs, bool clear_vf)
 static int mlx5_sriov_enable(struct pci_dev *pdev, int num_vfs)
 {
 	struct mlx5_core_dev *dev  = pci_get_drvdata(pdev);
+	u32 num_vf_msix;
 	int err;
 
 	err = mlx5_device_enable_sriov(dev, num_vfs);
@@ -152,11 +153,20 @@ static int mlx5_sriov_enable(struct pci_dev *pdev, int num_vfs)
 		return err;
 	}
 
+	num_vf_msix = MLX5_CAP_GEN_MAX(dev, num_total_dynamic_vf_msix);
+	pci_sriov_set_vf_total_msix(pdev, num_vf_msix);
 	err = pci_enable_sriov(pdev, num_vfs);
 	if (err) {
 		mlx5_core_warn(dev, "pci_enable_sriov failed : %d\n", err);
 		mlx5_device_disable_sriov(dev, num_vfs, true);
 	}
+	err = pci_enable_vfs_overlay(pdev);
+	if (err) {
+		mlx5_core_warn(dev, "pci_enable_vfs_overlay failed : %d\n",
+			       err);
+		pci_disable_sriov(pdev);
+		mlx5_device_disable_sriov(dev, num_vfs, true);
+	}
 	return err;
 }
 
@@ -165,6 +175,7 @@ static void mlx5_sriov_disable(struct pci_dev *pdev)
 	struct mlx5_core_dev *dev  = pci_get_drvdata(pdev);
 	int num_vfs = pci_num_vf(dev->pdev);
 
+	pci_disable_vfs_overlay(pdev);
 	pci_disable_sriov(pdev);
 	mlx5_device_disable_sriov(dev, num_vfs, true);
 }
@@ -187,6 +198,41 @@ int mlx5_core_sriov_configure(struct pci_dev *pdev, int num_vfs)
 	return err ? err : num_vfs;
 }
 
+int mlx5_core_sriov_set_msix_vec_count(struct pci_dev *vf, int msix_vec_count)
+{
+	struct pci_dev *pf = pci_physfn(vf);
+	struct mlx5_core_sriov *sriov;
+	struct mlx5_core_dev *dev;
+	int num_vf_msix, id;
+
+	dev = pci_get_drvdata(pf);
+	num_vf_msix = MLX5_CAP_GEN_MAX(dev, num_total_dynamic_vf_msix);
+	if (!num_vf_msix)
+		return -EOPNOTSUPP;
+
+	if (!msix_vec_count)
+		msix_vec_count =
+			mlx5_get_default_msix_vec_count(dev, pci_num_vf(pf));
+
+	sriov = &dev->priv.sriov;
+
+	/* Reversed translation of PCI VF function number to the internal
+	 * function_id, which exists in the name of virtfn symlink.
+	 */
+	for (id = 0; id < pci_num_vf(pf); id++) {
+		if (!sriov->vfs_ctx[id].enabled)
+			continue;
+
+		if (vf->devfn == pci_iov_virtfn_devfn(pf, id))
+			break;
+	}
+
+	if (id == pci_num_vf(pf) || !sriov->vfs_ctx[id].enabled)
+		return -EINVAL;
+
+	return mlx5_set_msix_vec_count(dev, id + 1, msix_vec_count);
+}
+
 int mlx5_sriov_attach(struct mlx5_core_dev *dev)
 {
 	if (!mlx5_core_is_pf(dev) || !pci_num_vf(dev->pdev))
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-24 13:11 ` [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
@ 2021-01-24 16:47   ` Alexander Duyck
  2021-01-24 19:00     ` Leon Romanovsky
  2021-01-25 21:52   ` Jakub Kicinski
  1 sibling, 1 reply; 16+ messages in thread
From: Alexander Duyck @ 2021-01-24 16:47 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Bjorn Helgaas, Saeed Mahameed, Leon Romanovsky, Jason Gunthorpe,
	Jakub Kicinski, linux-pci, linux-rdma, Netdev, Don Dutile,
	Alex Williamson, David S . Miller

On Sun, Jan 24, 2021 at 5:11 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> Extend PCI sysfs interface with a new callback that allows configure
> the number of MSI-X vectors for specific SR-IO VF. This is needed
> to optimize the performance of newly bound devices by allocating
> the number of vectors based on the administrator knowledge of targeted VM.
>
> This function is applicable for SR-IOV VF because such devices allocate
> their MSI-X table before they will run on the VMs and HW can't guess the
> right number of vectors, so the HW allocates them statically and equally.
>
> 1) The newly added /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
> file will be seen for the VFs and it is writable as long as a driver is not
> bounded to the VF.
>
> The values accepted are:
>  * > 0 - this will be number reported by the VF's MSI-X capability
>  * < 0 - not valid
>  * = 0 - will reset to the device default value
>
> 2) In order to make management easy, provide new read-only sysfs file that
> returns a total number of possible to configure MSI-X vectors.
>
> cat /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
>   = 0 - feature is not supported
>   > 0 - total number of MSI-X vectors to consume by the VFs
>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci |  32 +++++
>  drivers/pci/iov.c                       | 180 ++++++++++++++++++++++++
>  drivers/pci/msi.c                       |  47 +++++++
>  drivers/pci/pci.h                       |   4 +
>  include/linux/pci.h                     |  10 ++
>  5 files changed, 273 insertions(+)
>

<snip>

> +
> +static umode_t sriov_pf_attrs_are_visible(struct kobject *kobj,
> +                                         struct attribute *a, int n)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +
> +       if (!pdev->msix_cap || !dev_is_pf(dev))
> +               return 0;
> +
> +       return a->mode;
> +}
> +
> +static umode_t sriov_vf_attrs_are_visible(struct kobject *kobj,
> +                                         struct attribute *a, int n)
> +{
> +       struct device *dev = kobj_to_dev(kobj);
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +
> +       if (!pdev->msix_cap || dev_is_pf(dev))
> +               return 0;
> +
> +       return a->mode;
> +}
> +

Given the changes I don't see why we need to add the "visible"
functions. We are only registering this from the PF if there is a need
to make use of the interfaces, correct? If so we can just assume that
the interfaces should always be visible if they are requested.

Also you may want to look at placing a link to the VF folders in the
PF folder, although I suppose there are already links from the PF PCI
device to the VF PCI devices so maybe that isn't necessary. It just
takes a few extra steps to navigate between the two.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-24 16:47   ` Alexander Duyck
@ 2021-01-24 19:00     ` Leon Romanovsky
  2021-01-25 18:47       ` Leon Romanovsky
  0 siblings, 1 reply; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-24 19:00 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe, Jakub Kicinski,
	linux-pci, linux-rdma, Netdev, Don Dutile, Alex Williamson,
	David S . Miller

On Sun, Jan 24, 2021 at 08:47:44AM -0800, Alexander Duyck wrote:
> On Sun, Jan 24, 2021 at 5:11 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > Extend PCI sysfs interface with a new callback that allows configure
> > the number of MSI-X vectors for specific SR-IO VF. This is needed
> > to optimize the performance of newly bound devices by allocating
> > the number of vectors based on the administrator knowledge of targeted VM.
> >
> > This function is applicable for SR-IOV VF because such devices allocate
> > their MSI-X table before they will run on the VMs and HW can't guess the
> > right number of vectors, so the HW allocates them statically and equally.
> >
> > 1) The newly added /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
> > file will be seen for the VFs and it is writable as long as a driver is not
> > bounded to the VF.
> >
> > The values accepted are:
> >  * > 0 - this will be number reported by the VF's MSI-X capability
> >  * < 0 - not valid
> >  * = 0 - will reset to the device default value
> >
> > 2) In order to make management easy, provide new read-only sysfs file that
> > returns a total number of possible to configure MSI-X vectors.
> >
> > cat /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
> >   = 0 - feature is not supported
> >   > 0 - total number of MSI-X vectors to consume by the VFs
> >
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-pci |  32 +++++
> >  drivers/pci/iov.c                       | 180 ++++++++++++++++++++++++
> >  drivers/pci/msi.c                       |  47 +++++++
> >  drivers/pci/pci.h                       |   4 +
> >  include/linux/pci.h                     |  10 ++
> >  5 files changed, 273 insertions(+)
> >
>
> <snip>
>
> > +
> > +static umode_t sriov_pf_attrs_are_visible(struct kobject *kobj,
> > +                                         struct attribute *a, int n)
> > +{
> > +       struct device *dev = kobj_to_dev(kobj);
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +       if (!pdev->msix_cap || !dev_is_pf(dev))
> > +               return 0;
> > +
> > +       return a->mode;
> > +}
> > +
> > +static umode_t sriov_vf_attrs_are_visible(struct kobject *kobj,
> > +                                         struct attribute *a, int n)
> > +{
> > +       struct device *dev = kobj_to_dev(kobj);
> > +       struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +       if (!pdev->msix_cap || dev_is_pf(dev))
> > +               return 0;
> > +
> > +       return a->mode;
> > +}
> > +
>
> Given the changes I don't see why we need to add the "visible"
> functions. We are only registering this from the PF if there is a need
> to make use of the interfaces, correct? If so we can just assume that
> the interfaces should always be visible if they are requested.

I added them to make extension of this vfs_overlay interface more easy,
so we won't forget that current fields needs "msix_cap". Also I followed
same style as other attribute_group which has .is_visible.

>
> Also you may want to look at placing a link to the VF folders in the
> PF folder, although I suppose there are already links from the PF PCI
> device to the VF PCI devices so maybe that isn't necessary. It just
> takes a few extra steps to navigate between the two.

We already have, I don't think that we need to add extra links, it will
give nothing.

[leonro@vm ~]$ ls -l /sys/bus/pci/devices/0000\:01\:00.0/
....
drwxr-xr-x 2 root root        0 Jan 24 14:02 vfs_overlay
lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn0 -> ../0000:01:00.1
lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn1 -> ../0000:01:00.2
....

Thanks

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-24 19:00     ` Leon Romanovsky
@ 2021-01-25 18:47       ` Leon Romanovsky
  2021-01-25 18:50         ` Alexander Duyck
  0 siblings, 1 reply; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-25 18:47 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe, Jakub Kicinski,
	linux-pci, linux-rdma, Netdev, Don Dutile, Alex Williamson,
	David S . Miller

On Sun, Jan 24, 2021 at 09:00:32PM +0200, Leon Romanovsky wrote:
> On Sun, Jan 24, 2021 at 08:47:44AM -0800, Alexander Duyck wrote:
> > On Sun, Jan 24, 2021 at 5:11 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > >
> > > Extend PCI sysfs interface with a new callback that allows configure
> > > the number of MSI-X vectors for specific SR-IO VF. This is needed
> > > to optimize the performance of newly bound devices by allocating
> > > the number of vectors based on the administrator knowledge of targeted VM.
> > >
> > > This function is applicable for SR-IOV VF because such devices allocate
> > > their MSI-X table before they will run on the VMs and HW can't guess the
> > > right number of vectors, so the HW allocates them statically and equally.
> > >
> > > 1) The newly added /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
> > > file will be seen for the VFs and it is writable as long as a driver is not
> > > bounded to the VF.
> > >
> > > The values accepted are:
> > >  * > 0 - this will be number reported by the VF's MSI-X capability
> > >  * < 0 - not valid
> > >  * = 0 - will reset to the device default value
> > >
> > > 2) In order to make management easy, provide new read-only sysfs file that
> > > returns a total number of possible to configure MSI-X vectors.
> > >
> > > cat /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
> > >   = 0 - feature is not supported
> > >   > 0 - total number of MSI-X vectors to consume by the VFs
> > >
> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-pci |  32 +++++
> > >  drivers/pci/iov.c                       | 180 ++++++++++++++++++++++++
> > >  drivers/pci/msi.c                       |  47 +++++++
> > >  drivers/pci/pci.h                       |   4 +
> > >  include/linux/pci.h                     |  10 ++
> > >  5 files changed, 273 insertions(+)
> > >
> >
> > <snip>
> >
> > > +
> > > +static umode_t sriov_pf_attrs_are_visible(struct kobject *kobj,
> > > +                                         struct attribute *a, int n)
> > > +{
> > > +       struct device *dev = kobj_to_dev(kobj);
> > > +       struct pci_dev *pdev = to_pci_dev(dev);
> > > +
> > > +       if (!pdev->msix_cap || !dev_is_pf(dev))
> > > +               return 0;
> > > +
> > > +       return a->mode;
> > > +}
> > > +
> > > +static umode_t sriov_vf_attrs_are_visible(struct kobject *kobj,
> > > +                                         struct attribute *a, int n)
> > > +{
> > > +       struct device *dev = kobj_to_dev(kobj);
> > > +       struct pci_dev *pdev = to_pci_dev(dev);
> > > +
> > > +       if (!pdev->msix_cap || dev_is_pf(dev))
> > > +               return 0;
> > > +
> > > +       return a->mode;
> > > +}
> > > +
> >
> > Given the changes I don't see why we need to add the "visible"
> > functions. We are only registering this from the PF if there is a need
> > to make use of the interfaces, correct? If so we can just assume that
> > the interfaces should always be visible if they are requested.
>
> I added them to make extension of this vfs_overlay interface more easy,
> so we won't forget that current fields needs "msix_cap". Also I followed
> same style as other attribute_group which has .is_visible.
>
> >
> > Also you may want to look at placing a link to the VF folders in the
> > PF folder, although I suppose there are already links from the PF PCI
> > device to the VF PCI devices so maybe that isn't necessary. It just
> > takes a few extra steps to navigate between the two.
>
> We already have, I don't think that we need to add extra links, it will
> give nothing.
>
> [leonro@vm ~]$ ls -l /sys/bus/pci/devices/0000\:01\:00.0/
> ....
> drwxr-xr-x 2 root root        0 Jan 24 14:02 vfs_overlay
> lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn0 -> ../0000:01:00.1
> lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn1 -> ../0000:01:00.2
> ....

Alexander, are we clear here? Do you expect v5 without ".is_visible" from me?

Thanks

>
> Thanks

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-25 18:47       ` Leon Romanovsky
@ 2021-01-25 18:50         ` Alexander Duyck
  2021-01-25 18:54           ` Leon Romanovsky
  0 siblings, 1 reply; 16+ messages in thread
From: Alexander Duyck @ 2021-01-25 18:50 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe, Jakub Kicinski,
	linux-pci, linux-rdma, Netdev, Don Dutile, Alex Williamson,
	David S . Miller

On Mon, Jan 25, 2021 at 10:47 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sun, Jan 24, 2021 at 09:00:32PM +0200, Leon Romanovsky wrote:
> > On Sun, Jan 24, 2021 at 08:47:44AM -0800, Alexander Duyck wrote:
> > > On Sun, Jan 24, 2021 at 5:11 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > >
> > > > Extend PCI sysfs interface with a new callback that allows configure
> > > > the number of MSI-X vectors for specific SR-IO VF. This is needed
> > > > to optimize the performance of newly bound devices by allocating
> > > > the number of vectors based on the administrator knowledge of targeted VM.
> > > >
> > > > This function is applicable for SR-IOV VF because such devices allocate
> > > > their MSI-X table before they will run on the VMs and HW can't guess the
> > > > right number of vectors, so the HW allocates them statically and equally.
> > > >
> > > > 1) The newly added /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
> > > > file will be seen for the VFs and it is writable as long as a driver is not
> > > > bounded to the VF.
> > > >
> > > > The values accepted are:
> > > >  * > 0 - this will be number reported by the VF's MSI-X capability
> > > >  * < 0 - not valid
> > > >  * = 0 - will reset to the device default value
> > > >
> > > > 2) In order to make management easy, provide new read-only sysfs file that
> > > > returns a total number of possible to configure MSI-X vectors.
> > > >
> > > > cat /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
> > > >   = 0 - feature is not supported
> > > >   > 0 - total number of MSI-X vectors to consume by the VFs
> > > >
> > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-pci |  32 +++++
> > > >  drivers/pci/iov.c                       | 180 ++++++++++++++++++++++++
> > > >  drivers/pci/msi.c                       |  47 +++++++
> > > >  drivers/pci/pci.h                       |   4 +
> > > >  include/linux/pci.h                     |  10 ++
> > > >  5 files changed, 273 insertions(+)
> > > >
> > >
> > > <snip>
> > >
> > > > +
> > > > +static umode_t sriov_pf_attrs_are_visible(struct kobject *kobj,
> > > > +                                         struct attribute *a, int n)
> > > > +{
> > > > +       struct device *dev = kobj_to_dev(kobj);
> > > > +       struct pci_dev *pdev = to_pci_dev(dev);
> > > > +
> > > > +       if (!pdev->msix_cap || !dev_is_pf(dev))
> > > > +               return 0;
> > > > +
> > > > +       return a->mode;
> > > > +}
> > > > +
> > > > +static umode_t sriov_vf_attrs_are_visible(struct kobject *kobj,
> > > > +                                         struct attribute *a, int n)
> > > > +{
> > > > +       struct device *dev = kobj_to_dev(kobj);
> > > > +       struct pci_dev *pdev = to_pci_dev(dev);
> > > > +
> > > > +       if (!pdev->msix_cap || dev_is_pf(dev))
> > > > +               return 0;
> > > > +
> > > > +       return a->mode;
> > > > +}
> > > > +
> > >
> > > Given the changes I don't see why we need to add the "visible"
> > > functions. We are only registering this from the PF if there is a need
> > > to make use of the interfaces, correct? If so we can just assume that
> > > the interfaces should always be visible if they are requested.
> >
> > I added them to make extension of this vfs_overlay interface more easy,
> > so we won't forget that current fields needs "msix_cap". Also I followed
> > same style as other attribute_group which has .is_visible.
> >
> > >
> > > Also you may want to look at placing a link to the VF folders in the
> > > PF folder, although I suppose there are already links from the PF PCI
> > > device to the VF PCI devices so maybe that isn't necessary. It just
> > > takes a few extra steps to navigate between the two.
> >
> > We already have, I don't think that we need to add extra links, it will
> > give nothing.
> >
> > [leonro@vm ~]$ ls -l /sys/bus/pci/devices/0000\:01\:00.0/
> > ....
> > drwxr-xr-x 2 root root        0 Jan 24 14:02 vfs_overlay
> > lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn0 -> ../0000:01:00.1
> > lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn1 -> ../0000:01:00.2
> > ....
>
> Alexander, are we clear here? Do you expect v5 without ".is_visible" from me?

Yeah, I am okay with the .is_visible being left around. It just seems
redundant is all.

Thanks.

 -Alex

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-25 18:50         ` Alexander Duyck
@ 2021-01-25 18:54           ` Leon Romanovsky
  0 siblings, 0 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-25 18:54 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe, Jakub Kicinski,
	linux-pci, linux-rdma, Netdev, Don Dutile, Alex Williamson,
	David S . Miller

On Mon, Jan 25, 2021 at 10:50:53AM -0800, Alexander Duyck wrote:
> On Mon, Jan 25, 2021 at 10:47 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sun, Jan 24, 2021 at 09:00:32PM +0200, Leon Romanovsky wrote:
> > > On Sun, Jan 24, 2021 at 08:47:44AM -0800, Alexander Duyck wrote:
> > > > On Sun, Jan 24, 2021 at 5:11 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > >
> > > > > Extend PCI sysfs interface with a new callback that allows configure
> > > > > the number of MSI-X vectors for specific SR-IO VF. This is needed
> > > > > to optimize the performance of newly bound devices by allocating
> > > > > the number of vectors based on the administrator knowledge of targeted VM.
> > > > >
> > > > > This function is applicable for SR-IOV VF because such devices allocate
> > > > > their MSI-X table before they will run on the VMs and HW can't guess the
> > > > > right number of vectors, so the HW allocates them statically and equally.
> > > > >
> > > > > 1) The newly added /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_msix_count
> > > > > file will be seen for the VFs and it is writable as long as a driver is not
> > > > > bounded to the VF.
> > > > >
> > > > > The values accepted are:
> > > > >  * > 0 - this will be number reported by the VF's MSI-X capability
> > > > >  * < 0 - not valid
> > > > >  * = 0 - will reset to the device default value
> > > > >
> > > > > 2) In order to make management easy, provide new read-only sysfs file that
> > > > > returns a total number of possible to configure MSI-X vectors.
> > > > >
> > > > > cat /sys/bus/pci/devices/.../vfs_overlay/sriov_vf_total_msix
> > > > >   = 0 - feature is not supported
> > > > >   > 0 - total number of MSI-X vectors to consume by the VFs
> > > > >
> > > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-pci |  32 +++++
> > > > >  drivers/pci/iov.c                       | 180 ++++++++++++++++++++++++
> > > > >  drivers/pci/msi.c                       |  47 +++++++
> > > > >  drivers/pci/pci.h                       |   4 +
> > > > >  include/linux/pci.h                     |  10 ++
> > > > >  5 files changed, 273 insertions(+)
> > > > >
> > > >
> > > > <snip>
> > > >
> > > > > +
> > > > > +static umode_t sriov_pf_attrs_are_visible(struct kobject *kobj,
> > > > > +                                         struct attribute *a, int n)
> > > > > +{
> > > > > +       struct device *dev = kobj_to_dev(kobj);
> > > > > +       struct pci_dev *pdev = to_pci_dev(dev);
> > > > > +
> > > > > +       if (!pdev->msix_cap || !dev_is_pf(dev))
> > > > > +               return 0;
> > > > > +
> > > > > +       return a->mode;
> > > > > +}
> > > > > +
> > > > > +static umode_t sriov_vf_attrs_are_visible(struct kobject *kobj,
> > > > > +                                         struct attribute *a, int n)
> > > > > +{
> > > > > +       struct device *dev = kobj_to_dev(kobj);
> > > > > +       struct pci_dev *pdev = to_pci_dev(dev);
> > > > > +
> > > > > +       if (!pdev->msix_cap || dev_is_pf(dev))
> > > > > +               return 0;
> > > > > +
> > > > > +       return a->mode;
> > > > > +}
> > > > > +
> > > >
> > > > Given the changes I don't see why we need to add the "visible"
> > > > functions. We are only registering this from the PF if there is a need
> > > > to make use of the interfaces, correct? If so we can just assume that
> > > > the interfaces should always be visible if they are requested.
> > >
> > > I added them to make extension of this vfs_overlay interface more easy,
> > > so we won't forget that current fields needs "msix_cap". Also I followed
> > > same style as other attribute_group which has .is_visible.
> > >
> > > >
> > > > Also you may want to look at placing a link to the VF folders in the
> > > > PF folder, although I suppose there are already links from the PF PCI
> > > > device to the VF PCI devices so maybe that isn't necessary. It just
> > > > takes a few extra steps to navigate between the two.
> > >
> > > We already have, I don't think that we need to add extra links, it will
> > > give nothing.
> > >
> > > [leonro@vm ~]$ ls -l /sys/bus/pci/devices/0000\:01\:00.0/
> > > ....
> > > drwxr-xr-x 2 root root        0 Jan 24 14:02 vfs_overlay
> > > lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn0 -> ../0000:01:00.1
> > > lrwxrwxrwx 1 root root        0 Jan 24 14:02 virtfn1 -> ../0000:01:00.2
> > > ....
> >
> > Alexander, are we clear here? Do you expect v5 without ".is_visible" from me?
>
> Yeah, I am okay with the .is_visible being left around. It just seems
> redundant is all.

Thanks a lot for your review,
I appreciate the effort and time you invested into it.

>
> Thanks.
>
>  -Alex

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-24 13:11 ` [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
  2021-01-24 16:47   ` Alexander Duyck
@ 2021-01-25 21:52   ` Jakub Kicinski
  2021-01-26  6:01     ` Leon Romanovsky
  1 sibling, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2021-01-25 21:52 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Bjorn Helgaas, Saeed Mahameed, Leon Romanovsky, Jason Gunthorpe,
	Alexander Duyck, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}

s/static /static inline /

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-25 21:52   ` Jakub Kicinski
@ 2021-01-26  6:01     ` Leon Romanovsky
  2021-01-26  8:20       ` Joe Perches
  0 siblings, 1 reply; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-26  6:01 UTC (permalink / raw)
  To: Jakub Kicinski, Joe Perches
  Cc: Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe, Alexander Duyck,
	linux-pci, linux-rdma, netdev, Don Dutile, Alex Williamson,
	David S . Miller

On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
>
> s/static /static inline /

Thanks a lot, I think that we should extend checkpatch.pl to catch such
mistakes.

Joe,

How hard is it to extend checkpatch.pl to do regexp and warn if in *.h file
someone declared function with implementation but didn't add "inline" word?

Thanks

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-26  6:01     ` Leon Romanovsky
@ 2021-01-26  8:20       ` Joe Perches
  2021-01-26  8:48         ` Leon Romanovsky
  0 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2021-01-26  8:20 UTC (permalink / raw)
  To: Leon Romanovsky, Jakub Kicinski
  Cc: Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe, Alexander Duyck,
	linux-pci, linux-rdma, netdev, Don Dutile, Alex Williamson,
	David S . Miller

On Tue, 2021-01-26 at 08:01 +0200, Leon Romanovsky wrote:
> On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> > On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
> > 
> > s/static /static inline /
> 
> Thanks a lot, I think that we should extend checkpatch.pl to catch such
> mistakes.

Who is this "we" you refer to? ;)

> How hard is it to extend checkpatch.pl to do regexp and warn if in *.h file
> someone declared function with implementation but didn't add "inline" word?

Something like this seems reasonable and catches these instances in
include/linux/*.h

$ ./scripts/checkpatch.pl -f include/linux/*.h --types=static_inline --terse --nosummary
include/linux/dma-mapping.h:203: WARNING: static function definition might be better as static inline
include/linux/genl_magic_func.h:55: WARNING: static function definition might be better as static inline
include/linux/genl_magic_func.h:78: WARNING: static function definition might be better as static inline
include/linux/kernel.h:670: WARNING: static function definition might be better as static inline
include/linux/kprobes.h:213: WARNING: static function definition might be better as static inline
include/linux/kprobes.h:231: WARNING: static function definition might be better as static inline
include/linux/kprobes.h:511: WARNING: static function definition might be better as static inline
include/linux/skb_array.h:185: WARNING: static function definition might be better as static inline
include/linux/slab.h:606: WARNING: static function definition might be better as static inline
include/linux/stop_machine.h:62: WARNING: static function definition might be better as static inline
include/linux/vmw_vmci_defs.h:850: WARNING: static function definition might be better as static inline
include/linux/zstd.h:95: WARNING: static function definition might be better as static inline
include/linux/zstd.h:106: WARNING: static function definition might be better as static inline

A false positive exists when __must_check is used between
static and inline.  It's an unusual and IMO not a preferred use.
---
 scripts/checkpatch.pl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4f8494527139..0ac366481962 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4451,6 +4451,18 @@ sub process {
 			}
 		}
 
+# check for static function definitions without inline in .h files
+# only works for static in column 1 and avoids multiline macro definitions
+		if ($realfile =~ /\.h$/ &&
+		    defined($stat) &&
+		    $stat =~ /^\+static(?!\s+(?:$Inline|union|struct))\b.*\{.*\}\s*$/s &&
+		    $line =~ /^\+static(?!\s+(?:$Inline|union|struct))\b/ &&
+		    $line !~ /\\$/) {
+			WARN("STATIC_INLINE",
+			     "static function definition might be better as static inline\n" .
+				$herecurr);
+		}
+
 # check for non-global char *foo[] = {"bar", ...} declarations.
 		if ($line =~ /^.\s+(?:static\s+|const\s+)?char\s+\*\s*\w+\s*\[\s*\]\s*=\s*\{/) {
 			WARN("STATIC_CONST_CHAR_ARRAY",



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-26  8:20       ` Joe Perches
@ 2021-01-26  8:48         ` Leon Romanovsky
  2021-01-26  8:57           ` Joe Perches
  0 siblings, 1 reply; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-26  8:48 UTC (permalink / raw)
  To: Joe Perches
  Cc: Jakub Kicinski, Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe,
	Alexander Duyck, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

On Tue, Jan 26, 2021 at 12:20:11AM -0800, Joe Perches wrote:
> On Tue, 2021-01-26 at 08:01 +0200, Leon Romanovsky wrote:
> > On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> > > On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > > > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > > > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
> > >
> > > s/static /static inline /
> >
> > Thanks a lot, I think that we should extend checkpatch.pl to catch such
> > mistakes.
>
> Who is this "we" you refer to? ;)

"We" == community :)

>
> > How hard is it to extend checkpatch.pl to do regexp and warn if in *.h file
> > someone declared function with implementation but didn't add "inline" word?
>
> Something like this seems reasonable and catches these instances in
> include/linux/*.h

Thanks

>
> $ ./scripts/checkpatch.pl -f include/linux/*.h --types=static_inline --terse --nosummary
> include/linux/dma-mapping.h:203: WARNING: static function definition might be better as static inline
> include/linux/genl_magic_func.h:55: WARNING: static function definition might be better as static inline
> include/linux/genl_magic_func.h:78: WARNING: static function definition might be better as static inline
> include/linux/kernel.h:670: WARNING: static function definition might be better as static inline
> include/linux/kprobes.h:213: WARNING: static function definition might be better as static inline
> include/linux/kprobes.h:231: WARNING: static function definition might be better as static inline
> include/linux/kprobes.h:511: WARNING: static function definition might be better as static inline
> include/linux/skb_array.h:185: WARNING: static function definition might be better as static inline
> include/linux/slab.h:606: WARNING: static function definition might be better as static inline
> include/linux/stop_machine.h:62: WARNING: static function definition might be better as static inline
> include/linux/vmw_vmci_defs.h:850: WARNING: static function definition might be better as static inline
> include/linux/zstd.h:95: WARNING: static function definition might be better as static inline
> include/linux/zstd.h:106: WARNING: static function definition might be better as static inline
>
> A false positive exists when __must_check is used between
> static and inline.  It's an unusual and IMO not a preferred use.

Maybe just filter and ignore such functions for now?
Will you send proper patch or do you want me to do it?

> ---
>  scripts/checkpatch.pl | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4f8494527139..0ac366481962 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4451,6 +4451,18 @@ sub process {
>  			}
>  		}
>
> +# check for static function definitions without inline in .h files
> +# only works for static in column 1 and avoids multiline macro definitions
> +		if ($realfile =~ /\.h$/ &&
> +		    defined($stat) &&
> +		    $stat =~ /^\+static(?!\s+(?:$Inline|union|struct))\b.*\{.*\}\s*$/s &&
> +		    $line =~ /^\+static(?!\s+(?:$Inline|union|struct))\b/ &&
> +		    $line !~ /\\$/) {
> +			WARN("STATIC_INLINE",
> +			     "static function definition might be better as static inline\n" .
> +				$herecurr);
> +		}
> +
>  # check for non-global char *foo[] = {"bar", ...} declarations.
>  		if ($line =~ /^.\s+(?:static\s+|const\s+)?char\s+\*\s*\w+\s*\[\s*\]\s*=\s*\{/) {
>  			WARN("STATIC_CONST_CHAR_ARRAY",
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-26  8:48         ` Leon Romanovsky
@ 2021-01-26  8:57           ` Joe Perches
  2021-01-26  9:26             ` Leon Romanovsky
  0 siblings, 1 reply; 16+ messages in thread
From: Joe Perches @ 2021-01-26  8:57 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Jakub Kicinski, Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe,
	Alexander Duyck, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

On Tue, 2021-01-26 at 10:48 +0200, Leon Romanovsky wrote:
> On Tue, Jan 26, 2021 at 12:20:11AM -0800, Joe Perches wrote:
> > On Tue, 2021-01-26 at 08:01 +0200, Leon Romanovsky wrote:
> > > On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> > > > On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > > > > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > > > > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
[]
> > $ ./scripts/checkpatch.pl -f include/linux/*.h --types=static_inline --terse --nosummary
> > include/linux/dma-mapping.h:203: WARNING: static function definition might be better as static inline
> > include/linux/genl_magic_func.h:55: WARNING: static function definition might be better as static inline
> > include/linux/genl_magic_func.h:78: WARNING: static function definition might be better as static inline
> > include/linux/kernel.h:670: WARNING: static function definition might be better as static inline
> > include/linux/kprobes.h:213: WARNING: static function definition might be better as static inline
> > include/linux/kprobes.h:231: WARNING: static function definition might be better as static inline
> > include/linux/kprobes.h:511: WARNING: static function definition might be better as static inline
> > include/linux/skb_array.h:185: WARNING: static function definition might be better as static inline
> > include/linux/slab.h:606: WARNING: static function definition might be better as static inline
> > include/linux/stop_machine.h:62: WARNING: static function definition might be better as static inline
> > include/linux/vmw_vmci_defs.h:850: WARNING: static function definition might be better as static inline
> > include/linux/zstd.h:95: WARNING: static function definition might be better as static inline
> > include/linux/zstd.h:106: WARNING: static function definition might be better as static inline
> > 
> > A false positive exists when __must_check is used between
> > static and inline.  It's an unusual and IMO not a preferred use.
> 
> Maybe just filter and ignore such functions for now?

Not worth it.

> Will you send proper patch or do you want me to do it?

I'll do it eventually.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs
  2021-01-26  8:57           ` Joe Perches
@ 2021-01-26  9:26             ` Leon Romanovsky
  0 siblings, 0 replies; 16+ messages in thread
From: Leon Romanovsky @ 2021-01-26  9:26 UTC (permalink / raw)
  To: Joe Perches
  Cc: Jakub Kicinski, Bjorn Helgaas, Saeed Mahameed, Jason Gunthorpe,
	Alexander Duyck, linux-pci, linux-rdma, netdev, Don Dutile,
	Alex Williamson, David S . Miller

On Tue, Jan 26, 2021 at 12:57:06AM -0800, Joe Perches wrote:
> On Tue, 2021-01-26 at 10:48 +0200, Leon Romanovsky wrote:
> > On Tue, Jan 26, 2021 at 12:20:11AM -0800, Joe Perches wrote:
> > > On Tue, 2021-01-26 at 08:01 +0200, Leon Romanovsky wrote:
> > > > On Mon, Jan 25, 2021 at 01:52:29PM -0800, Jakub Kicinski wrote:
> > > > > On Sun, 24 Jan 2021 15:11:16 +0200 Leon Romanovsky wrote:
> > > > > > +static int pci_enable_vfs_overlay(struct pci_dev *dev) { return 0; }
> > > > > > +static void pci_disable_vfs_overlay(struct pci_dev *dev) {}
> []
> > > $ ./scripts/checkpatch.pl -f include/linux/*.h --types=static_inline --terse --nosummary
> > > include/linux/dma-mapping.h:203: WARNING: static function definition might be better as static inline
> > > include/linux/genl_magic_func.h:55: WARNING: static function definition might be better as static inline
> > > include/linux/genl_magic_func.h:78: WARNING: static function definition might be better as static inline
> > > include/linux/kernel.h:670: WARNING: static function definition might be better as static inline
> > > include/linux/kprobes.h:213: WARNING: static function definition might be better as static inline
> > > include/linux/kprobes.h:231: WARNING: static function definition might be better as static inline
> > > include/linux/kprobes.h:511: WARNING: static function definition might be better as static inline
> > > include/linux/skb_array.h:185: WARNING: static function definition might be better as static inline
> > > include/linux/slab.h:606: WARNING: static function definition might be better as static inline
> > > include/linux/stop_machine.h:62: WARNING: static function definition might be better as static inline
> > > include/linux/vmw_vmci_defs.h:850: WARNING: static function definition might be better as static inline
> > > include/linux/zstd.h:95: WARNING: static function definition might be better as static inline
> > > include/linux/zstd.h:106: WARNING: static function definition might be better as static inline
> > >
> > > A false positive exists when __must_check is used between
> > > static and inline.  It's an unusual and IMO not a preferred use.
> >
> > Maybe just filter and ignore such functions for now?
>
> Not worth it.
>
> > Will you send proper patch or do you want me to do it?
>
> I'll do it eventually.

Thanks

>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-01-26 20:00 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-24 13:11 [PATCH mlx5-next v4 0/4] Dynamically assign MSI-X vectors count Leon Romanovsky
2021-01-24 13:11 ` [PATCH mlx5-next v4 1/4] PCI: Add sysfs callback to allow MSI-X table size change of SR-IOV VFs Leon Romanovsky
2021-01-24 16:47   ` Alexander Duyck
2021-01-24 19:00     ` Leon Romanovsky
2021-01-25 18:47       ` Leon Romanovsky
2021-01-25 18:50         ` Alexander Duyck
2021-01-25 18:54           ` Leon Romanovsky
2021-01-25 21:52   ` Jakub Kicinski
2021-01-26  6:01     ` Leon Romanovsky
2021-01-26  8:20       ` Joe Perches
2021-01-26  8:48         ` Leon Romanovsky
2021-01-26  8:57           ` Joe Perches
2021-01-26  9:26             ` Leon Romanovsky
2021-01-24 13:11 ` [PATCH mlx5-next v4 2/4] net/mlx5: Add dynamic MSI-X capabilities bits Leon Romanovsky
2021-01-24 13:11 ` [PATCH mlx5-next v4 3/4] net/mlx5: Dynamically assign MSI-X vectors count Leon Romanovsky
2021-01-24 13:11 ` [PATCH mlx5-next v4 4/4] net/mlx5: Allow to the users to configure number of MSI-X vectors Leon Romanovsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).