[patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently, the user can instantiate new SF using "devlink port add"
command. That creates an E-switch representor devlink port.

When user activates this SF, there is an auxiliary device created and
probed for it which leads to SF devlink instance creation.

There is 1:1 relationship between E-switch representor devlink port and
the SF auxiliary device devlink instance.

Expose the relation to the user by introducing new netlink attribute
DEVLINK_PORT_FN_ATTR_DEVLINK which contains the devlink instance related
to devlink port function. This is done by patch #3.

Patch #4 implements this in mlx5 driver.

Patches #1 and #2 are just small dependencies.

Examples:
$ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 106
pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 106 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
$ devlink port function set pci/0000:08:00.0/32768 state active
$ devlink port show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 106 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state active opstate attached roce enable nested_devlink auxiliary/mlx5_core.sf.2

Jiri Pirko (4):
  net/mlx5: Disable eswitch as the first thing in mlx5_unload()
  net/mlx5: Lift reload limitation when SFs are present
  devlink: expose peer SF devlink instance
  net/mlx5: SF, Implement peer devlink set for SF representor devlink
    port

 .../net/ethernet/mellanox/mlx5/core/devlink.c | 11 ---
 .../net/ethernet/mellanox/mlx5/core/main.c    |  2 +-
 .../ethernet/mellanox/mlx5/core/sf/dev/dev.h  |  5 ++
 .../mellanox/mlx5/core/sf/dev/driver.c        | 14 ++++
 .../ethernet/mellanox/mlx5/core/sf/devlink.c  | 75 +++++++++++++++++++
 include/linux/mlx5/device.h                   |  1 +
 include/net/devlink.h                         |  4 +
 include/uapi/linux/devlink.h                  |  1 +
 net/devlink/leftover.c                        | 45 ++++++++++-
 9 files changed, 143 insertions(+), 15 deletions(-)

-- 
2.41.0

[patch net-next 1/4] net/mlx5: Disable eswitch as the first thing in mlx5_unload()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The eswitch disable call does removal of all representors. Do that
before clearing the SF device table and maintain the same flow as during
SF devlink port removal, where the representor is remove before
the actual SF is removed.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index f4fe06a5042e..4d36066e2f7a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1406,9 +1406,9 @@ static int mlx5_load(struct mlx5_core_dev *dev)
 
 static void mlx5_unload(struct mlx5_core_dev *dev)
 {
+	mlx5_eswitch_disable(dev->priv.eswitch);
 	mlx5_devlink_traps_unregister(priv_to_devlink(dev));
 	mlx5_sf_dev_table_destroy(dev);
-	mlx5_eswitch_disable(dev->priv.eswitch);
 	mlx5_sriov_detach(dev);
 	mlx5_lag_remove_mdev(dev);
 	mlx5_ec_cleanup(dev);
-- 
2.41.0

[patch net-next 2/4] net/mlx5: Lift reload limitation when SFs are present

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Historically, the shared devlink_mutex prevented devlink instances from
being registered/unregistered during another devlink instance reload
operation. However, devlink_muxex is gone for some time now, this
limitation is no longer needed. Lift it.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index 3d82ec890666..6eea65686c14 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
@@ -138,7 +138,6 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change,
 {
 	struct mlx5_core_dev *dev = devlink_priv(devlink);
 	struct pci_dev *pdev = dev->pdev;
-	bool sf_dev_allocated;
 	int ret = 0;
 
 	if (mlx5_dev_is_lightweight(dev)) {
@@ -148,16 +147,6 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change,
 		return 0;
 	}
 
-	sf_dev_allocated = mlx5_sf_dev_allocated(dev);
-	if (sf_dev_allocated) {
-		/* Reload results in deleting SF device which further results in
-		 * unregistering devlink instance while holding devlink_mutext.
-		 * Hence, do not support reload.
-		 */
-		NL_SET_ERR_MSG_MOD(extack, "reload is unsupported when SFs are allocated");
-		return -EOPNOTSUPP;
-	}
-
 	if (mlx5_lag_is_active(dev)) {
 		NL_SET_ERR_MSG_MOD(extack, "reload is unsupported in Lag mode");
 		return -EOPNOTSUPP;
-- 
2.41.0

[patch net-next 3/4] devlink: expose peer SF devlink instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Introduce a new helper devl_port_fn_devlink_set() to be used by driver
assigning a devlink instance to the peer devlink port function.

Expose this to user over new netlink attribute nested under port
function nest to expose devlink handle related to the port function.

This is particularly helpful for user to understand the relationship
between devlink instances created for SFs and the port functions
they belong to.

Note that caller of devlink_port_notify() needs to hold devlink
instance lock, put the assertion to devl_port_fn_devlink_set() to make
this requirement explicit. Also note the limitations that only allow to
make this assignment for registered objects.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h        |  4 ++++
 include/uapi/linux/devlink.h |  1 +
 net/devlink/leftover.c       | 45 +++++++++++++++++++++++++++++++++---
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index f7fec0791acc..49662d44471c 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -150,6 +150,8 @@ struct devlink_port {
 
 	struct devlink_rate *devlink_rate;
 	struct devlink_linecard *linecard;
+
+	struct devlink *fn_devlink; /* Peer function devlink instance */
 };
 
 struct devlink_port_new_attrs {
@@ -1667,6 +1669,8 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro
 void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port,
 				   u32 controller, u16 pf, u32 sf,
 				   bool external);
+void devl_port_fn_devlink_set(struct devlink_port *devlink_port,
+			      struct devlink *fn_devlink);
 struct devlink_rate *
 devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
 		      struct devlink_rate *parent);
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 3782d4219ac9..dd96086860ca 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -676,6 +676,7 @@ enum devlink_port_function_attr {
 	DEVLINK_PORT_FN_ATTR_STATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_OPSTATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_CAPS,	/* bitfield32 */
+	DEVLINK_PORT_FN_ATTR_DEVLINK,	/* nested */
 
 	__DEVLINK_PORT_FUNCTION_ATTR_MAX,
 	DEVLINK_PORT_FUNCTION_ATTR_MAX = __DEVLINK_PORT_FUNCTION_ATTR_MAX - 1
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 3883a90d32bb..89172c5e3eaa 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -591,11 +591,12 @@ devlink_region_snapshot_get_by_id(struct devlink_region *region, u32 id)
 	return NULL;
 }
 
-static int devlink_nl_put_nested_handle(struct sk_buff *msg, struct devlink *devlink)
+static int devlink_nl_put_nested_handle(struct sk_buff *msg, struct devlink *devlink,
+					int attrtype)
 {
 	struct nlattr *nested_attr;
 
-	nested_attr = nla_nest_start(msg, DEVLINK_ATTR_NESTED_DEVLINK);
+	nested_attr = nla_nest_start(msg, attrtype);
 	if (!nested_attr)
 		return -EMSGSIZE;
 	if (devlink_nl_put_handle(msg, devlink))
@@ -884,6 +885,15 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
 	if (err)
 		goto out;
 	err = devlink_port_fn_state_fill(port, msg, extack, &msg_updated);
+	if (err)
+		goto out;
+	if (port->fn_devlink) {
+		err = devlink_nl_put_nested_handle(msg, port->fn_devlink,
+						   DEVLINK_PORT_FN_ATTR_DEVLINK);
+		if (!err)
+			msg_updated = true;
+	}
+
 out:
 	if (err || !msg_updated)
 		nla_nest_cancel(msg, function_attr);
@@ -1785,7 +1795,8 @@ static int devlink_nl_linecard_fill(struct sk_buff *msg,
 	}
 
 	if (linecard->nested_devlink &&
-	    devlink_nl_put_nested_handle(msg, linecard->nested_devlink))
+	    devlink_nl_put_nested_handle(msg, linecard->nested_devlink,
+					 DEVLINK_ATTR_NESTED_DEVLINK))
 		goto nla_put_failure;
 
 	genlmsg_end(msg, hdr);
@@ -7133,6 +7144,34 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro
 }
 EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_sf_set);
 
+/**
+ * devl_port_fn_devlink_set - Attach/detach peer devlink
+ *			      instance to port function.
+ * @devlink_port: devlink port
+ * @fn_devlink: devlink instance to attach or NULL to detach
+ */
+void devl_port_fn_devlink_set(struct devlink_port *devlink_port,
+			      struct devlink *fn_devlink)
+{
+	lockdep_assert_held(&devlink_port->devlink->lock);
+	ASSERT_DEVLINK_PORT_REGISTERED(devlink_port);
+
+	if (fn_devlink)
+		ASSERT_DEVLINK_REGISTERED(fn_devlink);
+	else if (WARN_ON(!devlink_port->fn_devlink))
+		return;
+	else
+		ASSERT_DEVLINK_REGISTERED(devlink_port->fn_devlink);
+
+	if (WARN_ON(devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_PCI_SF ||
+		    devlink_port->attrs.pci_sf.external))
+		return;
+
+	devlink_port->fn_devlink = fn_devlink;
+	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
+}
+EXPORT_SYMBOL_GPL(devl_port_fn_devlink_set);
+
 /**
  * devl_rate_node_create - create devlink rate node
  * @devlink: devlink instance
-- 
2.41.0

[patch net-next 4/4] net/mlx5: SF, Implement peer devlink set for SF representor devlink port

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Benefit from the existence of internal mlx5 notifier and extend it by
event MLX5_DRIVER_EVENT_SF_PEER_DEVLINK. Use this event from SF
auxiliary device probe/remove functions to pass the registered SF
devlink instance to the SF representor.

Process the new event in SF representor code and call
devl_port_fn_devlink_set() to do the assignments. Implement this in work
to avoid possible deadlock when probe/remove function of SF may be
called with devlink instance lock held during devlink reload.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/sf/dev/dev.h  |  5 ++
 .../mellanox/mlx5/core/sf/dev/driver.c        | 14 ++++
 .../ethernet/mellanox/mlx5/core/sf/devlink.c  | 75 +++++++++++++++++++
 include/linux/mlx5/device.h                   |  1 +
 4 files changed, 95 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.h b/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.h
index 2a66a427ef15..c2e3277a418a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.h
@@ -19,6 +19,11 @@ struct mlx5_sf_dev {
 	u16 fn_id;
 };
 
+struct mlx5_sf_peer_devlink_event_info {
+	u16 fn_id;
+	struct devlink *devlink;
+};
+
 void mlx5_sf_dev_table_create(struct mlx5_core_dev *dev);
 void mlx5_sf_dev_table_destroy(struct mlx5_core_dev *dev);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c
index 8fe82f1191bb..7e45b338eb54 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c
@@ -8,6 +8,18 @@
 #include "dev.h"
 #include "devlink.h"
 
+static void mlx5_core_peer_devlink_set(struct mlx5_sf_dev *sf_dev, struct devlink *devlink)
+{
+	struct mlx5_sf_peer_devlink_event_info event_info = {
+		.fn_id = sf_dev->fn_id,
+		.devlink = devlink,
+	};
+
+	mlx5_blocking_notifier_call_chain(sf_dev->parent_mdev,
+					  MLX5_DRIVER_EVENT_SF_PEER_DEVLINK,
+					  &event_info);
+}
+
 static int mlx5_sf_dev_probe(struct auxiliary_device *adev, const struct auxiliary_device_id *id)
 {
 	struct mlx5_sf_dev *sf_dev = container_of(adev, struct mlx5_sf_dev, adev);
@@ -55,6 +67,7 @@ static int mlx5_sf_dev_probe(struct auxiliary_device *adev, const struct auxilia
 		goto init_one_err;
 	}
 	devlink_register(devlink);
+	mlx5_core_peer_devlink_set(sf_dev, devlink);
 	return 0;
 
 init_one_err:
@@ -72,6 +85,7 @@ static void mlx5_sf_dev_remove(struct auxiliary_device *adev)
 	struct devlink *devlink = priv_to_devlink(sf_dev->mdev);
 
 	mlx5_drain_health_wq(sf_dev->mdev);
+	mlx5_core_peer_devlink_set(sf_dev, NULL);
 	devlink_unregister(devlink);
 	if (mlx5_dev_is_lightweight(sf_dev->mdev))
 		mlx5_uninit_one_light(sf_dev->mdev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
index 6a3fa30b2bf2..06753032a9f1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
@@ -18,6 +18,10 @@ struct mlx5_sf {
 	u16 id;
 	u16 hw_fn_id;
 	u16 hw_state;
+	struct mlx5_core_dev *dev;
+	struct work_struct peer_devlink_set_work;
+	bool peer_devlink_set_work_disabled;
+	struct devlink *peer_devlink;
 };
 
 struct mlx5_sf_table {
@@ -28,6 +32,7 @@ struct mlx5_sf_table {
 	struct mutex sf_state_lock; /* Serializes sf state among user cmds & vhca event handler. */
 	struct notifier_block esw_nb;
 	struct notifier_block vhca_nb;
+	struct notifier_block mdev_nb;
 };
 
 static struct mlx5_sf *
@@ -59,6 +64,36 @@ static void mlx5_sf_id_erase(struct mlx5_sf_table *table, struct mlx5_sf *sf)
 	xa_erase(&table->port_indices, sf->port_index);
 }
 
+static void mlx5_sf_peer_devlink_set_work_flush(struct mlx5_sf *sf)
+{
+	flush_work(&sf->peer_devlink_set_work);
+}
+
+static void mlx5_sf_peer_devlink_set_work_queue(struct mlx5_sf *sf)
+{
+	if (sf->peer_devlink_set_work_disabled)
+		return;
+	mlx5_events_work_enqueue(sf->dev, &sf->peer_devlink_set_work);
+}
+
+static void mlx5_sf_peer_devlink_set_work_disable(struct mlx5_sf *sf)
+{
+	sf->peer_devlink_set_work_disabled = true;
+	cancel_work_sync(&sf->peer_devlink_set_work);
+}
+
+static void mlx5_sf_peer_devlink_set_work(struct work_struct *work)
+{
+	struct mlx5_sf *sf = container_of(work, struct mlx5_sf, peer_devlink_set_work);
+
+	if (!devl_trylock(sf->dl_port.devlink)) {
+		mlx5_sf_peer_devlink_set_work_queue(sf);
+		return;
+	}
+	devl_port_fn_devlink_set(&sf->dl_port, sf->peer_devlink);
+	devl_unlock(sf->dl_port.devlink);
+}
+
 static struct mlx5_sf *
 mlx5_sf_alloc(struct mlx5_sf_table *table, struct mlx5_eswitch *esw,
 	      u32 controller, u32 sfnum, struct netlink_ext_ack *extack)
@@ -93,6 +128,9 @@ mlx5_sf_alloc(struct mlx5_sf_table *table, struct mlx5_eswitch *esw,
 	sf->hw_state = MLX5_VHCA_STATE_ALLOCATED;
 	sf->controller = controller;
 
+	sf->dev = table->dev;
+	INIT_WORK(&sf->peer_devlink_set_work, &mlx5_sf_peer_devlink_set_work);
+
 	err = mlx5_sf_id_insert(table, sf);
 	if (err)
 		goto insert_err;
@@ -296,6 +334,7 @@ static int mlx5_sf_add(struct mlx5_core_dev *dev, struct mlx5_sf_table *table,
 						new_attr->controller, new_attr->sfnum);
 	if (err)
 		goto esw_err;
+
 	*dl_port = &sf->dl_port;
 	trace_mlx5_sf_add(dev, sf->port_index, sf->controller, sf->hw_fn_id, new_attr->sfnum);
 	return 0;
@@ -400,6 +439,7 @@ int mlx5_devlink_sf_port_del(struct devlink *devlink,
 		goto sf_err;
 	}
 
+	mlx5_sf_peer_devlink_set_work_disable(sf);
 	mlx5_esw_offloads_sf_vport_disable(esw, sf->hw_fn_id);
 	mlx5_sf_id_erase(table, sf);
 
@@ -472,6 +512,7 @@ static void mlx5_sf_deactivate_all(struct mlx5_sf_table *table)
 	 * arrive. It is safe to destroy all user created SFs.
 	 */
 	xa_for_each(&table->port_indices, index, sf) {
+		mlx5_sf_peer_devlink_set_work_disable(sf);
 		mlx5_esw_offloads_sf_vport_disable(esw, sf->hw_fn_id);
 		mlx5_sf_id_erase(table, sf);
 		mlx5_sf_dealloc(table, sf);
@@ -511,6 +552,36 @@ static int mlx5_sf_esw_event(struct notifier_block *nb, unsigned long event, voi
 	return 0;
 }
 
+static int mlx5_sf_mdev_event(struct notifier_block *nb, unsigned long event, void *data)
+{
+	struct mlx5_sf_table *table = container_of(nb, struct mlx5_sf_table, mdev_nb);
+	struct mlx5_sf_peer_devlink_event_info *event_info = data;
+	int ret = NOTIFY_DONE;
+	struct mlx5_sf *sf;
+
+	if (event != MLX5_DRIVER_EVENT_SF_PEER_DEVLINK)
+		return NOTIFY_DONE;
+
+	table = mlx5_sf_table_try_get(table->dev);
+	if (!table)
+		return NOTIFY_DONE;
+
+	mutex_lock(&table->sf_state_lock);
+	sf = mlx5_sf_lookup_by_function_id(table, event_info->fn_id);
+	if (!sf)
+		goto out;
+
+	mlx5_sf_peer_devlink_set_work_flush(sf);
+	sf->peer_devlink = event_info->devlink;
+	mlx5_sf_peer_devlink_set_work_queue(sf);
+
+	ret = NOTIFY_OK;
+out:
+	mutex_unlock(&table->sf_state_lock);
+	mlx5_sf_table_put(table);
+	return ret;
+}
+
 static bool mlx5_sf_table_supported(const struct mlx5_core_dev *dev)
 {
 	return dev->priv.eswitch && MLX5_ESWITCH_MANAGER(dev) &&
@@ -544,6 +615,9 @@ int mlx5_sf_table_init(struct mlx5_core_dev *dev)
 	if (err)
 		goto vhca_err;
 
+	table->mdev_nb.notifier_call = mlx5_sf_mdev_event;
+	mlx5_blocking_notifier_register(dev, &table->mdev_nb);
+
 	return 0;
 
 vhca_err:
@@ -562,6 +636,7 @@ void mlx5_sf_table_cleanup(struct mlx5_core_dev *dev)
 	if (!table)
 		return;
 
+	mlx5_blocking_notifier_unregister(dev, &table->mdev_nb);
 	mlx5_vhca_event_notifier_unregister(table->dev, &table->vhca_nb);
 	mlx5_esw_event_notifier_unregister(dev->priv.eswitch, &table->esw_nb);
 	WARN_ON(refcount_read(&table->refcount));
diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
index 80cc12a9a531..2473743f36b2 100644
--- a/include/linux/mlx5/device.h
+++ b/include/linux/mlx5/device.h
@@ -364,6 +364,7 @@ enum mlx5_event {
 enum mlx5_driver_event {
 	MLX5_DRIVER_EVENT_TYPE_TRAP = 0,
 	MLX5_DRIVER_EVENT_UPLINK_NETDEV,
+	MLX5_DRIVER_EVENT_SF_PEER_DEVLINK,
 };
 
 enum {
-- 
2.41.0

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jakub Kicinski]

On Tue, 15 Aug 2023 16:51:51 +0200 Jiri Pirko wrote:
> Currently, the user can instantiate new SF using "devlink port add"
> command. That creates an E-switch representor devlink port.
> 
> When user activates this SF, there is an auxiliary device created and
> probed for it which leads to SF devlink instance creation.
> 
> There is 1:1 relationship between E-switch representor devlink port and
> the SF auxiliary device devlink instance.
> 
> Expose the relation to the user by introducing new netlink attribute
> DEVLINK_PORT_FN_ATTR_DEVLINK which contains the devlink instance related
> to devlink port function. This is done by patch #3.

The devlink instance of the SF stays in the same network namespace 
as the PF?

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jiri Pirko]

Fri, Aug 18, 2023 at 04:34:20AM CEST, kuba@kernel.org wrote:
>On Tue, 15 Aug 2023 16:51:51 +0200 Jiri Pirko wrote:
>> Currently, the user can instantiate new SF using "devlink port add"
>> command. That creates an E-switch representor devlink port.
>> 
>> When user activates this SF, there is an auxiliary device created and
>> probed for it which leads to SF devlink instance creation.
>> 
>> There is 1:1 relationship between E-switch representor devlink port and
>> the SF auxiliary device devlink instance.
>> 
>> Expose the relation to the user by introducing new netlink attribute
>> DEVLINK_PORT_FN_ATTR_DEVLINK which contains the devlink instance related
>> to devlink port function. This is done by patch #3.
>
>The devlink instance of the SF stays in the same network namespace 
>as the PF?

SF devlink instance is created in init_ns and can move to another one.
So no.

I was thinking about this, as with the devlink handles we are kind of in
between sysfs and network. We have concept of network namespace in
devlink, but mainly because of the related netdevices.

There is no possibility of collision of devlink handles in between
separate namespaces, the handle is ns-unaware. Therefore the linkage to
instance in different ns is okay, I believe. Even more, It is handy as
the user knows that there exists such linkage.

What do you think?

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jakub Kicinski]

On Fri, 18 Aug 2023 09:30:17 +0200 Jiri Pirko wrote:
> >The devlink instance of the SF stays in the same network namespace 
> >as the PF?  
> 
> SF devlink instance is created in init_ns and can move to another one.
> So no.
> 
> I was thinking about this, as with the devlink handles we are kind of in
> between sysfs and network. We have concept of network namespace in
> devlink, but mainly because of the related netdevices.
> 
> There is no possibility of collision of devlink handles in between
> separate namespaces, the handle is ns-unaware. Therefore the linkage to
> instance in different ns is okay, I believe. Even more, It is handy as
> the user knows that there exists such linkage.
> 
> What do you think?

The way I was thinking about it is that the placement of the dl
instance should correspond to the entity which will be configuring it.

Assume a typical container setup where app has net admin in its
netns and there is an orchestration daemon with root in init_net 
which sets the containers up.

Will we ever want the app inside the netns to configure the interface
via the dl instance? Given that the SF is like giving the container
full access to the HW it seems to me that we should also delegate 
the devlink control to the app, i.e. move it to the netns?

Same thing for devlink instances of VFs.

The orchestration daemon has access to the "PF" / main dl instance of
the device, and to the ports / port fns so it has other ways to control
the HW. While the app would otherwise have no devlink access.

So my intuition is that the devlink instance should follow the SF
netdev into a namespace.

And then the next question is - once the devlink instances are in
different namespaces - do we still show the "nested_devlink" attribute?
Probably yes but we need to add netns id / link as well?

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jiri Pirko]

Fri, Aug 18, 2023 at 11:20:07PM CEST, kuba@kernel.org wrote:
>On Fri, 18 Aug 2023 09:30:17 +0200 Jiri Pirko wrote:
>> >The devlink instance of the SF stays in the same network namespace 
>> >as the PF?  
>> 
>> SF devlink instance is created in init_ns and can move to another one.
>> So no.
>> 
>> I was thinking about this, as with the devlink handles we are kind of in
>> between sysfs and network. We have concept of network namespace in
>> devlink, but mainly because of the related netdevices.
>> 
>> There is no possibility of collision of devlink handles in between
>> separate namespaces, the handle is ns-unaware. Therefore the linkage to
>> instance in different ns is okay, I believe. Even more, It is handy as
>> the user knows that there exists such linkage.
>> 
>> What do you think?
>

First of all, I'm having difficulties to understand exactly what you
say. I'll try my best with the reply :)


>The way I was thinking about it is that the placement of the dl
>instance should correspond to the entity which will be configuring it.
>
>Assume a typical container setup where app has net admin in its
>netns and there is an orchestration daemon with root in init_net 
>which sets the containers up.
>
>Will we ever want the app inside the netns to configure the interface
>via the dl instance? Given that the SF is like giving the container
>full access to the HW it seems to me that we should also delegate 

Nope. SF has limitations that could be set by devlink port function
caps. So no full HW access.


>the devlink control to the app, i.e. move it to the netns?
>
>Same thing for devlink instances of VFs.

Like VFs, SFs are getting probed by mlx5 driver. Both create the devlink
instances in init_ns. For both the user can reload them to a different
netns. It's consistent approach.

I see a possibility to provide user another ATTR to pass during SF
activation that would indicate the netns new instance is going to be
created in (of course only if it is local). That would provide
the flexibility to solve the case you are looking for I believe.
***

>
>The orchestration daemon has access to the "PF" / main dl instance of
>the device, and to the ports / port fns so it has other ways to control
>the HW. While the app would otherwise have no devlink access.
>
>So my intuition is that the devlink instance should follow the SF
>netdev into a namespace.

It works the other way around. The only way to change devlink netns is
to reload the instance to a different netns. The related
netdevice/netdevices are reinstantiated to that netns. If later on the
user decides to move a netdev to a different netns, he can do it.

This behavious is consistent for all devlink instances, devlink port and
related netdevice/netdevices, no matter if there is only one netdevice
of more. What you suggest, I can't see how that could work when instance
have multiple netdevices.


>
>And then the next question is - once the devlink instances are in
>different namespaces - do we still show the "nested_devlink" attribute?
>Probably yes but we need to add netns id / link as well?

Not sure what is the usecase. Currently, once VFs/SFs/ could be probed
and devlink instance created in init_ns, the orchestrator does not need
this info.

In future, if the extension I suggested above (***) would be
implemented, the orchestrator still knows the netns he asked the
instance to be created in.

So I would say is it not needed for anything. Plus it would make code
more complex making sure the notifications are coming in case of SF
devlink instance netns changes.

So do you see the usecase? If not, I would like to go with what I have
in this patchset version.

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jakub Kicinski]

On Mon, 21 Aug 2023 12:49:54 +0200 Jiri Pirko wrote:
> Fri, Aug 18, 2023 at 11:20:07PM CEST, kuba@kernel.org wrote:
> >On Fri, 18 Aug 2023 09:30:17 +0200 Jiri Pirko wrote:  
> >> SF devlink instance is created in init_ns and can move to another one.
> >> So no.
> >> 
> >> I was thinking about this, as with the devlink handles we are kind of in
> >> between sysfs and network. We have concept of network namespace in
> >> devlink, but mainly because of the related netdevices.
> >> 
> >> There is no possibility of collision of devlink handles in between
> >> separate namespaces, the handle is ns-unaware. Therefore the linkage to
> >> instance in different ns is okay, I believe. Even more, It is handy as
> >> the user knows that there exists such linkage.
> >> 
> >> What do you think?  
> 
> First of all, I'm having difficulties to understand exactly what you
> say. I'll try my best with the reply :)
> 
> >The way I was thinking about it is that the placement of the dl
> >instance should correspond to the entity which will be configuring it.
> >
> >Assume a typical container setup where app has net admin in its
> >netns and there is an orchestration daemon with root in init_net 
> >which sets the containers up.
> >
> >Will we ever want the app inside the netns to configure the interface
> >via the dl instance? Given that the SF is like giving the container
> >full access to the HW it seems to me that we should also delegate   
> 
> Nope. SF has limitations that could be set by devlink port function
> caps. So no full HW access.
> 
> 
> >the devlink control to the app, i.e. move it to the netns?
> >
> >Same thing for devlink instances of VFs.  
> 
> Like VFs, SFs are getting probed by mlx5 driver. Both create the devlink
> instances in init_ns. For both the user can reload them to a different
> netns. It's consistent approach.
> 
> I see a possibility to provide user another ATTR to pass during SF
> activation that would indicate the netns new instance is going to be
> created in (of course only if it is local). That would provide
> the flexibility to solve the case you are looking for I believe.
> ***
>
> >The orchestration daemon has access to the "PF" / main dl instance of
> >the device, and to the ports / port fns so it has other ways to control
> >the HW. While the app would otherwise have no devlink access.
> >
> >So my intuition is that the devlink instance should follow the SF
> >netdev into a namespace.  
> 
> It works the other way around. The only way to change devlink netns is
> to reload the instance to a different netns. The related
> netdevice/netdevices are reinstantiated to that netns. If later on the
> user decides to move a netdev to a different netns, he can do it.
> 
> This behavious is consistent for all devlink instances, devlink port and
> related netdevice/netdevices, no matter if there is only one netdevice
> of more. What you suggest, I can't see how that could work when instance
> have multiple netdevices.

Netdevs can move to netns without their devlink following (leaving
representors aside). We can't change that because uAPI.
But can we make it impossible to move SFs by themselves and require
devlink reload to move them?

> >And then the next question is - once the devlink instances are in
> >different namespaces - do we still show the "nested_devlink" attribute?
> >Probably yes but we need to add netns id / link as well?  
> 
> Not sure what is the usecase. Currently, once VFs/SFs/ could be probed
> and devlink instance created in init_ns, the orchestrator does not need
> this info.
> 
> In future, if the extension I suggested above (***) would be
> implemented, the orchestrator still knows the netns he asked the
> instance to be created in.
> 
> So I would say is it not needed for anything. Plus it would make code
> more complex making sure the notifications are coming in case of SF
> devlink instance netns changes.
> 
> So do you see the usecase? If not, I would like to go with what I have
> in this patchset version.

I'm thinking about containers. Since the SF configuration is currently
completely vendor ad-hoc I'm trying to establish who's supposed to be
in control of the devlink instance of an SF - orchestrator or the
workload. We should pick one and force everyone to fall in line.

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jiri Pirko]

Mon, Aug 21, 2023 at 10:19:37PM CEST, kuba@kernel.org wrote:
>On Mon, 21 Aug 2023 12:49:54 +0200 Jiri Pirko wrote:
>> Fri, Aug 18, 2023 at 11:20:07PM CEST, kuba@kernel.org wrote:
>> >On Fri, 18 Aug 2023 09:30:17 +0200 Jiri Pirko wrote:  
>> >> SF devlink instance is created in init_ns and can move to another one.
>> >> So no.
>> >> 
>> >> I was thinking about this, as with the devlink handles we are kind of in
>> >> between sysfs and network. We have concept of network namespace in
>> >> devlink, but mainly because of the related netdevices.
>> >> 
>> >> There is no possibility of collision of devlink handles in between
>> >> separate namespaces, the handle is ns-unaware. Therefore the linkage to
>> >> instance in different ns is okay, I believe. Even more, It is handy as
>> >> the user knows that there exists such linkage.
>> >> 
>> >> What do you think?  
>> 
>> First of all, I'm having difficulties to understand exactly what you
>> say. I'll try my best with the reply :)
>> 
>> >The way I was thinking about it is that the placement of the dl
>> >instance should correspond to the entity which will be configuring it.
>> >
>> >Assume a typical container setup where app has net admin in its
>> >netns and there is an orchestration daemon with root in init_net 
>> >which sets the containers up.
>> >
>> >Will we ever want the app inside the netns to configure the interface
>> >via the dl instance? Given that the SF is like giving the container
>> >full access to the HW it seems to me that we should also delegate   
>> 
>> Nope. SF has limitations that could be set by devlink port function
>> caps. So no full HW access.
>> 
>> 
>> >the devlink control to the app, i.e. move it to the netns?
>> >
>> >Same thing for devlink instances of VFs.  
>> 
>> Like VFs, SFs are getting probed by mlx5 driver. Both create the devlink
>> instances in init_ns. For both the user can reload them to a different
>> netns. It's consistent approach.
>> 
>> I see a possibility to provide user another ATTR to pass during SF
>> activation that would indicate the netns new instance is going to be
>> created in (of course only if it is local). That would provide
>> the flexibility to solve the case you are looking for I believe.
>> ***
>>
>> >The orchestration daemon has access to the "PF" / main dl instance of
>> >the device, and to the ports / port fns so it has other ways to control
>> >the HW. While the app would otherwise have no devlink access.
>> >
>> >So my intuition is that the devlink instance should follow the SF
>> >netdev into a namespace.  
>> 
>> It works the other way around. The only way to change devlink netns is
>> to reload the instance to a different netns. The related
>> netdevice/netdevices are reinstantiated to that netns. If later on the
>> user decides to move a netdev to a different netns, he can do it.
>> 
>> This behavious is consistent for all devlink instances, devlink port and
>> related netdevice/netdevices, no matter if there is only one netdevice
>> of more. What you suggest, I can't see how that could work when instance
>> have multiple netdevices.
>
>Netdevs can move to netns without their devlink following (leaving
>representors aside). We can't change that because uAPI.
>But can we make it impossible to move SFs by themselves and require
>devlink reload to move them?

That is how we currently implement SFs in mlx5. Example:
$ sudo devlink dev eswitch set pci/0000:08:00.0 mode switchdev
$ sudo devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 106
pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 106 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached roce enable
$ sudo devlink port function set pci/0000:08:00.0/32768 state active
$ devlink dev
pci/0000:08:00.0
pci/0000:08:00.1
auxiliary/mlx5_core.sf.2
$ sudo ip netns add ns1
$ sudo devlink dev reload auxiliary/mlx5_core.sf.2 netns ns1
$ devlink dev
pci/0000:08:00.0
pci/0000:08:00.1
$ sudo ip netns exec ns1 devlink dev
auxiliary/mlx5_core.sf.2


>
>> >And then the next question is - once the devlink instances are in
>> >different namespaces - do we still show the "nested_devlink" attribute?
>> >Probably yes but we need to add netns id / link as well?  
>> 
>> Not sure what is the usecase. Currently, once VFs/SFs/ could be probed
>> and devlink instance created in init_ns, the orchestrator does not need
>> this info.
>> 
>> In future, if the extension I suggested above (***) would be
>> implemented, the orchestrator still knows the netns he asked the
>> instance to be created in.
>> 
>> So I would say is it not needed for anything. Plus it would make code
>> more complex making sure the notifications are coming in case of SF
>> devlink instance netns changes.
>> 
>> So do you see the usecase? If not, I would like to go with what I have
>> in this patchset version.
>
>I'm thinking about containers. Since the SF configuration is currently
>completely vendor ad-hoc I'm trying to establish who's supposed to be
>in control of the devlink instance of an SF - orchestrator or the
>workload. We should pick one and force everyone to fall in line.

I think that both are valid. In the VF case, the workload (VM) owns the
devlink instance and netdev. In the SF case:
1) It could be the same. You can reload SF into netns, then
   the container has them both. That would provide the container
   more means (e.g. configuration of rdma,netdev,vdev etc).
2) Or, your can only put netdev into netns.

Both usecases are valid. But back to my question regarding to this
patchsets. Do you see the need to expose netns for nested port function
devlink instance? Even now, I still don't.

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jakub Kicinski]

On Tue, 22 Aug 2023 08:36:06 +0200 Jiri Pirko wrote:
> >I'm thinking about containers. Since the SF configuration is currently
> >completely vendor ad-hoc I'm trying to establish who's supposed to be
> >in control of the devlink instance of an SF - orchestrator or the
> >workload. We should pick one and force everyone to fall in line.  
> 
> I think that both are valid. In the VF case, the workload (VM) owns the
> devlink instance and netdev. In the SF case:
> 1) It could be the same. You can reload SF into netns, then
>    the container has them both. That would provide the container
>    more means (e.g. configuration of rdma,netdev,vdev etc).
> 2) Or, your can only put netdev into netns.

Okay, can you document that?

> Both usecases are valid. But back to my question regarding to this
> patchsets. Do you see the need to expose netns for nested port function
> devlink instance? Even now, I still don't.

It's not a huge deal but what's the problem with adding the netns id?
It's probably 50 LoC, trivial stuff.

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jiri Pirko]

Tue, Aug 22, 2023 at 05:28:33PM CEST, kuba@kernel.org wrote:
>On Tue, 22 Aug 2023 08:36:06 +0200 Jiri Pirko wrote:
>> >I'm thinking about containers. Since the SF configuration is currently
>> >completely vendor ad-hoc I'm trying to establish who's supposed to be
>> >in control of the devlink instance of an SF - orchestrator or the
>> >workload. We should pick one and force everyone to fall in line.  
>> 
>> I think that both are valid. In the VF case, the workload (VM) owns the
>> devlink instance and netdev. In the SF case:
>> 1) It could be the same. You can reload SF into netns, then
>>    the container has them both. That would provide the container
>>    more means (e.g. configuration of rdma,netdev,vdev etc).
>> 2) Or, your can only put netdev into netns.
>
>Okay, can you document that?

Okay. Will do that in a follow-up.


>
>> Both usecases are valid. But back to my question regarding to this
>> patchsets. Do you see the need to expose netns for nested port function
>> devlink instance? Even now, I still don't.
>
>It's not a huge deal but what's the problem with adding the netns id?
>It's probably 50 LoC, trivial stuff.

Well, you are right, okay.

Re: [patch net-next 0/4] net/mlx5: expose peer SF devlink instance

[Jiri Pirko]

Tue, Aug 22, 2023 at 05:28:33PM CEST, kuba@kernel.org wrote:
>On Tue, 22 Aug 2023 08:36:06 +0200 Jiri Pirko wrote:
>> >I'm thinking about containers. Since the SF configuration is currently
>> >completely vendor ad-hoc I'm trying to establish who's supposed to be
>> >in control of the devlink instance of an SF - orchestrator or the
>> >workload. We should pick one and force everyone to fall in line.  
>> 
>> I think that both are valid. In the VF case, the workload (VM) owns the
>> devlink instance and netdev. In the SF case:
>> 1) It could be the same. You can reload SF into netns, then
>>    the container has them both. That would provide the container
>>    more means (e.g. configuration of rdma,netdev,vdev etc).
>> 2) Or, your can only put netdev into netns.
>
>Okay, can you document that?
>
>> Both usecases are valid. But back to my question regarding to this
>> patchsets. Do you see the need to expose netns for nested port function
>> devlink instance? Even now, I still don't.
>
>It's not a huge deal but what's the problem with adding the netns id?
>It's probably 50 LoC, trivial stuff.

Not so trivial after all, with the locking and objects lifecycle
(port can disappear before nested instance). Uff.