[PATCH net-next 0/3] nfp: support VF multi-queues configuration

[PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Simon Horman]

Hi,

this short series adds the max_vf_queue generic devlink device parameter,
the intention of this is to allow configuration of the number of queues
associated with VFs, and facilitates having VFs with different queue
counts.

The series also adds support for multi-queue VFs to the nfp driver
and support for the max_vf_queue feature described above.

Diana Wang (1):
  nfp: support VF multi-queues configuration

Peng Zhang (2):
  devlink: Add new "max_vf_queue" generic device param
  nfp: devlink: add the devlink parameter "max_vf_queue" support

 .../networking/devlink/devlink-params.rst     |   5 +
 Documentation/networking/devlink/nfp.rst      |   2 +
 .../ethernet/netronome/nfp/devlink_param.c    | 114 ++++++++++++++++++
 drivers/net/ethernet/netronome/nfp/nfp_main.c |   6 +
 drivers/net/ethernet/netronome/nfp/nfp_main.h |  13 ++
 drivers/net/ethernet/netronome/nfp/nfp_net.h  |   1 +
 .../net/ethernet/netronome/nfp/nfp_net_main.c |   3 +
 .../ethernet/netronome/nfp/nfp_net_sriov.c    | 101 ++++++++++++++++
 .../ethernet/netronome/nfp/nfp_net_sriov.h    |   3 +
 include/net/devlink.h                         |   4 +
 net/core/devlink.c                            |   5 +
 11 files changed, 257 insertions(+)

-- 
2.30.2

[PATCH net-next 1/3] nfp: support VF multi-queues configuration

[Simon Horman]

From: Diana Wang <na.wang@corigine.com>

Add VF setting multi-queue feature.
It is to configure the max queue number for each VF,
user can still modify the queue number in use by
ethtool -l <intf>

The number set of configuring queues for every vf is
{16 8 4 2 1} and total number of configuring queues
is not allowed bigger than vf queues resource.

If quantity of created VF exceeds expectation, it will
check VF number validity based on the queues not used.
The condition is that quantity of the rest queues must
not smaller than redundant VFs' number. If it meets
the condition, it will set one queue per extra VF.

If not configured(default mode), the created VFs will
divide the total vf-queues equally and it rounds down
power of 2.

Signed-off-by: Diana Wang <na.wang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/nfp_main.c |   6 ++
 drivers/net/ethernet/netronome/nfp/nfp_main.h |  13 +++
 drivers/net/ethernet/netronome/nfp/nfp_net.h  |   1 +
 .../net/ethernet/netronome/nfp/nfp_net_main.c |   3 +
 .../ethernet/netronome/nfp/nfp_net_sriov.c    | 101 ++++++++++++++++++
 .../ethernet/netronome/nfp/nfp_net_sriov.h    |   3 +
 6 files changed, 127 insertions(+)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index e66e548919d4..f0e197067e08 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -29,6 +29,7 @@
 #include "nfp_app.h"
 #include "nfp_main.h"
 #include "nfp_net.h"
+#include "nfp_net_sriov.h"
 
 static const char nfp_driver_name[] = "nfp";
 
@@ -252,6 +253,10 @@ static int nfp_pcie_sriov_enable(struct pci_dev *pdev, int num_vfs)
 		return -EINVAL;
 	}
 
+	err = nfp_vf_queues_config(pf, num_vfs);
+	if (err)
+		return err;
+
 	err = pci_enable_sriov(pdev, num_vfs);
 	if (err) {
 		dev_warn(&pdev->dev, "Failed to enable PCI SR-IOV: %d\n", err);
@@ -847,6 +852,7 @@ static int nfp_pci_probe(struct pci_dev *pdev,
 	if (err)
 		goto err_fw_unload;
 
+	pf->default_config_vfs_queue = true;
 	pf->num_vfs = pci_num_vf(pdev);
 	if (pf->num_vfs > pf->limit_vfs) {
 		dev_err(&pdev->dev,
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
index afd3edfa2428..c24f990bcdbb 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
@@ -17,6 +17,12 @@
 #include <linux/workqueue.h>
 #include <net/devlink.h>
 
+ /* Define how many types of max-q-number is supported to
+  * configure, currently we support 16, 8, 4, 2, 1.
+  */
+#define NFP_NET_CFG_QUEUE_TYPE		5
+#define NFP_NET_CFG_MAX_Q(type)		(1 << (NFP_NET_CFG_QUEUE_TYPE - (type) - 1))
+
 struct dentry;
 struct device;
 struct pci_dev;
@@ -63,6 +69,10 @@ struct nfp_dumpspec {
  * @irq_entries:	Array of MSI-X entries for all vNICs
  * @limit_vfs:		Number of VFs supported by firmware (~0 for PCI limit)
  * @num_vfs:		Number of SR-IOV VFs enabled
+ * @max_vf_queues:	number of queues can be allocated to VFs
+ * @config_vfs_queue:	Array to indicate VF number of each max-queue-num type
+ *                      The quantity of distributable queues is {16, 8, 4, 2, 1}
+ * @default_config_vfs_queue:	Is the method of allocating queues to VFS evenly distributed
  * @fw_loaded:		Is the firmware loaded?
  * @unload_fw_on_remove:Do we need to unload firmware on driver removal?
  * @ctrl_vnic:		Pointer to the control vNIC if available
@@ -111,6 +121,9 @@ struct nfp_pf {
 
 	unsigned int limit_vfs;
 	unsigned int num_vfs;
+	unsigned int max_vf_queues;
+	u8 config_vfs_queue[NFP_NET_CFG_QUEUE_TYPE];
+	bool default_config_vfs_queue;
 
 	bool fw_loaded;
 	bool unload_fw_on_remove;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h
index a101ff30a1ae..5deeae87b684 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
@@ -78,6 +78,7 @@
 /* Queue/Ring definitions */
 #define NFP_NET_MAX_TX_RINGS	64	/* Max. # of Tx rings per device */
 #define NFP_NET_MAX_RX_RINGS	64	/* Max. # of Rx rings per device */
+#define NFP_NET_CTRL_RINGS	1	/* Max. # of Ctrl rings per device */
 #define NFP_NET_MAX_R_VECS	(NFP_NET_MAX_TX_RINGS > NFP_NET_MAX_RX_RINGS ? \
 				 NFP_NET_MAX_TX_RINGS : NFP_NET_MAX_RX_RINGS)
 #define NFP_NET_MAX_IRQS	(NFP_NET_NON_Q_VECTORS + NFP_NET_MAX_R_VECS)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index 3bae92dc899e..3c2e49813655 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -289,6 +289,7 @@ static int nfp_net_pf_init_vnics(struct nfp_pf *pf)
 		if (err)
 			goto err_prev_deinit;
 
+		pf->max_vf_queues -= nn->max_r_vecs;
 		id++;
 	}
 
@@ -754,6 +755,8 @@ int nfp_net_pci_probe(struct nfp_pf *pf)
 		}
 	}
 
+	pf->max_vf_queues = NFP_NET_MAX_R_VECS - NFP_NET_CTRL_RINGS;
+
 	err = nfp_net_pf_app_init(pf, qc_bar, stride);
 	if (err)
 		goto err_unmap;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
index 6eeeb0fda91f..eca6e65089f4 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
@@ -29,6 +29,9 @@ nfp_net_sriov_check(struct nfp_app *app, int vf, u16 cap, const char *msg, bool
 		return -EOPNOTSUPP;
 	}
 
+	if (cap == NFP_NET_VF_CFG_MB_CAP_QUEUE_CONFIG)
+		return 0;
+
 	if (vf < 0 || vf >= app->pf->num_vfs) {
 		if (warn)
 			nfp_warn(app->pf->cpp, "invalid VF id %d\n", vf);
@@ -309,3 +312,101 @@ int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 
 	return 0;
 }
+
+static int nfp_set_vf_queue_config(struct nfp_pf *pf, int num_vfs)
+{
+	unsigned char config_content[sizeof(u32)] = {0};
+	unsigned int i, j, k, cfg_vf_count, offset;
+	struct nfp_net *nn;
+	u32 raw;
+	int err;
+
+	raw = 0; k = 0; cfg_vf_count = 0;
+	offset = NFP_NET_VF_CFG_MB_SZ + pf->limit_vfs * NFP_NET_VF_CFG_SZ;
+
+	for (i = 0; i < NFP_NET_CFG_QUEUE_TYPE; i++) {
+		for (j = 0; j < pf->config_vfs_queue[i]; j++) {
+			config_content[k++] = NFP_NET_CFG_MAX_Q(i);
+			cfg_vf_count++;
+			if (k == sizeof(raw) || cfg_vf_count == num_vfs) {
+				raw = config_content[0] |
+				      (config_content[1] << BITS_PER_BYTE) |
+				      (config_content[2] << (2 * BITS_PER_BYTE)) |
+				      (config_content[3] << (3 * BITS_PER_BYTE));
+				writel(raw, pf->vfcfg_tbl2 + offset);
+				offset += sizeof(raw);
+				memset(config_content, 0, sizeof(u32));
+				k = 0;
+			}
+		}
+	}
+
+	writew(NFP_NET_VF_CFG_MB_UPD_QUEUE_CONFIG, pf->vfcfg_tbl2 + NFP_NET_VF_CFG_MB_UPD);
+
+	nn = list_first_entry(&pf->vnics, struct nfp_net, vnic_list);
+	err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_VF);
+	if (err) {
+		nfp_warn(pf->cpp,
+			 "FW reconfig VF config queue failed: %d\n", err);
+		return -EINVAL;
+	}
+
+	err = readw(pf->vfcfg_tbl2 + NFP_NET_VF_CFG_MB_RET);
+	if (err) {
+		nfp_warn(pf->cpp,
+			 "FW refused VF config queue update with errno: %d\n", err);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int nfp_vf_queues_config(struct nfp_pf *pf, int num_vfs)
+{
+	unsigned int i, j, cfg_num_queues = 0, cfg_num_vfs;
+
+	if (nfp_net_sriov_check(pf->app, 0, NFP_NET_VF_CFG_MB_CAP_QUEUE_CONFIG, "max_queue", true))
+		return 0;
+
+	/* In default mode, the created VFs divide all the VF queues equally,
+	 * and round down to power of 2
+	 */
+	if (pf->default_config_vfs_queue) {
+		memset(pf->config_vfs_queue, 0, NFP_NET_CFG_QUEUE_TYPE);
+		j = pf->max_vf_queues / num_vfs;
+		for (i = 0; i < NFP_NET_CFG_QUEUE_TYPE; i++) {
+			if (j >= NFP_NET_CFG_MAX_Q(i)) {
+				pf->config_vfs_queue[i] = num_vfs;
+				break;
+			}
+		}
+		return nfp_set_vf_queue_config(pf, num_vfs);
+	}
+
+	for (i = 0, cfg_num_vfs = 0; i < NFP_NET_CFG_QUEUE_TYPE; i++) {
+		cfg_num_queues += NFP_NET_CFG_MAX_Q(i) * pf->config_vfs_queue[i];
+		cfg_num_vfs += pf->config_vfs_queue[i];
+	}
+
+	if (cfg_num_queues > pf->max_vf_queues) {
+		dev_warn(&pf->pdev->dev,
+			 "Number of queues from configuration is bigger than total queues number.\n");
+		return -EINVAL;
+	}
+
+	cfg_num_queues = pf->max_vf_queues - cfg_num_queues;
+
+	if (num_vfs > cfg_num_vfs) {
+		cfg_num_vfs = num_vfs - cfg_num_vfs;
+		if (cfg_num_queues < cfg_num_vfs) {
+			dev_warn(&pf->pdev->dev,
+				 "Remaining queues are not enough to be allocated.\n");
+			return -EINVAL;
+		}
+		dev_info(&pf->pdev->dev,
+			 "The extra created VFs are allocated with single queue.\n");
+		pf->config_vfs_queue[NFP_NET_CFG_QUEUE_TYPE - 1] += cfg_num_vfs;
+	}
+
+	return nfp_set_vf_queue_config(pf, num_vfs);
+}
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
index 2d445fa199dc..36df29fdaf0e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
@@ -21,6 +21,7 @@
 #define   NFP_NET_VF_CFG_MB_CAP_TRUST			  (0x1 << 4)
 #define   NFP_NET_VF_CFG_MB_CAP_VLAN_PROTO		  (0x1 << 5)
 #define   NFP_NET_VF_CFG_MB_CAP_RATE			  (0x1 << 6)
+#define   NFP_NET_VF_CFG_MB_CAP_QUEUE_CONFIG		  (0x1 << 7)
 #define NFP_NET_VF_CFG_MB_RET				0x2
 #define NFP_NET_VF_CFG_MB_UPD				0x4
 #define   NFP_NET_VF_CFG_MB_UPD_MAC			  (0x1 << 0)
@@ -30,6 +31,7 @@
 #define   NFP_NET_VF_CFG_MB_UPD_TRUST			  (0x1 << 4)
 #define   NFP_NET_VF_CFG_MB_UPD_VLAN_PROTO		  (0x1 << 5)
 #define   NFP_NET_VF_CFG_MB_UPD_RATE			  (0x1 << 6)
+#define   NFP_NET_VF_CFG_MB_UPD_QUEUE_CONFIG		  (0x1 << 7)
 #define NFP_NET_VF_CFG_MB_VF_NUM			0x7
 
 /* VF config entry
@@ -67,5 +69,6 @@ int nfp_app_set_vf_link_state(struct net_device *netdev, int vf,
 			      int link_state);
 int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 			  struct ifla_vf_info *ivi);
+int nfp_vf_queues_config(struct nfp_pf *pf, int num_vfs);
 
 #endif /* _NFP_NET_SRIOV_H_ */
-- 
2.30.2

[PATCH net-next 2/3] devlink: Add new "max_vf_queue" generic device param

[Simon Horman]

From: Peng Zhang <peng.zhang@corigine.com>

VF max-queue-number is the MAX num of queues which the VF has.

Add new device generic parameter to configure the max-queue-number
of the each VF to be generated dynamically.

The string format is decided ad vendor specific. The suggested
format is ...-V-W-X-Y-Z, the V represents generating V VFs that
have 16 queues, the W represents generating W VFs that have 8
queues, and so on, the Z represents generating Z VFs that have
1 queue.

For example, to configure
* 1x VF with 128 queues
* 1x VF with 64 queues
* 0x VF with 32 queues
* 0x VF with 16 queues
* 12x VF with 8 queues
* 2x VF with 4 queues
* 2x VF with 2 queues
* 0x VF with 1 queue, execute:

$ devlink dev param set pci/0000:01:00.0 \
                          name max_vf_queue value \
                          "1-1-0-0-12-2-2-0" cmode runtime

When created VF number is bigger than that is configured by this
parameter, the extra VFs' max-queue-number is decided as vendor
specific.

If the config doesn't be set, the VFs' max-queue-number is decided
as vendor specific.

Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 Documentation/networking/devlink/devlink-params.rst | 5 +++++
 include/net/devlink.h                               | 4 ++++
 net/core/devlink.c                                  | 5 +++++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index 4e01dc32bc08..4b415b1acc9d 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -137,3 +137,8 @@ own name.
    * - ``event_eq_size``
      - u32
      - Control the size of asynchronous control events EQ.
+   * - ``max_vf_queue``
+     - String
+     - Configure the queue of the each VF to be generated dynamically. When
+       created VF number is bigger than that is configured by this parameter,
+       the extra VFs' max-queue-number is decided as vendor specific.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index ba6b8b094943..8cedc33c5992 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -498,6 +498,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_IWARP,
 	DEVLINK_PARAM_GENERIC_ID_IO_EQ_SIZE,
 	DEVLINK_PARAM_GENERIC_ID_EVENT_EQ_SIZE,
+	DEVLINK_PARAM_GENERIC_ID_MAX_VF_QUEUE,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -556,6 +557,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME "event_eq_size"
 #define DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE DEVLINK_PARAM_TYPE_U32
 
+#define DEVLINK_PARAM_GENERIC_MAX_VF_QUEUE_NAME "max_vf_queue"
+#define DEVLINK_PARAM_GENERIC_MAX_VF_QUEUE_TYPE DEVLINK_PARAM_TYPE_STRING
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 89baa7c0938b..c33f9040c570 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5152,6 +5152,11 @@ static const struct devlink_param devlink_param_generic[] = {
 		.name = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_NAME,
 		.type = DEVLINK_PARAM_GENERIC_EVENT_EQ_SIZE_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_MAX_VF_QUEUE,
+		.name = DEVLINK_PARAM_GENERIC_MAX_VF_QUEUE_NAME,
+		.type = DEVLINK_PARAM_GENERIC_MAX_VF_QUEUE_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
2.30.2

Re: [PATCH net-next 2/3] devlink: Add new "max_vf_queue" generic device param

[Gal Pressman]

On 19/10/2022 17:09, Simon Horman wrote:
> From: Peng Zhang <peng.zhang@corigine.com>
>
> VF max-queue-number is the MAX num of queues which the VF has.
>
> Add new device generic parameter to configure the max-queue-number
> of the each VF to be generated dynamically.
>
> The string format is decided ad vendor specific. The suggested
> format is ...-V-W-X-Y-Z, the V represents generating V VFs that
> have 16 queues, the W represents generating W VFs that have 8
> queues, and so on, the Z represents generating Z VFs that have
> 1 queue.

Having a vendor specific string contradicts the point of having a
generic parameter, why not do it as a vendor param, or generalize the
string?

>
> For example, to configure
> * 1x VF with 128 queues
> * 1x VF with 64 queues
> * 0x VF with 32 queues
> * 0x VF with 16 queues
> * 12x VF with 8 queues
> * 2x VF with 4 queues
> * 2x VF with 2 queues
> * 0x VF with 1 queue, execute:
>
> $ devlink dev param set pci/0000:01:00.0 \
>                           name max_vf_queue value \
>                           "1-1-0-0-12-2-2-0" cmode runtime
>
> When created VF number is bigger than that is configured by this
> parameter, the extra VFs' max-queue-number is decided as vendor
> specific.
>
> If the config doesn't be set, the VFs' max-queue-number is decided
> as vendor specific.
>
> Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>

Does this command have to be run before the VFs are created? What
happens to existing VFs?

Re: [PATCH net-next 2/3] devlink: Add new "max_vf_queue" generic device param

[Yinjun Zhang]

On Sun, Oct 23, 2022 at 02:28:24PM +0300, Gal Pressman wrote:
> [Some people who received this message don't often get email from gal@nvidia.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 19/10/2022 17:09, Simon Horman wrote:
> > From: Peng Zhang <peng.zhang@corigine.com>
> >
> > VF max-queue-number is the MAX num of queues which the VF has.
> >
> > Add new device generic parameter to configure the max-queue-number
> > of the each VF to be generated dynamically.
> >
> > The string format is decided ad vendor specific. The suggested
> > format is ...-V-W-X-Y-Z, the V represents generating V VFs that
> > have 16 queues, the W represents generating W VFs that have 8
> > queues, and so on, the Z represents generating Z VFs that have
> > 1 queue.
> 
> Having a vendor specific string contradicts the point of having a
> generic parameter, why not do it as a vendor param, or generalize the
> string?

As Jakub suggested, we'll try to utilize devlink resource API instead.

> 
> >
> > For example, to configure
> > * 1x VF with 128 queues
> > * 1x VF with 64 queues
> > * 0x VF with 32 queues
> > * 0x VF with 16 queues
> > * 12x VF with 8 queues
> > * 2x VF with 4 queues
> > * 2x VF with 2 queues
> > * 0x VF with 1 queue, execute:
> >
> > $ devlink dev param set pci/0000:01:00.0 \
> >                           name max_vf_queue value \
> >                           "1-1-0-0-12-2-2-0" cmode runtime
> >
> > When created VF number is bigger than that is configured by this
> > parameter, the extra VFs' max-queue-number is decided as vendor
> > specific.
> >
> > If the config doesn't be set, the VFs' max-queue-number is decided
> > as vendor specific.
> >
> > Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> >
> 
> Does this command have to be run before the VFs are created? What
> happens to existing VFs?

Yes in our current implementation, but it's up to the vendor's
implementation I think.

Re: [PATCH net-next 2/3] devlink: Add new "max_vf_queue" generic device param

[Leon Romanovsky]

On Mon, Oct 24, 2022 at 09:47:13AM +0800, Yinjun Zhang wrote:
> On Sun, Oct 23, 2022 at 02:28:24PM +0300, Gal Pressman wrote:
> > [Some people who received this message don't often get email from gal@nvidia.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > On 19/10/2022 17:09, Simon Horman wrote:
> > > From: Peng Zhang <peng.zhang@corigine.com>
> > >
> > > VF max-queue-number is the MAX num of queues which the VF has.
> > >
> > > Add new device generic parameter to configure the max-queue-number
> > > of the each VF to be generated dynamically.
> > >
> > > The string format is decided ad vendor specific. The suggested
> > > format is ...-V-W-X-Y-Z, the V represents generating V VFs that
> > > have 16 queues, the W represents generating W VFs that have 8
> > > queues, and so on, the Z represents generating Z VFs that have
> > > 1 queue.

<...>

> > 
> > Does this command have to be run before the VFs are created? What
> > happens to existing VFs?
> 
> Yes in our current implementation, but it's up to the vendor's
> implementation I think.

All vendors should give same look and feel for the users. It is very
frustrating to find that same command should be run in different point
of time just to perform same thing.

Thanks

[PATCH net-next 3/3] nfp: devlink: add the devlink parameter "max_vf_queue" support

[Simon Horman]

From: Peng Zhang <peng.zhang@corigine.com>

VF max-queue-number is the MAX num of queues which the VF has.

Currently, different users need that the number of VF max-queue-number
is different. Configuring the VF max-queue-number dynamically can
better match users. Hence, it is necessary to allow user to configure
the number of the VF max-queue-number.

The string format is V-W-X-Y-Z, the V represents generating
V VFs that have 16 queues, the W represents generating W VFs that
have 8 queues, and so on, the z represents  generating Z VF that
has 1 queues. As far, it supports the VF max-queue-number is 16.

For example, to configure
* 1x VF with 16 queue
* 1x VF with 8 queues
* 2x VF with 4 queues
* 2x VF with 2 queues
* 0x VF with 1 queue, execute:

$ devlink dev param set pci/0000:01:00.0 \
		   name max_vf_queue value "1-1-2-2-0" cmode runtime

The nfp also support the variable-length argument, Y-Z, X-Y-Z, W-X-Y-Z
and Z, it also is right format and represents that the VFs which aren't
configured are all zero.

For example, execute:
$ devlink dev param set pci/0000:01:00.0 \
                   name max_vf_queue value "1-1" cmode runtime

It represent configure the queue is as follows:
* 0x VF with 16 queue
* 0x VF with 8 queues
* 0x VF with 4 queues
* 1x VF with 2 queues
* 1x VF with 1 queue

When created VF number is bigger than that is configured by this parameter,
the extra VFs' max-queue-number is 1.

If the config doesn't be set, the nfp vf max-queue-number is 2^n which is
round_down the average of the total queue / VF nums. If setting the config
is "0-0-0-0-0", it also is as the default way to generate VF.

Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 Documentation/networking/devlink/nfp.rst      |   2 +
 .../ethernet/netronome/nfp/devlink_param.c    | 114 ++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/Documentation/networking/devlink/nfp.rst b/Documentation/networking/devlink/nfp.rst
index a1717db0dfcc..1936cee16dbe 100644
--- a/Documentation/networking/devlink/nfp.rst
+++ b/Documentation/networking/devlink/nfp.rst
@@ -18,6 +18,8 @@ Parameters
      - permanent
    * - ``reset_dev_on_drv_probe``
      - permanent
+   * - ``vf_max_queue``
+     - runtime
 
 Info versions
 =============
diff --git a/drivers/net/ethernet/netronome/nfp/devlink_param.c b/drivers/net/ethernet/netronome/nfp/devlink_param.c
index db297ee4d7ad..5856b45601f7 100644
--- a/drivers/net/ethernet/netronome/nfp/devlink_param.c
+++ b/drivers/net/ethernet/netronome/nfp/devlink_param.c
@@ -6,6 +6,7 @@
 #include "nfpcore/nfp.h"
 #include "nfpcore/nfp_nsp.h"
 #include "nfp_main.h"
+#include "nfp_net.h"
 
 /**
  * struct nfp_devlink_param_u8_arg - Devlink u8 parameter get/set arguments
@@ -191,7 +192,120 @@ nfp_devlink_param_u8_validate(struct devlink *devlink, u32 id,
 	return 0;
 }
 
+static int
+nfp_devlink_vq_config_get(struct devlink *devlink, u32 id,
+			  struct devlink_param_gset_ctx *ctx)
+{
+	struct nfp_pf *pf = devlink_priv(devlink);
+	char config[4 * NFP_NET_CFG_QUEUE_TYPE];
+	int i, len;
+
+	if (!pf)
+		return -ENODEV;
+
+	for (i = 0, len = 0; i < NFP_NET_CFG_QUEUE_TYPE; i++)
+		len += snprintf(config + len, sizeof(config), "%03d-", pf->config_vfs_queue[i]);
+	config[len - 1] = '\0';
+
+	strscpy(ctx->val.vstr, config, sizeof(ctx->val.vstr));
+
+	return 0;
+}
+
+static int
+nfp_devlink_vq_config_set(struct devlink *devlink, u32 id,
+			  struct devlink_param_gset_ctx *ctx)
+{
+	int config_vfs_queue[NFP_NET_CFG_QUEUE_TYPE];
+	char vq[__DEVLINK_PARAM_MAX_STRING_VALUE];
+	struct nfp_pf *pf = devlink_priv(devlink);
+	char *num_vf, *value;
+	u8 config = 0;
+	int i;
+
+	if (!pf)
+		return -ENODEV;
+
+	strscpy(vq, ctx->val.vstr, sizeof(vq));
+	value = vq;
+	memset(config_vfs_queue, 0, sizeof(config_vfs_queue));
+
+	num_vf = strsep(&value, "-");
+	while (num_vf) {
+		if (kstrtouint(num_vf, 10, &config_vfs_queue[config++]))
+			return -EINVAL;
+		num_vf = strsep(&value, "-");
+	}
+
+	pf->default_config_vfs_queue = true;
+	memset(pf->config_vfs_queue, 0, sizeof(pf->config_vfs_queue));
+
+	for (i = NFP_NET_CFG_QUEUE_TYPE - 1; i >= 0; i--) {
+		if (config >= 1) {
+			pf->config_vfs_queue[i] = config_vfs_queue[--config];
+			if (pf->config_vfs_queue[i] && pf->default_config_vfs_queue)
+				pf->default_config_vfs_queue = false;
+		}
+	}
+
+	return 0;
+}
+
+static int
+nfp_devlink_vq_config_validate(struct devlink *devlink, u32 id,
+			       union devlink_param_value val,
+			       struct netlink_ext_ack *extack)
+{
+	int config_vfs_queue[NFP_NET_CFG_QUEUE_TYPE];
+	char vq[__DEVLINK_PARAM_MAX_STRING_VALUE];
+	struct nfp_pf *pf = devlink_priv(devlink);
+	char *num_vf, *value;
+	u32 total_q_num = 0;
+	u32 config = 0;
+	int i;
+
+	if (!pf) {
+		NL_SET_ERR_MSG_MOD(extack, "Can't find the device.");
+		return -ENODEV;
+	}
+
+	strscpy(vq, val.vstr, sizeof(vq));
+	value = vq;
+	memset(config_vfs_queue, 0, sizeof(config_vfs_queue));
+
+	num_vf = strsep(&value, "-");
+	while (num_vf) {
+		if (kstrtouint(num_vf, 10, &config_vfs_queue[config++])) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "The input format is error, expected format: %d-%d-%d-%d-%d.");
+			return -EINVAL;
+		}
+
+		if (config > NFP_NET_CFG_QUEUE_TYPE) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "The config queue type is more than the max_cfg_queue_type.");
+			return -EINVAL;
+		}
+		num_vf = strsep(&value, "-");
+	}
+
+	for (i = NFP_NET_CFG_QUEUE_TYPE - 1; i >= 0 && config; i--)
+		total_q_num += config_vfs_queue[--config] * NFP_NET_CFG_MAX_Q(i);
+
+	if (total_q_num > pf->max_vf_queues) {
+		NL_SET_ERR_MSG_MOD(extack, "The set queue is more than the MAX queue.");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct devlink_param nfp_devlink_params[] = {
+	DEVLINK_PARAM_GENERIC(MAX_VF_QUEUE,
+			      BIT(DEVLINK_PARAM_CMODE_RUNTIME),
+			      nfp_devlink_vq_config_get,
+			      nfp_devlink_vq_config_set,
+			      nfp_devlink_vq_config_validate),
 	DEVLINK_PARAM_GENERIC(FW_LOAD_POLICY,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			      nfp_devlink_param_u8_get,
-- 
2.30.2

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Jakub Kicinski]

On Wed, 19 Oct 2022 16:09:40 +0200 Simon Horman wrote:
> this short series adds the max_vf_queue generic devlink device parameter,
> the intention of this is to allow configuration of the number of queues
> associated with VFs, and facilitates having VFs with different queue
> counts.
> 
> The series also adds support for multi-queue VFs to the nfp driver
> and support for the max_vf_queue feature described above.

I appreciate CCing a wider group this time, but my concerns about using
devlink params for resource allocation still stand. I don't remember
anyone refuting that.

https://lore.kernel.org/all/20220921063448.5b0dd32b@kernel.org/

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Yinjun Zhang]

On Wed, Oct 19, 2022 at 06:01:06PM -0700, Jakub Kicinski wrote:
> On Wed, 19 Oct 2022 16:09:40 +0200 Simon Horman wrote:
> > this short series adds the max_vf_queue generic devlink device parameter,
> > the intention of this is to allow configuration of the number of queues
> > associated with VFs, and facilitates having VFs with different queue
> > counts.
> > 
> > The series also adds support for multi-queue VFs to the nfp driver
> > and support for the max_vf_queue feature described above.
> 
> I appreciate CCing a wider group this time, but my concerns about using
> devlink params for resource allocation still stand. I don't remember
> anyone refuting that.
> 
> https://lore.kernel.org/all/20220921063448.5b0dd32b@kernel.org/

Sorry this part was neglected, we'll take a look into the resource APIs.
Thanks.

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Saeed Mahameed]

On 20 Oct 09:35, Yinjun Zhang wrote:
>On Wed, Oct 19, 2022 at 06:01:06PM -0700, Jakub Kicinski wrote:
>> On Wed, 19 Oct 2022 16:09:40 +0200 Simon Horman wrote:
>> > this short series adds the max_vf_queue generic devlink device parameter,
>> > the intention of this is to allow configuration of the number of queues
>> > associated with VFs, and facilitates having VFs with different queue
>> > counts.
>> >
>> > The series also adds support for multi-queue VFs to the nfp driver
>> > and support for the max_vf_queue feature described above.
>>
>> I appreciate CCing a wider group this time, but my concerns about using
>> devlink params for resource allocation still stand. I don't remember
>> anyone refuting that.
>>
>> https://lore.kernel.org/all/20220921063448.5b0dd32b@kernel.org/
>
>Sorry this part was neglected, we'll take a look into the resource APIs.
>Thanks.
>

The problem with this is that this should be a per function parameter,
devlink params or resources is not the right place for this as this
should be a configuration of a specific devlink object that is not the
parent device (namely devlink port function), otherwise we will have to
deal with ugly string parsing to address the specific vf attributes. 

let's use devlink port:
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-port.html

devlink ports have attributes and we should extend attributes to act like
devlink parameters.

   devlink port function set DEV/PORT_INDEX [ queue_count count ] ...

https://man7.org/linux/man-pages/man8/devlink-port.8.html

Alternatively you should also consider limiting vf msix, as we did in mlx5
https://patchwork.kernel.org/project/linux-rdma/cover/20210314124256.70253-1-leon@kernel.org/
  

RE: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Yinjun Zhang]

On Tue, 25 Oct 2022 08:51:41 +0100, Saeed Mahameed wrote:
> On 20 Oct 09:35, Yinjun Zhang wrote:
> >On Wed, Oct 19, 2022 at 06:01:06PM -0700, Jakub Kicinski wrote:
> >> On Wed, 19 Oct 2022 16:09:40 +0200 Simon Horman wrote:
> >> > this short series adds the max_vf_queue generic devlink device
> parameter,
> >> > the intention of this is to allow configuration of the number of queues
> >> > associated with VFs, and facilitates having VFs with different queue
> >> > counts.
> >> >
> >> > The series also adds support for multi-queue VFs to the nfp driver
> >> > and support for the max_vf_queue feature described above.
> >>
> >> I appreciate CCing a wider group this time, but my concerns about using
> >> devlink params for resource allocation still stand. I don't remember
> >> anyone refuting that.
> >>
> >> https://lore.kernel.org/all/20220921063448.5b0dd32b@kernel.org/
> >
> >Sorry this part was neglected, we'll take a look into the resource APIs.
> >Thanks.
> >
> 
> The problem with this is that this should be a per function parameter,
> devlink params or resources is not the right place for this as this
> should be a configuration of a specific devlink object that is not the
> parent device (namely devlink port function), otherwise we will have to
> deal with ugly string parsing to address the specific vf attributes.
> 
> let's use devlink port:
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-
> port.html
> 
> devlink ports have attributes and we should extend attributes to act like
> devlink parameters.
> 
>    devlink port function set DEV/PORT_INDEX [ queue_count count ] ...
> 
> https://man7.org/linux/man-pages/man8/devlink-port.8.html

Although the vf-max-queue is a per-VF property, it's configured from PF's 
perspective, so that the overall queue resource can be reallocated among VFs.
So a devlink object attached to the PF is used to configure, and resource seems
more appropriate than param.

> 
> Alternatively you should also consider limiting vf msix, as we did in mlx5
> https://patchwork.kernel.org/project/linux-
> rdma/cover/20210314124256.70253-1-leon@kernel.org/

Msix is not the limitation in our case.

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Saeed Mahameed]

On 25 Oct 10:41, Yinjun Zhang wrote:
>On Tue, 25 Oct 2022 08:51:41 +0100, Saeed Mahameed wrote:
>> On 20 Oct 09:35, Yinjun Zhang wrote:
>> >On Wed, Oct 19, 2022 at 06:01:06PM -0700, Jakub Kicinski wrote:
>> >> On Wed, 19 Oct 2022 16:09:40 +0200 Simon Horman wrote:
>> >> > this short series adds the max_vf_queue generic devlink device
>> parameter,
>> >> > the intention of this is to allow configuration of the number of queues
>> >> > associated with VFs, and facilitates having VFs with different queue
>> >> > counts.
>> >> >
>> >> > The series also adds support for multi-queue VFs to the nfp driver
>> >> > and support for the max_vf_queue feature described above.
>> >>
>> >> I appreciate CCing a wider group this time, but my concerns about using
>> >> devlink params for resource allocation still stand. I don't remember
>> >> anyone refuting that.
>> >>
>> >> https://lore.kernel.org/all/20220921063448.5b0dd32b@kernel.org/
>> >
>> >Sorry this part was neglected, we'll take a look into the resource APIs.
>> >Thanks.
>> >
>>
>> The problem with this is that this should be a per function parameter,
>> devlink params or resources is not the right place for this as this
>> should be a configuration of a specific devlink object that is not the
>> parent device (namely devlink port function), otherwise we will have to
>> deal with ugly string parsing to address the specific vf attributes.
>>
>> let's use devlink port:
>> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-
>> port.html
>>
>> devlink ports have attributes and we should extend attributes to act like
>> devlink parameters.
>>
>>    devlink port function set DEV/PORT_INDEX [ queue_count count ] ...
>>
>> https://man7.org/linux/man-pages/man8/devlink-port.8.html
>
>Although the vf-max-queue is a per-VF property, it's configured from PF's
>perspective, so that the overall queue resource can be reallocated among VFs.
>So a devlink object attached to the PF is used to configure, and resource seems
>more appropriate than param.
>

devlink port function is an object that's exposed on the PF. It will give
you a handle on the PF side to every sub-function (vf/sf) exposed via the
PF.

can you provide an example of how you imagine the reosurce vf-max-queue api
will look like ? 

RE: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Yinjun Zhang]

On Date: Tue, 25 Oct 2022 12:05:14 +0100, Saeed Mahameed wrote:
> On 25 Oct 10:41, Yinjun Zhang wrote:
> >On Tue, 25 Oct 2022 08:51:41 +0100, Saeed Mahameed wrote:
> >> The problem with this is that this should be a per function parameter,
> >> devlink params or resources is not the right place for this as this
> >> should be a configuration of a specific devlink object that is not the
> >> parent device (namely devlink port function), otherwise we will have to
> >> deal with ugly string parsing to address the specific vf attributes.
> >>
> >> let's use devlink port:
> >> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-
> >> port.html
> >>
> >> devlink ports have attributes and we should extend attributes to act like
> >> devlink parameters.
> >>
> >>    devlink port function set DEV/PORT_INDEX [ queue_count count ] ...
> >>
> >> https://man7.org/linux/man-pages/man8/devlink-port.8.html
> >
> >Although the vf-max-queue is a per-VF property, it's configured from PF's
> >perspective, so that the overall queue resource can be reallocated among
> VFs.
> >So a devlink object attached to the PF is used to configure, and resource
> seems
> >more appropriate than param.
> >
> 
> devlink port function is an object that's exposed on the PF. It will give
> you a handle on the PF side to every sub-function (vf/sf) exposed via the
> PF.

Sorry, I thought you meant each VF creates a devlink obj. So still one devlink obj
and each VF registers a devlink port, right? But the configuration is supposed to
be done before VFs are created, it maybe not appropriate to register ports before
relevant VFs are created I think.

> 
> can you provide an example of how you imagine the reosurce vf-max-queue
> api
> will look like ?

Two options, 
one is from VF's perspective, you need configure one by one, very straightforward:
```
pci/xxxx:xx:xx.x:
  name max_q size 128 unit entry
    resources:
      name VF0 size 1 unit entry size_min 1 size_max 128 size_gran 1
      name VF1 size 1 unit entry size_min 1 size_max 128 size_gran 1
      ...
```
another is from queue's perspective, several class is supported, not very flexible:
```
pci/xxxx:xx:xx.x:
  name max_q_class size 128 unit entry
    resources:
      # means how many VFs possess max-q-number of 16/8/..1 respectively
      name _16 size 0 unit entry size_min 0 size_max 128 size_gran 1
      name _8 size 0 unit entry size_min 0 size_max 128 size_gran 1
      ...
      name _1 size 0 unit entry size_min 0 size_max 128 size_gran 1
```

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Saeed Mahameed]

On 25 Oct 11:39, Yinjun Zhang wrote:
>On Date: Tue, 25 Oct 2022 12:05:14 +0100, Saeed Mahameed wrote:
>> On 25 Oct 10:41, Yinjun Zhang wrote:
>> >On Tue, 25 Oct 2022 08:51:41 +0100, Saeed Mahameed wrote:
>> >> The problem with this is that this should be a per function parameter,
>> >> devlink params or resources is not the right place for this as this
>> >> should be a configuration of a specific devlink object that is not the
>> >> parent device (namely devlink port function), otherwise we will have to
>> >> deal with ugly string parsing to address the specific vf attributes.
>> >>
>> >> let's use devlink port:
>> >> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-
>> >> port.html
>> >>
>> >> devlink ports have attributes and we should extend attributes to act like
>> >> devlink parameters.
>> >>
>> >>    devlink port function set DEV/PORT_INDEX [ queue_count count ] ...
>> >>
>> >> https://man7.org/linux/man-pages/man8/devlink-port.8.html
>> >
>> >Although the vf-max-queue is a per-VF property, it's configured from PF's
>> >perspective, so that the overall queue resource can be reallocated among
>> VFs.
>> >So a devlink object attached to the PF is used to configure, and resource
>> seems
>> >more appropriate than param.
>> >
>>
>> devlink port function is an object that's exposed on the PF. It will give
>> you a handle on the PF side to every sub-function (vf/sf) exposed via the
>> PF.
>
>Sorry, I thought you meant each VF creates a devlink obj. So still one devlink obj
>and each VF registers a devlink port, right? But the configuration is supposed to
>be done before VFs are created, it maybe not appropriate to register ports before
>relevant VFs are created I think.
>

Usually you create the VFs unbound, configure them and then bind them.
otherwise a query will have to query any possible VF which for some vendors
can be thousands ! it's better to work on created but not yet deployed vfs

>>
>> can you provide an example of how you imagine the reosurce vf-max-queue
>> api
>> will look like ?
>
>Two options,
>one is from VF's perspective, you need configure one by one, very straightforward:
>```
>pci/xxxx:xx:xx.x:
>  name max_q size 128 unit entry
>    resources:
>      name VF0 size 1 unit entry size_min 1 size_max 128 size_gran 1
>      name VF1 size 1 unit entry size_min 1 size_max 128 size_gran 1
>      ...

the above semantics are really weird, 
VF0 can't be a sub-resource of max_q ! 

sorry i can't think of a way where devlink resoruce semantics can work for
VF resource allocation.

Unless a VF becomes a resource and it's q_table becomes a sub resource of that
VF, which means you will have to register each vf as a resource individually.

Note that i called the resource "q_table" and not "max_queues",
since semantically max_queues is a parameter where q_table can be looked at
as a sub-resource of the VF, the q_table size decides the max_queues a VF
will accept, so there you go ! 
arghh weird.. just make it an attribute for devlink port function and name it
max_q as god intended it to be ;). Fix your FW to allow changing VF maxqueue for
unbound VFs if needed.


>```
>another is from queue's perspective, several class is supported, not very flexible:
>```
>pci/xxxx:xx:xx.x:
>  name max_q_class size 128 unit entry
>    resources:
>      # means how many VFs possess max-q-number of 16/8/..1 respectively
>      name _16 size 0 unit entry size_min 0 size_max 128 size_gran 1
>      name _8 size 0 unit entry size_min 0 size_max 128 size_gran 1
>      ...
>      name _1 size 0 unit entry size_min 0 size_max 128 size_gran 1
>```

weirder.

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Jakub Kicinski]

On Wed, 26 Oct 2022 15:22:21 +0100 Saeed Mahameed wrote:
> >Sorry, I thought you meant each VF creates a devlink obj. So still one devlink obj
> >and each VF registers a devlink port, right? But the configuration is supposed to
> >be done before VFs are created, it maybe not appropriate to register ports before
> >relevant VFs are created I think.
> 
> Usually you create the VFs unbound, configure them and then bind them.
> otherwise a query will have to query any possible VF which for some vendors
> can be thousands ! it's better to work on created but not yet deployed vfs

And the vendors who need to configure before spawning will do what,
exactly? Let's be practical.

> >> can you provide an example of how you imagine the reosurce vf-max-queue
> >> api
> >> will look like ?  
> >
> >Two options,
> >one is from VF's perspective, you need configure one by one, very straightforward:
> >```
> >pci/xxxx:xx:xx.x:
> >  name max_q size 128 unit entry
> >    resources:
> >      name VF0 size 1 unit entry size_min 1 size_max 128 size_gran 1
> >      name VF1 size 1 unit entry size_min 1 size_max 128 size_gran 1
> >      ...  
> 
> the above semantics are really weird, 
> VF0 can't be a sub-resource of max_q ! 
> 
> sorry i can't think of a way where devlink resoruce semantics can work for
> VF resource allocation.

It's just an API section. New forms of configuration can be added.
In fact they should so we can stop having this conversation.

> Unless a VF becomes a resource and it's q_table becomes a sub resource of that
> VF, which means you will have to register each vf as a resource individually.
> 
> Note that i called the resource "q_table" and not "max_queues",
> since semantically max_queues is a parameter where q_table can be looked at
> as a sub-resource of the VF, the q_table size decides the max_queues a VF
> will accept, so there you go ! 

Somewhere along the way you missed the other requirements to also allow
configuring guaranteed count that came from brcm as some point.

> arghh weird.. just make it an attribute for devlink port function and name it
> max_q as god intended it to be ;)

Let's not equate what fits the odd world of Melvidia FW with god.

> Fix your FW to allow changing VF maxqueue for unbound VFs if needed.

Not every device out there is all FW. Thankfully.

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Saeed Mahameed]

On 26 Oct 13:07, Jakub Kicinski wrote:
>On Wed, 26 Oct 2022 15:22:21 +0100 Saeed Mahameed wrote:
>> >Sorry, I thought you meant each VF creates a devlink obj. So still one devlink obj
>> >and each VF registers a devlink port, right? But the configuration is supposed to
>> >be done before VFs are created, it maybe not appropriate to register ports before
>> >relevant VFs are created I think.
>>
>> Usually you create the VFs unbound, configure them and then bind them.
>> otherwise a query will have to query any possible VF which for some vendors
>> can be thousands ! it's better to work on created but not yet deployed vfs
>
>And the vendors who need to configure before spawning will do what,
>exactly? Let's be practical.
>
>> >> can you provide an example of how you imagine the reosurce vf-max-queue
>> >> api
>> >> will look like ?
>> >
>> >Two options,
>> >one is from VF's perspective, you need configure one by one, very straightforward:
>> >```
>> >pci/xxxx:xx:xx.x:
>> >  name max_q size 128 unit entry
>> >    resources:
>> >      name VF0 size 1 unit entry size_min 1 size_max 128 size_gran 1
>> >      name VF1 size 1 unit entry size_min 1 size_max 128 size_gran 1
>> >      ...
>>
>> the above semantics are really weird,
>> VF0 can't be a sub-resource of max_q !
>>
>> sorry i can't think of a way where devlink resoruce semantics can work for
>> VF resource allocation.
>
>It's just an API section. New forms of configuration can be added.
>In fact they should so we can stop having this conversation.
>
>> Unless a VF becomes a resource and it's q_table becomes a sub resource of that
>> VF, which means you will have to register each vf as a resource individually.
>>
>> Note that i called the resource "q_table" and not "max_queues",
>> since semantically max_queues is a parameter where q_table can be looked at
>> as a sub-resource of the VF, the q_table size decides the max_queues a VF
>> will accept, so there you go !
>
>Somewhere along the way you missed the other requirements to also allow
>configuring guaranteed count that came from brcm as some point.
>

max_q and guarantee can be two separate attributes of a devlink port,
anyway see below before you answer, because i don't really care if it
devlink port or resource, but please don't allow the two suggested devlink
resource options to make it upstream as is, please, let's be practical and
correct.

>> arghh weird.. just make it an attribute for devlink port function and name it
>> max_q as god intended it to be ;)
>
>Let's not equate what fits the odd world of Melvidia FW with god.
>
>> Fix your FW to allow changing VF maxqueue for unbound VFs if needed.
>
>Not every device out there is all FW. Thankfully.

This has nothing to do with FW, all devices should be flexible enough to allow
configuring VFs after spawning.
anyway, my point is max_q isn't a resource period, you have to come-up with 
"q_table" concept. and sure, change the API to make it fit your needs, that's
acceptable when you want it to be acceptable, fine, but please don't allow
wrong semantics, I am interested in this feature for mlx5 as well, i don't care
if it's going to be resource or port, all I care about using the right
semantics, for that i suggested the devlink port option and the right semantics
for the devlink resource option, so i am being practical.

RE: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Yinjun Zhang]

On Wed, 26 Oct 2022 15:22:21 +0100, Saeed Mahameed wrote:
> On 25 Oct 11:39, Yinjun Zhang wrote:
> >On Date: Tue, 25 Oct 2022 12:05:14 +0100, Saeed Mahameed wrote:
> 
> Usually you create the VFs unbound, configure them and then bind them.
> otherwise a query will have to query any possible VF which for some vendors
> can be thousands ! it's better to work on created but not yet deployed vfs

Usually creating and binding are not separated, that's why `sriov_drivers_autoprobe`
is default true, unless some particular configuration requires it, like mlnx's msix
practice. 

> 
> >Two options,
> >one is from VF's perspective, you need configure one by one, very
> straightforward:
> >```
> >pci/xxxx:xx:xx.x:
> >  name max_q size 128 unit entry
> >    resources:
> >      name VF0 size 1 unit entry size_min 1 size_max 128 size_gran 1
> >      name VF1 size 1 unit entry size_min 1 size_max 128 size_gran 1
> >      ...
> 
> the above semantics are really weird,
> VF0 can't be a sub-resource of max_q !

Sorry, I admit the naming is not appropriate here. It should be "total_q_for_VF "
for parent resource, and "q_for_VFx" for the sub resources.

> 
> Note that i called the resource "q_table" and not "max_queues",
> since semantically max_queues is a parameter where q_table can be looked
> at
> as a sub-resource of the VF, the q_table size decides the max_queues a VF
> will accept, so there you go !

Queue itself is a kind of resource, why "q_table"? Just because the unit is entry?
I think we need introduce a new generic unit, so that its usage won't be limited.

> arghh weird.. just make it an attribute for devlink port function and name it
> max_q as god intended it to be ;). Fix your FW to allow changing VF
> maxqueue for
> unbound VFs if needed.
> 

It's not the FW constraints, the reason I don't prefer port way is it needs:
1. separate VF creating and binding, which needs extra operation
2. register extra ports for VFs
Both can be avoided when using resource way.

> 
> >```
> >another is from queue's perspective, several class is supported, not very
> flexible:
> >```
> >pci/xxxx:xx:xx.x:
> >  name max_q_class size 128 unit entry
> >    resources:
> >      # means how many VFs possess max-q-number of 16/8/..1 respectively
> >      name _16 size 0 unit entry size_min 0 size_max 128 size_gran 1
> >      name _8 size 0 unit entry size_min 0 size_max 128 size_gran 1
> >      ...
> >      name _1 size 0 unit entry size_min 0 size_max 128 size_gran 1
> >```
> 
> weirder.

Yes, kind of obscure. The intention is to avoid configuring one by one, especially
when there're thousands of VFs. Any better idea is welcomed.

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Leon Romanovsky]

On Thu, Oct 27, 2022 at 02:11:55AM +0000, Yinjun Zhang wrote:
> On Wed, 26 Oct 2022 15:22:21 +0100, Saeed Mahameed wrote:
> > On 25 Oct 11:39, Yinjun Zhang wrote:
> > >On Date: Tue, 25 Oct 2022 12:05:14 +0100, Saeed Mahameed wrote:
> > 
> > Usually you create the VFs unbound, configure them and then bind them.
> > otherwise a query will have to query any possible VF which for some vendors
> > can be thousands ! it's better to work on created but not yet deployed vfs
> 
> Usually creating and binding are not separated, that's why `sriov_drivers_autoprobe`
> is default true.

No, the situation is completely an opposite in a world which heavily uses SR-IOV.
Data centers which rely on SR-IOV to provide VMs to customers separate creation and
bind, as they two different stages in container/VM lifetime. Create is done when
physical server is booted and bind is done when user requested specific properties
of VM.

Various container orchestration frameworks do it for them.

> that's why `sriov_drivers_autoprobe` is default true.

It is not true either. The default value is chosen to keep kernel
backward compatible behavior. 

> unless some particular configuration requires it, like mlnx's msix
> practice. 

And it is not true either. I did MLNX implementation to be aligned with
PCI spec and in-kernel PCI subsystem implementation. Our device can change
MSI-X on-fly and from HW perspective unbind is not important.

Saeed is right "Usually you create the VFs unbound, configure them and
then bind them".

Thanks

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Leon Romanovsky]

On Thu, Oct 27, 2022 at 02:11:55AM +0000, Yinjun Zhang wrote:
> On Wed, 26 Oct 2022 15:22:21 +0100, Saeed Mahameed wrote:
> > On 25 Oct 11:39, Yinjun Zhang wrote:
> > >On Date: Tue, 25 Oct 2022 12:05:14 +0100, Saeed Mahameed wrote:

<...>

> > >```
> > >another is from queue's perspective, several class is supported, not very
> > flexible:
> > >```
> > >pci/xxxx:xx:xx.x:
> > >  name max_q_class size 128 unit entry
> > >    resources:
> > >      # means how many VFs possess max-q-number of 16/8/..1 respectively
> > >      name _16 size 0 unit entry size_min 0 size_max 128 size_gran 1
> > >      name _8 size 0 unit entry size_min 0 size_max 128 size_gran 1
> > >      ...
> > >      name _1 size 0 unit entry size_min 0 size_max 128 size_gran 1
> > >```
> > 
> > weirder.
> 
> Yes, kind of obscure. The intention is to avoid configuring one by one, especially
> when there're thousands of VFs. Any better idea is welcomed.

In parallel to netdev world, we extended netlink UAPI to receive ranges
and gave an option for users to write something like this: "cmd ... 1-100 ...",
which in kernel was translated to loop from 1 to 100.

Of course, we are talking about very specific fields (IDs) which can receive range.

It is just an idea how devlink can be extended to support batch configuration
of same values.

Thanks

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Saeed Mahameed]

On 27 Oct 02:11, Yinjun Zhang wrote:
>On Wed, 26 Oct 2022 15:22:21 +0100, Saeed Mahameed wrote:
>> On 25 Oct 11:39, Yinjun Zhang wrote:
>> >On Date: Tue, 25 Oct 2022 12:05:14 +0100, Saeed Mahameed wrote:
>>
>> Usually you create the VFs unbound, configure them and then bind them.
>> otherwise a query will have to query any possible VF which for some vendors
>> can be thousands ! it's better to work on created but not yet deployed vfs
>
>Usually creating and binding are not separated, that's why `sriov_drivers_autoprobe`
>is default true, unless some particular configuration requires it, like mlnx's msix
>practice.
>
>>
>> >Two options,
>> >one is from VF's perspective, you need configure one by one, very
>> straightforward:
>> >```
>> >pci/xxxx:xx:xx.x:
>> >  name max_q size 128 unit entry
>> >    resources:
>> >      name VF0 size 1 unit entry size_min 1 size_max 128 size_gran 1
>> >      name VF1 size 1 unit entry size_min 1 size_max 128 size_gran 1
>> >      ...
>>
>> the above semantics are really weird,
>> VF0 can't be a sub-resource of max_q !
>
>Sorry, I admit the naming is not appropriate here. It should be "total_q_for_VF "
>for parent resource, and "q_for_VFx" for the sub resources.
>
>>
>> Note that i called the resource "q_table" and not "max_queues",
>> since semantically max_queues is a parameter where q_table can be looked
>> at
>> as a sub-resource of the VF, the q_table size decides the max_queues a VF
>> will accept, so there you go !
>
>Queue itself is a kind of resource, why "q_table"? Just because the unit is entry?
>I think we need introduce a new generic unit, so that its usage won't be limited.
>
it's all abut semantics, yes q is a resource, but max_q is not.
if you want to go with q as a resource, then you will have to start
assigning individual queues to vfs one by one.. hence q_table per VF will
make it easier to control q table size per vf, with max size and guaranteed
size.
  
>> arghh weird.. just make it an attribute for devlink port function and name it
>> max_q as god intended it to be ;). Fix your FW to allow changing VF
>> maxqueue for
>> unbound VFs if needed.
>>
>
>It's not the FW constraints, the reason I don't prefer port way is it needs:
>1. separate VF creating and binding, which needs extra operation
>2. register extra ports for VFs
>Both can be avoided when using resource way.
>

Thanks, good to know it's not a FW/ASIC constraint, 
I am trying to push for one unified orchestration model for all VFs,SFs and the
upcoming intel's SIOV function.

create->configure->deploy. This aligns with all standard virtualization
orchestration modles, libvirt, kr8, etc .. 

Again i am worried we will have to support a config query for ALL possible
functions prior to creation.

Anyway i am flexible, I am ok with having a configure option prior to
creation as long as it doesn't create clutter, and user confusion, and it's
semantically correct.

we can also extend devlink port API to allow configure ops on "future"
ports and we can always extend the API to accept yaml file as an extension
of what Jakub suggested in LPC, to avoid one by one configurations.

RE: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Yinjun Zhang]

On Thu, 27 Oct 2022 09:46:54 +0100, Saeed Mahameed wrote:
<...>
> if you want to go with q as a resource, then you will have to start
> assigning individual queues to vfs one by one.. hence q_table per VF will
> make it easier to control q table size per vf, with max size and guaranteed
> size.

Excuse my foolishness, I still don't get your q_table. What I want is allocating
a certain amount of queues from a queue pool for different VFs, can you 
provide an example of q_table?

<...>
> 
> Thanks, good to know it's not a FW/ASIC constraint,
> I am trying to push for one unified orchestration model for all VFs,SFs and
> the
> upcoming intel's SIOV function.
> 
> create->configure->deploy. This aligns with all standard virtualization
> orchestration modles, libvirt, kr8, etc ..
> 
> Again i am worried we will have to support a config query for ALL possible
> functions prior to creation.
> 
> Anyway i am flexible, I am ok with having a configure option prior to
> creation as long as it doesn't create clutter, and user confusion, and it's
> semantically correct.

Thanks for your ok and thanks to Leon's explanation, I understand your
create->config->deploy proposal. But I have to say the resource way
doesn't break it, you can config it after creating, and it's not constrained
to it, you can config it before creating as well.

> 
> we can also extend devlink port API to allow configure ops on "future"
> ports and we can always extend the API to accept yaml file as an extension
> of what Jakub suggested in LPC, to avoid one by one configurations.
> 
> 

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Saeed Mahameed]

On 27 Oct 09:46, Yinjun Zhang wrote:
>On Thu, 27 Oct 2022 09:46:54 +0100, Saeed Mahameed wrote:
><...>
>> if you want to go with q as a resource, then you will have to start
>> assigning individual queues to vfs one by one.. hence q_table per VF will
>> make it easier to control q table size per vf, with max size and guaranteed
>> size.
>
>Excuse my foolishness, I still don't get your q_table. What I want is allocating
>a certain amount of queues from a queue pool for different VFs, can you
>provide an example of q_table?
>

queue pool and queue table are the same concept. Q as a resource is an
individual entity, so it can't be used as the abstract.
for simplicity you can just call the resource "queues" plural, maybe .. 

Also do we want to manage different queue types separately ?
Rx/Tx/Cq/EQ/command etc ?

how about the individual max sizes of these queues ? 

BTW do you plan to have further customization per VF? not in particular
resource oriented customization, more like capability control type of
configs, for example enabling/disabling crypto offloads per VF? admin
policies ? etc .. 
If so i would be happy if we collaborated on defining the APIs.

><...>
>>
>> Thanks, good to know it's not a FW/ASIC constraint,
>> I am trying to push for one unified orchestration model for all VFs,SFs and
>> the
>> upcoming intel's SIOV function.
>>
>> create->configure->deploy. This aligns with all standard virtualization
>> orchestration modles, libvirt, kr8, etc ..
>>
>> Again i am worried we will have to support a config query for ALL possible
>> functions prior to creation.
>>
>> Anyway i am flexible, I am ok with having a configure option prior to
>> creation as long as it doesn't create clutter, and user confusion, and it's
>> semantically correct.
>
>Thanks for your ok and thanks to Leon's explanation, I understand your
>create->config->deploy proposal. But I have to say the resource way
>doesn't break it, you can config it after creating, and it's not constrained
>to it, you can config it before creating as well.
>

Think about the poor admin who will need to have different config steps for
different port flavors.

Re: [PATCH net-next 0/3] nfp: support VF multi-queues configuration

[Yinjun Zhang]

On 2022/10/27 18:49, Saeed Mahameed wrote:
> On 27 Oct 09:46, Yinjun Zhang wrote:
>> On Thu, 27 Oct 2022 09:46:54 +0100, Saeed Mahameed wrote:
>> <...>
>>> if you want to go with q as a resource, then you will have to start
>>> assigning individual queues to vfs one by one.. hence q_table per VF 
>>> will
>>> make it easier to control q table size per vf, with max size and 
>>> guaranteed
>>> size.
>>
>> Excuse my foolishness, I still don't get your q_table. What I want is 
>> allocating
>> a certain amount of queues from a queue pool for different VFs, can you
>> provide an example of q_table?
>>
>
> queue pool and queue table are the same concept. Q as a resource is an
> individual entity, so it can't be used as the abstract.
> for simplicity you can just call the resource "queues" plural, maybe ..

I see, you're always standing on VF's point, so "queues" is just one of 
the VF's
properties, so that port way sounds better indeed. And I'm standing on
queues' point, and VFs happen to be its recipients.

> Also do we want to manage different queue types separately ?
> Rx/Tx/Cq/EQ/command etc ?

We didn't expect those variants in our case for now.

>
> how about the individual max sizes of these queues ?
> BTW do you plan to have further customization per VF? not in particular
> resource oriented customization, more like capability control type of
> configs, for example enabling/disabling crypto offloads per VF? admin
> policies ? etc .. If so i would be happy if we collaborated on 
> defining the APIs.

Not scheduled either.
I think it's better to introduce port param back, it's more flexible 
than port function
and can avoid frequent change to uAPIs.