Re: [PATCH net-next 09/11] devlink: convert occ_get op to separate registration

[David Ahern <dsahern@gmail.com>]

On 3/29/18 2:33 PM, Ido Schimmel wrote:
> From: Jiri Pirko <jiri@mellanox.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.

Also, this should be a standalone patch rather than embedded in a
'mlxsw: Various cleanups' set.


> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.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);
>