IB/mlx5: Support flow counters offset for bulk counters

PATCH In review Leon Romanovsky 3 participants 4 messages Oct 29, 2019

Subsystems (2)

InfiniBand Mellanox MLX5 IB driver

lore

Share & tools

lore

Leon Romanovsky

author

2019-10-29 05:59 UTC #0 lore raw

PATCH 1/1 IB/mlx5: Support flow counters offset for bulk counters

From: Yevgeny Kliteynik <[email protected]>

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 <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/hw/mlx5/devx.c        | 12 ++++++++-
 drivers/infiniband/hw/mlx5/flow.c        | 34 ++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/mlx5_ib.h     |  2 +-
 include/uapi/rdma/mlx5_user_ioctl_cmds.h |  1 +
 4 files changed, 45 insertions(+), 4 deletions(-)

drivers/infiniband/hw/mlx5/devx.c +11 -1

index 6b1fca91d7d3..3900fcb1ccaf 100644
@@ -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,7 +193,7 @@ 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 *counter_id, u32 *bulk_size)
 {
 	struct devx_obj *devx_obj = obj;
 	u16 opcode = MLX5_GET(general_obj_in_cmd_hdr, devx_obj->dinbox, opcode);

@@ -201,6 +202,7 @@ bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id)
 		*counter_id = MLX5_GET(dealloc_flow_counter_in,
 				       devx_obj->dinbox,
 				       flow_counter_id);
+		*bulk_size = devx_obj->flow_counter_bulk_size;
 		return true;
 	}

@@ -1463,6 +1465,14 @@ 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);
+		if (bulk)
+			obj->flow_counter_bulk_size = 64UL << ffs(bulk);
+	}
+
 	uobj->object = obj;
 	INIT_LIST_HEAD(&obj->event_sub);
 	obj->ib_dev = dev;

drivers/infiniband/hw/mlx5/flow.c +32 -2

index b198ff10cde9..05637039bcd7 100644
@@ -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 bulk_size = 0;
+	u32 *offset;
 
 	if (!capable(CAP_NET_RAW))
 		return -EPERM;

@@ -151,8 +153,32 @@ 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 (!mlx5_ib_devx_is_flow_counter(devx_obj,
+						  &counter_id,
+						  &bulk_size))
 			return -EINVAL;
+
+		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(uint32_t));
+
+			if (num_offsets != 1)
+				return -EINVAL;
+
+			offset = uverbs_attr_get_alloced_ptr(
+				attrs,
+				MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET);
+
+			if (*offset && *offset >= bulk_size)
+				return -EINVAL;
+
+			counter_id += *offset;
+		}
+
 		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
 	}

@@ -598,7 +624,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(uint32_t)),
+			   UA_OPTIONAL,
+			   UA_ALLOC_AND_COPY));
 
 DECLARE_UVERBS_NAMED_METHOD_DESTROY(
 	MLX5_IB_METHOD_DESTROY_FLOW,

drivers/infiniband/hw/mlx5/mlx5_ib.h +1 -1

index 0bdb8b45ea15..0fb58ecccb7e 100644
@@ -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 *counter_id, u32 *bulk_size);
 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

include/uapi/rdma/mlx5_user_ioctl_cmds.h +1 -0

index d0da070cf0ab..20d88307f75f 100644
@@ -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 {

Signature · 1 line

2.20.1

Mark Bloch

2019-10-29 06:48 UTC #1 lore raw

Hey Leon,

Quoted 49 lines · Leon Romanovsky

From: Yevgeny Kliteynik <[email protected]>

Add support for flow steering counters action with a non-base counter ID (offset) for bulk counters.

Signed-off-by Yevgeny Kliteynik <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/hw/mlx5/devx.c        | 12 ++++++++-
 drivers/infiniband/hw/mlx5/flow.c        | 34 ++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/mlx5_ib.h     |  2 +-
 include/uapi/rdma/mlx5_user_ioctl_cmds.h |  1 +
 4 files changed, 45 insertions(+), 4 deletions(-)

drivers/infiniband/hw/mlx5/devx.c +3 -1

index 6b1fca91d7d3..3900fcb1ccaf 100644
@@ -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,7 +193,7 @@ 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 *counter_id, u32 *bulk_size)
 {
 	struct devx_obj *devx_obj = obj;
 	u16 opcode = MLX5_GET(general_obj_in_cmd_hdr, devx_obj->dinbox, opcode);

@@ -201,6 +202,7 @@ bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id)
 		*counter_id = MLX5_GET(dealloc_flow_counter_in,
 				       devx_obj->dinbox,
 				       flow_counter_id);
+		*bulk_size = devx_obj->flow_counter_bulk_size;
 		return true;
 	}

drivers/infiniband/hw/mlx5/devx.c static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)( · line 1473

Quoted 9 lines

drivers/infiniband/hw/mlx5/devx.c +6 -0

@@ -1463,6 +1465,14 @@ 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);
+		if (bulk)
+			obj->flow_counter_bulk_size = 64UL << ffs(bulk);

Why do you need ffs and not just: 64 << bulk ?
As this field is a bitmask, only a single bit should be set and that should already be validated by the FW.

Quoted 34 lines

drivers/infiniband/hw/mlx5/devx.c +2 -0

+	}
+
 	uobj->object = obj;
 	INIT_LIST_HEAD(&obj->event_sub);
 	obj->ib_dev = dev;

drivers/infiniband/hw/mlx5/flow.c +12 -1

index b198ff10cde9..05637039bcd7 100644
@@ -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 bulk_size = 0;
+	u32 *offset;
 
 	if (!capable(CAP_NET_RAW))
 		return -EPERM;

@@ -151,8 +153,32 @@ 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 (!mlx5_ib_devx_is_flow_counter(devx_obj,
+						  &counter_id,
+						  &bulk_size))
 			return -EINVAL;
+
+		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,

drivers/infiniband/hw/mlx5/flow.c static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)( · line 176

Quoted 10 lines

drivers/infiniband/hw/mlx5/flow.c +10 -0

+				sizeof(uint32_t));
+
+			if (num_offsets != 1)
+				return -EINVAL;> +
+			offset = uverbs_attr_get_alloced_ptr(
+				attrs,
+				MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET);
+
+			if (*offset && *offset >= bulk_size)
+				return -EINVAL;

This logic/validity check should probably be in: mlx5_ib_devx_is_flow_counter(). you pass it the the offset (or 0) and you get back a counter_id and false/true if valid.

Quoted 8 lines

drivers/infiniband/hw/mlx5/flow.c +4 -0

+
+			counter_id += *offset;
+		}
+
 		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
 	}

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 629

Quoted 7 lines

drivers/infiniband/hw/mlx5/flow.c +3 -1

@@ -598,7 +624,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(uint32_t)),

Why uint32_t and not u32?

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 631

Quoted 2 lines

drivers/infiniband/hw/mlx5/flow.c +2 -0
+			   UA_OPTIONAL,
+			   UA_ALLOC_AND_COPY));

side note, both MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET and MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX are optional, but you should provide MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET only
if you are also passing MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX.

Which means you can pass MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET without MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX and everything will work.

I wonder if we should have a way to define such things.

Mark

Quoted 29 lines

drivers/infiniband/hw/mlx5/flow.c +0 -0

 DECLARE_UVERBS_NAMED_METHOD_DESTROY(
 	MLX5_IB_METHOD_DESTROY_FLOW,

drivers/infiniband/hw/mlx5/mlx5_ib.h +1 -1

index 0bdb8b45ea15..0fb58ecccb7e 100644
@@ -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 *counter_id, u32 *bulk_size);
 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

include/uapi/rdma/mlx5_user_ioctl_cmds.h +1 -0

index d0da070cf0ab..20d88307f75f 100644
@@ -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 {

Leon Romanovsky

author

2019-10-29 07:42 UTC #2 lore raw

Quoted 55 lines · Mark Bloch

Hey Leon,

Quoted 52 lines · Leon Romanovsky

From: Yevgeny Kliteynik <[email protected]>

Add support for flow steering counters action with a non-base counter ID (offset) for bulk counters.

Signed-off-by Yevgeny Kliteynik <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/hw/mlx5/devx.c        | 12 ++++++++-
 drivers/infiniband/hw/mlx5/flow.c        | 34 ++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/mlx5_ib.h     |  2 +-
 include/uapi/rdma/mlx5_user_ioctl_cmds.h |  1 +
 4 files changed, 45 insertions(+), 4 deletions(-)

drivers/infiniband/hw/mlx5/devx.c +3 -1

index 6b1fca91d7d3..3900fcb1ccaf 100644
@@ -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,7 +193,7 @@ 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 *counter_id, u32 *bulk_size)
 {
 	struct devx_obj *devx_obj = obj;
 	u16 opcode = MLX5_GET(general_obj_in_cmd_hdr, devx_obj->dinbox, opcode);

@@ -201,6 +202,7 @@ bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id)
 		*counter_id = MLX5_GET(dealloc_flow_counter_in,
 				       devx_obj->dinbox,
 				       flow_counter_id);
+		*bulk_size = devx_obj->flow_counter_bulk_size;
 		return true;
 	}

@@ -1463,6 +1465,14 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)(
 	if (err)
 		goto obj_free;

Quoted 10 lines

drivers/infiniband/hw/mlx5/devx.c static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)( · line 1473
Quoted 6 lines
drivers/infiniband/hw/mlx5/devx.c +6 -0
+	if (opcode == MLX5_CMD_OP_ALLOC_FLOW_COUNTER) {
+		u8 bulk = MLX5_GET(alloc_flow_counter_in,
+				   cmd_in,
+				   flow_counter_bulk);
+		if (bulk)
+			obj->flow_counter_bulk_size = 64UL << ffs(bulk);
Why do you need ffs and not just: 64 << bulk ?
As this field is a bitmask, only a single bit should be set and that should already be validated by the FW.

I preferred this approach instead of checking if "64UL << bulk" overflow while doing shift, but if you insist, we can change the code below to be something like this:
check_shl_overflow(64UL, bulk, obj->flow_counter_bulk_size)

Quoted 40 lines

drivers/infiniband/hw/mlx5/devx.c +2 -0

+	}
+
 	uobj->object = obj;
 	INIT_LIST_HEAD(&obj->event_sub);
 	obj->ib_dev = dev;

drivers/infiniband/hw/mlx5/flow.c +17 -1

index b198ff10cde9..05637039bcd7 100644
@@ -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 bulk_size = 0;
+	u32 *offset;
 
 	if (!capable(CAP_NET_RAW))
 		return -EPERM;

@@ -151,8 +153,32 @@ 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 (!mlx5_ib_devx_is_flow_counter(devx_obj,
+						  &counter_id,
+						  &bulk_size))
 			return -EINVAL;
+
+		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(uint32_t));
+
+			if (num_offsets != 1)
+				return -EINVAL;> +
+			offset = uverbs_attr_get_alloced_ptr(

Quoted 8 lines

drivers/infiniband/hw/mlx5/flow.c static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)( · line 177
Quoted 5 lines
drivers/infiniband/hw/mlx5/flow.c +5 -0
+				attrs,
+				MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET);
+
+			if (*offset && *offset >= bulk_size)
+				return -EINVAL;
This logic/validity check should probably be in: mlx5_ib_devx_is_flow_counter(). you pass it the the offset (or 0) and you get back a counter_id and false/true if valid.

Agree.

Quoted 8 lines

drivers/infiniband/hw/mlx5/flow.c +4 -0

+
+			counter_id += *offset;
+		}
+
 		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
 	}

Quoted 10 lines

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 629

Quoted 8 lines

drivers/infiniband/hw/mlx5/flow.c +3 -1

@@ -598,7 +624,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(uint32_t)),

Why uint32_t and not u32?

Copy/paste from user space.

Quoted 11 lines

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 631
Quoted 3 lines
drivers/infiniband/hw/mlx5/flow.c +2 -0
+			   UA_OPTIONAL,
+			   UA_ALLOC_AND_COPY));
side note, both MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET and MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX are optional, but you should provide MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET only
if you are also passing MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX.
Which means you can pass MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET without MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX and everything will work.
I wonder if we should have a way to define such things.

Jason ???

Quoted 32 lines

Mark

Quoted 29 lines

drivers/infiniband/hw/mlx5/flow.c +0 -0

 DECLARE_UVERBS_NAMED_METHOD_DESTROY(
 	MLX5_IB_METHOD_DESTROY_FLOW,

drivers/infiniband/hw/mlx5/mlx5_ib.h +1 -1

index 0bdb8b45ea15..0fb58ecccb7e 100644
@@ -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 *counter_id, u32 *bulk_size);
 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

include/uapi/rdma/mlx5_user_ioctl_cmds.h +1 -0

index d0da070cf0ab..20d88307f75f 100644
@@ -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 {

Yevgeny Kliteynik

2019-10-29 11:57 UTC #3 lore raw

In reply to Mark Bloch

Hi Mark,

Quoted 63 lines

From: Mark Bloch <[email protected]>
Sent: Tuesday, October 29, 2019 08:49
Hey Leon,
drivers/infiniband/hw/mlx5/devx.c bool mlx5_ib_devx_is_flow_dest(void *obj, int
Quoted 32 lines · Leon Romanovsky
From: Yevgeny Kliteynik <[email protected]>
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 <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx5/devx.c        | 12 ++++++++-
 drivers/infiniband/hw/mlx5/flow.c        | 34 ++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/mlx5_ib.h     |  2 +-
 include/uapi/rdma/mlx5_user_ioctl_cmds.h |  1 +
 4 files changed, 45 insertions(+), 4 deletions(-)
drivers/infiniband/hw/mlx5/devx.c +1 -0
index 6b1fca91d7d3..3900fcb1ccaf 100644
@@ -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,7 +193,7 @@ bool mlx5_ib_devx_is_flow_dest(void *obj, int
*dest_id, int *dest_type)
drivers/infiniband/hw/mlx5/devx.c bool mlx5_ib_devx_is_flow_dest(void *obj, int *dest_id, int *dest_type)
Quoted 5 lines
drivers/infiniband/hw/mlx5/devx.c +1 -1
 	}
 }
 
-bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id)
+bool mlx5_ib_devx_is_flow_counter(void *obj, u32 *counter_id, u32
*bulk_size)
drivers/infiniband/hw/mlx5/devx.c bool mlx5_ib_devx_is_flow_dest(void *obj, int *dest_id, int *dest_type)
Quoted 3 lines
drivers/infiniband/hw/mlx5/devx.c +0 -0
 {
 	struct devx_obj *devx_obj = obj;
 	u16 opcode = MLX5_GET(general_obj_in_cmd_hdr, devx_obj->dinbox,
opcode);
drivers/infiniband/hw/mlx5/devx.c bool mlx5_ib_devx_is_flow_counter(void *obj, u32
Quoted 1 line
drivers/infiniband/hw/mlx5/devx.c +0 -0
@@ -201,6 +202,7 @@ bool mlx5_ib_devx_is_flow_counter(void *obj, u32
*counter_id)
drivers/infiniband/hw/mlx5/devx.c static int
Quoted 8 lines
drivers/infiniband/hw/mlx5/devx.c +1 -0
 		*counter_id = MLX5_GET(dealloc_flow_counter_in,
 				       devx_obj->dinbox,
 				       flow_counter_id);
+		*bulk_size = devx_obj->flow_counter_bulk_size;
 		return true;
 	}
 
@@ -1463,6 +1465,14 @@ static int
UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)(
Quoted 3 lines
drivers/infiniband/hw/mlx5/devx.c +0 -0
 	if (err)
 		goto obj_free;
 

Quoted 10 lines

drivers/infiniband/hw/mlx5/devx.c static int UVERBS_HANDLER(MLX5_IB_METHOD_DEVX_OBJ_CREATE)( · line 1473
Quoted 6 lines
drivers/infiniband/hw/mlx5/devx.c +6 -0
+	if (opcode == MLX5_CMD_OP_ALLOC_FLOW_COUNTER) {
+		u8 bulk = MLX5_GET(alloc_flow_counter_in,
+				   cmd_in,
+				   flow_counter_bulk);
+		if (bulk)
+			obj->flow_counter_bulk_size = 64UL << ffs(bulk);
Why do you need ffs and not just: 64 << bulk ?
As this field is a bitmask, only a single bit should be set and that should already be validated by the FW.

Because we want the index of bit that is set and not the value of the bitmask. I can, however, do (128 * bulk) instead, if you don't like ffs. This probably looks nicer, and I don't need to check 'if (bulk)'.

Quoted 45 lines

drivers/infiniband/hw/mlx5/flow.c static int
Quoted 10 lines
drivers/infiniband/hw/mlx5/devx.c +2 -0
+	}
+
 	uobj->object = obj;
 	INIT_LIST_HEAD(&obj->event_sub);
 	obj->ib_dev = dev;
drivers/infiniband/hw/mlx5/flow.c +0 -0
index b198ff10cde9..05637039bcd7 100644
@@ -85,6 +85,8 @@ static int
UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
Quoted 11 lines
drivers/infiniband/hw/mlx5/flow.c +2 -0
 	struct mlx5_ib_dev *dev = mlx5_udata_to_mdev(&attrs->driver_udata);
 	int len, ret, i;
 	u32 counter_id = 0;
+	u32 bulk_size = 0;
+	u32 *offset;
 
 	if (!capable(CAP_NET_RAW))
 		return -EPERM;
@@ -151,8 +153,32 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
 	if (len) {
 		devx_obj = arr_flow_actions[0]->object;
drivers/infiniband/hw/mlx5/flow.c static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)( · line 163
Quoted 10 lines
drivers/infiniband/hw/mlx5/flow.c +7 -1
 
-		if (!mlx5_ib_devx_is_flow_counter(devx_obj, &counter_id))
+		if (!mlx5_ib_devx_is_flow_counter(devx_obj,
+						  &counter_id,
+						  &bulk_size))
 			return -EINVAL;
+
+		if (uverbs_attr_is_valid(
+			    attrs,
+ 
	MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET)) {
drivers/infiniband/hw/mlx5/flow.c static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)( · line 168
Quoted 3 lines
drivers/infiniband/hw/mlx5/flow.c +3 -0
+			int num_offsets = uverbs_attr_ptr_get_array_size(
+				attrs,
+
MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET,
drivers/infiniband/hw/mlx5/flow.c static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)( · line 175
Quoted 7 lines
drivers/infiniband/hw/mlx5/flow.c +7 -0
+				sizeof(uint32_t));
+
+			if (num_offsets != 1)
+				return -EINVAL;> +
+			offset = uverbs_attr_get_alloced_ptr(
+				attrs,
+
	MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET);

Quoted 7 lines

drivers/infiniband/hw/mlx5/flow.c static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)( · line 177
Quoted 3 lines
drivers/infiniband/hw/mlx5/flow.c +3 -0
+
+			if (*offset && *offset >= bulk_size)
+				return -EINVAL;
This logic/validity check should probably be in: mlx5_ib_devx_is_flow_counter(). you pass it the the offset (or 0) and you get back a counter_id and false/true if valid.

Sure, we can go this way as well.

Quoted 17 lines

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 624
Quoted 9 lines
drivers/infiniband/hw/mlx5/flow.c +4 -0
+
+			counter_id += *offset;
+		}
+
 		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_COUNT;
 	}
 
@@ -598,7 +624,11 @@ DECLARE_UVERBS_NAMED_METHOD(
 
UVERBS_ATTR_IDRS_ARR(MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX,
drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD(
Quoted 5 lines
drivers/infiniband/hw/mlx5/flow.c +2 -1
 			     MLX5_IB_OBJECT_DEVX_OBJ,
 			     UVERBS_ACCESS_READ, 1, 1,
-			     UA_OPTIONAL));
+			     UA_OPTIONAL),
+
UVERBS_ATTR_PTR_IN(MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTER
S_DEVX_OFFSET,

Quoted 3 lines

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 629
Quoted 1 line
drivers/infiniband/hw/mlx5/flow.c +1 -0
+			   UVERBS_ATTR_MIN_SIZE(sizeof(uint32_t)),
Why uint32_t and not u32?

Oops, here and in other places :)

Quoted 8 lines

drivers/infiniband/hw/mlx5/flow.c DECLARE_UVERBS_NAMED_METHOD( · line 631
Quoted 2 lines
drivers/infiniband/hw/mlx5/flow.c +2 -0
+			   UA_OPTIONAL,
+			   UA_ALLOC_AND_COPY));
side note, both MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET and MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX
are optional, but you should provide MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET only if you are also passing MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX.

Quoted 4 lines

Which means you can pass MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX_OFFSET without MLX5_IB_ATTR_CREATE_FLOW_ARR_COUNTERS_DEVX and everything will work.

True. You don't have a straight-forward way to do it because rdma-core provides COUNTERS_DEVX_OFFSET only as part of providing COUNTERS_DEVX, but nothing prevents you from writing your own code to do so.

Quoted 1 line

I wonder if we should have a way to define such things.

We can discuss this.
In the meantime, I'll send a V2 of the patch with all the aforementioned fixes.

Thanks!

-- YK

Quoted 1 line

Mark