| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 Apr 2018 09:32:12 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: David Ahern <dsahern@...il.com>
Cc: Ido Schimmel <idosch@...lanox.com>, netdev@...r.kernel.org,
davem@...emloft.net, jiri@...lanox.com, petrm@...lanox.com,
mlxsw@...lanox.com
Subject: Re: [PATCH net-next 09/11] devlink: convert occ_get op to separate
registration
Fri, Mar 30, 2018 at 04:45:50PM CEST, dsahern@...il.com wrote:
>On 3/29/18 2:33 PM, Ido Schimmel wrote:
>> From: Jiri Pirko <jiri@...lanox.com>
>>
>> This resolves race during initialization where the resources with
>> ops are registered before driver and the structures used by occ_get
>> op is initialized. So keep occ_get callbacks registered only when
>> all structs are initialized.
>
>Why can't the occ_get handler look at some flag in an mlxsw struct to
>know if the system has initialized?
>
>Separate registration here is awkward. You register a resource and then
>register its op later.
The separation is exactly why this patch is made. Note that devlink
resouce is registered by core way before the initialization is done and
the driver is actually able to perform the op. Also consider "reload"
case, when the resource is still registered and the driver unloads and
loads again. For that makes perfect sense to have that separated.
Flag would just make things odd. Also, the priv could not be used in
that case.
>
>Also, this should be a standalone patch rather than embedded in a
>'mlxsw: Various cleanups' set.
>
>
>>
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>> Signed-off-by: Ido Schimmel <idosch@...lanox.com>
>> ---
>> drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 24 ++-----
>> drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 1 -
>> .../net/ethernet/mellanox/mlxsw/spectrum_kvdl.c | 67 ++++++++++++--------
>> include/net/devlink.h | 40 ++++++++----
>> net/core/devlink.c | 74 +++++++++++++++++++---
>> 5 files changed, 134 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> index b831af38e0a1..0d95d2cb73e3 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> @@ -3805,18 +3805,6 @@ static const struct mlxsw_config_profile mlxsw_sp_config_profile = {
>> },
>> };
>>
>> -static u64 mlxsw_sp_resource_kvd_linear_occ_get(struct devlink *devlink)
>> -{
>> - struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>> - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>> -
>> - return mlxsw_sp_kvdl_occ_get(mlxsw_sp);
>> -}
>> -
>> -static const struct devlink_resource_ops mlxsw_sp_resource_kvd_linear_ops = {
>> - .occ_get = mlxsw_sp_resource_kvd_linear_occ_get,
>> -};
>> -
>> static void
>> mlxsw_sp_resource_size_params_prepare(struct mlxsw_core *mlxsw_core,
>> struct devlink_resource_size_params *kvd_size_params,
>> @@ -3877,8 +3865,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
>> err = devlink_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD,
>> kvd_size, MLXSW_SP_RESOURCE_KVD,
>> DEVLINK_RESOURCE_ID_PARENT_TOP,
>> - &kvd_size_params,
>> - NULL);
>> + &kvd_size_params);
>> if (err)
>> return err;
>>
>> @@ -3887,8 +3874,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
>> linear_size,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> MLXSW_SP_RESOURCE_KVD,
>> - &linear_size_params,
>> - &mlxsw_sp_resource_kvd_linear_ops);
>> + &linear_size_params);
>> if (err)
>> return err;
>>
>> @@ -3905,8 +3891,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
>> double_size,
>> MLXSW_SP_RESOURCE_KVD_HASH_DOUBLE,
>> MLXSW_SP_RESOURCE_KVD,
>> - &hash_double_size_params,
>> - NULL);
>> + &hash_double_size_params);
>> if (err)
>> return err;
>>
>> @@ -3915,8 +3900,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
>> single_size,
>> MLXSW_SP_RESOURCE_KVD_HASH_SINGLE,
>> MLXSW_SP_RESOURCE_KVD,
>> - &hash_single_size_params,
>> - NULL);
>> + &hash_single_size_params);
>> if (err)
>> return err;
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> index 21bee8f19894..c59a0d7d81d5 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> @@ -442,7 +442,6 @@ void mlxsw_sp_kvdl_free(struct mlxsw_sp *mlxsw_sp, int entry_index);
>> int mlxsw_sp_kvdl_alloc_size_query(struct mlxsw_sp *mlxsw_sp,
>> unsigned int entry_count,
>> unsigned int *p_alloc_size);
>> -u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp);
>> int mlxsw_sp_kvdl_resources_register(struct devlink *devlink);
>>
>> struct mlxsw_sp_acl_rule_info {
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>> index 7b28f65d6407..1b7280168e6b 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>> @@ -315,8 +315,9 @@ static u64 mlxsw_sp_kvdl_part_occ(struct mlxsw_sp_kvdl_part *part)
>> return occ;
>> }
>>
>> -u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
>> +static u64 mlxsw_sp_kvdl_occ_get(void *priv)
>> {
>> + const struct mlxsw_sp *mlxsw_sp = priv;
>> u64 occ = 0;
>> int i;
>>
>> @@ -326,48 +327,33 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
>> return occ;
>> }
>>
>> -static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
>> +static u64 mlxsw_sp_kvdl_single_occ_get(void *priv)
>> {
>> - struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>> - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>> + const struct mlxsw_sp *mlxsw_sp = priv;
>> struct mlxsw_sp_kvdl_part *part;
>>
>> part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE];
>> return mlxsw_sp_kvdl_part_occ(part);
>> }
>>
>> -static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
>> +static u64 mlxsw_sp_kvdl_chunks_occ_get(void *priv)
>> {
>> - struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>> - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>> + const struct mlxsw_sp *mlxsw_sp = priv;
>> struct mlxsw_sp_kvdl_part *part;
>>
>> part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS];
>> return mlxsw_sp_kvdl_part_occ(part);
>> }
>>
>> -static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink)
>> +static u64 mlxsw_sp_kvdl_large_chunks_occ_get(void *priv)
>> {
>> - struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>> - struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>> + const struct mlxsw_sp *mlxsw_sp = priv;
>> struct mlxsw_sp_kvdl_part *part;
>>
>> part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS];
>> return mlxsw_sp_kvdl_part_occ(part);
>> }
>>
>> -static const struct devlink_resource_ops mlxsw_sp_kvdl_single_ops = {
>> - .occ_get = mlxsw_sp_kvdl_single_occ_get,
>> -};
>> -
>> -static const struct devlink_resource_ops mlxsw_sp_kvdl_chunks_ops = {
>> - .occ_get = mlxsw_sp_kvdl_chunks_occ_get,
>> -};
>> -
>> -static const struct devlink_resource_ops mlxsw_sp_kvdl_chunks_large_ops = {
>> - .occ_get = mlxsw_sp_kvdl_large_chunks_occ_get,
>> -};
>> -
>> int mlxsw_sp_kvdl_resources_register(struct devlink *devlink)
>> {
>> struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>> @@ -386,8 +372,7 @@ int mlxsw_sp_kvdl_resources_register(struct devlink *devlink)
>> MLXSW_SP_KVDL_SINGLE_SIZE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - &size_params,
>> - &mlxsw_sp_kvdl_single_ops);
>> + &size_params);
>> if (err)
>> return err;
>>
>> @@ -398,8 +383,7 @@ int mlxsw_sp_kvdl_resources_register(struct devlink *devlink)
>> MLXSW_SP_KVDL_CHUNKS_SIZE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - &size_params,
>> - &mlxsw_sp_kvdl_chunks_ops);
>> + &size_params);
>> if (err)
>> return err;
>>
>> @@ -410,13 +394,13 @@ int mlxsw_sp_kvdl_resources_register(struct devlink *devlink)
>> MLXSW_SP_KVDL_LARGE_CHUNKS_SIZE,
>> MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> MLXSW_SP_RESOURCE_KVD_LINEAR,
>> - &size_params,
>> - &mlxsw_sp_kvdl_chunks_large_ops);
>> + &size_params);
>> return err;
>> }
>>
>> int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp)
>> {
>> + struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
>> struct mlxsw_sp_kvdl *kvdl;
>> int err;
>>
>> @@ -429,6 +413,23 @@ int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp)
>> if (err)
>> goto err_kvdl_parts_init;
>>
>> + devlink_resource_occ_get_register(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR,
>> + mlxsw_sp_kvdl_occ_get,
>> + mlxsw_sp);
>> + devlink_resource_occ_get_register(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> + mlxsw_sp_kvdl_single_occ_get,
>> + mlxsw_sp);
>> + devlink_resource_occ_get_register(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> + mlxsw_sp_kvdl_chunks_occ_get,
>> + mlxsw_sp);
>> + devlink_resource_occ_get_register(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> + mlxsw_sp_kvdl_large_chunks_occ_get,
>> + mlxsw_sp);
>> +
>> return 0;
>>
>> err_kvdl_parts_init:
>> @@ -438,6 +439,16 @@ int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp)
>>
>> void mlxsw_sp_kvdl_fini(struct mlxsw_sp *mlxsw_sp)
>> {
>> + struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
>> +
>> + devlink_resource_occ_get_unregister(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS);
>> + devlink_resource_occ_get_unregister(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS);
>> + devlink_resource_occ_get_unregister(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE);
>> + devlink_resource_occ_get_unregister(devlink,
>> + MLXSW_SP_RESOURCE_KVD_LINEAR);
>> mlxsw_sp_kvdl_parts_fini(mlxsw_sp);
>> kfree(mlxsw_sp->kvdl);
>> }
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index e21d8cadd480..2e4f71e16e95 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -231,14 +231,6 @@ struct devlink_dpipe_headers {
>> unsigned int headers_count;
>> };
>>
>> -/**
>> - * struct devlink_resource_ops - resource ops
>> - * @occ_get: get the occupied size
>> - */
>> -struct devlink_resource_ops {
>> - u64 (*occ_get)(struct devlink *devlink);
>> -};
>> -
>> /**
>> * struct devlink_resource_size_params - resource's size parameters
>> * @size_min: minimum size which can be set
>> @@ -265,6 +257,8 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
>> size_params->unit = unit;
>> }
>>
>> +typedef u64 devlink_resource_occ_get_t(void *priv);
>> +
>> /**
>> * struct devlink_resource - devlink resource
>> * @name: name of the resource
>> @@ -277,7 +271,6 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
>> * @size_params: size parameters
>> * @list: parent list
>> * @resource_list: list of child resources
>> - * @resource_ops: resource ops
>> */
>> struct devlink_resource {
>> const char *name;
>> @@ -289,7 +282,8 @@ struct devlink_resource {
>> struct devlink_resource_size_params size_params;
>> struct list_head list;
>> struct list_head resource_list;
>> - const struct devlink_resource_ops *resource_ops;
>> + devlink_resource_occ_get_t *occ_get;
>> + void *occ_get_priv;
>> };
>>
>> #define DEVLINK_RESOURCE_ID_PARENT_TOP 0
>> @@ -409,8 +403,7 @@ int devlink_resource_register(struct devlink *devlink,
>> u64 resource_size,
>> u64 resource_id,
>> u64 parent_resource_id,
>> - const struct devlink_resource_size_params *size_params,
>> - const struct devlink_resource_ops *resource_ops);
>> + const struct devlink_resource_size_params *size_params);
>> void devlink_resources_unregister(struct devlink *devlink,
>> struct devlink_resource *resource);
>> int devlink_resource_size_get(struct devlink *devlink,
>> @@ -419,6 +412,12 @@ int devlink_resource_size_get(struct devlink *devlink,
>> int devlink_dpipe_table_resource_set(struct devlink *devlink,
>> const char *table_name, u64 resource_id,
>> u64 resource_units);
>> +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);
>>
>> #else
>>
>> @@ -562,8 +561,7 @@ devlink_resource_register(struct devlink *devlink,
>> u64 resource_size,
>> u64 resource_id,
>> u64 parent_resource_id,
>> - const struct devlink_resource_size_params *size_params,
>> - const struct devlink_resource_ops *resource_ops)
>> + const struct devlink_resource_size_params *size_params)
>> {
>> return 0;
>> }
>> @@ -589,6 +587,20 @@ devlink_dpipe_table_resource_set(struct devlink *devlink,
>> return -EOPNOTSUPP;
>> }
>>
>> +static inline void
>> +devlink_resource_occ_get_register(struct devlink *devlink,
>> + u64 resource_id,
>> + devlink_resource_occ_get_t *occ_get,
>> + void *occ_get_priv)
>> +{
>> +}
>> +
>> +static inline void
>> +devlink_resource_occ_get_unregister(struct devlink *devlink,
>> + u64 resource_id)
>> +{
>> +}
>> +
>> #endif
>>
>> #endif /* _NET_DEVLINK_H_ */
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 9236e421bd62..ad1317376798 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -2405,6 +2405,16 @@ devlink_resource_size_params_put(struct devlink_resource *resource,
>> return 0;
>> }
>>
>> +static int devlink_resource_occ_put(struct devlink_resource *resource,
>> + struct sk_buff *skb)
>> +{
>> + if (!resource->occ_get)
>> + return 0;
>> + return nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC,
>> + resource->occ_get(resource->occ_get_priv),
>> + DEVLINK_ATTR_PAD);
>> +}
>> +
>> static int devlink_resource_put(struct devlink *devlink, struct sk_buff *skb,
>> struct devlink_resource *resource)
>> {
>> @@ -2425,11 +2435,8 @@ static int devlink_resource_put(struct devlink *devlink, struct sk_buff *skb,
>> if (resource->size != resource->size_new)
>> nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_SIZE_NEW,
>> resource->size_new, DEVLINK_ATTR_PAD);
>> - if (resource->resource_ops && resource->resource_ops->occ_get)
>> - if (nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC,
>> - resource->resource_ops->occ_get(devlink),
>> - DEVLINK_ATTR_PAD))
>> - goto nla_put_failure;
>> + if (devlink_resource_occ_put(resource, skb))
>> + goto nla_put_failure;
>> if (devlink_resource_size_params_put(resource, skb))
>> goto nla_put_failure;
>> if (list_empty(&resource->resource_list))
>> @@ -3162,15 +3169,13 @@ EXPORT_SYMBOL_GPL(devlink_dpipe_table_unregister);
>> * @resource_id: resource's id
>> * @parent_reosurce_id: resource's parent id
>> * @size params: size parameters
>> - * @resource_ops: resource ops
>> */
>> 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,
>> - const struct devlink_resource_ops *resource_ops)
>> + const struct devlink_resource_size_params *size_params)
>> {
>> struct devlink_resource *resource;
>> struct list_head *resource_list;
>> @@ -3213,7 +3218,6 @@ int devlink_resource_register(struct devlink *devlink,
>> resource->size = resource_size;
>> resource->size_new = resource_size;
>> resource->id = resource_id;
>> - resource->resource_ops = resource_ops;
>> resource->size_valid = true;
>> memcpy(&resource->size_params, size_params,
>> sizeof(resource->size_params));
>> @@ -3315,6 +3319,58 @@ 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
>> + * @resource_id: resource id
>> + * @occ_get: occupancy getter callback
>> + * @occ_get_priv: occupancy getter callback priv
>> + */
>> +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;
>> +
>> + 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 = occ_get;
>> + resource->occ_get_priv = occ_get_priv;
>> +out:
>> + mutex_unlock(&devlink->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
>> +
>> +/**
>> + * devlink_resource_occ_get_unregister - unregister occupancy getter
>> + *
>> + * @devlink: devlink
>> + * @resource_id: resource id
>> + */
>> +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:
>> + mutex_unlock(&devlink->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
>> +
>> static int __init devlink_module_init(void)
>> {
>> return genl_register_family(&devlink_nl_family);
>>
>
Powered by blists - more mailing lists