Re: [PATCH mlx5-next v2 1/6] net/mlx5: Refactor HCA capability set flow

[Leon Romanovsky <leon@kernel.org>]

On Mon, Apr 13, 2020 at 08:47:39PM +0000, Saeed Mahameed wrote:
> On Mon, 2020-04-13 at 16:36 +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Reduce the amount of kzalloc/kfree cycles by allocating
> > command structure in the parent function and leverage the
> > knowledge that set_caps() is called for HCA capabilities
> > only with specific HW structure as parameter to calculate
> > mailbox size.
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
> >  .../net/ethernet/mellanox/mlx5/core/main.c    | 66 +++++++--------
> > ----
> >  1 file changed, 24 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 7af4210c1b96..8b9182add689 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -407,20 +407,19 @@ int mlx5_core_get_caps(struct mlx5_core_dev
> > *dev, enum mlx5_cap_type cap_type)
> >  	return mlx5_core_get_caps_mode(dev, cap_type,
> > HCA_CAP_OPMOD_GET_MAX);
> >  }
> >
> > -static int set_caps(struct mlx5_core_dev *dev, void *in, int in_sz,
> > int opmod)
> > +static int set_caps(struct mlx5_core_dev *dev, void *in, int opmod)
> >  {
> > -	u32 out[MLX5_ST_SZ_DW(set_hca_cap_out)] = {0};
> > +	u32 out[MLX5_ST_SZ_DW(set_hca_cap_out)] = {};
> >
> >  	MLX5_SET(set_hca_cap_in, in, opcode, MLX5_CMD_OP_SET_HCA_CAP);
> >  	MLX5_SET(set_hca_cap_in, in, op_mod, opmod << 1);
> > -	return mlx5_cmd_exec(dev, in, in_sz, out, sizeof(out));
> > +	return mlx5_cmd_exec(dev, in, MLX5_ST_SZ_BYTES(set_hca_cap_in),
> > out,
> > +			     sizeof(out));
> >  }
> >
> > -static int handle_hca_cap_atomic(struct mlx5_core_dev *dev)
> > +static int handle_hca_cap_atomic(struct mlx5_core_dev *dev, void
> > *set_ctx)
> >  {
> > -	void *set_ctx;
> >  	void *set_hca_cap;
> > -	int set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> >  	int req_endianness;
> >  	int err;
> >
> > @@ -439,27 +438,19 @@ static int handle_hca_cap_atomic(struct
> > mlx5_core_dev *dev)
> >  	if (req_endianness != MLX5_ATOMIC_REQ_MODE_HOST_ENDIANNESS)
> >  		return 0;
> >
> > -	set_ctx = kzalloc(set_sz, GFP_KERNEL);
> > -	if (!set_ctx)
> > -		return -ENOMEM;
> > -
> >  	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> > capability);
> >
> >  	/* Set requestor to host endianness */
> >  	MLX5_SET(atomic_caps, set_hca_cap,
> > atomic_req_8B_endianness_mode,
> >  		 MLX5_ATOMIC_REQ_MODE_HOST_ENDIANNESS);
> >
> > -	err = set_caps(dev, set_ctx, set_sz,
> > MLX5_SET_HCA_CAP_OP_MOD_ATOMIC);
> > -
> > -	kfree(set_ctx);
> > +	err = set_caps(dev, set_ctx, MLX5_SET_HCA_CAP_OP_MOD_ATOMIC);
> >  	return err;
> >  }
> >
> > -static int handle_hca_cap_odp(struct mlx5_core_dev *dev)
> > +static int handle_hca_cap_odp(struct mlx5_core_dev *dev, void
> > *set_ctx)
> >  {
> >  	void *set_hca_cap;
> > -	void *set_ctx;
> > -	int set_sz;
> >  	bool do_set = false;
> >  	int err;
> >
> > @@ -471,11 +462,6 @@ static int handle_hca_cap_odp(struct
> > mlx5_core_dev *dev)
> >  	if (err)
> >  		return err;
> >
> > -	set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> > -	set_ctx = kzalloc(set_sz, GFP_KERNEL);
> > -	if (!set_ctx)
> > -		return -ENOMEM;
> > -
> >  	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> > capability);
> >  	memcpy(set_hca_cap, dev->caps.hca_cur[MLX5_CAP_ODP],
> >  	       MLX5_ST_SZ_BYTES(odp_cap));
> > @@ -505,29 +491,20 @@ static int handle_hca_cap_odp(struct
> > mlx5_core_dev *dev)
> >  	ODP_CAP_SET_MAX(dev, dc_odp_caps.atomic);
> >
> >  	if (do_set)
>
> assigning to err is now redundant in all of the functions and the next
> patch.
>
> you can do early exits when required and then just "return set_caps();"
>
>
> in this example:
>
>        if (!do_set)
>            return 0;
>
>        return set_caps(...);
>
>
> > -		err = set_caps(dev, set_ctx, set_sz,
> > -			       MLX5_SET_HCA_CAP_OP_MOD_ODP);
> > -
> > -	kfree(set_ctx);
> > +		err = set_caps(dev, set_ctx,
> > MLX5_SET_HCA_CAP_OP_MOD_ODP);
> >
> >  	return err;
> >  }
> >
> > -static int handle_hca_cap(struct mlx5_core_dev *dev)
> > +static int handle_hca_cap(struct mlx5_core_dev *dev, void *set_ctx)
> >  {
> > -	void *set_ctx = NULL;
> >  	struct mlx5_profile *prof = dev->profile;
> > -	int err = -ENOMEM;
> > -	int set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> >  	void *set_hca_cap;
> > -
> > -	set_ctx = kzalloc(set_sz, GFP_KERNEL);
> > -	if (!set_ctx)
> > -		goto query_ex;
> > +	int err;
> >
> >  	err = mlx5_core_get_caps(dev, MLX5_CAP_GENERAL);
> >  	if (err)
> > -		goto query_ex;
> > +		return err;
> >
> >  	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> >  				   capability);
> > @@ -578,37 +555,42 @@ static int handle_hca_cap(struct mlx5_core_dev
> > *dev)
> >  			 num_vhca_ports,
> >  			 MLX5_CAP_GEN_MAX(dev, num_vhca_ports));
> >
> > -	err = set_caps(dev, set_ctx, set_sz,
> > -		       MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE);
> > -
> > -query_ex:
> > -	kfree(set_ctx);
> > +	err = set_caps(dev, set_ctx,
> > MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE);
> >  	return err;
>
> just return set_caps();
>
> other than this the series is ok,
>
> Acked-by: Saeed Mahameed <saeedm@mellanox.com>

Thanks, I'll fix while will take to mlx5-next.