Re: [PATCH rdma-next v2] IB/mlx5: Support flow counters offset for bulk counters

[Mark Bloch <markb@mellanox.com>]

On 11/3/2019 06:07, Leon Romanovsky wrote:
> From: Yevgeny Kliteynik <kliteyn@mellanox.com>
> 
> Add support for flow steering counters action with
> a non-base counter ID (offset) for bulk counters.
> 
> When creating a flow counter object, save the bulk value.
> This value is used when a flow action with a non-base
> counter ID is requested - to validate that the required
> offset is in the range of the allocated bulk.
> 
> Signed-off-by: Yevgeny Kliteynik <kliteyn@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  Changelog
>  v1->v2: https://lore.kernel.org/linux-rdma/20191029155020.20792-1-leon@kernel.org
>  * Fixed mlx5_ib_devx_is_flow_counter() logic
>  v0 -> v1: https://lore.kernel.org/linux-rdma/20191029055916.7322-1-leon@kernel.org
>  * Change ffs to multiply bitmap
>  * Changed uint32_t to be u32
>  * Added offset to mlx5_ib_devx_is_flow_counter()
> ---
>  drivers/infiniband/hw/mlx5/devx.c        | 15 +++++++++++-
>  drivers/infiniband/hw/mlx5/flow.c        | 29 ++++++++++++++++++++++--
>  drivers/infiniband/hw/mlx5/mlx5_ib.h     |  2 +-
>  include/uapi/rdma/mlx5_user_ioctl_cmds.h |  1 +
>  4 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> index 6b1fca91d7d3..2a5812a4c3bb 100644
> --- a/drivers/infiniband/hw/mlx5/devx.c
> +++ b/drivers/infiniband/hw/mlx5/devx.c
> @@ -100,6 +100,7 @@ struct devx_obj {
>  		struct mlx5_ib_devx_mr	devx_mr;
>  		struct mlx5_core_dct	core_dct;
>  		struct mlx5_core_cq	core_cq;
> +		u32			flow_counter_bulk_size;
>  	};
>  	struct list_head event_sub; /* holds devx_event_subscription entries */
>  };
> @@ -192,15 +193,20 @@ bool mlx5_ib_devx_is_flow_dest(void *obj, int *dest_id, int *dest_type)
>  	}
>  }
> 
> -bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id)
> +bool mlx5_ib_devx_is_flow_counter(void *obj, u32 offset, u32 *counter_id)
>  {
>  	struct devx_obj *devx_obj = obj;
>  	u16 opcode = MLX5_GET(general_obj_in_cmd_hdr, devx_obj->dinbox, opcode);
> 
>  	if (opcode == MLX5_CMD_OP_DEALLOC_FLOW_COUNTER) {
> +
> +		if (offset && offset >= devx_obj->flow_counter_bulk_size)
> +			return false;
> +
>  		*counter_id = MLX5_GET(dealloc_flow_counter_in,
>  				       devx_obj->dinbox,
>  				       flow_counter_id);
> +		*counter_id += offset;
>  		return true;
>  	}
> 
> @@ -1463,6 +1469,13 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)(
>  	if (err)
>  		goto obj_free;
> 
> +	if (opcode == MLX5_CMD_OP_ALLOC_FLOW_COUNTER) {
> +		u8 bulk = MLX5_GET(alloc_flow_counter_in,
> +				   cmd_in,
> +				   flow_counter_bulk);
> +		obj->flow_counter_bulk_size = 128UL * bulk;
> +	}
> +
>  	uobj->object = obj;
>  	INIT_LIST_HEAD(&obj->event_sub);
>  	obj->ib_dev = dev;
> diff --git a/drivers/infiniband/hw/mlx5/flow.c b/drivers/infiniband/hw/mlx5/flow.c
> index b198ff10cde9..dbee17d22d50 100644
> --- a/drivers/infiniband/hw/mlx5/flow.c
> +++ b/drivers/infiniband/hw/mlx5/flow.c
> @@ -85,6 +85,8 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
>  	struct mlx5_ib_dev *dev = mlx5_udata_to_mdev(&attrs->driver_udata);
>  	int len, ret, i;
>  	u32 counter_id = 0;
> +	u32 *offset_attr;
> +	u32 offset = 0;
> 
>  	if (!capable(CAP_NET_RAW))
>  		return -EPERM;
> @@ -151,8 +153,27 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
>  	if (len) {
>  		devx_obj = arr_flow_actions[0]->object;
> 
> -		if (!mlx5_ib_devx_is_flow_counter(devx_obj, &counter_id))
> +		if (uverbs_attr_is_valid(attrs,
> +					 MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET)) {
> +
> +			int num_offsets = uverbs_attr_ptr_get_array_size(
> +				attrs,
> +				MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET,
> +				sizeof(u32));
> +
> +			if (num_offsets != 1)
> +				return -EINVAL;
> +
> +			offset_attr = uverbs_attr_get_alloced_ptr(
> +				attrs,
> +				MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET);
> +			offset = *offset_attr;

I would have used offset_arr and would have done:
offset = offset_attr[0] seems more appropriate but I don't mind either way.

Reviewed-by: Mark Bloch <markb@mellanox.com>

> +		}
> +
> +		if (!mlx5_ib_devx_is_flow_counter(devx_obj, offset,
> +						  &counter_id))
>  			return -EINVAL;
> +
>  		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
>  	}
> 
> @@ -598,7 +619,11 @@ DECLARE_UVERBS_NAMED_METHOD(
>  	UVERBS_ATTR_IDRS_ARR(MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX,
>  			     MLX5_IB_OBJECT_DEVX_OBJ,
>  			     UVERBS_ACCESS_READ, 1, 1,
> -			     UA_OPTIONAL));
> +			     UA_OPTIONAL),
> +	UVERBS_ATTR_PTR_IN(MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET,
> +			   UVERBS_ATTR_MIN_SIZE(sizeof(u32)),
> +			   UA_OPTIONAL,
> +			   UA_ALLOC_AND_COPY));
> 
>  DECLARE_UVERBS_NAMED_METHOD_DESTROY(
>  	MLX5_IB_METHOD_DESTROY_FLOW,
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index 0bdb8b45ea15..8ca72cc8797e 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -1367,7 +1367,7 @@ struct mlx5_ib_flow_handler *mlx5_ib_raw_fs_rule_add(
>  	struct mlx5_flow_act *flow_act, u32 counter_id,
>  	void *cmd_in, int inlen, int dest_id, int dest_type);
>  bool mlx5_ib_devx_is_flow_dest(void *obj, int *dest_id, int *dest_type);
> -bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id);
> +bool mlx5_ib_devx_is_flow_counter(void *obj, u32 offset, u32 *counter_id);
>  int mlx5_ib_get_flow_trees(const struct uverbs_object_tree_def **root);
>  void mlx5_ib_destroy_flow_action_raw(struct mlx5_ib_flow_action *maction);
>  #else
> diff --git a/include/uapi/rdma/mlx5_user_ioctl_cmds.h b/include/uapi/rdma/mlx5_user_ioctl_cmds.h
> index d0da070cf0ab..20d88307f75f 100644
> --- a/include/uapi/rdma/mlx5_user_ioctl_cmds.h
> +++ b/include/uapi/rdma/mlx5_user_ioctl_cmds.h
> @@ -198,6 +198,7 @@ enum mlx5_ib_create_flow_attrs {
>  	MLX5_IB_ATTR_CREATE_FLOW_ARR_FLOW_ACTIONS,
>  	MLX5_IB_ATTR_CREATE_FLOW_TAG,
>  	MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX,
> +	MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET,
>  };
> 
>  enum mlx5_ib_destoy_flow_attrs {
> --
> 2.20.1
>