Re: [PATCH rdma-next v3 10/14] IB/uverbs: Add support for flow counters

[Leon Romanovsky <leon@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6930 bytes --]

On Thu, May 31, 2018 at 02:49:44PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Leon Romanovsky [mailto:leon@kernel.org]
> >Sent: Thursday, May 31, 2018 9:44 AM
> >To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe
> ><jgg@mellanox.com>
> >Cc: Leon Romanovsky <leonro@mellanox.com>; RDMA mailing list <linux-
> >rdma@vger.kernel.org>; Boris Pismenny <borisp@mellanox.com>; Matan
> >Barak <matanb@mellanox.com>; Ruhl, Michael J <michael.j.ruhl@intel.com>;
> >Or Gerlitz <ogerlitz@mellanox.com>; Raed Salem <raeds@mellanox.com>;
> >Yishai Hadas <yishaih@mellanox.com>; Saeed Mahameed
> ><saeedm@mellanox.com>; linux-netdev <netdev@vger.kernel.org>
> >Subject: [PATCH rdma-next v3 10/14] IB/uverbs: Add support for flow
> >counters
> >
> >From: Raed Salem <raeds@mellanox.com>
> >
> >The struct ib_uverbs_flow_spec_action_count associates
> >a counters object with the flow.
> >
> >Post this association the flow counters can be read via
> >the counters object.
> >
> >Reviewed-by: Yishai Hadas <yishaih@mellanox.com>
> >Signed-off-by: Raed Salem <raeds@mellanox.com>
> >Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >---
> > drivers/infiniband/core/uverbs.h     |  1 +
> > drivers/infiniband/core/uverbs_cmd.c | 81
> >+++++++++++++++++++++++++++++++-----
> > include/uapi/rdma/ib_user_verbs.h    | 13 ++++++
> > 3 files changed, 84 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/infiniband/core/uverbs.h
> >b/drivers/infiniband/core/uverbs.h
> >index 5b2461fa634d..c0d40fc3a53a 100644
> >--- a/drivers/infiniband/core/uverbs.h
> >+++ b/drivers/infiniband/core/uverbs.h
> >@@ -263,6 +263,7 @@ struct ib_uverbs_flow_spec {
> > 		struct ib_uverbs_flow_spec_action_tag	flow_tag;
> > 		struct ib_uverbs_flow_spec_action_drop	drop;
> > 		struct ib_uverbs_flow_spec_action_handle action;
> >+		struct ib_uverbs_flow_spec_action_count flow_count;
> > 	};
> > };
> >
> >diff --git a/drivers/infiniband/core/uverbs_cmd.c
> >b/drivers/infiniband/core/uverbs_cmd.c
> >index ddb9d79691be..3179a95c6f5e 100644
> >--- a/drivers/infiniband/core/uverbs_cmd.c
> >+++ b/drivers/infiniband/core/uverbs_cmd.c
> >@@ -2748,43 +2748,82 @@ ssize_t ib_uverbs_detach_mcast(struct
> >ib_uverbs_file *file,
> > struct ib_uflow_resources {
> > 	size_t			max;
> > 	size_t			num;
> >-	struct ib_flow_action	*collection[0];
> >+	size_t			collection_num;
> >+	size_t			counters_num;
> >+	struct ib_counters	**counters;
> >+	struct ib_flow_action	**collection;
> > };
> >
> > static struct ib_uflow_resources *flow_resources_alloc(size_t num_specs)
> > {
> > 	struct ib_uflow_resources *resources;
> >
> >-	resources =
> >-		kmalloc(sizeof(*resources) +
> >-			num_specs * sizeof(*resources->collection),
> >GFP_KERNEL);
> >+	resources = kzalloc(sizeof(*resources), GFP_KERNEL);
> >
> > 	if (!resources)
> >-		return NULL;
> >+		goto err_res;
>
> Why the new goto?

No real reason :)

>
> >+
> >+	resources->counters =
> >+		kcalloc(num_specs, sizeof(*resources->counters),
> >GFP_KERNEL);
> >+
> >+	if (!resources->counters)
> >+		goto err_cnt;
>
> kcalloc() zeros stuff.  Could you just have a single common goto for the
> cleanup?

I have mixed feelings regarding such approach, technically you are
right, but I think that it will hurt readability.

I can send followup patch, will it work for you?

Thanks for review.

>
> Mike
>
> >+
> >+	resources->collection =
> >+		kcalloc(num_specs, sizeof(*resources->collection),
> >GFP_KERNEL);
> >+
> >+	if (!resources->collection)
> >+		goto err_collection;
> >
> >-	resources->num = 0;
> > 	resources->max = num_specs;
> >
> > 	return resources;
> >+
> >+err_collection:
> >+	kfree(resources->counters);
> >+err_cnt:
> >+	kfree(resources);
> >+err_res:
> >+	return NULL;
> > }
> >
> > void ib_uverbs_flow_resources_free(struct ib_uflow_resources *uflow_res)
> > {
> > 	unsigned int i;
> >
> >-	for (i = 0; i < uflow_res->num; i++)
> >+	for (i = 0; i < uflow_res->collection_num; i++)
> > 		atomic_dec(&uflow_res->collection[i]->usecnt);
> >
> >+	for (i = 0; i < uflow_res->counters_num; i++)
> >+		atomic_dec(&uflow_res->counters[i]->usecnt);
> >+
> >+	kfree(uflow_res->collection);
> >+	kfree(uflow_res->counters);
> > 	kfree(uflow_res);
> > }
> >
> > static void flow_resources_add(struct ib_uflow_resources *uflow_res,
> >-			       struct ib_flow_action *action)
> >+			       enum ib_flow_spec_type type,
> >+			       void *ibobj)
> > {
> > 	WARN_ON(uflow_res->num >= uflow_res->max);
> >
> >-	atomic_inc(&action->usecnt);
> >-	uflow_res->collection[uflow_res->num++] = action;
> >+	switch (type) {
> >+	case IB_FLOW_SPEC_ACTION_HANDLE:
> >+		atomic_inc(&((struct ib_flow_action *)ibobj)->usecnt);
> >+		uflow_res->collection[uflow_res->collection_num++] =
> >+			(struct ib_flow_action *)ibobj;
> >+		break;
> >+	case IB_FLOW_SPEC_ACTION_COUNT:
> >+		atomic_inc(&((struct ib_counters *)ibobj)->usecnt);
> >+		uflow_res->counters[uflow_res->counters_num++] =
> >+			(struct ib_counters *)ibobj;
> >+		break;
> >+	default:
> >+		WARN_ON(1);
> >+	}
> >+
> >+	uflow_res->num++;
> > }
> >
> > static int kern_spec_to_ib_spec_action(struct ib_ucontext *ucontext,
> >@@ -2821,9 +2860,29 @@ static int kern_spec_to_ib_spec_action(struct
> >ib_ucontext *ucontext,
> > 			return -EINVAL;
> > 		ib_spec->action.size =
> > 			sizeof(struct ib_flow_spec_action_handle);
> >-		flow_resources_add(uflow_res, ib_spec->action.act);
> >+		flow_resources_add(uflow_res,
> >+				   IB_FLOW_SPEC_ACTION_HANDLE,
> >+				   ib_spec->action.act);
> > 		uobj_put_obj_read(ib_spec->action.act);
> > 		break;
> >+	case IB_FLOW_SPEC_ACTION_COUNT:
> >+		if (kern_spec->flow_count.size !=
> >+			sizeof(struct ib_uverbs_flow_spec_action_count))
> >+			return -EINVAL;
> >+		ib_spec->flow_count.counters =
> >+			uobj_get_obj_read(counters,
> >+					  UVERBS_OBJECT_COUNTERS,
> >+					  kern_spec->flow_count.handle,
> >+					  ucontext);
> >+		if (!ib_spec->flow_count.counters)
> >+			return -EINVAL;
> >+		ib_spec->flow_count.size =
> >+				sizeof(struct ib_flow_spec_action_count);
> >+		flow_resources_add(uflow_res,
> >+				   IB_FLOW_SPEC_ACTION_COUNT,
> >+				   ib_spec->flow_count.counters);
> >+		uobj_put_obj_read(ib_spec->flow_count.counters);
> >+		break;
> > 	default:
> > 		return -EINVAL;
> > 	}
> >diff --git a/include/uapi/rdma/ib_user_verbs.h
> >b/include/uapi/rdma/ib_user_verbs.h
> >index 409507f83b91..4f9991de8e3a 100644
> >--- a/include/uapi/rdma/ib_user_verbs.h
> >+++ b/include/uapi/rdma/ib_user_verbs.h
> >@@ -998,6 +998,19 @@ struct ib_uverbs_flow_spec_action_handle {
> > 	__u32			      reserved1;
> > };
> >
> >+struct ib_uverbs_flow_spec_action_count {
> >+	union {
> >+		struct ib_uverbs_flow_spec_hdr hdr;
> >+		struct {
> >+			__u32 type;
> >+			__u16 size;
> >+			__u16 reserved;
> >+		};
> >+	};
> >+	__u32			      handle;
> >+	__u32			      reserved1;
> >+};
> >+
> > struct ib_uverbs_flow_tunnel_filter {
> > 	__be32 tunnel_id;
> > };
> >--
> >2.14.3
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]