On Wed, Dec 19, 2018 at 05:08:54AM +0000, Jason Gunthorpe wrote:
> ib_umem_get can only be called in a method callback, which always has a
> udata parameter. This allows ib_umem_get() to derive the ucontext pointer
> directly from the udata without requiring the drivers to find it in some
> way or another.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> ---
> drivers/infiniband/core/umem.c | 31 +++++++++++++++++--
> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 11 +++----
> drivers/infiniband/hw/cxgb3/iwch_provider.c | 2 +-
> drivers/infiniband/hw/cxgb4/mem.c | 2 +-
> drivers/infiniband/hw/hns/hns_roce_cq.c | 9 +++---
> drivers/infiniband/hw/hns/hns_roce_db.c | 6 ++--
> drivers/infiniband/hw/hns/hns_roce_device.h | 3 +-
> drivers/infiniband/hw/hns/hns_roce_mr.c | 7 ++---
> drivers/infiniband/hw/hns/hns_roce_qp.c | 13 ++++----
> drivers/infiniband/hw/hns/hns_roce_srq.c | 7 ++---
> drivers/infiniband/hw/i40iw/i40iw_verbs.c | 2 +-
> drivers/infiniband/hw/mlx4/cq.c | 16 +++++-----
> drivers/infiniband/hw/mlx4/doorbell.c | 6 ++--
> drivers/infiniband/hw/mlx4/mlx4_ib.h | 3 +-
> drivers/infiniband/hw/mlx4/mr.c | 13 ++++----
> drivers/infiniband/hw/mlx4/qp.c | 8 +++--
> drivers/infiniband/hw/mlx4/srq.c | 5 ++-
> drivers/infiniband/hw/mlx5/cq.c | 7 ++---
> drivers/infiniband/hw/mlx5/devx.c | 2 +-
> drivers/infiniband/hw/mlx5/doorbell.c | 6 ++--
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 4 ++-
> drivers/infiniband/hw/mlx5/mr.c | 22 ++++++-------
> drivers/infiniband/hw/mlx5/odp.c | 4 +--
> drivers/infiniband/hw/mlx5/qp.c | 25 +++++++--------
> drivers/infiniband/hw/mlx5/srq.c | 5 ++-
> drivers/infiniband/hw/mthca/mthca_provider.c | 2 +-
> drivers/infiniband/hw/nes/nes_verbs.c | 4 +--
> drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 2 +-
> drivers/infiniband/hw/qedr/verbs.c | 28 ++++++++---------
> drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c | 2 +-
> drivers/infiniband/hw/vmw_pvrdma/pvrdma_mr.c | 3 +-
> drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c | 6 ++--
> drivers/infiniband/hw/vmw_pvrdma/pvrdma_srq.c | 4 +--
> drivers/infiniband/sw/rdmavt/mr.c | 3 +-
> drivers/infiniband/sw/rxe/rxe_mr.c | 2 +-
> include/rdma/ib_umem.h | 5 +--
> 36 files changed, 150 insertions(+), 130 deletions(-)
>
> Shamir, this is the next step I see in your series - this alone chops
> out another 30 cases. Nearly all core APIs toward the driver should be
> adjusted similarly in patches like this (ie rdma_user_mmap_page() and
> so forth)
Great!
>
> The immediate step beyond this is to make rdma_get_ucontext into a
> driver exported function and replace all *->uobject->context with
> it. Maybe also adjust things so it can't fail, hard to say right now
> if failure will be inconvenient.
>
I did something like this based on your recent work to unify the uverbs
& ioctl code path. Will add the patches to this email and if they are
fine with you we could use them to fill the ucontext in the udata from
uverbs & ioctl.
> Then we can see what is left and make a decision how to handle it.
>
> I made this patch pretty fast, you should check it, test it, make the
> other similarly related ones, etc.
>
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index c6144df47ea47e..37aa3fbda56fc3 100644
> --- a/drivers/infiniband/core/umem.c
> +++ b/drivers/infiniband/core/umem.c
> @@ -43,6 +43,28 @@
>
> #include "uverbs.h"
>
> +/* rdma_get_ucontext - Return the ucontext from a udata
> + * @udata: The udata to get the context from
> + *
> + * This can only be called from within a uapi method that was passed ib_udata
> + * as a parameter. It returns the ucontext associated with the udata, or ERR_PTR
> + * if the udata is NULL or the ucontext has been disassociated.
> + */
> +static struct ib_ucontext *rdma_get_ucontext(struct ib_udata *udata)
> +{
> + if (!udata)
> + return ERR_PTR(-EIO);
> +
> + /*
> + * FIXME: Really all cases that get here with a udata will have
> + * already called ib_uverbs_get_ucontext_file, or located a uobject
> + * that points to a ucontext. We could store that result in the udata
> + * so this function can't fail.
> + */
> + return ib_uverbs_get_ucontext_file(
> + container_of(udata, struct uverbs_attr_bundle, driver_udata)
> + ->ufile);
I think the patches I attached here solve this so if you are OK with
them I think there is no need for this function unless I missed
something.
> +}
>
Thanks
>From dc6f3a023c627832a93aaede5cfea052f3973fc4 Mon Sep 17 00:00:00 2001
From: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
Date: Sun, 14 Oct 2018 09:11:12 +0300
Subject: [PATCH 1/2] IB/verbs: add ib_ucontext to ib_udata
ib_udata is used in (almost) all the places in the code that require
ib_ucontext and can convey this information to the ib core and driver
layers.
Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
---
include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index b8ddb2d82372..b7ec9da0b22b 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1533,8 +1533,36 @@ struct ib_udata {
void __user *outbuf;
size_t inlen;
size_t outlen;
+ struct ib_ucontext *context; /* associated user context */
};
+/**
+ * _rdma_udata_context - Get ucontext from udata
+ * @udata: The udata object
+ * @warn: Warn true/false if ucontext is NULL
+ *
+ * NOTE:
+ * Most of the code should use rdma_udata_context which turn
+ * the NULL ucontext warning on. Use this function if you need
+ * to *test* if udata has ucontext and want to turn the warning
+ * off.
+ */
+static inline struct ib_ucontext *_rdma_udata_context(struct ib_udata *udata,
+ bool warn)
+{
+ WARN_ON(warn && !udata->context);
+ return udata->context;
+}
+
+/**
+ * rdma_udata_context - Get ucontext from udata
+ * @udata: The udata object
+ *
+ * NOTE: Assume valid ucontext & warn if ucontext is NULL!
+ */
+#define rdma_udata_context(udata) \
+ _rdma_udata_context(udata, true)
+
struct ib_pd {
u32 local_dma_lkey;
u32 flags;
--
2.17.2
>From 033dc509ba8ec9d5521c0b04cbffba203273324d Mon Sep 17 00:00:00 2001
From: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
Date: Tue, 18 Dec 2018 17:13:53 +0200
Subject: [PATCH 2/2] IB/uverbs: initialize context field in ib_udata
initialize the context field in ib_udata created from uverbs commands
and extended commands path.
NOTE:
follow this change, any code path that call uobj_get_[read|write]
directly/indirectly can no longer define the uverbs_attr_bundle as
'const'.
Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
---
drivers/infiniband/core/uverbs.h | 1 +
drivers/infiniband/core/uverbs_cmd.c | 2 +-
drivers/infiniband/core/uverbs_main.c | 1 +
include/rdma/uverbs_std_types.h | 20 ++++++++++++++++----
4 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 8b41c95300c6..ce0800de1145 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -61,6 +61,7 @@ ib_uverbs_init_udata(struct ib_udata *udata,
udata->outbuf = obuf;
udata->inlen = ilen;
udata->outlen = olen;
+ udata->context = NULL;
}
static inline void
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index f68d234b09f2..d37bc7401064 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2670,7 +2670,7 @@ void flow_resources_add(struct ib_uflow_resources *uflow_res,
}
EXPORT_SYMBOL(flow_resources_add);
-static int kern_spec_to_ib_spec_action(const struct uverbs_attr_bundle *attrs,
+static int kern_spec_to_ib_spec_action(struct uverbs_attr_bundle *attrs,
struct ib_uverbs_flow_spec *kern_spec,
union ib_flow_spec *ib_spec,
struct ib_uflow_resources *uflow_res)
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 96a5f89bbb75..084c1cd06b2f 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -705,6 +705,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
bundle.driver_udata.inbuf = buf + in_len;
else
bundle.driver_udata.inbuf = NULL;
+ bundle.driver_udata.context = NULL;
} else {
memset(&bundle.driver_udata, 0,
sizeof(bundle.driver_udata));
diff --git a/include/rdma/uverbs_std_types.h b/include/rdma/uverbs_std_types.h
index 883abcf6d36e..a0fcd5818c0f 100644
--- a/include/rdma/uverbs_std_types.h
+++ b/include/rdma/uverbs_std_types.h
@@ -48,9 +48,20 @@
#define uobj_get_type(_attrs, _object) \
uapi_get_object((_attrs)->ufile->device->uapi, _object)
+static inline struct ib_uobject *
+uobj_attrs_set_context(struct ib_uobject *uobj,
+ struct uverbs_attr_bundle *attrs)
+{
+ if (IS_ERR(uobj))
+ return uobj;
+ attrs->driver_udata.context = uobj->context;
+ return uobj;
+}
+
#define uobj_get_read(_type, _id, _attrs) \
- rdma_lookup_get_uobject(uobj_get_type(_attrs, _type), (_attrs)->ufile, \
- _uobj_check_id(_id), UVERBS_LOOKUP_READ)
+ uobj_attrs_set_context(rdma_lookup_get_uobject( \
+ uobj_get_type(_attrs, _type), (_attrs)->ufile, \
+ _uobj_check_id(_id), UVERBS_LOOKUP_READ), _attrs)
#define ufd_get_read(_type, _fdnum, _attrs) \
rdma_lookup_get_uobject(uobj_get_type(_attrs, _type), (_attrs)->ufile, \
@@ -68,8 +79,9 @@ static inline void *_uobj_get_obj_read(struct ib_uobject *uobj)
uobj_get_read(_type, _id, _attrs)))
#define uobj_get_write(_type, _id, _attrs) \
- rdma_lookup_get_uobject(uobj_get_type(_attrs, _type), (_attrs)->ufile, \
- _uobj_check_id(_id), UVERBS_LOOKUP_WRITE)
+ uobj_attrs_set_context(rdma_lookup_get_uobject( \
+ uobj_get_type(_attrs, _type), (_attrs)->ufile, \
+ _uobj_check_id(_id), UVERBS_LOOKUP_WRITE), _attrs)
int __uobj_perform_destroy(const struct uverbs_api_object *obj, u32 id,
const struct uverbs_attr_bundle *attrs);
--
2.17.2
