* [RFC 0/5] devlink: add an explicit locking API
@ 2021-10-30 23:12 Jakub Kicinski
2021-10-30 23:12 ` [RFC 1/5] devlink: add unlocked APIs Jakub Kicinski
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-10-30 23:12 UTC (permalink / raw)
To: leon, idosch; +Cc: edwin.peer, jiri, netdev, Jakub Kicinski
This implements what I think is the right direction for devlink
API overhaul. It's an early RFC/PoC because the body of code is
rather large so I'd appreciate feedback early... The patches are
very roughly split, the point of the posting is primarily to prove
that from the driver side the conversion is an net improvement
in complexity.
IMO the problems with devlink locking are caused by two things:
(1) driver has no way to block devlink calls like it does in case
of netedev / rtnl_lock, note that for devlink each driver has
it's own lock so contention is not an issue;
(2) sometimes devlink calls into the driver without holding its lock
- for operations which may need the driver to call devlink (port
splitting, switch config, reload etc.), the circular dependency
is there, the driver can't solve this problem.
This set "fixes" the ordering by allowing the driver to participate
in locking. The lock order remains:
device_lock -> [devlink_mutex] -> devlink instance -> rtnl_lock
but now driver can take devlink lock itself, and _ALL_ devlink ops
are locked.
The expectation is that driver will take the devlink instance lock
on its probe and remove paths, hence protecting all configuration
paths with the devlink instance lock.
This is clearly demonstrated by the netdevsim conversion. All entry
points to driver config are protected by devlink instance lock, so
the driver switches to the "I'm already holding the devlink lock" API
when calling devlink. All driver locks and trickery is removed.
The part which is slightly more challanging is quiescing async entry
points which need to be closed on the devlink reload path (workqueue,
debugfs etc.) and which also take devlink instance lock. For that we
need to be able to take refs on the devlink instance and let them
clean up after themselves rather than waiting synchronously.
That last part is not 100% finished in this patch set - it works but
we need the driver to take devlink_mutex (the global lock) from its
probe/remove path. I hope this is good enough for an RFC, the problem
is easily solved by protecting the devlink XArray with something else
than devlink_mutex and/or not holding devlink_mutex around each op
(so that there is no ordering between the global and instance locks).
Speaking of not holding devlink_mutex around each op this patch set
also opens the path for parallel ops to different devlink instances
which is currently impossible because all user space calls take
devlink_mutex...
The patches are on top of the cleanups I posted earlier.
Jakub Kicinski (5):
devlink: add unlocked APIs
devlink: add API for explicit locking
devlink: allow locking of all ops
netdevsim: minor code move
netdevsim: use devlink locking
drivers/net/netdevsim/bus.c | 19 -
drivers/net/netdevsim/dev.c | 450 ++++++++-------
drivers/net/netdevsim/fib.c | 62 +--
drivers/net/netdevsim/netdevsim.h | 5 -
include/net/devlink.h | 88 +++
net/core/devlink.c | 875 +++++++++++++++++++++---------
6 files changed, 996 insertions(+), 503 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC 1/5] devlink: add unlocked APIs
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
@ 2021-10-30 23:12 ` Jakub Kicinski
2021-10-30 23:12 ` [RFC 2/5] devlink: add API for explicit locking Jakub Kicinski
` (5 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-10-30 23:12 UTC (permalink / raw)
To: leon, idosch; +Cc: edwin.peer, jiri, netdev, Jakub Kicinski
Much noise...
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/devlink.h | 60 ++++
net/core/devlink.c | 696 +++++++++++++++++++++++++++---------------
2 files changed, 514 insertions(+), 242 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index aab3d007c577..d8e4274e2af4 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1514,7 +1514,11 @@ void devlink_free(struct devlink *devlink);
int devlink_port_register(struct devlink *devlink,
struct devlink_port *devlink_port,
unsigned int port_index);
+int __devlink_port_register(struct devlink *devlink,
+ struct devlink_port *devlink_port,
+ unsigned int port_index);
void devlink_port_unregister(struct devlink_port *devlink_port);
+void __devlink_port_unregister(struct devlink_port *devlink_port);
void devlink_port_type_eth_set(struct devlink_port *devlink_port,
struct net_device *netdev);
void devlink_port_type_ib_set(struct devlink_port *devlink_port,
@@ -1530,8 +1534,11 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port,
u32 controller, u16 pf, u32 sf,
bool external);
int devlink_rate_leaf_create(struct devlink_port *port, void *priv);
+int __devlink_rate_leaf_create(struct devlink_port *port, void *priv);
void devlink_rate_leaf_destroy(struct devlink_port *devlink_port);
+void __devlink_rate_leaf_destroy(struct devlink_port *devlink_port);
void devlink_rate_nodes_destroy(struct devlink *devlink);
+void __devlink_rate_nodes_destroy(struct devlink *devlink);
int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
u32 size, u16 ingress_pools_count,
u16 egress_pools_count, u16 ingress_tc_count,
@@ -1567,11 +1574,23 @@ int devlink_resource_register(struct devlink *devlink,
u64 resource_id,
u64 parent_resource_id,
const struct devlink_resource_size_params *size_params);
+int __devlink_resource_register(struct devlink *devlink,
+ const char *resource_name,
+ u64 resource_size,
+ u64 resource_id,
+ u64 parent_resource_id,
+ const struct devlink_resource_size_params *size_params);
+
void devlink_resources_unregister(struct devlink *devlink,
struct devlink_resource *resource);
+void __devlink_resources_unregister(struct devlink *devlink,
+ struct devlink_resource *resource);
int devlink_resource_size_get(struct devlink *devlink,
u64 resource_id,
u64 *p_resource_size);
+int __devlink_resource_size_get(struct devlink *devlink,
+ u64 resource_id,
+ u64 *p_resource_size);
int devlink_dpipe_table_resource_set(struct devlink *devlink,
const char *table_name, u64 resource_id,
u64 resource_units);
@@ -1579,14 +1598,26 @@ void devlink_resource_occ_get_register(struct devlink *devlink,
u64 resource_id,
devlink_resource_occ_get_t *occ_get,
void *occ_get_priv);
+void __devlink_resource_occ_get_register(struct devlink *devlink,
+ u64 resource_id,
+ devlink_resource_occ_get_t *occ_get,
+ void *occ_get_priv);
void devlink_resource_occ_get_unregister(struct devlink *devlink,
u64 resource_id);
+void __devlink_resource_occ_get_unregister(struct devlink *devlink,
+ u64 resource_id);
int devlink_params_register(struct devlink *devlink,
const struct devlink_param *params,
size_t params_count);
+int __devlink_params_register(struct devlink *devlink,
+ const struct devlink_param *params,
+ size_t params_count);
void devlink_params_unregister(struct devlink *devlink,
const struct devlink_param *params,
size_t params_count);
+void __devlink_params_unregister(struct devlink *devlink,
+ const struct devlink_param *params,
+ size_t params_count);
int devlink_param_register(struct devlink *devlink,
const struct devlink_param *param);
void devlink_param_unregister(struct devlink *devlink,
@@ -1601,16 +1632,25 @@ devlink_region_create(struct devlink *devlink,
const struct devlink_region_ops *ops,
u32 region_max_snapshots, u64 region_size);
struct devlink_region *
+__devlink_region_create(struct devlink *devlink,
+ const struct devlink_region_ops *ops,
+ u32 region_max_snapshots, u64 region_size);
+struct devlink_region *
devlink_port_region_create(struct devlink_port *port,
const struct devlink_port_region_ops *ops,
u32 region_max_snapshots, u64 region_size);
void devlink_region_destroy(struct devlink_region *region);
+void __devlink_region_destroy(struct devlink_region *region);
void devlink_port_region_destroy(struct devlink_region *region);
int devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id);
+int __devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id);
void devlink_region_snapshot_id_put(struct devlink *devlink, u32 id);
+void __devlink_region_snapshot_id_put(struct devlink *devlink, u32 id);
int devlink_region_snapshot_create(struct devlink_region *region,
u8 *data, u32 snapshot_id);
+int __devlink_region_snapshot_create(struct devlink_region *region,
+ u8 *data, u32 snapshot_id);
int devlink_info_serial_number_put(struct devlink_info_req *req,
const char *sn);
int devlink_info_driver_name_put(struct devlink_info_req *req,
@@ -1702,9 +1742,15 @@ void devlink_flash_update_timeout_notify(struct devlink *devlink,
int devlink_traps_register(struct devlink *devlink,
const struct devlink_trap *traps,
size_t traps_count, void *priv);
+int __devlink_traps_register(struct devlink *devlink,
+ const struct devlink_trap *traps,
+ size_t traps_count, void *priv);
void devlink_traps_unregister(struct devlink *devlink,
const struct devlink_trap *traps,
size_t traps_count);
+void __devlink_traps_unregister(struct devlink *devlink,
+ const struct devlink_trap *traps,
+ size_t traps_count);
void devlink_trap_report(struct devlink *devlink, struct sk_buff *skb,
void *trap_ctx, struct devlink_port *in_devlink_port,
const struct flow_action_cookie *fa_cookie);
@@ -1712,17 +1758,31 @@ void *devlink_trap_ctx_priv(void *trap_ctx);
int devlink_trap_groups_register(struct devlink *devlink,
const struct devlink_trap_group *groups,
size_t groups_count);
+int __devlink_trap_groups_register(struct devlink *devlink,
+ const struct devlink_trap_group *groups,
+ size_t groups_count);
void devlink_trap_groups_unregister(struct devlink *devlink,
const struct devlink_trap_group *groups,
size_t groups_count);
+void __devlink_trap_groups_unregister(struct devlink *devlink,
+ const struct devlink_trap_group *groups,
+ size_t groups_count);
int
devlink_trap_policers_register(struct devlink *devlink,
const struct devlink_trap_policer *policers,
size_t policers_count);
+int
+__devlink_trap_policers_register(struct devlink *devlink,
+ const struct devlink_trap_policer *policers,
+ size_t policers_count);
void
devlink_trap_policers_unregister(struct devlink *devlink,
const struct devlink_trap_policer *policers,
size_t policers_count);
+void
+__devlink_trap_policers_unregister(struct devlink *devlink,
+ const struct devlink_trap_policer *policers,
+ size_t policers_count);
#if IS_ENABLED(CONFIG_NET_DEVLINK)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6b5ee862429e..9ea0c0bbc796 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5323,13 +5323,14 @@ static int __devlink_snapshot_id_insert(struct devlink *devlink, u32 id)
* avoid race conditions. The caller must release its hold on the
* snapshot by using devlink_region_snapshot_id_put.
*/
-static int __devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
+int __devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
{
lockdep_assert_held(&devlink->lock);
return xa_alloc(&devlink->snapshot_ids, id, xa_mk_value(1),
xa_limit_32b, GFP_KERNEL);
}
+EXPORT_SYMBOL_GPL(__devlink_region_snapshot_id_get);
/**
* __devlink_region_snapshot_create - create a new snapshot
@@ -5345,9 +5346,8 @@ static int __devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
* @data: snapshot data
* @snapshot_id: snapshot id to be created
*/
-static int
-__devlink_region_snapshot_create(struct devlink_region *region,
- u8 *data, u32 snapshot_id)
+int __devlink_region_snapshot_create(struct devlink_region *region,
+ u8 *data, u32 snapshot_id)
{
struct devlink *devlink = region->devlink;
struct devlink_snapshot *snapshot;
@@ -5385,6 +5385,7 @@ __devlink_region_snapshot_create(struct devlink_region *region,
kfree(snapshot);
return err;
}
+EXPORT_SYMBOL_GPL(__devlink_region_snapshot_create);
static void devlink_region_snapshot_del(struct devlink_region *region,
struct devlink_snapshot *snapshot)
@@ -9204,6 +9205,32 @@ static void devlink_port_type_warn_cancel(struct devlink_port *devlink_port)
cancel_delayed_work_sync(&devlink_port->type_warn_dw);
}
+int __devlink_port_register(struct devlink *devlink,
+ struct devlink_port *devlink_port,
+ unsigned int port_index)
+{
+ lockdep_assert_held(&devlink->lock);
+
+ if (devlink_port_index_exists(devlink, port_index))
+ return -EEXIST;
+
+ WARN_ON(devlink_port->devlink);
+ devlink_port->devlink = devlink;
+ devlink_port->index = port_index;
+ spin_lock_init(&devlink_port->type_lock);
+ INIT_LIST_HEAD(&devlink_port->reporter_list);
+ mutex_init(&devlink_port->reporters_lock);
+ list_add_tail(&devlink_port->list, &devlink->port_list);
+ INIT_LIST_HEAD(&devlink_port->param_list);
+ INIT_LIST_HEAD(&devlink_port->region_list);
+
+ INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
+ devlink_port_type_warn_schedule(devlink_port);
+ devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(__devlink_port_register);
+
/**
* devlink_port_register - Register devlink port
*
@@ -9221,29 +9248,28 @@ int devlink_port_register(struct devlink *devlink,
struct devlink_port *devlink_port,
unsigned int port_index)
{
- mutex_lock(&devlink->lock);
- if (devlink_port_index_exists(devlink, port_index)) {
- mutex_unlock(&devlink->lock);
- return -EEXIST;
- }
+ int err;
- WARN_ON(devlink_port->devlink);
- devlink_port->devlink = devlink;
- devlink_port->index = port_index;
- spin_lock_init(&devlink_port->type_lock);
- INIT_LIST_HEAD(&devlink_port->reporter_list);
- mutex_init(&devlink_port->reporters_lock);
- list_add_tail(&devlink_port->list, &devlink->port_list);
- INIT_LIST_HEAD(&devlink_port->param_list);
- INIT_LIST_HEAD(&devlink_port->region_list);
+ mutex_lock(&devlink->lock);
+ err = __devlink_port_register(devlink, devlink_port, port_index);
mutex_unlock(&devlink->lock);
- INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
- devlink_port_type_warn_schedule(devlink_port);
- devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
- return 0;
+ return err;
}
EXPORT_SYMBOL_GPL(devlink_port_register);
+void __devlink_port_unregister(struct devlink_port *devlink_port)
+{
+ lockdep_assert_held(&devlink_port->devlink->lock);
+
+ devlink_port_type_warn_cancel(devlink_port);
+ devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
+ list_del(&devlink_port->list);
+ WARN_ON(!list_empty(&devlink_port->reporter_list));
+ WARN_ON(!list_empty(&devlink_port->region_list));
+ mutex_destroy(&devlink_port->reporters_lock);
+}
+EXPORT_SYMBOL_GPL(__devlink_port_unregister);
+
/**
* devlink_port_unregister - Unregister devlink port
*
@@ -9253,14 +9279,9 @@ void devlink_port_unregister(struct devlink_port *devlink_port)
{
struct devlink *devlink = devlink_port->devlink;
- devlink_port_type_warn_cancel(devlink_port);
- devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
mutex_lock(&devlink->lock);
- list_del(&devlink_port->list);
+ __devlink_port_unregister(devlink_port);
mutex_unlock(&devlink->lock);
- WARN_ON(!list_empty(&devlink_port->reporter_list));
- WARN_ON(!list_empty(&devlink_port->region_list));
- mutex_destroy(&devlink_port->reporters_lock);
}
EXPORT_SYMBOL_GPL(devlink_port_unregister);
@@ -9480,30 +9501,17 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro
}
EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_sf_set);
-/**
- * devlink_rate_leaf_create - create devlink rate leaf
- *
- * @devlink_port: devlink port object to create rate object on
- * @priv: driver private data
- *
- * Create devlink rate object of type leaf on provided @devlink_port.
- * Throws call trace if @devlink_port already has a devlink rate object.
- *
- * Context: Takes and release devlink->lock <mutex>.
- *
- * Return: -ENOMEM if failed to allocate rate object, 0 otherwise.
- */
-int
-devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
+int __devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
{
struct devlink *devlink = devlink_port->devlink;
struct devlink_rate *devlink_rate;
+ lockdep_assert_held(&devlink->lock);
+
devlink_rate = kzalloc(sizeof(*devlink_rate), GFP_KERNEL);
if (!devlink_rate)
return -ENOMEM;
- mutex_lock(&devlink->lock);
WARN_ON(devlink_port->devlink_rate);
devlink_rate->type = DEVLINK_RATE_TYPE_LEAF;
devlink_rate->devlink = devlink;
@@ -9512,54 +9520,79 @@ devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
list_add_tail(&devlink_rate->list, &devlink->rate_list);
devlink_port->devlink_rate = devlink_rate;
devlink_rate_notify(devlink_rate, DEVLINK_CMD_RATE_NEW);
- mutex_unlock(&devlink->lock);
return 0;
}
-EXPORT_SYMBOL_GPL(devlink_rate_leaf_create);
+EXPORT_SYMBOL_GPL(__devlink_rate_leaf_create);
/**
- * devlink_rate_leaf_destroy - destroy devlink rate leaf
+ * devlink_rate_leaf_create - create devlink rate leaf
*
- * @devlink_port: devlink port linked to the rate object
+ * @devlink_port: devlink port object to create rate object on
+ * @priv: driver private data
+ *
+ * Create devlink rate object of type leaf on provided @devlink_port.
+ * Throws call trace if @devlink_port already has a devlink rate object.
*
* Context: Takes and release devlink->lock <mutex>.
+ *
+ * Return: -ENOMEM if failed to allocate rate object, 0 otherwise.
*/
-void devlink_rate_leaf_destroy(struct devlink_port *devlink_port)
+int
+devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
{
- struct devlink_rate *devlink_rate = devlink_port->devlink_rate;
struct devlink *devlink = devlink_port->devlink;
+ int err;
+
+ mutex_lock(&devlink->lock);
+ err = __devlink_rate_leaf_create(devlink_port, priv);
+ mutex_unlock(&devlink->lock);
+ return err;
+}
+EXPORT_SYMBOL_GPL(devlink_rate_leaf_create);
+
+void __devlink_rate_leaf_destroy(struct devlink_port *devlink_port)
+{
+ struct devlink_rate *devlink_rate = devlink_port->devlink_rate;
+
+ lockdep_assert_held(&devlink_port->devlink->lock);
if (!devlink_rate)
return;
- mutex_lock(&devlink->lock);
devlink_rate_notify(devlink_rate, DEVLINK_CMD_RATE_DEL);
if (devlink_rate->parent)
refcount_dec(&devlink_rate->parent->refcnt);
list_del(&devlink_rate->list);
devlink_port->devlink_rate = NULL;
- mutex_unlock(&devlink->lock);
kfree(devlink_rate);
}
-EXPORT_SYMBOL_GPL(devlink_rate_leaf_destroy);
+EXPORT_SYMBOL_GPL(__devlink_rate_leaf_destroy);
/**
- * devlink_rate_nodes_destroy - destroy all devlink rate nodes on device
- *
- * @devlink: devlink instance
+ * devlink_rate_leaf_destroy - destroy devlink rate leaf
*
- * Unset parent for all rate objects and destroy all rate nodes
- * on specified device.
+ * @devlink_port: devlink port linked to the rate object
*
* Context: Takes and release devlink->lock <mutex>.
*/
-void devlink_rate_nodes_destroy(struct devlink *devlink)
+void devlink_rate_leaf_destroy(struct devlink_port *devlink_port)
+{
+ struct devlink *devlink = devlink_port->devlink;
+
+ mutex_lock(&devlink->lock);
+ __devlink_rate_leaf_destroy(devlink_port);
+ mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_rate_leaf_destroy);
+
+void __devlink_rate_nodes_destroy(struct devlink *devlink)
{
static struct devlink_rate *devlink_rate, *tmp;
const struct devlink_ops *ops = devlink->ops;
- mutex_lock(&devlink->lock);
+ lockdep_assert_held(&devlink->lock);
+
list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
if (!devlink_rate->parent)
continue;
@@ -9580,6 +9613,23 @@ void devlink_rate_nodes_destroy(struct devlink *devlink)
kfree(devlink_rate);
}
}
+}
+EXPORT_SYMBOL_GPL(__devlink_rate_nodes_destroy);
+
+/**
+ * devlink_rate_nodes_destroy - destroy all devlink rate nodes on device
+ *
+ * @devlink: devlink instance
+ *
+ * Unset parent for all rate objects and destroy all rate nodes
+ * on specified device.
+ *
+ * Context: Takes and release devlink->lock <mutex>.
+ */
+void devlink_rate_nodes_destroy(struct devlink *devlink)
+{
+ mutex_lock(&devlink->lock);
+ __devlink_rate_nodes_destroy(devlink);
mutex_unlock(&devlink->lock);
}
EXPORT_SYMBOL_GPL(devlink_rate_nodes_destroy);
@@ -9830,46 +9880,28 @@ void devlink_dpipe_table_unregister(struct devlink *devlink,
}
EXPORT_SYMBOL_GPL(devlink_dpipe_table_unregister);
-/**
- * devlink_resource_register - devlink resource register
- *
- * @devlink: devlink
- * @resource_name: resource's name
- * @resource_size: resource's size
- * @resource_id: resource's id
- * @parent_resource_id: resource's parent id
- * @size_params: size parameters
- *
- * Generic resources should reuse the same names across drivers.
- * Please see the generic resources list at:
- * Documentation/networking/devlink/devlink-resource.rst
- */
-int devlink_resource_register(struct devlink *devlink,
- const char *resource_name,
- u64 resource_size,
- u64 resource_id,
- u64 parent_resource_id,
- const struct devlink_resource_size_params *size_params)
+int __devlink_resource_register(struct devlink *devlink,
+ const char *resource_name,
+ u64 resource_size,
+ u64 resource_id,
+ u64 parent_resource_id,
+ const struct devlink_resource_size_params *size_params)
{
struct devlink_resource *resource;
struct list_head *resource_list;
bool top_hierarchy;
- int err = 0;
+
+ lockdep_assert_held(&devlink->lock);
top_hierarchy = parent_resource_id == DEVLINK_RESOURCE_ID_PARENT_TOP;
- mutex_lock(&devlink->lock);
resource = devlink_resource_find(devlink, NULL, resource_id);
- if (resource) {
- err = -EINVAL;
- goto out;
- }
+ if (resource)
+ return -EINVAL;
resource = kzalloc(sizeof(*resource), GFP_KERNEL);
- if (!resource) {
- err = -ENOMEM;
- goto out;
- }
+ if (!resource)
+ return -ENOMEM;
if (top_hierarchy) {
resource_list = &devlink->resource_list;
@@ -9883,8 +9915,7 @@ int devlink_resource_register(struct devlink *devlink,
resource->parent = parent_resource;
} else {
kfree(resource);
- err = -EINVAL;
- goto out;
+ return -EINVAL;
}
}
@@ -9897,20 +9928,45 @@ int devlink_resource_register(struct devlink *devlink,
sizeof(resource->size_params));
INIT_LIST_HEAD(&resource->resource_list);
list_add_tail(&resource->list, resource_list);
-out:
- mutex_unlock(&devlink->lock);
- return err;
+
+ return 0;
}
-EXPORT_SYMBOL_GPL(devlink_resource_register);
+EXPORT_SYMBOL_GPL(__devlink_resource_register);
/**
- * devlink_resources_unregister - free all resources
+ * devlink_resource_register - devlink resource register
*
* @devlink: devlink
- * @resource: resource
+ * @resource_name: resource's name
+ * @resource_size: resource's size
+ * @resource_id: resource's id
+ * @parent_resource_id: resource's parent id
+ * @size_params: size parameters
+ *
+ * Generic resources should reuse the same names across drivers.
+ * Please see the generic resources list at:
+ * Documentation/networking/devlink/devlink-resource.rst
*/
-void devlink_resources_unregister(struct devlink *devlink,
- struct devlink_resource *resource)
+int devlink_resource_register(struct devlink *devlink,
+ const char *resource_name,
+ u64 resource_size,
+ u64 resource_id,
+ u64 parent_resource_id,
+ const struct devlink_resource_size_params *size_params)
+{
+ int err;
+
+ mutex_lock(&devlink->lock);
+ err = __devlink_resource_register(devlink, resource_name, resource_size,
+ resource_id, parent_resource_id,
+ size_params);
+ mutex_unlock(&devlink->lock);
+ return err;
+}
+
+static void ____devlink_resources_unregister(struct devlink *devlink,
+ struct devlink_resource *resource,
+ bool locked)
{
struct devlink_resource *tmp, *child_resource;
struct list_head *resource_list;
@@ -9920,7 +9976,7 @@ void devlink_resources_unregister(struct devlink *devlink,
else
resource_list = &devlink->resource_list;
- if (!resource)
+ if (!resource && !locked)
mutex_lock(&devlink->lock);
list_for_each_entry_safe(child_resource, tmp, resource_list, list) {
@@ -9929,11 +9985,49 @@ void devlink_resources_unregister(struct devlink *devlink,
kfree(child_resource);
}
- if (!resource)
+ if (!resource && !locked)
mutex_unlock(&devlink->lock);
}
+
+void __devlink_resources_unregister(struct devlink *devlink,
+ struct devlink_resource *resource)
+{
+ lockdep_assert_held(&devlink->lock);
+ ____devlink_resources_unregister(devlink, resource, true);
+}
+EXPORT_SYMBOL_GPL(__devlink_resources_unregister);
+
+/**
+ * devlink_resources_unregister - free all resources
+ *
+ * @devlink: devlink
+ * @resource: resource
+ */
+void devlink_resources_unregister(struct devlink *devlink,
+ struct devlink_resource *resource)
+{
+ ____devlink_resources_unregister(devlink, resource, false);
+}
EXPORT_SYMBOL_GPL(devlink_resources_unregister);
+int __devlink_resource_size_get(struct devlink *devlink,
+ u64 resource_id,
+ u64 *p_resource_size)
+{
+ struct devlink_resource *resource;
+
+ lockdep_assert_held(&devlink->lock);
+
+ resource = devlink_resource_find(devlink, NULL, resource_id);
+ if (!resource)
+ return -EINVAL;
+
+ *p_resource_size = resource->size_new;
+ resource->size = resource->size_new;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(__devlink_resource_size_get);
+
/**
* devlink_resource_size_get - get and update size
*
@@ -9945,18 +10039,11 @@ int devlink_resource_size_get(struct devlink *devlink,
u64 resource_id,
u64 *p_resource_size)
{
- struct devlink_resource *resource;
- int err = 0;
+ int err;
mutex_lock(&devlink->lock);
- resource = devlink_resource_find(devlink, NULL, resource_id);
- if (!resource) {
- err = -EINVAL;
- goto out;
- }
- *p_resource_size = resource->size_new;
- resource->size = resource->size_new;
-out:
+ err = __devlink_resource_size_get(devlink, resource_id,
+ p_resource_size);
mutex_unlock(&devlink->lock);
return err;
}
@@ -9993,10 +10080,29 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
}
EXPORT_SYMBOL_GPL(devlink_dpipe_table_resource_set);
-/**
- * devlink_resource_occ_get_register - register occupancy getter
- *
- * @devlink: devlink
+void __devlink_resource_occ_get_register(struct devlink *devlink,
+ u64 resource_id,
+ devlink_resource_occ_get_t *occ_get,
+ void *occ_get_priv)
+{
+ struct devlink_resource *resource;
+
+ lockdep_assert_held(&devlink->lock);
+
+ resource = devlink_resource_find(devlink, NULL, resource_id);
+ if (WARN_ON(!resource))
+ return;
+ WARN_ON(resource->occ_get);
+
+ resource->occ_get = occ_get;
+ resource->occ_get_priv = occ_get_priv;
+}
+EXPORT_SYMBOL_GPL(__devlink_resource_occ_get_register);
+
+/**
+ * devlink_resource_occ_get_register - register occupancy getter
+ *
+ * @devlink: devlink
* @resource_id: resource id
* @occ_get: occupancy getter callback
* @occ_get_priv: occupancy getter callback priv
@@ -10005,21 +10111,30 @@ void devlink_resource_occ_get_register(struct devlink *devlink,
u64 resource_id,
devlink_resource_occ_get_t *occ_get,
void *occ_get_priv)
+{
+ mutex_lock(&devlink->lock);
+ __devlink_resource_occ_get_register(devlink, resource_id,
+ occ_get, occ_get_priv);
+ mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
+
+void __devlink_resource_occ_get_unregister(struct devlink *devlink,
+ u64 resource_id)
{
struct devlink_resource *resource;
- mutex_lock(&devlink->lock);
+ lockdep_assert_held(&devlink->lock);
+
resource = devlink_resource_find(devlink, NULL, resource_id);
if (WARN_ON(!resource))
- goto out;
- WARN_ON(resource->occ_get);
+ return;
+ WARN_ON(!resource->occ_get);
- resource->occ_get = occ_get;
- resource->occ_get_priv = occ_get_priv;
-out:
- mutex_unlock(&devlink->lock);
+ resource->occ_get = NULL;
+ resource->occ_get_priv = NULL;
}
-EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
+EXPORT_SYMBOL_GPL(__devlink_resource_occ_get_unregister);
/**
* devlink_resource_occ_get_unregister - unregister occupancy getter
@@ -10030,17 +10145,8 @@ EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
void devlink_resource_occ_get_unregister(struct devlink *devlink,
u64 resource_id)
{
- struct devlink_resource *resource;
-
mutex_lock(&devlink->lock);
- resource = devlink_resource_find(devlink, NULL, resource_id);
- if (WARN_ON(!resource))
- goto out;
- WARN_ON(!resource->occ_get);
-
- resource->occ_get = NULL;
- resource->occ_get_priv = NULL;
-out:
+ __devlink_resource_occ_get_unregister(devlink, resource_id);
mutex_unlock(&devlink->lock);
}
EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
@@ -10055,24 +10161,13 @@ static int devlink_param_verify(const struct devlink_param *param)
return devlink_param_driver_verify(param);
}
-/**
- * devlink_params_register - register configuration parameters
- *
- * @devlink: devlink
- * @params: configuration parameters array
- * @params_count: number of parameters provided
- *
- * Register the configuration parameters supported by the driver.
- */
-int devlink_params_register(struct devlink *devlink,
- const struct devlink_param *params,
- size_t params_count)
+int __devlink_params_register(struct devlink *devlink,
+ const struct devlink_param *params,
+ size_t params_count)
{
const struct devlink_param *param = params;
int i, err;
- ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
for (i = 0; i < params_count; i++, param++) {
err = devlink_param_register(devlink, param);
if (err)
@@ -10088,8 +10183,39 @@ int devlink_params_register(struct devlink *devlink,
devlink_param_unregister(devlink, param);
return err;
}
+EXPORT_SYMBOL_GPL(__devlink_params_register);
+
+/**
+ * devlink_params_register - register configuration parameters
+ *
+ * @devlink: devlink
+ * @params: configuration parameters array
+ * @params_count: number of parameters provided
+ *
+ * Register the configuration parameters supported by the driver.
+ */
+int devlink_params_register(struct devlink *devlink,
+ const struct devlink_param *params,
+ size_t params_count)
+{
+ ASSERT_DEVLINK_NOT_REGISTERED(devlink);
+
+ return __devlink_params_register(devlink, params, params_count);
+}
EXPORT_SYMBOL_GPL(devlink_params_register);
+void __devlink_params_unregister(struct devlink *devlink,
+ const struct devlink_param *params,
+ size_t params_count)
+{
+ const struct devlink_param *param = params;
+ int i;
+
+ for (i = 0; i < params_count; i++, param++)
+ devlink_param_unregister(devlink, param);
+}
+EXPORT_SYMBOL_GPL(__devlink_params_unregister);
+
/**
* devlink_params_unregister - unregister configuration parameters
* @devlink: devlink
@@ -10100,13 +10226,9 @@ void devlink_params_unregister(struct devlink *devlink,
const struct devlink_param *params,
size_t params_count)
{
- const struct devlink_param *param = params;
- int i;
-
ASSERT_DEVLINK_NOT_REGISTERED(devlink);
- for (i = 0; i < params_count; i++, param++)
- devlink_param_unregister(devlink, param);
+ __devlink_params_unregister(devlink, params, params_count);
}
EXPORT_SYMBOL_GPL(devlink_params_unregister);
@@ -10261,37 +10383,24 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
}
EXPORT_SYMBOL_GPL(devlink_param_value_changed);
-/**
- * devlink_region_create - create a new address region
- *
- * @devlink: devlink
- * @ops: region operations and name
- * @region_max_snapshots: Maximum supported number of snapshots for region
- * @region_size: size of region
- */
struct devlink_region *
-devlink_region_create(struct devlink *devlink,
- const struct devlink_region_ops *ops,
- u32 region_max_snapshots, u64 region_size)
+__devlink_region_create(struct devlink *devlink,
+ const struct devlink_region_ops *ops,
+ u32 region_max_snapshots, u64 region_size)
{
struct devlink_region *region;
- int err = 0;
+
+ lockdep_assert_held(&devlink->lock);
if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
return ERR_PTR(-EINVAL);
- mutex_lock(&devlink->lock);
-
- if (devlink_region_get_by_name(devlink, ops->name)) {
- err = -EEXIST;
- goto unlock;
- }
+ if (devlink_region_get_by_name(devlink, ops->name))
+ return ERR_PTR(-EEXIST);
region = kzalloc(sizeof(*region), GFP_KERNEL);
- if (!region) {
- err = -ENOMEM;
- goto unlock;
- }
+ if (!region)
+ return ERR_PTR(-ENOMEM);
region->devlink = devlink;
region->max_snapshots = region_max_snapshots;
@@ -10301,12 +10410,30 @@ devlink_region_create(struct devlink *devlink,
list_add_tail(®ion->list, &devlink->region_list);
devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
- mutex_unlock(&devlink->lock);
return region;
+}
+EXPORT_SYMBOL_GPL(__devlink_region_create);
-unlock:
+/**
+ * devlink_region_create - create a new address region
+ *
+ * @devlink: devlink
+ * @ops: region operations and name
+ * @region_max_snapshots: Maximum supported number of snapshots for region
+ * @region_size: size of region
+ */
+struct devlink_region *
+devlink_region_create(struct devlink *devlink,
+ const struct devlink_region_ops *ops,
+ u32 region_max_snapshots, u64 region_size)
+{
+ struct devlink_region *region;
+
+ mutex_lock(&devlink->lock);
+ region = __devlink_region_create(devlink, ops, region_max_snapshots,
+ region_size);
mutex_unlock(&devlink->lock);
- return ERR_PTR(err);
+ return region;
}
EXPORT_SYMBOL_GPL(devlink_region_create);
@@ -10361,17 +10488,11 @@ devlink_port_region_create(struct devlink_port *port,
}
EXPORT_SYMBOL_GPL(devlink_port_region_create);
-/**
- * devlink_region_destroy - destroy address region
- *
- * @region: devlink region to destroy
- */
-void devlink_region_destroy(struct devlink_region *region)
+void __devlink_region_destroy(struct devlink_region *region)
{
- struct devlink *devlink = region->devlink;
struct devlink_snapshot *snapshot, *ts;
- mutex_lock(&devlink->lock);
+ lockdep_assert_held(®ion->devlink->lock);
/* Free all snapshots of region */
list_for_each_entry_safe(snapshot, ts, ®ion->snapshot_list, list)
@@ -10380,9 +10501,23 @@ void devlink_region_destroy(struct devlink_region *region)
list_del(®ion->list);
devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_DEL);
- mutex_unlock(&devlink->lock);
kfree(region);
}
+EXPORT_SYMBOL_GPL(__devlink_region_destroy);
+
+/**
+ * devlink_region_destroy - destroy address region
+ *
+ * @region: devlink region to destroy
+ */
+void devlink_region_destroy(struct devlink_region *region)
+{
+ struct devlink *devlink = region->devlink;
+
+ mutex_lock(&devlink->lock);
+ __devlink_region_destroy(region);
+ mutex_unlock(&devlink->lock);
+}
EXPORT_SYMBOL_GPL(devlink_region_destroy);
/**
@@ -10412,6 +10547,14 @@ int devlink_region_snapshot_id_get(struct devlink *devlink, u32 *id)
}
EXPORT_SYMBOL_GPL(devlink_region_snapshot_id_get);
+void __devlink_region_snapshot_id_put(struct devlink *devlink, u32 id)
+{
+ lockdep_assert_held(&devlink->lock);
+
+ __devlink_snapshot_id_decrement(devlink, id);
+}
+EXPORT_SYMBOL_GPL(__devlink_region_snapshot_id_put);
+
/**
* devlink_region_snapshot_id_put - put snapshot ID reference
*
@@ -10815,25 +10958,17 @@ static void devlink_trap_disable(struct devlink *devlink,
trap_item->action = DEVLINK_TRAP_ACTION_DROP;
}
-/**
- * devlink_traps_register - Register packet traps with devlink.
- * @devlink: devlink.
- * @traps: Packet traps.
- * @traps_count: Count of provided packet traps.
- * @priv: Driver private information.
- *
- * Return: Non-zero value on failure.
- */
-int devlink_traps_register(struct devlink *devlink,
- const struct devlink_trap *traps,
- size_t traps_count, void *priv)
+int __devlink_traps_register(struct devlink *devlink,
+ const struct devlink_trap *traps,
+ size_t traps_count, void *priv)
{
int i, err;
+ lockdep_assert_held(&devlink->lock);
+
if (!devlink->ops->trap_init || !devlink->ops->trap_action_set)
return -EINVAL;
- mutex_lock(&devlink->lock);
for (i = 0; i < traps_count; i++) {
const struct devlink_trap *trap = &traps[i];
@@ -10845,7 +10980,6 @@ int devlink_traps_register(struct devlink *devlink,
if (err)
goto err_trap_register;
}
- mutex_unlock(&devlink->lock);
return 0;
@@ -10853,24 +10987,40 @@ int devlink_traps_register(struct devlink *devlink,
err_trap_verify:
for (i--; i >= 0; i--)
devlink_trap_unregister(devlink, &traps[i]);
- mutex_unlock(&devlink->lock);
return err;
}
-EXPORT_SYMBOL_GPL(devlink_traps_register);
+EXPORT_SYMBOL_GPL(__devlink_traps_register);
/**
- * devlink_traps_unregister - Unregister packet traps from devlink.
+ * devlink_traps_register - Register packet traps with devlink.
* @devlink: devlink.
* @traps: Packet traps.
* @traps_count: Count of provided packet traps.
+ * @priv: Driver private information.
+ *
+ * Return: Non-zero value on failure.
*/
-void devlink_traps_unregister(struct devlink *devlink,
- const struct devlink_trap *traps,
- size_t traps_count)
+int devlink_traps_register(struct devlink *devlink,
+ const struct devlink_trap *traps,
+ size_t traps_count, void *priv)
{
- int i;
+ int err;
mutex_lock(&devlink->lock);
+ err = __devlink_traps_register(devlink, traps, traps_count, priv);
+ mutex_unlock(&devlink->lock);
+ return err;
+}
+EXPORT_SYMBOL_GPL(devlink_traps_register);
+
+void __devlink_traps_unregister(struct devlink *devlink,
+ const struct devlink_trap *traps,
+ size_t traps_count)
+{
+ int i;
+
+ lockdep_assert_held(&devlink->lock);
+
/* Make sure we do not have any packets in-flight while unregistering
* traps by disabling all of them and waiting for a grace period.
*/
@@ -10879,6 +11029,21 @@ void devlink_traps_unregister(struct devlink *devlink,
synchronize_rcu();
for (i = traps_count - 1; i >= 0; i--)
devlink_trap_unregister(devlink, &traps[i]);
+}
+EXPORT_SYMBOL_GPL(__devlink_traps_unregister);
+
+/**
+ * devlink_traps_unregister - Unregister packet traps from devlink.
+ * @devlink: devlink.
+ * @traps: Packet traps.
+ * @traps_count: Count of provided packet traps.
+ */
+void devlink_traps_unregister(struct devlink *devlink,
+ const struct devlink_trap *traps,
+ size_t traps_count)
+{
+ mutex_lock(&devlink->lock);
+ __devlink_traps_unregister(devlink, traps, traps_count);
mutex_unlock(&devlink->lock);
}
EXPORT_SYMBOL_GPL(devlink_traps_unregister);
@@ -11037,21 +11202,14 @@ devlink_trap_group_unregister(struct devlink *devlink,
kfree(group_item);
}
-/**
- * devlink_trap_groups_register - Register packet trap groups with devlink.
- * @devlink: devlink.
- * @groups: Packet trap groups.
- * @groups_count: Count of provided packet trap groups.
- *
- * Return: Non-zero value on failure.
- */
-int devlink_trap_groups_register(struct devlink *devlink,
- const struct devlink_trap_group *groups,
- size_t groups_count)
+int __devlink_trap_groups_register(struct devlink *devlink,
+ const struct devlink_trap_group *groups,
+ size_t groups_count)
{
int i, err;
- mutex_lock(&devlink->lock);
+ lockdep_assert_held(&devlink->lock);
+
for (i = 0; i < groups_count; i++) {
const struct devlink_trap_group *group = &groups[i];
@@ -11063,7 +11221,6 @@ int devlink_trap_groups_register(struct devlink *devlink,
if (err)
goto err_trap_group_register;
}
- mutex_unlock(&devlink->lock);
return 0;
@@ -11071,11 +11228,44 @@ int devlink_trap_groups_register(struct devlink *devlink,
err_trap_group_verify:
for (i--; i >= 0; i--)
devlink_trap_group_unregister(devlink, &groups[i]);
+ return err;
+}
+EXPORT_SYMBOL_GPL(__devlink_trap_groups_register);
+
+/**
+ * devlink_trap_groups_register - Register packet trap groups with devlink.
+ * @devlink: devlink.
+ * @groups: Packet trap groups.
+ * @groups_count: Count of provided packet trap groups.
+ *
+ * Return: Non-zero value on failure.
+ */
+int devlink_trap_groups_register(struct devlink *devlink,
+ const struct devlink_trap_group *groups,
+ size_t groups_count)
+{
+ int err;
+
+ mutex_lock(&devlink->lock);
+ err = __devlink_trap_groups_register(devlink, groups, groups_count);
mutex_unlock(&devlink->lock);
return err;
}
EXPORT_SYMBOL_GPL(devlink_trap_groups_register);
+void __devlink_trap_groups_unregister(struct devlink *devlink,
+ const struct devlink_trap_group *groups,
+ size_t groups_count)
+{
+ int i;
+
+ lockdep_assert_held(&devlink->lock);
+
+ for (i = groups_count - 1; i >= 0; i--)
+ devlink_trap_group_unregister(devlink, &groups[i]);
+}
+EXPORT_SYMBOL_GPL(__devlink_trap_groups_unregister);
+
/**
* devlink_trap_groups_unregister - Unregister packet trap groups from devlink.
* @devlink: devlink.
@@ -11086,11 +11276,8 @@ void devlink_trap_groups_unregister(struct devlink *devlink,
const struct devlink_trap_group *groups,
size_t groups_count)
{
- int i;
-
mutex_lock(&devlink->lock);
- for (i = groups_count - 1; i >= 0; i--)
- devlink_trap_group_unregister(devlink, &groups[i]);
+ __devlink_trap_groups_unregister(devlink, groups, groups_count);
mutex_unlock(&devlink->lock);
}
EXPORT_SYMBOL_GPL(devlink_trap_groups_unregister);
@@ -11176,22 +11363,15 @@ devlink_trap_policer_unregister(struct devlink *devlink,
kfree(policer_item);
}
-/**
- * devlink_trap_policers_register - Register packet trap policers with devlink.
- * @devlink: devlink.
- * @policers: Packet trap policers.
- * @policers_count: Count of provided packet trap policers.
- *
- * Return: Non-zero value on failure.
- */
int
-devlink_trap_policers_register(struct devlink *devlink,
- const struct devlink_trap_policer *policers,
- size_t policers_count)
+__devlink_trap_policers_register(struct devlink *devlink,
+ const struct devlink_trap_policer *policers,
+ size_t policers_count)
{
int i, err;
- mutex_lock(&devlink->lock);
+ lockdep_assert_held(&devlink->lock);
+
for (i = 0; i < policers_count; i++) {
const struct devlink_trap_policer *policer = &policers[i];
@@ -11206,7 +11386,6 @@ devlink_trap_policers_register(struct devlink *devlink,
if (err)
goto err_trap_policer_register;
}
- mutex_unlock(&devlink->lock);
return 0;
@@ -11214,11 +11393,47 @@ devlink_trap_policers_register(struct devlink *devlink,
err_trap_policer_verify:
for (i--; i >= 0; i--)
devlink_trap_policer_unregister(devlink, &policers[i]);
+ return err;
+}
+EXPORT_SYMBOL_GPL(__devlink_trap_policers_register);
+
+/**
+ * devlink_trap_policers_register - Register packet trap policers with devlink.
+ * @devlink: devlink.
+ * @policers: Packet trap policers.
+ * @policers_count: Count of provided packet trap policers.
+ *
+ * Return: Non-zero value on failure.
+ */
+int
+devlink_trap_policers_register(struct devlink *devlink,
+ const struct devlink_trap_policer *policers,
+ size_t policers_count)
+{
+ int err;
+
+ mutex_lock(&devlink->lock);
+ err = __devlink_trap_policers_register(devlink, policers,
+ policers_count);
mutex_unlock(&devlink->lock);
return err;
}
EXPORT_SYMBOL_GPL(devlink_trap_policers_register);
+void
+__devlink_trap_policers_unregister(struct devlink *devlink,
+ const struct devlink_trap_policer *policers,
+ size_t policers_count)
+{
+ int i;
+
+ lockdep_assert_held(&devlink->lock);
+
+ for (i = policers_count - 1; i >= 0; i--)
+ devlink_trap_policer_unregister(devlink, &policers[i]);
+}
+EXPORT_SYMBOL_GPL(__devlink_trap_policers_unregister);
+
/**
* devlink_trap_policers_unregister - Unregister packet trap policers from devlink.
* @devlink: devlink.
@@ -11230,11 +11445,8 @@ devlink_trap_policers_unregister(struct devlink *devlink,
const struct devlink_trap_policer *policers,
size_t policers_count)
{
- int i;
-
mutex_lock(&devlink->lock);
- for (i = policers_count - 1; i >= 0; i--)
- devlink_trap_policer_unregister(devlink, &policers[i]);
+ __devlink_trap_policers_unregister(devlink, policers, policers_count);
mutex_unlock(&devlink->lock);
}
EXPORT_SYMBOL_GPL(devlink_trap_policers_unregister);
--
2.31.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC 2/5] devlink: add API for explicit locking
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
2021-10-30 23:12 ` [RFC 1/5] devlink: add unlocked APIs Jakub Kicinski
@ 2021-10-30 23:12 ` Jakub Kicinski
2021-10-30 23:12 ` [RFC 3/5] devlink: allow locking of all ops Jakub Kicinski
` (4 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-10-30 23:12 UTC (permalink / raw)
To: leon, idosch; +Cc: edwin.peer, jiri, netdev, Jakub Kicinski
Expose devlink instance lock and create the registration
helpers as needed. The new unregistration helper does
not wait for all refs to be gone, and free just gives
up the reference instead of actually freeing the memory.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/devlink.h | 16 ++++++
net/core/devlink.c | 122 +++++++++++++++++++++++++++++++++++++++---
2 files changed, 130 insertions(+), 8 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index d8e4274e2af4..66c1951a6f0e 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1191,6 +1191,7 @@ enum {
};
struct devlink_ops {
+ struct module *owner;
/**
* @supported_flash_update_params:
* mask of parameters supported by the driver's .flash_update
@@ -1490,6 +1491,8 @@ void *devlink_priv(struct devlink *devlink);
struct devlink *priv_to_devlink(void *priv);
struct device *devlink_to_dev(const struct devlink *devlink);
+int lockdep_devlink_is_held(struct devlink *devlink);
+
struct ib_device;
struct net *devlink_net(const struct devlink *devlink);
@@ -1507,10 +1510,23 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
{
return devlink_alloc_ns(ops, priv_size, &init_net, dev);
}
+
+void devlink_lock(struct devlink *devlink);
+void devlink_unlock(struct devlink *devlink);
+
+void devlink_lock_reg(struct devlink *devlink);
+void devlink_unlock_reg(struct devlink *devlink);
+
+void devlink_get(struct devlink *devlink);
+bool devlink_is_alive(const struct devlink *devlink);
+
void devlink_set_features(struct devlink *devlink, u64 features);
void devlink_register(struct devlink *devlink);
+void __devlink_register(struct devlink *devlink);
void devlink_unregister(struct devlink *devlink);
+void __devlink_unregister(struct devlink *devlink);
void devlink_free(struct devlink *devlink);
+void __devlink_free(struct devlink *devlink);
int devlink_port_register(struct devlink *devlink,
struct devlink_port *devlink_port,
unsigned int port_index);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9ea0c0bbc796..6783b066f9a7 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -182,11 +182,24 @@ struct net *devlink_net(const struct devlink *devlink)
}
EXPORT_SYMBOL_GPL(devlink_net);
+void devlink_free(struct devlink *devlink);
+
void devlink_put(struct devlink *devlink)
{
- if (refcount_dec_and_test(&devlink->refcount))
- complete(&devlink->comp);
+ if (refcount_dec_and_test(&devlink->refcount)) {
+ if (devlink->ops->lock_flags & DEVLINK_LOCK_REF_MODE)
+ devlink_free(devlink);
+ else
+ complete(&devlink->comp);
+ }
}
+EXPORT_SYMBOL_GPL(devlink_put);
+
+void devlink_get(struct devlink *devlink)
+{
+ refcount_inc(&devlink->refcount);
+}
+EXPORT_SYMBOL_GPL(devlink_get);
struct devlink *__must_check devlink_try_get(struct devlink *devlink)
{
@@ -195,6 +208,60 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
return NULL;
}
+bool devlink_is_alive(const struct devlink *devlink)
+{
+ return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+}
+EXPORT_SYMBOL_GPL(devlink_is_alive);
+
+/**
+ * devlink_lock_reg() - take devlink registration lock as well as instance lock
+ * @devlink: instance to lock
+ */
+void devlink_lock_reg(struct devlink *devlink)
+{
+ mutex_lock(&devlink_mutex);
+ mutex_lock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_lock_reg);
+
+/**
+ * devlink_unlock_reg()
+ * @devlink: instance to lock
+ */
+void devlink_unlock_reg(struct devlink *devlink)
+{
+ mutex_unlock(&devlink->lock);
+ mutex_unlock(&devlink_mutex);
+}
+EXPORT_SYMBOL_GPL(devlink_unlock_reg);
+
+/**
+ * devlink_lock() - lock devlink instance
+ * @devlink: instance to lock
+ */
+void devlink_lock(struct devlink *devlink)
+{
+ mutex_lock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_lock);
+
+/**
+ * devlink_unlock() - release devlink instance lock
+ * @devlink: instance to unlock
+ */
+void devlink_unlock(struct devlink *devlink)
+{
+ mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_unlock);
+
+int lockdep_devlink_is_held(struct devlink *devlink)
+{
+ return lockdep_is_held(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(lockdep_devlink_is_held);
+
static struct devlink *devlink_get_from_attrs(struct net *net,
struct nlattr **attrs)
{
@@ -9016,6 +9083,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
mutex_init(&devlink->reporters_lock);
refcount_set(&devlink->refcount, 1);
init_completion(&devlink->comp);
+ __module_get(devlink->ops->owner);
return devlink;
}
@@ -9105,6 +9173,18 @@ static void devlink_notify_unregister(struct devlink *devlink)
devlink_notify(devlink, DEVLINK_CMD_DEL);
}
+void __devlink_register(struct devlink *devlink)
+{
+ ASSERT_DEVLINK_NOT_REGISTERED(devlink);
+ lockdep_assert_held(&devlink->lock);
+ lockdep_assert_held(&devlink_mutex);
+ /* Make sure that we are in .probe() routine */
+
+ xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+ devlink_notify_register(devlink);
+}
+EXPORT_SYMBOL_GPL(__devlink_register);
+
/**
* devlink_register - Register devlink instance
*
@@ -9112,16 +9192,27 @@ static void devlink_notify_unregister(struct devlink *devlink)
*/
void devlink_register(struct devlink *devlink)
{
- ASSERT_DEVLINK_NOT_REGISTERED(devlink);
- /* Make sure that we are in .probe() routine */
-
mutex_lock(&devlink_mutex);
- xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
- devlink_notify_register(devlink);
+ __devlink_register(devlink);
mutex_unlock(&devlink_mutex);
}
EXPORT_SYMBOL_GPL(devlink_register);
+/* Note: this is NOT an equivalent to devlink_unregister() sans locking
+ * it also accounts for the ability to take references on the instance.
+ */
+void __devlink_unregister(struct devlink *devlink)
+{
+ ASSERT_DEVLINK_REGISTERED(devlink);
+ /* Make sure that we are in .remove() routine */
+ WARN_ON(!(devlink->ops->lock_flags & DEVLINK_LOCK_REF_MODE));
+ lockdep_assert_held(&devlink->lock);
+
+ devlink_notify_unregister(devlink);
+ xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+}
+EXPORT_SYMBOL_GPL(__devlink_unregister);
+
/**
* devlink_unregister - Unregister devlink instance
*
@@ -9131,6 +9222,7 @@ void devlink_unregister(struct devlink *devlink)
{
ASSERT_DEVLINK_REGISTERED(devlink);
/* Make sure that we are in .remove() routine */
+ WARN_ON(devlink->ops->lock_flags & DEVLINK_LOCK_REF_MODE);
devlink_put(devlink);
wait_for_completion(&devlink->comp);
@@ -9142,6 +9234,12 @@ void devlink_unregister(struct devlink *devlink)
}
EXPORT_SYMBOL_GPL(devlink_unregister);
+void __devlink_free(struct devlink *devlink)
+{
+ devlink_put(devlink);
+}
+EXPORT_SYMBOL_GPL(__devlink_free);
+
/**
* devlink_free - Free devlink instance resources
*
@@ -9168,6 +9266,8 @@ void devlink_free(struct devlink *devlink)
xa_destroy(&devlink->snapshot_ids);
xa_erase(&devlinks, devlink->index);
+ module_put(devlink->ops->owner);
+
kfree(devlink);
}
EXPORT_SYMBOL_GPL(devlink_free);
@@ -11502,7 +11602,8 @@ void devlink_compat_running_version(struct devlink *devlink,
return;
mutex_lock(&devlink->lock);
- __devlink_compat_running_version(devlink, buf, len);
+ if (devlink_is_alive(devlink))
+ __devlink_compat_running_version(devlink, buf, len);
mutex_unlock(&devlink->lock);
}
@@ -11519,9 +11620,14 @@ int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
return ret;
mutex_lock(&devlink->lock);
+ if (!devlink_is_alive(devlink)) {
+ ret = -ENODEV;
+ goto exit_unlock;
+ }
devlink_flash_update_begin_notify(devlink);
ret = devlink->ops->flash_update(devlink, ¶ms, NULL);
devlink_flash_update_end_notify(devlink);
+exit_unlock:
mutex_unlock(&devlink->lock);
release_firmware(params.fw);
--
2.31.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC 3/5] devlink: allow locking of all ops
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
2021-10-30 23:12 ` [RFC 1/5] devlink: add unlocked APIs Jakub Kicinski
2021-10-30 23:12 ` [RFC 2/5] devlink: add API for explicit locking Jakub Kicinski
@ 2021-10-30 23:12 ` Jakub Kicinski
2021-10-30 23:12 ` [RFC 4/5] netdevsim: minor code move Jakub Kicinski
` (3 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-10-30 23:12 UTC (permalink / raw)
To: leon, idosch; +Cc: edwin.peer, jiri, netdev, Jakub Kicinski
Drivers using the "explicit locking" API can opt-in
to have all callbacks locked by the devlink instance
lock.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/devlink.h | 12 +++++++++
net/core/devlink.c | 63 ++++++++++++++++++++++++++++++++++++++-----
2 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 66c1951a6f0e..41ab6a2b63f0 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1190,7 +1190,19 @@ enum {
DEVLINK_F_RELOAD = 1UL << 0,
};
+enum {
+ DEVLINK_LOCK_REF_MODE = 1UL << 0,
+ DEVLINK_LOCK_ESWITCH = 1UL << 1,
+ DEVLINK_LOCK_PORT_OPS = 1UL << 2,
+ DEVLINK_LOCK_HEALTH_OPS = 1UL << 3,
+};
+#define DEVLINK_LOCK_ALL_OPS (DEVLINK_LOCK_REF_MODE | \
+ DEVLINK_LOCK_ESWITCH | \
+ DEVLINK_LOCK_PORT_OPS | \
+ DEVLINK_LOCK_HEALTH_OPS)
+
struct devlink_ops {
+ u8 lock_flags;
struct module *owner;
/**
* @supported_flash_update_params:
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6783b066f9a7..70b3f725cb53 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -256,6 +256,18 @@ void devlink_unlock(struct devlink *devlink)
}
EXPORT_SYMBOL_GPL(devlink_unlock);
+static void devlink_lock_cond(struct devlink *devlink, unsigned long flag)
+{
+ if (devlink->ops->lock_flags & flag)
+ mutex_lock(&devlink->lock);
+}
+
+static void devlink_unlock_cond(struct devlink *devlink, unsigned long flag)
+{
+ if (devlink->ops->lock_flags & flag)
+ mutex_unlock(&devlink->lock);
+}
+
int lockdep_devlink_is_held(struct devlink *devlink)
{
return lockdep_is_held(&devlink->lock);
@@ -1595,6 +1607,7 @@ static int devlink_nl_cmd_port_split_doit(struct sk_buff *skb,
struct devlink_port *devlink_port;
u32 port_index;
u32 count;
+ int ret;
if (!info->attrs[DEVLINK_ATTR_PORT_INDEX] ||
!info->attrs[DEVLINK_ATTR_PORT_SPLIT_COUNT])
@@ -1621,7 +1634,10 @@ static int devlink_nl_cmd_port_split_doit(struct sk_buff *skb,
return -EINVAL;
}
- return devlink_port_split(devlink, port_index, count, info->extack);
+ devlink_lock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
+ ret = devlink_port_split(devlink, port_index, count, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
+ return ret;
}
static int devlink_port_unsplit(struct devlink *devlink, u32 port_index,
@@ -1638,12 +1654,17 @@ static int devlink_nl_cmd_port_unsplit_doit(struct sk_buff *skb,
{
struct devlink *devlink = info->user_ptr[0];
u32 port_index;
+ int ret;
if (!info->attrs[DEVLINK_ATTR_PORT_INDEX])
return -EINVAL;
port_index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
- return devlink_port_unsplit(devlink, port_index, info->extack);
+
+ devlink_lock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
+ ret = devlink_port_unsplit(devlink, port_index, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
+ return ret;
}
static int devlink_port_new_notifiy(struct devlink *devlink,
@@ -1718,15 +1739,19 @@ static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb,
new_attrs.sfnum_valid = true;
}
+ devlink_lock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
err = devlink->ops->port_new(devlink, &new_attrs, extack,
&new_port_index);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
if (err)
return err;
err = devlink_port_new_notifiy(devlink, new_port_index, info);
if (err && err != -ENODEV) {
/* Fail to send the response; destroy newly created port. */
+ devlink_lock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
devlink->ops->port_del(devlink, new_port_index, extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
}
return err;
}
@@ -1737,6 +1762,7 @@ static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb,
struct netlink_ext_ack *extack = info->extack;
struct devlink *devlink = info->user_ptr[0];
unsigned int port_index;
+ int ret;
if (!devlink->ops->port_del)
return -EOPNOTSUPP;
@@ -1747,7 +1773,10 @@ static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb,
}
port_index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
- return devlink->ops->port_del(devlink, port_index, extack);
+ devlink_lock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
+ ret = devlink->ops->port_del(devlink, port_index, extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_PORT_OPS);
+ return ret;
}
static int
@@ -2841,7 +2870,9 @@ static int devlink_nl_eswitch_fill(struct sk_buff *msg, struct devlink *devlink,
goto nla_put_failure;
if (ops->eswitch_mode_get) {
+ devlink_lock_cond(devlink, DEVLINK_LOCK_ESWITCH);
err = ops->eswitch_mode_get(devlink, &mode);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_ESWITCH);
if (err)
goto nla_put_failure;
err = nla_put_u16(msg, DEVLINK_ATTR_ESWITCH_MODE, mode);
@@ -2932,7 +2963,9 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
err = devlink_rate_nodes_check(devlink, mode, info->extack);
if (err)
return err;
+ devlink_lock_cond(devlink, DEVLINK_LOCK_ESWITCH);
err = ops->eswitch_mode_set(devlink, mode, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_ESWITCH);
if (err)
return err;
}
@@ -4227,7 +4260,9 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
return -EINVAL;
}
}
+ devlink_lock_cond(devlink, DEVLINK_LOCK_REF_MODE);
err = devlink_reload(devlink, dest_net, action, limit, &actions_performed, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_REF_MODE);
if (dest_net)
put_net(dest_net);
@@ -7116,8 +7151,10 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
if (err)
goto dump_err;
+ devlink_lock_cond(reporter->devlink, DEVLINK_LOCK_HEALTH_OPS);
err = reporter->ops->dump(reporter, reporter->dump_fmsg,
priv_ctx, extack);
+ devlink_unlock_cond(reporter->devlink, DEVLINK_LOCK_HEALTH_OPS);
if (err)
goto dump_err;
@@ -7141,6 +7178,7 @@ int devlink_health_report(struct devlink_health_reporter *reporter,
enum devlink_health_reporter_state prev_health_state;
struct devlink *devlink = reporter->devlink;
unsigned long recover_ts_threshold;
+ int ret;
/* write a log message of the current error */
WARN_ON(!msg);
@@ -7174,11 +7212,14 @@ int devlink_health_report(struct devlink_health_reporter *reporter,
mutex_unlock(&reporter->dump_lock);
}
- if (reporter->auto_recover)
- return devlink_health_reporter_recover(reporter,
- priv_ctx, NULL);
+ ret = 0;
+ if (reporter->auto_recover) {
+ devlink_lock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
+ ret = devlink_health_reporter_recover(reporter, priv_ctx, NULL);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
+ }
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(devlink_health_report);
@@ -7430,7 +7471,9 @@ static int devlink_nl_cmd_health_reporter_recover_doit(struct sk_buff *skb,
if (!reporter)
return -EINVAL;
+ devlink_lock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
err = devlink_health_reporter_recover(reporter, NULL, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
devlink_health_reporter_put(reporter);
return err;
@@ -7463,7 +7506,9 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
if (err)
goto out;
+ devlink_lock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
err = reporter->ops->diagnose(reporter, fmsg, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
if (err)
goto out;
@@ -7557,7 +7602,9 @@ static int devlink_nl_cmd_health_reporter_test_doit(struct sk_buff *skb,
return -EOPNOTSUPP;
}
+ devlink_lock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
err = reporter->ops->test(reporter, info->extack);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_HEALTH_OPS);
devlink_health_reporter_put(reporter);
return err;
@@ -11690,10 +11737,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
goto retry;
WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
+ devlink_lock_cond(devlink, DEVLINK_LOCK_REF_MODE);
err = devlink_reload(devlink, &init_net,
DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
DEVLINK_RELOAD_LIMIT_UNSPEC,
&actions_performed, NULL);
+ devlink_unlock_cond(devlink, DEVLINK_LOCK_REF_MODE);
if (err && err != -EOPNOTSUPP)
pr_warn("Failed to reload devlink instance into init_net\n");
retry:
--
2.31.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC 4/5] netdevsim: minor code move
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
` (2 preceding siblings ...)
2021-10-30 23:12 ` [RFC 3/5] devlink: allow locking of all ops Jakub Kicinski
@ 2021-10-30 23:12 ` Jakub Kicinski
2021-10-30 23:12 ` [RFC 5/5] netdevsim: use devlink locking Jakub Kicinski
` (2 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-10-30 23:12 UTC (permalink / raw)
To: leon, idosch; +Cc: edwin.peer, jiri, netdev, Jakub Kicinski
Move the port add/del helpers
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/netdevsim/dev.c | 54 ++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 5db40d713d2a..b15763a8e89a 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -567,7 +567,32 @@ static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev)
devlink_region_destroy(nsim_dev->dummy_region);
}
-static void __nsim_dev_port_del(struct nsim_dev_port *nsim_dev_port);
+static struct nsim_dev_port *
+__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
+ unsigned int port_index)
+{
+ struct nsim_dev_port *nsim_dev_port;
+
+ port_index = nsim_dev_port_index(type, port_index);
+ list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
+ if (nsim_dev_port->port_index == port_index)
+ return nsim_dev_port;
+ return NULL;
+}
+
+static void __nsim_dev_port_del(struct nsim_dev_port *nsim_dev_port)
+{
+ struct devlink_port *devlink_port = &nsim_dev_port->devlink_port;
+
+ list_del(&nsim_dev_port->list);
+ if (nsim_dev_port_is_vf(nsim_dev_port))
+ devlink_rate_leaf_destroy(&nsim_dev_port->devlink_port);
+ devlink_port_type_clear(devlink_port);
+ nsim_destroy(nsim_dev_port->ns);
+ nsim_dev_port_debugfs_exit(nsim_dev_port);
+ devlink_port_unregister(devlink_port);
+ kfree(nsim_dev_port);
+}
static int nsim_esw_legacy_enable(struct nsim_dev *nsim_dev,
struct netlink_ext_ack *extack)
@@ -1418,20 +1443,6 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, enum nsim_dev_port_typ
return err;
}
-static void __nsim_dev_port_del(struct nsim_dev_port *nsim_dev_port)
-{
- struct devlink_port *devlink_port = &nsim_dev_port->devlink_port;
-
- list_del(&nsim_dev_port->list);
- if (nsim_dev_port_is_vf(nsim_dev_port))
- devlink_rate_leaf_destroy(&nsim_dev_port->devlink_port);
- devlink_port_type_clear(devlink_port);
- nsim_destroy(nsim_dev_port->ns);
- nsim_dev_port_debugfs_exit(nsim_dev_port);
- devlink_port_unregister(devlink_port);
- kfree(nsim_dev_port);
-}
-
static void nsim_dev_port_del_all(struct nsim_dev *nsim_dev)
{
struct nsim_dev_port *nsim_dev_port, *tmp;
@@ -1674,19 +1685,6 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
dev_set_drvdata(&nsim_bus_dev->dev, NULL);
}
-static struct nsim_dev_port *
-__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
- unsigned int port_index)
-{
- struct nsim_dev_port *nsim_dev_port;
-
- port_index = nsim_dev_port_index(type, port_index);
- list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
- if (nsim_dev_port->port_index == port_index)
- return nsim_dev_port;
- return NULL;
-}
-
int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
unsigned int port_index)
{
--
2.31.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [RFC 5/5] netdevsim: use devlink locking
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
` (3 preceding siblings ...)
2021-10-30 23:12 ` [RFC 4/5] netdevsim: minor code move Jakub Kicinski
@ 2021-10-30 23:12 ` Jakub Kicinski
2021-10-31 7:23 ` [RFC 0/5] devlink: add an explicit locking API Leon Romanovsky
2021-11-03 9:03 ` Jiri Pirko
6 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-10-30 23:12 UTC (permalink / raw)
To: leon, idosch; +Cc: edwin.peer, jiri, netdev, Jakub Kicinski
We can remove all 3 mutexes and use devlink lock
instead.
For trap delayed work we need to take a ref on
the devlink instance and check if the instance
is still active.
Similar thing for the debugfs handlers which
need locking.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/netdevsim/bus.c | 19 --
drivers/net/netdevsim/dev.c | 400 +++++++++++++++++-------------
drivers/net/netdevsim/fib.c | 62 ++---
drivers/net/netdevsim/netdevsim.h | 5 -
4 files changed, 263 insertions(+), 223 deletions(-)
diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index 25cb2e600d53..b5f4df1a07a3 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -72,16 +72,7 @@ new_port_store(struct device *dev, struct device_attribute *attr,
if (ret)
return ret;
- if (!mutex_trylock(&nsim_bus_dev->nsim_bus_reload_lock))
- return -EBUSY;
-
- if (nsim_bus_dev->in_reload) {
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
- return -EBUSY;
- }
-
ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
return ret ? ret : count;
}
@@ -102,16 +93,7 @@ del_port_store(struct device *dev, struct device_attribute *attr,
if (ret)
return ret;
- if (!mutex_trylock(&nsim_bus_dev->nsim_bus_reload_lock))
- return -EBUSY;
-
- if (nsim_bus_dev->in_reload) {
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
- return -EBUSY;
- }
-
ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
return ret ? ret : count;
}
@@ -298,7 +280,6 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count, unsigned int num_queu
nsim_bus_dev->num_queues = num_queues;
nsim_bus_dev->initial_net = current->nsproxy->net_ns;
nsim_bus_dev->max_vfs = NSIM_BUS_DEV_MAX_VFS;
- mutex_init(&nsim_bus_dev->nsim_bus_reload_lock);
/* Disallow using nsim_bus_dev */
smp_store_release(&nsim_bus_dev->init, false);
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index b15763a8e89a..3255498cadbf 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -58,8 +58,9 @@ static struct dentry *nsim_dev_ddir;
unsigned int nsim_dev_get_vfs(struct nsim_dev *nsim_dev)
{
- WARN_ON(!lockdep_rtnl_is_held() &&
- !lockdep_is_held(&nsim_dev->vfs_lock));
+ struct devlink *devlink = priv_to_devlink(nsim_dev);
+
+ WARN_ON(!lockdep_rtnl_is_held() && !lockdep_devlink_is_held(devlink));
return nsim_dev->nsim_bus_dev->num_vfs;
}
@@ -74,6 +75,28 @@ nsim_bus_dev_set_vfs(struct nsim_bus_dev *nsim_bus_dev, unsigned int num_vfs)
#define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32)
+static int nsim_devlink_ref_open(struct inode *inode, struct file *file)
+{
+ struct nsim_dev *nsim_dev = inode->i_private;
+ struct devlink *devlink = priv_to_devlink(nsim_dev);
+
+ /* debugfs promises us that open does not race with file removal,
+ * so if we're here the driver is still holding its main ref.
+ */
+ devlink_get(devlink);
+ file->private_data = inode->i_private;
+ return 0;
+}
+
+static int nsim_devlink_ref_release(struct inode *inode, struct file *file)
+{
+ struct nsim_dev *nsim_dev = inode->i_private;
+ struct devlink *devlink = priv_to_devlink(nsim_dev);
+
+ devlink_put(devlink);
+ return 0;
+}
+
static int
nsim_dev_take_snapshot(struct devlink *devlink,
const struct devlink_region_ops *ops,
@@ -109,26 +132,38 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
if (err)
return err;
- err = devlink_region_snapshot_id_get(devlink, &id);
+ devlink_lock(devlink);
+ if (!devlink_is_alive(devlink)) {
+ err = -ENODEV;
+ goto err_free;
+ }
+
+ err = __devlink_region_snapshot_id_get(devlink, &id);
if (err) {
pr_err("Failed to get snapshot id\n");
- kfree(dummy_data);
- return err;
+ goto err_free;
}
- err = devlink_region_snapshot_create(nsim_dev->dummy_region,
- dummy_data, id);
- devlink_region_snapshot_id_put(devlink, id);
+
+ err = __devlink_region_snapshot_create(nsim_dev->dummy_region,
+ dummy_data, id);
+ __devlink_region_snapshot_id_put(devlink, id);
if (err) {
pr_err("Failed to create region snapshot\n");
- kfree(dummy_data);
- return err;
+ goto err_free;
}
+ devlink_unlock(devlink);
return count;
+
+err_free:
+ kfree(dummy_data);
+ devlink_unlock(devlink);
+ return err;
}
static const struct file_operations nsim_dev_take_snapshot_fops = {
- .open = simple_open,
+ .open = nsim_devlink_ref_open,
+ .release = nsim_devlink_ref_release,
.write = nsim_dev_take_snapshot_write,
.llseek = generic_file_llseek,
.owner = THIS_MODULE,
@@ -246,6 +281,7 @@ static ssize_t nsim_bus_dev_max_vfs_write(struct file *file,
{
struct nsim_vf_config *vfconfigs;
struct nsim_dev *nsim_dev;
+ struct devlink *devlink;
char buf[10];
ssize_t ret;
u32 val;
@@ -275,9 +311,12 @@ static ssize_t nsim_bus_dev_max_vfs_write(struct file *file,
return -ENOMEM;
nsim_dev = file->private_data;
- mutex_lock(&nsim_dev->vfs_lock);
- /* Reject if VFs are configured */
- if (nsim_dev_get_vfs(nsim_dev)) {
+ devlink = priv_to_devlink(nsim_dev);
+ devlink_lock(devlink);
+ if (!devlink_is_alive(devlink)) {
+ ret = -ENODEV;
+ } else if (nsim_dev_get_vfs(nsim_dev)) {
+ /* Reject if VFs are configured */
ret = -EBUSY;
} else {
swap(nsim_dev->vfconfigs, vfconfigs);
@@ -285,14 +324,15 @@ static ssize_t nsim_bus_dev_max_vfs_write(struct file *file,
*ppos += count;
ret = count;
}
- mutex_unlock(&nsim_dev->vfs_lock);
+ devlink_unlock(devlink);
kfree(vfconfigs);
return ret;
}
static const struct file_operations nsim_dev_max_vfs_fops = {
- .open = simple_open,
+ .open = nsim_devlink_ref_open,
+ .release = nsim_devlink_ref_release,
.read = nsim_bus_dev_max_vfs_read,
.write = nsim_bus_dev_max_vfs_write,
.llseek = generic_file_llseek,
@@ -319,11 +359,10 @@ static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
&nsim_dev->max_macs);
debugfs_create_bool("test1", 0600, nsim_dev->ddir,
&nsim_dev->test1);
- nsim_dev->take_snapshot = debugfs_create_file("take_snapshot",
- 0200,
- nsim_dev->ddir,
- nsim_dev,
- &nsim_dev_take_snapshot_fops);
+ nsim_dev->take_snapshot =
+ debugfs_create_file_unsafe("take_snapshot", 0200,
+ nsim_dev->ddir, nsim_dev,
+ &nsim_dev_take_snapshot_fops);
debugfs_create_bool("dont_allow_reload", 0600, nsim_dev->ddir,
&nsim_dev->dont_allow_reload);
debugfs_create_bool("fail_reload", 0600, nsim_dev->ddir,
@@ -339,8 +378,8 @@ static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
debugfs_create_bool("fail_trap_policer_counter_get", 0600,
nsim_dev->ddir,
&nsim_dev->fail_trap_policer_counter_get);
- debugfs_create_file("max_vfs", 0600, nsim_dev->ddir,
- nsim_dev, &nsim_dev_max_vfs_fops);
+ debugfs_create_file_unsafe("max_vfs", 0600, nsim_dev->ddir,
+ nsim_dev, &nsim_dev_max_vfs_fops);
nsim_dev->nodes_ddir = debugfs_create_dir("rate_nodes", nsim_dev->ddir);
if (IS_ERR(nsim_dev->nodes_ddir)) {
@@ -435,62 +474,62 @@ static int nsim_dev_resources_register(struct devlink *devlink)
int err;
/* Resources for IPv4 */
- err = devlink_resource_register(devlink, "IPv4", (u64)-1,
- NSIM_RESOURCE_IPV4,
- DEVLINK_RESOURCE_ID_PARENT_TOP,
- ¶ms);
+ err = __devlink_resource_register(devlink, "IPv4", (u64)-1,
+ NSIM_RESOURCE_IPV4,
+ DEVLINK_RESOURCE_ID_PARENT_TOP,
+ ¶ms);
if (err) {
pr_err("Failed to register IPv4 top resource\n");
goto out;
}
- err = devlink_resource_register(devlink, "fib", (u64)-1,
- NSIM_RESOURCE_IPV4_FIB,
- NSIM_RESOURCE_IPV4, ¶ms);
+ err = __devlink_resource_register(devlink, "fib", (u64)-1,
+ NSIM_RESOURCE_IPV4_FIB,
+ NSIM_RESOURCE_IPV4, ¶ms);
if (err) {
pr_err("Failed to register IPv4 FIB resource\n");
- return err;
+ goto out;
}
- err = devlink_resource_register(devlink, "fib-rules", (u64)-1,
- NSIM_RESOURCE_IPV4_FIB_RULES,
- NSIM_RESOURCE_IPV4, ¶ms);
+ err = __devlink_resource_register(devlink, "fib-rules", (u64)-1,
+ NSIM_RESOURCE_IPV4_FIB_RULES,
+ NSIM_RESOURCE_IPV4, ¶ms);
if (err) {
pr_err("Failed to register IPv4 FIB rules resource\n");
- return err;
+ goto out;
}
/* Resources for IPv6 */
- err = devlink_resource_register(devlink, "IPv6", (u64)-1,
- NSIM_RESOURCE_IPV6,
- DEVLINK_RESOURCE_ID_PARENT_TOP,
- ¶ms);
+ err = __devlink_resource_register(devlink, "IPv6", (u64)-1,
+ NSIM_RESOURCE_IPV6,
+ DEVLINK_RESOURCE_ID_PARENT_TOP,
+ ¶ms);
if (err) {
pr_err("Failed to register IPv6 top resource\n");
goto out;
}
- err = devlink_resource_register(devlink, "fib", (u64)-1,
- NSIM_RESOURCE_IPV6_FIB,
- NSIM_RESOURCE_IPV6, ¶ms);
+ err = __devlink_resource_register(devlink, "fib", (u64)-1,
+ NSIM_RESOURCE_IPV6_FIB,
+ NSIM_RESOURCE_IPV6, ¶ms);
if (err) {
pr_err("Failed to register IPv6 FIB resource\n");
- return err;
+ goto out;
}
- err = devlink_resource_register(devlink, "fib-rules", (u64)-1,
- NSIM_RESOURCE_IPV6_FIB_RULES,
- NSIM_RESOURCE_IPV6, ¶ms);
+ err = __devlink_resource_register(devlink, "fib-rules", (u64)-1,
+ NSIM_RESOURCE_IPV6_FIB_RULES,
+ NSIM_RESOURCE_IPV6, ¶ms);
if (err) {
pr_err("Failed to register IPv6 FIB rules resource\n");
- return err;
+ goto out;
}
/* Resources for nexthops */
- err = devlink_resource_register(devlink, "nexthops", (u64)-1,
- NSIM_RESOURCE_NEXTHOPS,
- DEVLINK_RESOURCE_ID_PARENT_TOP,
- ¶ms);
+ err = __devlink_resource_register(devlink, "nexthops", (u64)-1,
+ NSIM_RESOURCE_NEXTHOPS,
+ DEVLINK_RESOURCE_ID_PARENT_TOP,
+ ¶ms);
out:
return err;
@@ -556,15 +595,15 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
struct devlink *devlink)
{
nsim_dev->dummy_region =
- devlink_region_create(devlink, &dummy_region_ops,
- NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
- NSIM_DEV_DUMMY_REGION_SIZE);
+ __devlink_region_create(devlink, &dummy_region_ops,
+ NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
+ NSIM_DEV_DUMMY_REGION_SIZE);
return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
}
static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev)
{
- devlink_region_destroy(nsim_dev->dummy_region);
+ __devlink_region_destroy(nsim_dev->dummy_region);
}
static struct nsim_dev_port *
@@ -586,26 +625,50 @@ static void __nsim_dev_port_del(struct nsim_dev_port *nsim_dev_port)
list_del(&nsim_dev_port->list);
if (nsim_dev_port_is_vf(nsim_dev_port))
- devlink_rate_leaf_destroy(&nsim_dev_port->devlink_port);
+ __devlink_rate_leaf_destroy(&nsim_dev_port->devlink_port);
devlink_port_type_clear(devlink_port);
nsim_destroy(nsim_dev_port->ns);
nsim_dev_port_debugfs_exit(nsim_dev_port);
- devlink_port_unregister(devlink_port);
+ __devlink_port_unregister(devlink_port);
kfree(nsim_dev_port);
}
+static int __nsim_dev_port_add(struct nsim_dev *nsim_dev,
+ enum nsim_dev_port_type type,
+ unsigned int port_index);
+
+static int
+nsim_dev_port_add(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
+ unsigned int port_index)
+{
+ if (__nsim_dev_port_lookup(nsim_dev, type, port_index))
+ return -EEXIST;
+ return __nsim_dev_port_add(nsim_dev, type, port_index);
+}
+
+static int
+nsim_dev_port_del(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
+ unsigned int port_index)
+{
+ struct nsim_dev_port *nsim_dev_port;
+
+ nsim_dev_port = __nsim_dev_port_lookup(nsim_dev, type, port_index);
+ if (!nsim_dev_port)
+ return -ENOENT;
+ __nsim_dev_port_del(nsim_dev_port);
+ return 0;
+}
+
static int nsim_esw_legacy_enable(struct nsim_dev *nsim_dev,
struct netlink_ext_ack *extack)
{
struct devlink *devlink = priv_to_devlink(nsim_dev);
struct nsim_dev_port *nsim_dev_port, *tmp;
- devlink_rate_nodes_destroy(devlink);
- mutex_lock(&nsim_dev->port_list_lock);
+ __devlink_rate_nodes_destroy(devlink);
list_for_each_entry_safe(nsim_dev_port, tmp, &nsim_dev->port_list, list)
if (nsim_dev_port_is_vf(nsim_dev_port))
__nsim_dev_port_del(nsim_dev_port);
- mutex_unlock(&nsim_dev->port_list_lock);
nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
return 0;
}
@@ -613,11 +676,10 @@ static int nsim_esw_legacy_enable(struct nsim_dev *nsim_dev,
static int nsim_esw_switchdev_enable(struct nsim_dev *nsim_dev,
struct netlink_ext_ack *extack)
{
- struct nsim_bus_dev *nsim_bus_dev = nsim_dev->nsim_bus_dev;
int i, err;
for (i = 0; i < nsim_dev_get_vfs(nsim_dev); i++) {
- err = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_VF, i);
+ err = nsim_dev_port_add(nsim_dev, NSIM_DEV_PORT_TYPE_VF, i);
if (err) {
NL_SET_ERR_MSG_MOD(extack, "Failed to initialize VFs' netdevsim ports");
pr_err("Failed to initialize VF id=%d. %d.\n", i, err);
@@ -629,7 +691,7 @@ static int nsim_esw_switchdev_enable(struct nsim_dev *nsim_dev,
err_port_add_vfs:
for (i--; i >= 0; i--)
- nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_VF, i);
+ nsim_dev_port_del(nsim_dev, NSIM_DEV_PORT_TYPE_VF, i);
return err;
}
@@ -637,22 +699,16 @@ static int nsim_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
struct netlink_ext_ack *extack)
{
struct nsim_dev *nsim_dev = devlink_priv(devlink);
- int err = 0;
- mutex_lock(&nsim_dev->vfs_lock);
if (mode == nsim_dev->esw_mode)
- goto unlock;
+ return 0;
if (mode == DEVLINK_ESWITCH_MODE_LEGACY)
- err = nsim_esw_legacy_enable(nsim_dev, extack);
+ return nsim_esw_legacy_enable(nsim_dev, extack);
else if (mode == DEVLINK_ESWITCH_MODE_SWITCHDEV)
- err = nsim_esw_switchdev_enable(nsim_dev, extack);
- else
- err = -EINVAL;
+ return nsim_esw_switchdev_enable(nsim_dev, extack);
-unlock:
- mutex_unlock(&nsim_dev->vfs_lock);
- return err;
+ return -EINVAL;
}
static int nsim_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
@@ -847,30 +903,51 @@ static void nsim_dev_trap_report(struct nsim_dev_port *nsim_dev_port)
#define NSIM_TRAP_REPORT_INTERVAL_MS 100
+static void nsim_dev_trap_free(struct devlink *devlink,
+ struct nsim_trap_data *nsim_trap_data)
+{
+ kfree(nsim_trap_data->trap_policers_cnt_arr);
+ kfree(nsim_trap_data->trap_items_arr);
+ kfree(nsim_trap_data);
+
+ devlink_put(devlink);
+}
+
static void nsim_dev_trap_report_work(struct work_struct *work)
{
struct nsim_trap_data *nsim_trap_data;
struct nsim_dev_port *nsim_dev_port;
struct nsim_dev *nsim_dev;
+ struct devlink *devlink;
+ bool down;
nsim_trap_data = container_of(work, struct nsim_trap_data,
trap_report_dw.work);
nsim_dev = nsim_trap_data->nsim_dev;
+ devlink = priv_to_devlink(nsim_dev);
+
+ devlink_lock(devlink);
+ down = nsim_dev->trap_data != nsim_trap_data;
+ if (down)
+ goto unlock;
/* For each running port and enabled packet trap, generate a UDP
* packet with a random 5-tuple and report it.
*/
- mutex_lock(&nsim_dev->port_list_lock);
list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list) {
if (!netif_running(nsim_dev_port->ns->netdev))
continue;
nsim_dev_trap_report(nsim_dev_port);
}
- mutex_unlock(&nsim_dev->port_list_lock);
schedule_delayed_work(&nsim_dev->trap_data->trap_report_dw,
msecs_to_jiffies(NSIM_TRAP_REPORT_INTERVAL_MS));
+unlock:
+ devlink_unlock(devlink);
+
+ if (down)
+ nsim_dev_trap_free(devlink, nsim_trap_data);
}
static int nsim_dev_traps_init(struct devlink *devlink)
@@ -908,35 +985,36 @@ static int nsim_dev_traps_init(struct devlink *devlink)
nsim_trap_data->nsim_dev = nsim_dev;
nsim_dev->trap_data = nsim_trap_data;
- err = devlink_trap_policers_register(devlink, nsim_trap_policers_arr,
- policers_count);
+ err = __devlink_trap_policers_register(devlink, nsim_trap_policers_arr,
+ policers_count);
if (err)
goto err_trap_policers_cnt_free;
- err = devlink_trap_groups_register(devlink, nsim_trap_groups_arr,
- ARRAY_SIZE(nsim_trap_groups_arr));
+ err = __devlink_trap_groups_register(devlink, nsim_trap_groups_arr,
+ ARRAY_SIZE(nsim_trap_groups_arr));
if (err)
goto err_trap_policers_unregister;
- err = devlink_traps_register(devlink, nsim_traps_arr,
- ARRAY_SIZE(nsim_traps_arr), NULL);
+ err = __devlink_traps_register(devlink, nsim_traps_arr,
+ ARRAY_SIZE(nsim_traps_arr), NULL);
if (err)
goto err_trap_groups_unregister;
INIT_DELAYED_WORK(&nsim_dev->trap_data->trap_report_dw,
nsim_dev_trap_report_work);
+ devlink_get(devlink);
schedule_delayed_work(&nsim_dev->trap_data->trap_report_dw,
msecs_to_jiffies(NSIM_TRAP_REPORT_INTERVAL_MS));
return 0;
err_trap_groups_unregister:
- devlink_trap_groups_unregister(devlink, nsim_trap_groups_arr,
- ARRAY_SIZE(nsim_trap_groups_arr));
+ __devlink_trap_groups_unregister(devlink, nsim_trap_groups_arr,
+ ARRAY_SIZE(nsim_trap_groups_arr));
err_trap_policers_unregister:
- devlink_trap_policers_unregister(devlink, nsim_trap_policers_arr,
- ARRAY_SIZE(nsim_trap_policers_arr));
-err_trap_policers_cnt_free:
+ __devlink_trap_policers_unregister(devlink, nsim_trap_policers_arr,
+ ARRAY_SIZE(nsim_trap_policers_arr));
+err_trap_policers_cnt_free:;
kfree(nsim_trap_data->trap_policers_cnt_arr);
err_trap_items_free:
kfree(nsim_trap_data->trap_items_arr);
@@ -949,16 +1027,17 @@ static void nsim_dev_traps_exit(struct devlink *devlink)
{
struct nsim_dev *nsim_dev = devlink_priv(devlink);
- cancel_delayed_work_sync(&nsim_dev->trap_data->trap_report_dw);
- devlink_traps_unregister(devlink, nsim_traps_arr,
- ARRAY_SIZE(nsim_traps_arr));
- devlink_trap_groups_unregister(devlink, nsim_trap_groups_arr,
- ARRAY_SIZE(nsim_trap_groups_arr));
- devlink_trap_policers_unregister(devlink, nsim_trap_policers_arr,
- ARRAY_SIZE(nsim_trap_policers_arr));
- kfree(nsim_dev->trap_data->trap_policers_cnt_arr);
- kfree(nsim_dev->trap_data->trap_items_arr);
- kfree(nsim_dev->trap_data);
+ __devlink_traps_unregister(devlink, nsim_traps_arr,
+ ARRAY_SIZE(nsim_traps_arr));
+ __devlink_trap_groups_unregister(devlink, nsim_trap_groups_arr,
+ ARRAY_SIZE(nsim_trap_groups_arr));
+ __devlink_trap_policers_unregister(devlink, nsim_trap_policers_arr,
+ ARRAY_SIZE(nsim_trap_policers_arr));
+
+ if (cancel_delayed_work(&nsim_dev->trap_data->trap_report_dw))
+ nsim_dev_trap_free(devlink, nsim_dev->trap_data);
+ else
+ nsim_dev->trap_data = NULL;
}
static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
@@ -970,24 +1049,16 @@ static int nsim_dev_reload_down(struct devlink *devlink, bool netns_change,
struct netlink_ext_ack *extack)
{
struct nsim_dev *nsim_dev = devlink_priv(devlink);
- struct nsim_bus_dev *nsim_bus_dev;
-
- nsim_bus_dev = nsim_dev->nsim_bus_dev;
- if (!mutex_trylock(&nsim_bus_dev->nsim_bus_reload_lock))
- return -EOPNOTSUPP;
if (nsim_dev->dont_allow_reload) {
/* For testing purposes, user set debugfs dont_allow_reload
* value to true. So forbid it.
*/
NL_SET_ERR_MSG_MOD(extack, "User forbid the reload for testing purposes");
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
return -EOPNOTSUPP;
}
- nsim_bus_dev->in_reload = true;
nsim_dev_reload_destroy(nsim_dev);
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
return 0;
}
@@ -996,26 +1067,17 @@ static int nsim_dev_reload_up(struct devlink *devlink, enum devlink_reload_actio
struct netlink_ext_ack *extack)
{
struct nsim_dev *nsim_dev = devlink_priv(devlink);
- struct nsim_bus_dev *nsim_bus_dev;
- int ret;
-
- nsim_bus_dev = nsim_dev->nsim_bus_dev;
- mutex_lock(&nsim_bus_dev->nsim_bus_reload_lock);
- nsim_bus_dev->in_reload = false;
if (nsim_dev->fail_reload) {
/* For testing purposes, user set debugfs fail_reload
* value to true. Fail right away.
*/
NL_SET_ERR_MSG_MOD(extack, "User setup the reload to fail for testing purposes");
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
return -EINVAL;
}
*actions_performed = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT);
- ret = nsim_dev_reload_create(nsim_dev, extack);
- mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);
- return ret;
+ return nsim_dev_reload_create(nsim_dev, extack);
}
static int nsim_dev_info_get(struct devlink *devlink,
@@ -1348,6 +1410,8 @@ nsim_dev_devlink_trap_drop_counter_get(struct devlink *devlink,
}
static const struct devlink_ops nsim_dev_devlink_ops = {
+ .lock_flags = DEVLINK_LOCK_ALL_OPS,
+ .owner = THIS_MODULE,
.eswitch_mode_set = nsim_devlink_eswitch_mode_set,
.eswitch_mode_get = nsim_devlink_eswitch_mode_get,
.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT |
@@ -1405,8 +1469,9 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, enum nsim_dev_port_typ
memcpy(attrs.switch_id.id, nsim_dev->switch_id.id, nsim_dev->switch_id.id_len);
attrs.switch_id.id_len = nsim_dev->switch_id.id_len;
devlink_port_attrs_set(devlink_port, &attrs);
- err = devlink_port_register(priv_to_devlink(nsim_dev), devlink_port,
- nsim_dev_port->port_index);
+
+ err = __devlink_port_register(priv_to_devlink(nsim_dev), devlink_port,
+ nsim_dev_port->port_index);
if (err)
goto err_port_free;
@@ -1421,8 +1486,8 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, enum nsim_dev_port_typ
}
if (nsim_dev_port_is_vf(nsim_dev_port)) {
- err = devlink_rate_leaf_create(&nsim_dev_port->devlink_port,
- nsim_dev_port);
+ err = __devlink_rate_leaf_create(&nsim_dev_port->devlink_port,
+ nsim_dev_port);
if (err)
goto err_nsim_destroy;
}
@@ -1437,7 +1502,7 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, enum nsim_dev_port_typ
err_port_debugfs_exit:
nsim_dev_port_debugfs_exit(nsim_dev_port);
err_dl_port_unregister:
- devlink_port_unregister(devlink_port);
+ __devlink_port_unregister(devlink_port);
err_port_free:
kfree(nsim_dev_port);
return err;
@@ -1447,11 +1512,9 @@ static void nsim_dev_port_del_all(struct nsim_dev *nsim_dev)
{
struct nsim_dev_port *nsim_dev_port, *tmp;
- mutex_lock(&nsim_dev->port_list_lock);
list_for_each_entry_safe(nsim_dev_port, tmp,
&nsim_dev->port_list, list)
__nsim_dev_port_del(nsim_dev_port);
- mutex_unlock(&nsim_dev->port_list_lock);
}
static int nsim_dev_port_add_all(struct nsim_dev *nsim_dev,
@@ -1481,7 +1544,6 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
devlink = priv_to_devlink(nsim_dev);
nsim_dev = devlink_priv(devlink);
INIT_LIST_HEAD(&nsim_dev->port_list);
- mutex_init(&nsim_dev->port_list_lock);
nsim_dev->fw_update_status = true;
nsim_dev->fw_update_overwrite_mask = 0;
@@ -1489,7 +1551,12 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
err = nsim_dev_dummy_region_init(nsim_dev, devlink);
if (err)
- return err;
+ goto err_unlock;
+
+ nsim_dev->take_snapshot =
+ debugfs_create_file_unsafe("take_snapshot", 0200,
+ nsim_dev->ddir, nsim_dev,
+ &nsim_dev_take_snapshot_fops);
err = nsim_dev_traps_init(devlink);
if (err)
@@ -1512,12 +1579,6 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
err = nsim_dev_port_add_all(nsim_dev, nsim_bus_dev->port_count);
if (err)
goto err_psample_exit;
-
- nsim_dev->take_snapshot = debugfs_create_file("take_snapshot",
- 0200,
- nsim_dev->ddir,
- nsim_dev,
- &nsim_dev_take_snapshot_fops);
return 0;
err_psample_exit:
@@ -1530,6 +1591,7 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
nsim_dev_traps_exit(devlink);
err_dummy_region_exit:
nsim_dev_dummy_region_exit(nsim_dev);
+err_unlock:
return err;
}
@@ -1548,8 +1610,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
nsim_dev->switch_id.id_len = sizeof(nsim_dev->switch_id.id);
get_random_bytes(nsim_dev->switch_id.id, nsim_dev->switch_id.id_len);
INIT_LIST_HEAD(&nsim_dev->port_list);
- mutex_init(&nsim_dev->vfs_lock);
- mutex_init(&nsim_dev->port_list_lock);
nsim_dev->fw_update_status = true;
nsim_dev->fw_update_overwrite_mask = 0;
nsim_dev->max_macs = NSIM_DEV_MAX_MACS_DEFAULT;
@@ -1566,12 +1626,13 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
goto err_devlink_free;
}
+ devlink_lock_reg(devlink);
err = nsim_dev_resources_register(devlink);
if (err)
goto err_vfc_free;
- err = devlink_params_register(devlink, nsim_devlink_params,
- ARRAY_SIZE(nsim_devlink_params));
+ err = __devlink_params_register(devlink, nsim_devlink_params,
+ ARRAY_SIZE(nsim_devlink_params));
if (err)
goto err_dl_unregister;
nsim_devlink_set_params_init_values(nsim_dev, devlink);
@@ -1612,7 +1673,9 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
devlink_set_features(devlink, DEVLINK_F_RELOAD);
- devlink_register(devlink);
+ __devlink_register(devlink);
+ devlink_unlock_reg(devlink);
+
return 0;
err_psample_exit:
@@ -1630,14 +1693,15 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
err_dummy_region_exit:
nsim_dev_dummy_region_exit(nsim_dev);
err_params_unregister:
- devlink_params_unregister(devlink, nsim_devlink_params,
- ARRAY_SIZE(nsim_devlink_params));
+ __devlink_params_unregister(devlink, nsim_devlink_params,
+ ARRAY_SIZE(nsim_devlink_params));
err_dl_unregister:
- devlink_resources_unregister(devlink, NULL);
+ __devlink_resources_unregister(devlink, NULL);
err_vfc_free:
kfree(nsim_dev->vfconfigs);
+ devlink_unlock_reg(devlink);
err_devlink_free:
- devlink_free(devlink);
+ __devlink_free(devlink);
dev_set_drvdata(&nsim_bus_dev->dev, NULL);
return err;
}
@@ -1650,13 +1714,11 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
return;
debugfs_remove(nsim_dev->take_snapshot);
- mutex_lock(&nsim_dev->vfs_lock);
if (nsim_dev_get_vfs(nsim_dev)) {
nsim_bus_dev_set_vfs(nsim_dev->nsim_bus_dev, 0);
if (nsim_esw_mode_is_switchdev(nsim_dev))
nsim_esw_legacy_enable(nsim_dev, NULL);
}
- mutex_unlock(&nsim_dev->vfs_lock);
nsim_dev_port_del_all(nsim_dev);
nsim_dev_psample_exit(nsim_dev);
@@ -1664,7 +1726,6 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
nsim_fib_destroy(devlink, nsim_dev->fib_data);
nsim_dev_traps_exit(devlink);
nsim_dev_dummy_region_exit(nsim_dev);
- mutex_destroy(&nsim_dev->port_list_lock);
}
void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
@@ -1672,58 +1733,61 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
struct devlink *devlink = priv_to_devlink(nsim_dev);
- devlink_unregister(devlink);
+ devlink_lock_reg(devlink);
+ __devlink_unregister(devlink);
nsim_dev_reload_destroy(nsim_dev);
nsim_bpf_dev_exit(nsim_dev);
nsim_dev_debugfs_exit(nsim_dev);
- devlink_params_unregister(devlink, nsim_devlink_params,
- ARRAY_SIZE(nsim_devlink_params));
- devlink_resources_unregister(devlink, NULL);
+ __devlink_params_unregister(devlink, nsim_devlink_params,
+ ARRAY_SIZE(nsim_devlink_params));
+ __devlink_resources_unregister(devlink, NULL);
kfree(nsim_dev->vfconfigs);
- devlink_free(devlink);
dev_set_drvdata(&nsim_bus_dev->dev, NULL);
+ devlink_unlock_reg(devlink);
+ __devlink_free(devlink);
}
-int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
+int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev,
+ enum nsim_dev_port_type type,
unsigned int port_index)
{
struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
- int err;
+ struct devlink *devlink = priv_to_devlink(nsim_dev);
+ int ret;
- mutex_lock(&nsim_dev->port_list_lock);
- if (__nsim_dev_port_lookup(nsim_dev, type, port_index))
- err = -EEXIST;
- else
- err = __nsim_dev_port_add(nsim_dev, type, port_index);
- mutex_unlock(&nsim_dev->port_list_lock);
- return err;
+ devlink_lock(devlink);
+ ret = -EBUSY;
+ if (!devlink_is_reload_failed(devlink))
+ ret = nsim_dev_port_add(nsim_dev, type, port_index);
+ devlink_unlock(devlink);
+ return ret;
}
-int nsim_drv_port_del(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
+int nsim_drv_port_del(struct nsim_bus_dev *nsim_bus_dev,
+ enum nsim_dev_port_type type,
unsigned int port_index)
{
struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
- struct nsim_dev_port *nsim_dev_port;
- int err = 0;
+ struct devlink *devlink = priv_to_devlink(nsim_dev);
+ int ret;
- mutex_lock(&nsim_dev->port_list_lock);
- nsim_dev_port = __nsim_dev_port_lookup(nsim_dev, type, port_index);
- if (!nsim_dev_port)
- err = -ENOENT;
- else
- __nsim_dev_port_del(nsim_dev_port);
- mutex_unlock(&nsim_dev->port_list_lock);
- return err;
+ devlink_lock(devlink);
+ ret = -EBUSY;
+ if (!devlink_is_reload_failed(devlink))
+ ret = nsim_dev_port_del(nsim_dev, type, port_index);
+ devlink_unlock(devlink);
+ return ret;
}
int nsim_drv_configure_vfs(struct nsim_bus_dev *nsim_bus_dev,
unsigned int num_vfs)
{
struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
+ struct devlink *devlink = priv_to_devlink(nsim_dev);
int ret;
- mutex_lock(&nsim_dev->vfs_lock);
+ devlink_lock(devlink);
if (nsim_bus_dev->num_vfs == num_vfs) {
ret = 0;
goto exit_unlock;
@@ -1751,7 +1815,7 @@ int nsim_drv_configure_vfs(struct nsim_bus_dev *nsim_bus_dev,
}
exit_unlock:
- mutex_unlock(&nsim_dev->vfs_lock);
+ devlink_unlock(devlink);
return ret;
}
diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index 4300261e2f9e..e77670324c2a 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -1452,7 +1452,7 @@ static void nsim_fib_set_max_all(struct nsim_fib_data *data,
int err;
u64 val;
- err = devlink_resource_size_get(devlink, res_ids[i], &val);
+ err = __devlink_resource_size_get(devlink, res_ids[i], &val);
if (err)
val = (u64) -1;
nsim_fib_set_max(data, res_ids[i], val);
@@ -1561,26 +1561,26 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
goto err_nexthop_nb_unregister;
}
- devlink_resource_occ_get_register(devlink,
- NSIM_RESOURCE_IPV4_FIB,
- nsim_fib_ipv4_resource_occ_get,
- data);
- devlink_resource_occ_get_register(devlink,
- NSIM_RESOURCE_IPV4_FIB_RULES,
- nsim_fib_ipv4_rules_res_occ_get,
- data);
- devlink_resource_occ_get_register(devlink,
- NSIM_RESOURCE_IPV6_FIB,
- nsim_fib_ipv6_resource_occ_get,
- data);
- devlink_resource_occ_get_register(devlink,
- NSIM_RESOURCE_IPV6_FIB_RULES,
- nsim_fib_ipv6_rules_res_occ_get,
- data);
- devlink_resource_occ_get_register(devlink,
- NSIM_RESOURCE_NEXTHOPS,
- nsim_fib_nexthops_res_occ_get,
- data);
+ __devlink_resource_occ_get_register(devlink,
+ NSIM_RESOURCE_IPV4_FIB,
+ nsim_fib_ipv4_resource_occ_get,
+ data);
+ __devlink_resource_occ_get_register(devlink,
+ NSIM_RESOURCE_IPV4_FIB_RULES,
+ nsim_fib_ipv4_rules_res_occ_get,
+ data);
+ __devlink_resource_occ_get_register(devlink,
+ NSIM_RESOURCE_IPV6_FIB,
+ nsim_fib_ipv6_resource_occ_get,
+ data);
+ __devlink_resource_occ_get_register(devlink,
+ NSIM_RESOURCE_IPV6_FIB_RULES,
+ nsim_fib_ipv6_rules_res_occ_get,
+ data);
+ __devlink_resource_occ_get_register(devlink,
+ NSIM_RESOURCE_NEXTHOPS,
+ nsim_fib_nexthops_res_occ_get,
+ data);
return data;
err_nexthop_nb_unregister:
@@ -1603,16 +1603,16 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *data)
{
- devlink_resource_occ_get_unregister(devlink,
- NSIM_RESOURCE_NEXTHOPS);
- devlink_resource_occ_get_unregister(devlink,
- NSIM_RESOURCE_IPV6_FIB_RULES);
- devlink_resource_occ_get_unregister(devlink,
- NSIM_RESOURCE_IPV6_FIB);
- devlink_resource_occ_get_unregister(devlink,
- NSIM_RESOURCE_IPV4_FIB_RULES);
- devlink_resource_occ_get_unregister(devlink,
- NSIM_RESOURCE_IPV4_FIB);
+ __devlink_resource_occ_get_unregister(devlink,
+ NSIM_RESOURCE_NEXTHOPS);
+ __devlink_resource_occ_get_unregister(devlink,
+ NSIM_RESOURCE_IPV6_FIB_RULES);
+ __devlink_resource_occ_get_unregister(devlink,
+ NSIM_RESOURCE_IPV6_FIB);
+ __devlink_resource_occ_get_unregister(devlink,
+ NSIM_RESOURCE_IPV4_FIB_RULES);
+ __devlink_resource_occ_get_unregister(devlink,
+ NSIM_RESOURCE_IPV4_FIB);
unregister_fib_notifier(devlink_net(devlink), &data->fib_nb);
unregister_nexthop_notifier(devlink_net(devlink), &data->nexthop_nb);
flush_work(&data->fib_event_work);
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index c49771f27f17..97ff4e2e72ac 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -239,7 +239,6 @@ struct nsim_dev {
struct dentry *take_snapshot;
struct dentry *nodes_ddir;
- struct mutex vfs_lock; /* Protects vfconfigs */
struct nsim_vf_config *vfconfigs;
struct bpf_offload_dev *bpf_dev;
@@ -252,7 +251,6 @@ struct nsim_dev {
struct list_head bpf_bound_maps;
struct netdev_phys_item_id switch_id;
struct list_head port_list;
- struct mutex port_list_lock; /* protects port list */
bool fw_update_status;
u32 fw_update_overwrite_mask;
u32 max_macs;
@@ -355,9 +353,6 @@ struct nsim_bus_dev {
*/
unsigned int max_vfs;
unsigned int num_vfs;
- /* Lock for devlink->reload_enabled in netdevsim module */
- struct mutex nsim_bus_reload_lock;
- bool in_reload;
bool init;
};
--
2.31.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
` (4 preceding siblings ...)
2021-10-30 23:12 ` [RFC 5/5] netdevsim: use devlink locking Jakub Kicinski
@ 2021-10-31 7:23 ` Leon Romanovsky
2021-11-01 14:32 ` Jakub Kicinski
2021-11-01 20:04 ` Edwin Peer
2021-11-03 9:03 ` Jiri Pirko
6 siblings, 2 replies; 23+ messages in thread
From: Leon Romanovsky @ 2021-10-31 7:23 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: idosch, edwin.peer, jiri, netdev
On Sat, Oct 30, 2021 at 04:12:49PM -0700, Jakub Kicinski wrote:
> This implements what I think is the right direction for devlink
> API overhaul. It's an early RFC/PoC because the body of code is
> rather large so I'd appreciate feedback early... The patches are
> very roughly split, the point of the posting is primarily to prove
> that from the driver side the conversion is an net improvement
> in complexity.
>
> IMO the problems with devlink locking are caused by two things:
>
> (1) driver has no way to block devlink calls like it does in case
> of netedev / rtnl_lock, note that for devlink each driver has
> it's own lock so contention is not an issue;
>
> (2) sometimes devlink calls into the driver without holding its lock
> - for operations which may need the driver to call devlink (port
> splitting, switch config, reload etc.), the circular dependency
> is there, the driver can't solve this problem.
>
> This set "fixes" the ordering by allowing the driver to participate
> in locking. The lock order remains:
>
> device_lock -> [devlink_mutex] -> devlink instance -> rtnl_lock
>
> but now driver can take devlink lock itself, and _ALL_ devlink ops
> are locked.
>
> The expectation is that driver will take the devlink instance lock
> on its probe and remove paths, hence protecting all configuration
> paths with the devlink instance lock.
>
> This is clearly demonstrated by the netdevsim conversion. All entry
> points to driver config are protected by devlink instance lock, so
> the driver switches to the "I'm already holding the devlink lock" API
> when calling devlink. All driver locks and trickery is removed.
>
> The part which is slightly more challanging is quiescing async entry
> points which need to be closed on the devlink reload path (workqueue,
> debugfs etc.) and which also take devlink instance lock. For that we
> need to be able to take refs on the devlink instance and let them
> clean up after themselves rather than waiting synchronously.
>
> That last part is not 100% finished in this patch set - it works but
> we need the driver to take devlink_mutex (the global lock) from its
> probe/remove path. I hope this is good enough for an RFC, the problem
> is easily solved by protecting the devlink XArray with something else
> than devlink_mutex and/or not holding devlink_mutex around each op
> (so that there is no ordering between the global and instance locks).
> Speaking of not holding devlink_mutex around each op this patch set
> also opens the path for parallel ops to different devlink instances
> which is currently impossible because all user space calls take
> devlink_mutex...
No, please no.
This RFC goes against everything that I tried to make before.
It pushes complexity from the devlink core code to the drivers by
hiding bugs in the drivers. The average driver author doesn't know
locking well and won't be able to use devlink reference counting
correctly.
Already your netdevsim conversion shows that it is hard to do and for
complex drivers it will be a nightmare to convert and maintain.
At the end (long run), we will find ourselves with a maze of completely
random devlink_get and devlink_lock, exactly like we have now with RTNL
lock, which people add/delete based on their testing if to judge by the
commit messages.
Regarding devlink_mutex, I'm on the path to removing it, this is why
XArray and reference counting was used in the first place.
Our current complexity is due to the situation that devlink_reload doesn't
behave like any other _set_ commands, where it supposed to take devlink->lock
internally.
Please give me (testing) time and I'll post a full solution that separates
_get_ from _set_ commands by changing devlink->lock to be RW semaphore.
Together with logic to understand that we are in devlink_reload will
give us a solution that won't require changing drivers and won't push
locking burden on them.
Please, let's not give up on standalone devlink implementation without
drivers need to know internal devlink details. It is hard to do but possible.
Thanks
>
> The patches are on top of the cleanups I posted earlier.
>
> Jakub Kicinski (5):
> devlink: add unlocked APIs
> devlink: add API for explicit locking
> devlink: allow locking of all ops
> netdevsim: minor code move
> netdevsim: use devlink locking
>
> drivers/net/netdevsim/bus.c | 19 -
> drivers/net/netdevsim/dev.c | 450 ++++++++-------
> drivers/net/netdevsim/fib.c | 62 +--
> drivers/net/netdevsim/netdevsim.h | 5 -
> include/net/devlink.h | 88 +++
> net/core/devlink.c | 875 +++++++++++++++++++++---------
> 6 files changed, 996 insertions(+), 503 deletions(-)
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-10-31 7:23 ` [RFC 0/5] devlink: add an explicit locking API Leon Romanovsky
@ 2021-11-01 14:32 ` Jakub Kicinski
2021-11-01 18:36 ` Leon Romanovsky
2021-11-01 20:04 ` Edwin Peer
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-01 14:32 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: idosch, edwin.peer, jiri, netdev
On Sun, 31 Oct 2021 09:23:42 +0200 Leon Romanovsky wrote:
> On Sat, Oct 30, 2021 at 04:12:49PM -0700, Jakub Kicinski wrote:
> > This implements what I think is the right direction for devlink
> > API overhaul. It's an early RFC/PoC because the body of code is
> > rather large so I'd appreciate feedback early... The patches are
> > very roughly split, the point of the posting is primarily to prove
> > that from the driver side the conversion is an net improvement
> > in complexity.
> >
> > IMO the problems with devlink locking are caused by two things:
> >
> > (1) driver has no way to block devlink calls like it does in case
> > of netedev / rtnl_lock, note that for devlink each driver has
> > it's own lock so contention is not an issue;
> >
> > (2) sometimes devlink calls into the driver without holding its lock
> > - for operations which may need the driver to call devlink (port
> > splitting, switch config, reload etc.), the circular dependency
> > is there, the driver can't solve this problem.
> >
> > This set "fixes" the ordering by allowing the driver to participate
> > in locking. The lock order remains:
> >
> > device_lock -> [devlink_mutex] -> devlink instance -> rtnl_lock
> >
> > but now driver can take devlink lock itself, and _ALL_ devlink ops
> > are locked.
> >
> > The expectation is that driver will take the devlink instance lock
> > on its probe and remove paths, hence protecting all configuration
> > paths with the devlink instance lock.
> >
> > This is clearly demonstrated by the netdevsim conversion. All entry
> > points to driver config are protected by devlink instance lock, so
> > the driver switches to the "I'm already holding the devlink lock" API
> > when calling devlink. All driver locks and trickery is removed.
> >
> > The part which is slightly more challanging is quiescing async entry
> > points which need to be closed on the devlink reload path (workqueue,
> > debugfs etc.) and which also take devlink instance lock. For that we
> > need to be able to take refs on the devlink instance and let them
> > clean up after themselves rather than waiting synchronously.
> >
> > That last part is not 100% finished in this patch set - it works but
> > we need the driver to take devlink_mutex (the global lock) from its
> > probe/remove path. I hope this is good enough for an RFC, the problem
> > is easily solved by protecting the devlink XArray with something else
> > than devlink_mutex and/or not holding devlink_mutex around each op
> > (so that there is no ordering between the global and instance locks).
> > Speaking of not holding devlink_mutex around each op this patch set
> > also opens the path for parallel ops to different devlink instances
> > which is currently impossible because all user space calls take
> > devlink_mutex...
>
> No, please no.
>
> This RFC goes against everything that I tried to make before.
Yup, that we agree on :) I think the reload makes your approach
a dead end.
> It pushes complexity from the devlink core code to the drivers by
> hiding bugs in the drivers. The average driver author doesn't know
> locking well and won't be able to use devlink reference counting
> correctly.
>
> Already your netdevsim conversion shows that it is hard to do and for
> complex drivers it will be a nightmare to convert and maintain.
IDK, the code is very idiomatic for the kernel. Most drivers should not
have to use the ref counts, and I'll provide good enough docs to understand
how to use it.
In fact drivers which don't implement currently unlocked ops require no
changes between you API and this one.
I think "complex core, simple drivers" is the right trade off. With
this set it is now and order of magnitude easier to reason about
netdevsim's locking.
> At the end (long run), we will find ourselves with a maze of completely
> random devlink_get and devlink_lock, exactly like we have now with RTNL
> lock, which people add/delete based on their testing if to judge by the
> commit messages.
>
> Regarding devlink_mutex, I'm on the path to removing it, this is why
> XArray and reference counting was used in the first place.
devlink_mutex is the only thing protecting the "unlocked" calls right
now, you'd be placing a lot of implicit expectations about driver's
internal locking if you do that.
> Our current complexity is due to the situation that devlink_reload doesn't
> behave like any other _set_ commands, where it supposed to take devlink->lock
> internally.
>
> Please give me (testing) time and I'll post a full solution that separates
> _get_ from _set_ commands by changing devlink->lock to be RW semaphore.
> Together with logic to understand that we are in devlink_reload will
> give us a solution that won't require changing drivers and won't push
> locking burden on them.
How is RW semaphore going to solve the problem that ops are unlocked
and have to take the instance lock from within to add/remove ports?
> Please, let's not give up on standalone devlink implementation without
> drivers need to know internal devlink details. It is hard to do but possible.
We may just disagree on this one. Please answer my question above -
so far IDK how you're going to fix the problem of re-reg'ing subobjects
from the reload path.
My experience writing drivers is that it was painfully unclear what
the devlink locking rules are. Sounds like you'd make them even more
complicated.
This RFC makes the rules simple - all devlink ops are locked.
For the convenience of the drivers they can also take the instance lock
whenever they want to prevent ops from being called. Experience with
rtnl_lock teaches us that this is very useful for drivers.
Accessible lock + refs is how driver core works.
Let me sketch out the driver facing documentation here.
---->8------
Devlink locking guide
=====================
Basic locking
-------------
After calling devlink_register() and until devlink_unregister() is
called the devlink instance is "live" and any callback can be invoked.
If you want to prevent devlink from calling the driver take the
instance lock (devlink_lock()), you will need to call the locked
version of devlink API (prefixed with '__devlink') while holding the
lock.
All devlink callbacks (including health callbacks) are locked so locked
devlink API needs to be used from within them.
Note that devlink locks are per-instance (like device_lock but unlike
rtnl_lock).
Reference use
-------------
Advanced use of devlink may require taking references on the instance.
The primary use for this is to avoid deadlocks between reload/remove
paths and async works which need to take the devlink instance lock.
devlink_get() takes a reference on the instance and should only be
called when the driver is sure to already have some references.
The reference alone does _not_ prevent devlink instance from getting
unregistered so you probably want to check if devlink_is_alive() before
using that ref.
For example a use with a workqueue may look like this:
```
/* schedule work from a path which knows devlink is alive */
void sched_my_work()
{
devlink_get(devlink);
if (!schedule_work(priv->some_work))
/* it was already scheduled */
devlink_put(devlink);
}
void work_fn()
{
devlink_lock(devlink);
if (devlink_is_alive(devlink))
perform_the_task();
devlink_lock(devlink);
/* give up our reference */
devlink_put(devlink);
}
```
If you want to use devlink references from a context which does not take
a reference on the driver module (e.g. workqueue) - make sure to set
.owner of devlink_ops to THIS_MODULE.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-01 14:32 ` Jakub Kicinski
@ 2021-11-01 18:36 ` Leon Romanovsky
2021-11-01 21:16 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Leon Romanovsky @ 2021-11-01 18:36 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: idosch, edwin.peer, jiri, netdev
On Mon, Nov 01, 2021 at 07:32:59AM -0700, Jakub Kicinski wrote:
> On Sun, 31 Oct 2021 09:23:42 +0200 Leon Romanovsky wrote:
> > On Sat, Oct 30, 2021 at 04:12:49PM -0700, Jakub Kicinski wrote:
> > > This implements what I think is the right direction for devlink
> > > API overhaul. It's an early RFC/PoC because the body of code is
> > > rather large so I'd appreciate feedback early... The patches are
> > > very roughly split, the point of the posting is primarily to prove
> > > that from the driver side the conversion is an net improvement
> > > in complexity.
> > >
> > > IMO the problems with devlink locking are caused by two things:
> > >
> > > (1) driver has no way to block devlink calls like it does in case
> > > of netedev / rtnl_lock, note that for devlink each driver has
> > > it's own lock so contention is not an issue;
> > >
> > > (2) sometimes devlink calls into the driver without holding its lock
> > > - for operations which may need the driver to call devlink (port
> > > splitting, switch config, reload etc.), the circular dependency
> > > is there, the driver can't solve this problem.
> > >
> > > This set "fixes" the ordering by allowing the driver to participate
> > > in locking. The lock order remains:
> > >
> > > device_lock -> [devlink_mutex] -> devlink instance -> rtnl_lock
> > >
> > > but now driver can take devlink lock itself, and _ALL_ devlink ops
> > > are locked.
> > >
> > > The expectation is that driver will take the devlink instance lock
> > > on its probe and remove paths, hence protecting all configuration
> > > paths with the devlink instance lock.
> > >
> > > This is clearly demonstrated by the netdevsim conversion. All entry
> > > points to driver config are protected by devlink instance lock, so
> > > the driver switches to the "I'm already holding the devlink lock" API
> > > when calling devlink. All driver locks and trickery is removed.
> > >
> > > The part which is slightly more challanging is quiescing async entry
> > > points which need to be closed on the devlink reload path (workqueue,
> > > debugfs etc.) and which also take devlink instance lock. For that we
> > > need to be able to take refs on the devlink instance and let them
> > > clean up after themselves rather than waiting synchronously.
> > >
> > > That last part is not 100% finished in this patch set - it works but
> > > we need the driver to take devlink_mutex (the global lock) from its
> > > probe/remove path. I hope this is good enough for an RFC, the problem
> > > is easily solved by protecting the devlink XArray with something else
> > > than devlink_mutex and/or not holding devlink_mutex around each op
> > > (so that there is no ordering between the global and instance locks).
> > > Speaking of not holding devlink_mutex around each op this patch set
> > > also opens the path for parallel ops to different devlink instances
> > > which is currently impossible because all user space calls take
> > > devlink_mutex...
> >
> > No, please no.
<...>
> How is RW semaphore going to solve the problem that ops are unlocked
> and have to take the instance lock from within to add/remove ports?
This is three step process, but mainly it is first step. We need to make
sure that functions that can re-entry will use nested locks.
Steps:
1. Use proper locking API that supports nesting:
https://lore.kernel.org/netdev/YYABqfFy%2F%2Fg5Gdis@nanopsycho/T/#mf9dc5cac2013abe413545bbe4a09cc231ae209a4
2. Convert devlink->lock to be RW semaphore:
commit 4506dd3a90a82a0b6bde238f507907747ab88407
Author: Leon Romanovsky <leon@kernel.org>
Date: Sun Oct 24 16:54:16 2021 +0300
devlink: Convert devlink lock to be RW semaphore
This is naive conversion of devlink->lock to RW semaphore, so we will be
able to differentiate commands that require exclusive access vs. parallel
ready-to-run ones.
All "set" commands that used devlink->lock are converted to write lock,
while all "get" commands are marked with read lock.
@@ -578,8 +584,12 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
mutex_unlock(&devlink_mutex);
return PTR_ERR(devlink);
}
- if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
- mutex_lock(&devlink->lock);
+
+ if (~ops->internal_flags & DEVLINK_NL_FLAG_SHARED_ACCESS)
+ down_write(&devlink->rwsem);
+ else
+ down_read(&devlink->rwsem);
+
3. Drop devlink_mutex:
commit 3177af9971c4cd95f9633aeb9b0434687da62fd0
Author: Leon Romanovsky <leon@kernel.org>
Date: Sun Oct 31 16:05:40 2021 +0200
devlink: Use xarray locking mechanism instead big devlink lock
The conversion to XArray together with devlink reference counting
allows us reuse the following locking pattern:
xa_lock()
xa_for_each() {
devlink_try_get()
xa_unlock()
....
xa_lock()
}
This pattern gives us a way to run any commands between xa_unlock() and
xa_lock() without big devlink mutex, while making sure that devlink instance
won't be released.
Steps 2 and 3 were not posted due to merge window and my desire to get
mileage in our regression.
>
> > Please, let's not give up on standalone devlink implementation without
> > drivers need to know internal devlink details. It is hard to do but possible.
>
> We may just disagree on this one. Please answer my question above -
> so far IDK how you're going to fix the problem of re-reg'ing subobjects
> from the reload path.
>
> My experience writing drivers is that it was painfully unclear what
> the devlink locking rules are. Sounds like you'd make them even more
> complicated.
How? I have only one rule:
* Driver author should be aware that between devlink_register() and
devlink_unregister(), users can send netlink commands.
In your solution, driver authors will need to follow whole devlink
how-to-use book.
>
> This RFC makes the rules simple - all devlink ops are locked.
>
> For the convenience of the drivers they can also take the instance lock
> whenever they want to prevent ops from being called. Experience with
> rtnl_lock teaches us that this is very useful for drivers.
Strange, I see completely opposite picture in the git log with so much
changes that have Fixes line and add/remove/mention rtnl_lock. Maybe it
is useful, but almost all if not all drivers full of fixes of rtnl_lock
usage. I don't want same for the devlink.
Thanks
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-10-31 7:23 ` [RFC 0/5] devlink: add an explicit locking API Leon Romanovsky
2021-11-01 14:32 ` Jakub Kicinski
@ 2021-11-01 20:04 ` Edwin Peer
2021-11-02 7:44 ` Leon Romanovsky
1 sibling, 1 reply; 23+ messages in thread
From: Edwin Peer @ 2021-11-01 20:04 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jakub Kicinski, Ido Schimmel, Jiri Pirko, netdev
On Sun, Oct 31, 2021 at 12:23 AM Leon Romanovsky <leon@kernel.org> wrote:
> The average driver author doesn't know locking well and won't be able to
> use devlink reference counting correctly.
I think this problem largely only exists to the extent that locking
and lifecycle requirements are poorly documented. :P
Regards,
Edwin Peer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-01 18:36 ` Leon Romanovsky
@ 2021-11-01 21:16 ` Jakub Kicinski
2021-11-02 8:08 ` Leon Romanovsky
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-01 21:16 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: idosch, edwin.peer, jiri, netdev
On Mon, 1 Nov 2021 20:36:15 +0200 Leon Romanovsky wrote:
> > How is RW semaphore going to solve the problem that ops are unlocked
> > and have to take the instance lock from within to add/remove ports?
>
> This is three step process, but mainly it is first step. We need to make
> sure that functions that can re-entry will use nested locks.
>
> Steps:
> 1. Use proper locking API that supports nesting:
> https://lore.kernel.org/netdev/YYABqfFy%2F%2Fg5Gdis@nanopsycho/T/#mf9dc5cac2013abe413545bbe4a09cc231ae209a4
Whether we provide the an unlocked API or allow lock nesting
on the driver API is not that important to me.
> 2. Convert devlink->lock to be RW semaphore:
> commit 4506dd3a90a82a0b6bde238f507907747ab88407
> Author: Leon Romanovsky <leon@kernel.org>
> Date: Sun Oct 24 16:54:16 2021 +0300
>
> devlink: Convert devlink lock to be RW semaphore
>
> This is naive conversion of devlink->lock to RW semaphore, so we will be
> able to differentiate commands that require exclusive access vs. parallel
> ready-to-run ones.
>
> All "set" commands that used devlink->lock are converted to write lock,
> while all "get" commands are marked with read lock.
>
> @@ -578,8 +584,12 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
> mutex_unlock(&devlink_mutex);
> return PTR_ERR(devlink);
> }
> - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> - mutex_lock(&devlink->lock);
> +
> + if (~ops->internal_flags & DEVLINK_NL_FLAG_SHARED_ACCESS)
> + down_write(&devlink->rwsem);
> + else
> + down_read(&devlink->rwsem);
> +
IIUC the RW sem thing is an optimization, it's besides the point.
> 3. Drop devlink_mutex:
> commit 3177af9971c4cd95f9633aeb9b0434687da62fd0
> Author: Leon Romanovsky <leon@kernel.org>
> Date: Sun Oct 31 16:05:40 2021 +0200
>
> devlink: Use xarray locking mechanism instead big devlink lock
>
> The conversion to XArray together with devlink reference counting
> allows us reuse the following locking pattern:
> xa_lock()
> xa_for_each() {
> devlink_try_get()
> xa_unlock()
> ....
> xa_lock()
> }
>
> This pattern gives us a way to run any commands between xa_unlock() and
> xa_lock() without big devlink mutex, while making sure that devlink instance
> won't be released.
Yup, I think this part we agree on.
> Steps 2 and 3 were not posted due to merge window and my desire to get
> mileage in our regression.
:)
> > > Please, let's not give up on standalone devlink implementation without
> > > drivers need to know internal devlink details. It is hard to do but possible.
> >
> > We may just disagree on this one. Please answer my question above -
> > so far IDK how you're going to fix the problem of re-reg'ing subobjects
> > from the reload path.
> >
> > My experience writing drivers is that it was painfully unclear what
> > the devlink locking rules are. Sounds like you'd make them even more
> > complicated.
>
> How? I have only one rule:
> * Driver author should be aware that between devlink_register() and
> devlink_unregister(), users can send netlink commands.
>
> In your solution, driver authors will need to follow whole devlink
> how-to-use book.
Only if they need refs. If you don't the API is the same as yours.
IOW you don't provide an API for advanced use cases at all. You force
the drivers to implement their own reference counting and locking.
I want them to just rely on devlink as a framework.
How many driver locks do you have in netdevsim after conversion?
All 3? How do you add a port to the instance from sysfs without a
AB / BA deadlock between the port lock and the devlink lock if the
driver can't take the devlink lock? Are you still going to need the
"in_reload" wart?
> > This RFC makes the rules simple - all devlink ops are locked.
> >
> > For the convenience of the drivers they can also take the instance lock
> > whenever they want to prevent ops from being called. Experience with
> > rtnl_lock teaches us that this is very useful for drivers.
>
> Strange, I see completely opposite picture in the git log with so much
> changes that have Fixes line and add/remove/mention rtnl_lock. Maybe it
> is useful, but almost all if not all drivers full of fixes of rtnl_lock
> usage. I don't want same for the devlink.
If you don't provide a locking API to the drivers you'll have to fix 2x
the bugs, and each of them subtly different. At least maintainers know
what rtnl_lock rules are.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-01 20:04 ` Edwin Peer
@ 2021-11-02 7:44 ` Leon Romanovsky
2021-11-02 15:16 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Leon Romanovsky @ 2021-11-02 7:44 UTC (permalink / raw)
To: Edwin Peer; +Cc: Jakub Kicinski, Ido Schimmel, Jiri Pirko, netdev
On Mon, Nov 01, 2021 at 01:04:03PM -0700, Edwin Peer wrote:
> On Sun, Oct 31, 2021 at 12:23 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> > The average driver author doesn't know locking well and won't be able to
> > use devlink reference counting correctly.
>
> I think this problem largely only exists to the extent that locking
> and lifecycle requirements are poorly documented. :P
I'm talking about general locking concepts that are perfectly documented
and still people do crazy things with it. :)
>
> Regards,
> Edwin Peer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-01 21:16 ` Jakub Kicinski
@ 2021-11-02 8:08 ` Leon Romanovsky
2021-11-02 15:14 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Leon Romanovsky @ 2021-11-02 8:08 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: idosch, edwin.peer, jiri, netdev
On Mon, Nov 01, 2021 at 02:16:13PM -0700, Jakub Kicinski wrote:
> On Mon, 1 Nov 2021 20:36:15 +0200 Leon Romanovsky wrote:
> > > How is RW semaphore going to solve the problem that ops are unlocked
> > > and have to take the instance lock from within to add/remove ports?
> >
> > This is three step process, but mainly it is first step. We need to make
> > sure that functions that can re-entry will use nested locks.
> >
> > Steps:
> > 1. Use proper locking API that supports nesting:
> > https://lore.kernel.org/netdev/YYABqfFy%2F%2Fg5Gdis@nanopsycho/T/#mf9dc5cac2013abe413545bbe4a09cc231ae209a4
>
> Whether we provide the an unlocked API or allow lock nesting
> on the driver API is not that important to me.
Thanks
>
> > 2. Convert devlink->lock to be RW semaphore:
> > commit 4506dd3a90a82a0b6bde238f507907747ab88407
> > Author: Leon Romanovsky <leon@kernel.org>
> > Date: Sun Oct 24 16:54:16 2021 +0300
> >
> > devlink: Convert devlink lock to be RW semaphore
> >
> > This is naive conversion of devlink->lock to RW semaphore, so we will be
> > able to differentiate commands that require exclusive access vs. parallel
> > ready-to-run ones.
> >
> > All "set" commands that used devlink->lock are converted to write lock,
> > while all "get" commands are marked with read lock.
> >
> > @@ -578,8 +584,12 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
> > mutex_unlock(&devlink_mutex);
> > return PTR_ERR(devlink);
> > }
> > - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> > - mutex_lock(&devlink->lock);
> > +
> > + if (~ops->internal_flags & DEVLINK_NL_FLAG_SHARED_ACCESS)
> > + down_write(&devlink->rwsem);
> > + else
> > + down_read(&devlink->rwsem);
> > +
>
> IIUC the RW sem thing is an optimization, it's besides the point.
I need RW as a way to ensure "exclusive" access during _set_ operation.
It is not an optimization, but simple way to understand if parallel
access is possible at this specific point of time.
>
> > 3. Drop devlink_mutex:
> > commit 3177af9971c4cd95f9633aeb9b0434687da62fd0
> > Author: Leon Romanovsky <leon@kernel.org>
> > Date: Sun Oct 31 16:05:40 2021 +0200
> >
> > devlink: Use xarray locking mechanism instead big devlink lock
> >
> > The conversion to XArray together with devlink reference counting
> > allows us reuse the following locking pattern:
> > xa_lock()
> > xa_for_each() {
> > devlink_try_get()
> > xa_unlock()
> > ....
> > xa_lock()
> > }
> >
> > This pattern gives us a way to run any commands between xa_unlock() and
> > xa_lock() without big devlink mutex, while making sure that devlink instance
> > won't be released.
>
> Yup, I think this part we agree on.
>
> > Steps 2 and 3 were not posted due to merge window and my desire to get
> > mileage in our regression.
>
> :)
>
> > > > Please, let's not give up on standalone devlink implementation without
> > > > drivers need to know internal devlink details. It is hard to do but possible.
> > >
> > > We may just disagree on this one. Please answer my question above -
> > > so far IDK how you're going to fix the problem of re-reg'ing subobjects
> > > from the reload path.
> > >
> > > My experience writing drivers is that it was painfully unclear what
> > > the devlink locking rules are. Sounds like you'd make them even more
> > > complicated.
> >
> > How? I have only one rule:
> > * Driver author should be aware that between devlink_register() and
> > devlink_unregister(), users can send netlink commands.
> >
> > In your solution, driver authors will need to follow whole devlink
> > how-to-use book.
>
> Only if they need refs. If you don't the API is the same as yours.
>
> IOW you don't provide an API for advanced use cases at all. You force
> the drivers to implement their own reference counting and locking.
The complex drivers already do it anyway, because they need to reference
counting their own structures to make sure that the lifetime of these
structures meats their model.
> I want them to just rely on devlink as a framework.
And I don't :). For me devlink is a way to configure device, not manage
lifetime of driver specific data structures.
>
> How many driver locks do you have in netdevsim after conversion?
> All 3? How do you add a port to the instance from sysfs without a
> AB / BA deadlock between the port lock and the devlink lock if the
> driver can't take the devlink lock? Are you still going to need the
> "in_reload" wart?
I don't know yet, because as you wrote before netdevsim is for
prototyping and such ABBA deadlock doesn't exist in real devices.
My current focus is real devices for now.
Maybe the solution will be to go away from sysfs completely. We will see.
>
> > > This RFC makes the rules simple - all devlink ops are locked.
> > >
> > > For the convenience of the drivers they can also take the instance lock
> > > whenever they want to prevent ops from being called. Experience with
> > > rtnl_lock teaches us that this is very useful for drivers.
> >
> > Strange, I see completely opposite picture in the git log with so much
> > changes that have Fixes line and add/remove/mention rtnl_lock. Maybe it
> > is useful, but almost all if not all drivers full of fixes of rtnl_lock
> > usage. I don't want same for the devlink.
>
> If you don't provide a locking API to the drivers you'll have to fix 2x
> the bugs, and each of them subtly different. At least maintainers know
> what rtnl_lock rules are.
I clearly remember this patch and the sentence "...in
some devices' resume function(igb_resum,igc_resume) they calls rtnl_lock()
again". The word "... some ..." hints to me that maintainers have different
opinion on how to use rtnl_lock.
https://lore.kernel.org/netdev/20210809032809.1224002-1-acelan.kao@canonical.com/
Thanks
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-02 8:08 ` Leon Romanovsky
@ 2021-11-02 15:14 ` Jakub Kicinski
2021-11-02 18:14 ` Leon Romanovsky
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-02 15:14 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: idosch, edwin.peer, jiri, netdev
On Tue, 2 Nov 2021 10:08:39 +0200 Leon Romanovsky wrote:
> On Mon, Nov 01, 2021 at 02:16:13PM -0700, Jakub Kicinski wrote:
> > On Mon, 1 Nov 2021 20:36:15 +0200 Leon Romanovsky wrote:
> > > This is three step process, but mainly it is first step. We need to make
> > > sure that functions that can re-entry will use nested locks.
> > >
> > > Steps:
> > > 1. Use proper locking API that supports nesting:
> > > https://lore.kernel.org/netdev/YYABqfFy%2F%2Fg5Gdis@nanopsycho/T/#mf9dc5cac2013abe413545bbe4a09cc231ae209a4
> >
> > Whether we provide the an unlocked API or allow lock nesting
> > on the driver API is not that important to me.
>
> Thanks
Not sure what you're thanking for. I still prefer two explicit APIs.
Allowing nesting is not really necessary here. Callers know whether
they hold the lock or not.
Please see netdevsim, it takes the devlink_lock() at entry points to
the driver - which in turn means the API which takes a lock can be
removed once all drivers are converted.
> > > 2. Convert devlink->lock to be RW semaphore:
> > > commit 4506dd3a90a82a0b6bde238f507907747ab88407
> > > Author: Leon Romanovsky <leon@kernel.org>
> > > Date: Sun Oct 24 16:54:16 2021 +0300
> > >
> > > devlink: Convert devlink lock to be RW semaphore
> > >
> > > This is naive conversion of devlink->lock to RW semaphore, so we will be
> > > able to differentiate commands that require exclusive access vs. parallel
> > > ready-to-run ones.
> > >
> > > All "set" commands that used devlink->lock are converted to write lock,
> > > while all "get" commands are marked with read lock.
> > >
> > > @@ -578,8 +584,12 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
> > > mutex_unlock(&devlink_mutex);
> > > return PTR_ERR(devlink);
> > > }
> > > - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> > > - mutex_lock(&devlink->lock);
> > > +
> > > + if (~ops->internal_flags & DEVLINK_NL_FLAG_SHARED_ACCESS)
> > > + down_write(&devlink->rwsem);
> > > + else
> > > + down_read(&devlink->rwsem);
> > > +
> >
> > IIUC the RW sem thing is an optimization, it's besides the point.
>
> I need RW as a way to ensure "exclusive" access during _set_ operation.
> It is not an optimization, but simple way to understand if parallel
> access is possible at this specific point of time.
How is this not an optimization to allow parallel "reads"?
Anything that calls the driver has no idea whether the driver needs
read or write locking (assuming drivers depend on devlink for locking
as they should) so I presume you only plan this for dumps which don't
call the driver?
Anyway, consider this a nack, it should most definitely not be a part
of the initial conversions. We have enough fallout to deal with.
> > > How? I have only one rule:
> > > * Driver author should be aware that between devlink_register() and
> > > devlink_unregister(), users can send netlink commands.
> > >
> > > In your solution, driver authors will need to follow whole devlink
> > > how-to-use book.
> >
> > Only if they need refs. If you don't the API is the same as yours.
> >
> > IOW you don't provide an API for advanced use cases at all. You force
> > the drivers to implement their own reference counting and locking.
>
> The complex drivers already do it anyway, because they need to reference
> counting their own structures to make sure that the lifetime of these
> structures meats their model.
Well, no comments on what mlx5 does or has to do but it's likely the
most complex driver. Don't judge what others will benefit from based
on mlx5.
> > I want them to just rely on devlink as a framework.
>
> And I don't :). For me devlink is a way to configure device, not manage
> lifetime of driver specific data structures.
My mind is set here. Obviously "community" can override, so gather
support from significant devs or it's my way... sorry to be blunt.
> > How many driver locks do you have in netdevsim after conversion?
> > All 3? How do you add a port to the instance from sysfs without a
> > AB / BA deadlock between the port lock and the devlink lock if the
> > driver can't take the devlink lock? Are you still going to need the
> > "in_reload" wart?
>
> I don't know yet, because as you wrote before netdevsim is for
> prototyping and such ABBA deadlock doesn't exist in real devices.
>
> My current focus is real devices for now.
I wrote it with nfp in mind as well. It has a delayed work which needs
to take the port lock. Sadly I don't have any nfps handy and I didn't
want to post an untested patch.
> Maybe the solution will be to go away from sysfs completely. We will see.
Sigh, I feel like I just fixed netdevsim after it has been getting bit
rotted for a couple of years.
> > > Strange, I see completely opposite picture in the git log with so much
> > > changes that have Fixes line and add/remove/mention rtnl_lock. Maybe it
> > > is useful, but almost all if not all drivers full of fixes of rtnl_lock
> > > usage. I don't want same for the devlink.
> >
> > If you don't provide a locking API to the drivers you'll have to fix 2x
> > the bugs, and each of them subtly different. At least maintainers know
> > what rtnl_lock rules are.
>
> I clearly remember this patch and the sentence "...in
> some devices' resume function(igb_resum,igc_resume) they calls rtnl_lock()
> again". The word "... some ..." hints to me that maintainers have different
> opinion on how to use rtnl_lock.
>
> https://lore.kernel.org/netdev/20210809032809.1224002-1-acelan.kao@canonical.com/
Yes, using rtnl_lock for PM is certainly a bad idea, and I'm not sure
why Intel does it. There are 10s of drivers which take rtnl_lock
correctly and it greatly simplifies their lives.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-02 7:44 ` Leon Romanovsky
@ 2021-11-02 15:16 ` Jakub Kicinski
2021-11-02 17:50 ` Leon Romanovsky
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-02 15:16 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Edwin Peer, Ido Schimmel, Jiri Pirko, netdev
On Tue, 2 Nov 2021 09:44:17 +0200 Leon Romanovsky wrote:
> On Mon, Nov 01, 2021 at 01:04:03PM -0700, Edwin Peer wrote:
> > On Sun, Oct 31, 2021 at 12:23 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > > The average driver author doesn't know locking well and won't be able to
> > > use devlink reference counting correctly.
> >
> > I think this problem largely only exists to the extent that locking
> > and lifecycle requirements are poorly documented. :P
>
> I'm talking about general locking concepts that are perfectly documented
> and still people do crazy things with it. :)
Yet you seem to be pushing for drivers to implement their own locking.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-02 15:16 ` Jakub Kicinski
@ 2021-11-02 17:50 ` Leon Romanovsky
0 siblings, 0 replies; 23+ messages in thread
From: Leon Romanovsky @ 2021-11-02 17:50 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Edwin Peer, Ido Schimmel, Jiri Pirko, netdev
On Tue, Nov 02, 2021 at 08:16:06AM -0700, Jakub Kicinski wrote:
> On Tue, 2 Nov 2021 09:44:17 +0200 Leon Romanovsky wrote:
> > On Mon, Nov 01, 2021 at 01:04:03PM -0700, Edwin Peer wrote:
> > > On Sun, Oct 31, 2021 at 12:23 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > The average driver author doesn't know locking well and won't be able to
> > > > use devlink reference counting correctly.
> > >
> > > I think this problem largely only exists to the extent that locking
> > > and lifecycle requirements are poorly documented. :P
> >
> > I'm talking about general locking concepts that are perfectly documented
> > and still people do crazy things with it. :)
>
> Yet you seem to be pushing for drivers to implement their own locking.
It wasn't me who suggested to expose devlink internal locks and
reference counting to the drivers. In my implementation, drivers
don't need to manage devlink at all and we will be able internally
understand if lock is needed or not.
At least in theory, working to make it true.
Thanks
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-02 15:14 ` Jakub Kicinski
@ 2021-11-02 18:14 ` Leon Romanovsky
2021-11-03 0:05 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Leon Romanovsky @ 2021-11-02 18:14 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: idosch, edwin.peer, jiri, netdev
On Tue, Nov 02, 2021 at 08:14:12AM -0700, Jakub Kicinski wrote:
> On Tue, 2 Nov 2021 10:08:39 +0200 Leon Romanovsky wrote:
> > On Mon, Nov 01, 2021 at 02:16:13PM -0700, Jakub Kicinski wrote:
> > > On Mon, 1 Nov 2021 20:36:15 +0200 Leon Romanovsky wrote:
> > > > This is three step process, but mainly it is first step. We need to make
> > > > sure that functions that can re-entry will use nested locks.
> > > >
> > > > Steps:
> > > > 1. Use proper locking API that supports nesting:
> > > > https://lore.kernel.org/netdev/YYABqfFy%2F%2Fg5Gdis@nanopsycho/T/#mf9dc5cac2013abe413545bbe4a09cc231ae209a4
> > >
> > > Whether we provide the an unlocked API or allow lock nesting
> > > on the driver API is not that important to me.
> >
> > Thanks
>
> Not sure what you're thanking for. I still prefer two explicit APIs.
> Allowing nesting is not really necessary here. Callers know whether
> they hold the lock or not.
I'm doubt about. It maybe easy to tell in reload flow, but it is much
harder inside eswitch mode change (as an example).
>
> Please see netdevsim, it takes the devlink_lock() at entry points to
> the driver - which in turn means the API which takes a lock can be
> removed once all drivers are converted.
Netdevsim is a nice example, but it is very different from the real device.
>
> > > > 2. Convert devlink->lock to be RW semaphore:
> > > > commit 4506dd3a90a82a0b6bde238f507907747ab88407
> > > > Author: Leon Romanovsky <leon@kernel.org>
> > > > Date: Sun Oct 24 16:54:16 2021 +0300
> > > >
> > > > devlink: Convert devlink lock to be RW semaphore
> > > >
> > > > This is naive conversion of devlink->lock to RW semaphore, so we will be
> > > > able to differentiate commands that require exclusive access vs. parallel
> > > > ready-to-run ones.
> > > >
> > > > All "set" commands that used devlink->lock are converted to write lock,
> > > > while all "get" commands are marked with read lock.
> > > >
> > > > @@ -578,8 +584,12 @@ static int devlink_nl_pre_doit(const struct genl_ops *ops,
> > > > mutex_unlock(&devlink_mutex);
> > > > return PTR_ERR(devlink);
> > > > }
> > > > - if (~ops->internal_flags & DEVLINK_NL_FLAG_NO_LOCK)
> > > > - mutex_lock(&devlink->lock);
> > > > +
> > > > + if (~ops->internal_flags & DEVLINK_NL_FLAG_SHARED_ACCESS)
> > > > + down_write(&devlink->rwsem);
> > > > + else
> > > > + down_read(&devlink->rwsem);
> > > > +
> > >
> > > IIUC the RW sem thing is an optimization, it's besides the point.
> >
> > I need RW as a way to ensure "exclusive" access during _set_ operation.
> > It is not an optimization, but simple way to understand if parallel
> > access is possible at this specific point of time.
>
> How is this not an optimization to allow parallel "reads"?
You need to stop everything when _set_ command is called. One way is to
require for all netlink devlink calls to have lock, another solution is
to use RW semaphore. This is why it is not optimization, but an implementation.
Parallel "reads" are nice bonus.
>
> Anything that calls the driver has no idea whether the driver needs
> read or write locking (assuming drivers depend on devlink for locking
> as they should) so I presume you only plan this for dumps which don't
> call the driver?
I planned to separate based on command nature:
* _set_ == write lock
* _get_ == read lock
>
> Anyway, consider this a nack, it should most definitely not be a part
> of the initial conversions. We have enough fallout to deal with.
Any solution should take this into account. We can't design scheme that
will be needed to be rewritten immediately after merge.
>
> > > > How? I have only one rule:
> > > > * Driver author should be aware that between devlink_register() and
> > > > devlink_unregister(), users can send netlink commands.
> > > >
> > > > In your solution, driver authors will need to follow whole devlink
> > > > how-to-use book.
> > >
> > > Only if they need refs. If you don't the API is the same as yours.
> > >
> > > IOW you don't provide an API for advanced use cases at all. You force
> > > the drivers to implement their own reference counting and locking.
> >
> > The complex drivers already do it anyway, because they need to reference
> > counting their own structures to make sure that the lifetime of these
> > structures meats their model.
>
> Well, no comments on what mlx5 does or has to do but it's likely the
> most complex driver. Don't judge what others will benefit from based
> on mlx5.
>
> > > I want them to just rely on devlink as a framework.
> >
> > And I don't :). For me devlink is a way to configure device, not manage
> > lifetime of driver specific data structures.
>
> My mind is set here. Obviously "community" can override, so gather
> support from significant devs or it's my way... sorry to be blunt.
No problem
>
> > > How many driver locks do you have in netdevsim after conversion?
> > > All 3? How do you add a port to the instance from sysfs without a
> > > AB / BA deadlock between the port lock and the devlink lock if the
> > > driver can't take the devlink lock? Are you still going to need the
> > > "in_reload" wart?
> >
> > I don't know yet, because as you wrote before netdevsim is for
> > prototyping and such ABBA deadlock doesn't exist in real devices.
> >
> > My current focus is real devices for now.
>
> I wrote it with nfp in mind as well. It has a delayed work which needs
> to take the port lock. Sadly I don't have any nfps handy and I didn't
> want to post an untested patch.
Do you remember why was port configuration implemented with delayed work?
>
> > Maybe the solution will be to go away from sysfs completely. We will see.
>
> Sigh, I feel like I just fixed netdevsim after it has been getting bit
> rotted for a couple of years.
>
> > > > Strange, I see completely opposite picture in the git log with so much
> > > > changes that have Fixes line and add/remove/mention rtnl_lock. Maybe it
> > > > is useful, but almost all if not all drivers full of fixes of rtnl_lock
> > > > usage. I don't want same for the devlink.
> > >
> > > If you don't provide a locking API to the drivers you'll have to fix 2x
> > > the bugs, and each of them subtly different. At least maintainers know
> > > what rtnl_lock rules are.
> >
> > I clearly remember this patch and the sentence "...in
> > some devices' resume function(igb_resum,igc_resume) they calls rtnl_lock()
> > again". The word "... some ..." hints to me that maintainers have different
> > opinion on how to use rtnl_lock.
> >
> > https://lore.kernel.org/netdev/20210809032809.1224002-1-acelan.kao@canonical.com/
>
> Yes, using rtnl_lock for PM is certainly a bad idea, and I'm not sure
> why Intel does it. There are 10s of drivers which take rtnl_lock
> correctly and it greatly simplifies their lives.
I would say that you are ignoring that most of such drivers don't add
new functionality.
Anyway, I got your point, please give me time to see what I can do.
In case, we will adopt your model, will you convert all drivers?
Thanks
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-02 18:14 ` Leon Romanovsky
@ 2021-11-03 0:05 ` Jakub Kicinski
2021-11-03 7:23 ` Leon Romanovsky
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-03 0:05 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: idosch, edwin.peer, jiri, netdev
On Tue, 2 Nov 2021 20:14:22 +0200 Leon Romanovsky wrote:
> > > Thanks
> >
> > Not sure what you're thanking for. I still prefer two explicit APIs.
> > Allowing nesting is not really necessary here. Callers know whether
> > they hold the lock or not.
>
> I'm doubt about. It maybe easy to tell in reload flow, but it is much
> harder inside eswitch mode change (as an example).
Hm, interesting counter example, why is eswitch mode change harder?
From devlink side they should be locked the same, and I take the
devlink lock on all driver callbacks (probe, remove, sriov).
> > > I need RW as a way to ensure "exclusive" access during _set_ operation.
> > > It is not an optimization, but simple way to understand if parallel
> > > access is possible at this specific point of time.
> >
> > How is this not an optimization to allow parallel "reads"?
>
> You need to stop everything when _set_ command is called. One way is to
> require for all netlink devlink calls to have lock, another solution is
> to use RW semaphore. This is why it is not optimization, but an implementation.
> Parallel "reads" are nice bonus.
Sorry I still don't understand. Why is devlink instance lock not
enough? Are you saying parallel get is a hard requirement for the
rework?
> > > I don't know yet, because as you wrote before netdevsim is for
> > > prototyping and such ABBA deadlock doesn't exist in real devices.
> > >
> > > My current focus is real devices for now.
> >
> > I wrote it with nfp in mind as well. It has a delayed work which needs
> > to take the port lock. Sadly I don't have any nfps handy and I didn't
> > want to post an untested patch.
>
> Do you remember why was port configuration implemented with delayed work?
IIRC it was because of the FW API for link info, all ports would get
accessed at once so we used a work which would lock out devlink port
splitting and update state of all ports.
Link state has to be read under rtnl_lock, yet port splitting needs
to take rtnl_lock to register new netdevs so there was no prettier
way to solve this.
> > > I clearly remember this patch and the sentence "...in
> > > some devices' resume function(igb_resum,igc_resume) they calls rtnl_lock()
> > > again". The word "... some ..." hints to me that maintainers have different
> > > opinion on how to use rtnl_lock.
> > >
> > > https://lore.kernel.org/netdev/20210809032809.1224002-1-acelan.kao@canonical.com/
> >
> > Yes, using rtnl_lock for PM is certainly a bad idea, and I'm not sure
> > why Intel does it. There are 10s of drivers which take rtnl_lock
> > correctly and it greatly simplifies their lives.
>
> I would say that you are ignoring that most of such drivers don't add
> new functionality.
You lost me again. You don't disagree that ability to lock out higher
layers is useful for drivers but... ?
> Anyway, I got your point, please give me time to see what I can do.
>
> In case, we will adopt your model, will you convert all drivers?
Yes, sure. The way this RFC is done it should be possible to land
it without any driver changes and then go driver by driver. I find
that approach much more manageable.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-03 0:05 ` Jakub Kicinski
@ 2021-11-03 7:23 ` Leon Romanovsky
2021-11-03 14:12 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Leon Romanovsky @ 2021-11-03 7:23 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: idosch, edwin.peer, jiri, netdev
On Tue, Nov 02, 2021 at 05:05:30PM -0700, Jakub Kicinski wrote:
> On Tue, 2 Nov 2021 20:14:22 +0200 Leon Romanovsky wrote:
> > > > Thanks
> > >
> > > Not sure what you're thanking for. I still prefer two explicit APIs.
> > > Allowing nesting is not really necessary here. Callers know whether
> > > they hold the lock or not.
> >
> > I'm doubt about. It maybe easy to tell in reload flow, but it is much
> > harder inside eswitch mode change (as an example).
>
> Hm, interesting counter example, why is eswitch mode change harder?
> From devlink side they should be locked the same, and I take the
> devlink lock on all driver callbacks (probe, remove, sriov).
I chose it as an example, because I see calls to eswitch enable/disable
in so many driver paths that I can't tell for sure which API to use and
if I need to take devlink lock or not inside the driver.
We also have other troublesome paths, like PCI recovery and health recovery
which need some sort of protection.
It can be seen as an example that bringing devlink locking complexity to
the real HW drivers won't be as good as it is for netdevsim.
>
> > > > I need RW as a way to ensure "exclusive" access during _set_ operation.
> > > > It is not an optimization, but simple way to understand if parallel
> > > > access is possible at this specific point of time.
> > >
> > > How is this not an optimization to allow parallel "reads"?
> >
> > You need to stop everything when _set_ command is called. One way is to
> > require for all netlink devlink calls to have lock, another solution is
> > to use RW semaphore. This is why it is not optimization, but an implementation.
> > Parallel "reads" are nice bonus.
>
> Sorry I still don't understand. Why is devlink instance lock not
> enough? Are you saying parallel get is a hard requirement for the
> rework?
Let's try to use the following example:
terminal A: | terminal B:
|
while [ true ] | while [ true ]
devlink sb pool show pci/0000:00:09.0 | devlink dev eswitch set pci/0000:00:09.0 mode switchdev
| devlink dev eswitch set pci/0000:00:09.0 mode legacy
In current implementation without global devlink_mutex, it works ok,
because every devlink command takes that global mutex and only one
command runs at the same time. Plus no parallel access is allowed in
rtneltink level.
So imagine that we allow parallel access without requiring devlink->lock
for all .doit() and .dumpit() and continue rely on DEVLINK_NL_FLAG_NO_LOCK
flag. In this case, terminal A will access driver without any protection
while terminal B continues to use devlink->lock.
For this case, we need RW semaphore, terminal A will take read lock,
terminal B will take write lock.
So unless you want to require that all devlink calls will have
devlink->lock, which I think is wrong, we need RW semaphore.
>
> > > > I don't know yet, because as you wrote before netdevsim is for
> > > > prototyping and such ABBA deadlock doesn't exist in real devices.
> > > >
> > > > My current focus is real devices for now.
> > >
> > > I wrote it with nfp in mind as well. It has a delayed work which needs
> > > to take the port lock. Sadly I don't have any nfps handy and I didn't
> > > want to post an untested patch.
> >
> > Do you remember why was port configuration implemented with delayed work?
>
> IIRC it was because of the FW API for link info, all ports would get
> accessed at once so we used a work which would lock out devlink port
> splitting and update state of all ports.
>
> Link state has to be read under rtnl_lock, yet port splitting needs
> to take rtnl_lock to register new netdevs so there was no prettier
> way to solve this.
>
> > > > I clearly remember this patch and the sentence "...in
> > > > some devices' resume function(igb_resum,igc_resume) they calls rtnl_lock()
> > > > again". The word "... some ..." hints to me that maintainers have different
> > > > opinion on how to use rtnl_lock.
> > > >
> > > > https://lore.kernel.org/netdev/20210809032809.1224002-1-acelan.kao@canonical.com/
> > >
> > > Yes, using rtnl_lock for PM is certainly a bad idea, and I'm not sure
> > > why Intel does it. There are 10s of drivers which take rtnl_lock
> > > correctly and it greatly simplifies their lives.
> >
> > I would say that you are ignoring that most of such drivers don't add
> > new functionality.
>
> You lost me again. You don't disagree that ability to lock out higher
> layers is useful for drivers but... ?
I disagree, but our views are so different here that nothing good will
come out of arguing.
>
> > Anyway, I got your point, please give me time to see what I can do.
> >
> > In case, we will adopt your model, will you convert all drivers?
>
> Yes, sure. The way this RFC is done it should be possible to land
> it without any driver changes and then go driver by driver. I find
> that approach much more manageable.
I still believe that we can do everything inside devlink.c without
touching drivers at all.
Thanks
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
` (5 preceding siblings ...)
2021-10-31 7:23 ` [RFC 0/5] devlink: add an explicit locking API Leon Romanovsky
@ 2021-11-03 9:03 ` Jiri Pirko
2021-11-03 14:52 ` Jakub Kicinski
6 siblings, 1 reply; 23+ messages in thread
From: Jiri Pirko @ 2021-11-03 9:03 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: leon, idosch, edwin.peer, netdev
Sun, Oct 31, 2021 at 01:12:49AM CEST, kuba@kernel.org wrote:
>This implements what I think is the right direction for devlink
>API overhaul. It's an early RFC/PoC because the body of code is
>rather large so I'd appreciate feedback early... The patches are
>very roughly split, the point of the posting is primarily to prove
>that from the driver side the conversion is an net improvement
>in complexity.
>
>IMO the problems with devlink locking are caused by two things:
>
> (1) driver has no way to block devlink calls like it does in case
> of netedev / rtnl_lock, note that for devlink each driver has
> it's own lock so contention is not an issue;
>
> (2) sometimes devlink calls into the driver without holding its lock
> - for operations which may need the driver to call devlink (port
> splitting, switch config, reload etc.), the circular dependency
> is there, the driver can't solve this problem.
>
>This set "fixes" the ordering by allowing the driver to participate
>in locking. The lock order remains:
>
> device_lock -> [devlink_mutex] -> devlink instance -> rtnl_lock
>
>but now driver can take devlink lock itself, and _ALL_ devlink ops
>are locked.
>
>The expectation is that driver will take the devlink instance lock
>on its probe and remove paths, hence protecting all configuration
>paths with the devlink instance lock.
>
>This is clearly demonstrated by the netdevsim conversion. All entry
>points to driver config are protected by devlink instance lock, so
>the driver switches to the "I'm already holding the devlink lock" API
>when calling devlink. All driver locks and trickery is removed.
>
>The part which is slightly more challanging is quiescing async entry
>points which need to be closed on the devlink reload path (workqueue,
>debugfs etc.) and which also take devlink instance lock. For that we
>need to be able to take refs on the devlink instance and let them
>clean up after themselves rather than waiting synchronously.
>
>That last part is not 100% finished in this patch set - it works but
>we need the driver to take devlink_mutex (the global lock) from its
>probe/remove path. I hope this is good enough for an RFC, the problem
>is easily solved by protecting the devlink XArray with something else
>than devlink_mutex and/or not holding devlink_mutex around each op
>(so that there is no ordering between the global and instance locks).
>Speaking of not holding devlink_mutex around each op this patch set
>also opens the path for parallel ops to different devlink instances
>which is currently impossible because all user space calls take
>devlink_mutex...
>
>The patches are on top of the cleanups I posted earlier.
Hi Jakub.
I took my time to read this thread and talked with Leon as well.
My original intention of locking in devlink was to maintain the locking
inside devlink.c to avoid the rtnl_lock-scenario.
However, I always feared that eventually we'll get to the point,
when it won't be possible to maintain any longer. I think may be it.
In general, I like your approach. It is very clean and explicit. The
driver knows what to do, in which context it is and it can behave
accordingly. In theory or course, but the reality of drivers code tells
us often something different :)
One small thing I don't fully undestand is the "opt-out" scenario which
makes things a bit tangled. But perhaps you can explain it a bit more.
Leon claims that he thinks that he would be able to solve the locking
scheme leaving all locking internal to devlink.c. I suggest to give
him a week or 2 to present the solution. If he is not successful, lets
continue on your approach.
What do you think?
Thanks!
>
>Jakub Kicinski (5):
> devlink: add unlocked APIs
> devlink: add API for explicit locking
> devlink: allow locking of all ops
> netdevsim: minor code move
> netdevsim: use devlink locking
>
> drivers/net/netdevsim/bus.c | 19 -
> drivers/net/netdevsim/dev.c | 450 ++++++++-------
> drivers/net/netdevsim/fib.c | 62 +--
> drivers/net/netdevsim/netdevsim.h | 5 -
> include/net/devlink.h | 88 +++
> net/core/devlink.c | 875 +++++++++++++++++++++---------
> 6 files changed, 996 insertions(+), 503 deletions(-)
>
>--
>2.31.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-03 7:23 ` Leon Romanovsky
@ 2021-11-03 14:12 ` Jakub Kicinski
0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-03 14:12 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: idosch, edwin.peer, jiri, netdev
On Wed, 3 Nov 2021 09:23:21 +0200 Leon Romanovsky wrote:
> > > I'm doubt about. It maybe easy to tell in reload flow, but it is much
> > > harder inside eswitch mode change (as an example).
> >
> > Hm, interesting counter example, why is eswitch mode change harder?
> > From devlink side they should be locked the same, and I take the
> > devlink lock on all driver callbacks (probe, remove, sriov).
>
> I chose it as an example, because I see calls to eswitch enable/disable
> in so many driver paths that I can't tell for sure which API to use and
> if I need to take devlink lock or not inside the driver.
Really? Certainly not the case for nfp and bnxt. The two paths that
care are devlink mode setting callback (already locked), and sriov
config for spawning the right ports (I recommend all PCI callbacks
take the devlink lock).
> We also have other troublesome paths, like PCI recovery and health recovery
> which need some sort of protection.
PCI recovery should take the devlink lock like any PCI callback.
Health callbacks are locked in my RFC.
> It can be seen as an example that bringing devlink locking complexity to
> the real HW drivers won't be as good as it is for netdevsim.
I don't see that. Again, do whatever you want for mlx5, but don't stop
others from creating shared infra.
> > > You need to stop everything when _set_ command is called. One way is to
> > > require for all netlink devlink calls to have lock, another solution is
> > > to use RW semaphore. This is why it is not optimization, but an implementation.
> > > Parallel "reads" are nice bonus.
> >
> > Sorry I still don't understand. Why is devlink instance lock not
> > enough? Are you saying parallel get is a hard requirement for the
> > rework?
>
> Let's try to use the following example:
> terminal A: | terminal B:
> |
> while [ true ] | while [ true ]
> devlink sb pool show pci/0000:00:09.0 | devlink dev eswitch set pci/0000:00:09.0 mode switchdev
> | devlink dev eswitch set pci/0000:00:09.0 mode legacy
>
> In current implementation without global devlink_mutex, it works ok,
> because every devlink command takes that global mutex and only one
> command runs at the same time. Plus no parallel access is allowed in
> rtneltink level.
>
> So imagine that we allow parallel access without requiring devlink->lock
> for all .doit() and .dumpit() and continue rely on DEVLINK_NL_FLAG_NO_LOCK
> flag. In this case, terminal A will access driver without any protection
> while terminal B continues to use devlink->lock.
>
> For this case, we need RW semaphore, terminal A will take read lock,
> terminal B will take write lock.
>
> So unless you want to require that all devlink calls will have
> devlink->lock, which I think is wrong, we need RW semaphore.
Oh! I don't know how many times I said already that all callbacks should
take the instance lock. Let me try one more time - all callbacks should
take the instance lock.
> > > I would say that you are ignoring that most of such drivers don't add
> > > new functionality.
> >
> > You lost me again. You don't disagree that ability to lock out higher
> > layers is useful for drivers but... ?
>
> I disagree, but our views are so different here that nothing good will
> come out of arguing.
You can't disagree with facts.
If I'm counting right there are ~80 Ethernet drivers which take
rtnl_lock. Do you really think they would all do that if it was easier
to implement their own locking? Or that they are all buggy?
> > > Anyway, I got your point, please give me time to see what I can do.
> > >
> > > In case, we will adopt your model, will you convert all drivers?
> >
> > Yes, sure. The way this RFC is done it should be possible to land
> > it without any driver changes and then go driver by driver. I find
> > that approach much more manageable.
>
> I still believe that we can do everything inside devlink.c without
> touching drivers at all.
?? The git history says you have been touching the drivers quite a bit.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-03 9:03 ` Jiri Pirko
@ 2021-11-03 14:52 ` Jakub Kicinski
2021-11-03 19:19 ` Jiri Pirko
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2021-11-03 14:52 UTC (permalink / raw)
To: Jiri Pirko; +Cc: leon, idosch, edwin.peer, netdev
On Wed, 3 Nov 2021 10:03:32 +0100 Jiri Pirko wrote:
> Hi Jakub.
>
> I took my time to read this thread and talked with Leon as well.
> My original intention of locking in devlink was to maintain the locking
> inside devlink.c to avoid the rtnl_lock-scenario.
>
> However, I always feared that eventually we'll get to the point,
> when it won't be possible to maintain any longer. I think may be it.
Indeed, the two things I think we can avoid from rtnl_lock pitfalls
is that lock should be per-instance and that we should not wait for
all refs to be gone at unregister time.
Both are rather trivial to achieve with devlink.
> In general, I like your approach. It is very clean and explicit. The
> driver knows what to do, in which context it is and it can behave
> accordingly. In theory or course, but the reality of drivers code tells
> us often something different :)
Right. I'll convert a few more drivers but the real test will be
seeing if potential races are gone - hard to measure.
> One small thing I don't fully undestand is the "opt-out" scenario which
> makes things a bit tangled. But perhaps you can explain it a bit more.
Do you mean the .lock_flags? That's a transitional thing so that people
can convert drivers callback by callback to make prettier patch sets.
I may collapse all those flags into one, remains to be seen how useful
it is when I create proper patches. This RFC is more of a code dump.
The whole opt-out is to create the entire new API at once, and then
convert drivers one-by-one (or allow the maintainers who care to do
it themselves). I find that easier and more friendly than slicing
the API and drivers multiple times.
Long story short I expect the opt-out would be gone by the time 5.17
merge window rolls around.
> Leon claims that he thinks that he would be able to solve the locking
> scheme leaving all locking internal to devlink.c. I suggest to give
> him a week or 2 to present the solution. If he is not successful, lets
> continue on your approach.
>
> What do you think?
I do worry a little bit that our goals differ. Seems like Leon wants to
fix devlink for devlink and I want drivers to be able to lean on it.
But I'm not attached to the exact approach or code, so as long as
nobody is attached to theirs more RFCs can only help.
Please be courteous and send as RFCs, tho.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 0/5] devlink: add an explicit locking API
2021-11-03 14:52 ` Jakub Kicinski
@ 2021-11-03 19:19 ` Jiri Pirko
0 siblings, 0 replies; 23+ messages in thread
From: Jiri Pirko @ 2021-11-03 19:19 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: leon, idosch, edwin.peer, netdev
Wed, Nov 03, 2021 at 03:52:31PM CET, kuba@kernel.org wrote:
>On Wed, 3 Nov 2021 10:03:32 +0100 Jiri Pirko wrote:
>> Hi Jakub.
>>
>> I took my time to read this thread and talked with Leon as well.
>> My original intention of locking in devlink was to maintain the locking
>> inside devlink.c to avoid the rtnl_lock-scenario.
>>
>> However, I always feared that eventually we'll get to the point,
>> when it won't be possible to maintain any longer. I think may be it.
>
>Indeed, the two things I think we can avoid from rtnl_lock pitfalls
>is that lock should be per-instance and that we should not wait for
>all refs to be gone at unregister time.
>
>Both are rather trivial to achieve with devlink.
>
>> In general, I like your approach. It is very clean and explicit. The
>> driver knows what to do, in which context it is and it can behave
>> accordingly. In theory or course, but the reality of drivers code tells
>> us often something different :)
>
>Right. I'll convert a few more drivers but the real test will be
>seeing if potential races are gone - hard to measure.
>
>> One small thing I don't fully undestand is the "opt-out" scenario which
>> makes things a bit tangled. But perhaps you can explain it a bit more.
>
>Do you mean the .lock_flags? That's a transitional thing so that people
>can convert drivers callback by callback to make prettier patch sets.
>I may collapse all those flags into one, remains to be seen how useful
>it is when I create proper patches. This RFC is more of a code dump.
>
>The whole opt-out is to create the entire new API at once, and then
>convert drivers one-by-one (or allow the maintainers who care to do
>it themselves). I find that easier and more friendly than slicing
>the API and drivers multiple times.
>
>Long story short I expect the opt-out would be gone by the time 5.17
>merge window rolls around.
Okay, got it.
>
>> Leon claims that he thinks that he would be able to solve the locking
>> scheme leaving all locking internal to devlink.c. I suggest to give
>> him a week or 2 to present the solution. If he is not successful, lets
>> continue on your approach.
>>
>> What do you think?
>
>I do worry a little bit that our goals differ. Seems like Leon wants to
>fix devlink for devlink and I want drivers to be able to lean on it.
>
>But I'm not attached to the exact approach or code, so as long as
>nobody is attached to theirs more RFCs can only help.
>
>Please be courteous and send as RFCs, tho.
Makes sense. Thanks!
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2021-11-03 19:19 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-30 23:12 [RFC 0/5] devlink: add an explicit locking API Jakub Kicinski
2021-10-30 23:12 ` [RFC 1/5] devlink: add unlocked APIs Jakub Kicinski
2021-10-30 23:12 ` [RFC 2/5] devlink: add API for explicit locking Jakub Kicinski
2021-10-30 23:12 ` [RFC 3/5] devlink: allow locking of all ops Jakub Kicinski
2021-10-30 23:12 ` [RFC 4/5] netdevsim: minor code move Jakub Kicinski
2021-10-30 23:12 ` [RFC 5/5] netdevsim: use devlink locking Jakub Kicinski
2021-10-31 7:23 ` [RFC 0/5] devlink: add an explicit locking API Leon Romanovsky
2021-11-01 14:32 ` Jakub Kicinski
2021-11-01 18:36 ` Leon Romanovsky
2021-11-01 21:16 ` Jakub Kicinski
2021-11-02 8:08 ` Leon Romanovsky
2021-11-02 15:14 ` Jakub Kicinski
2021-11-02 18:14 ` Leon Romanovsky
2021-11-03 0:05 ` Jakub Kicinski
2021-11-03 7:23 ` Leon Romanovsky
2021-11-03 14:12 ` Jakub Kicinski
2021-11-01 20:04 ` Edwin Peer
2021-11-02 7:44 ` Leon Romanovsky
2021-11-02 15:16 ` Jakub Kicinski
2021-11-02 17:50 ` Leon Romanovsky
2021-11-03 9:03 ` Jiri Pirko
2021-11-03 14:52 ` Jakub Kicinski
2021-11-03 19:19 ` Jiri Pirko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.