On Wed, Jan 11, 2017 at 12:53:47PM +0200, Matan Barak wrote:
> From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>
> The current code creates an IDR per type. Since types are currently
> common for all vendors and known in advance, this was good enough.
> However, the proposed ioctl based infrastructure allows each vendor
> to declare only some of the common types and declare its own specific
> types.
>
> Thus, we decided to implement IDR to be per device and refactor it to
> use a new file.
I'm still unclear why we want the idr to be global and not per
/dev/uverbs0 open fd.
__idr_get_uobj forces a strong association:
if (uobj->context == context)
Why on earth should we have a shared IDR when the objects cannot
actually be shared?
Surely that just wastes memory and harms performance.
I know we have talked about this before, but I think there should be a
strong reason why we don't move this into struct ib_ucontext in this
patch.
> +static void ib_device_allocate_idrs(struct ib_device *device)
> +{
> + spin_lock_init(&device->idr_lock);
> + idr_init(&device->idr);
> +}
> +
> +static void ib_device_destroy_idrs(struct ib_device *device)
> +{
> + idr_destroy(&device->idr);
> +}
Not sure why we need these wrappers..
> @@ -230,24 +228,24 @@ static void put_cq_read(struct ib_cq *cq)
> put_uobj_read(cq->uobject);
> }
>
> -static struct ib_ah *idr_read_ah(int ah_handle, struct ib_ucontext *context)
> +static void put_ah_read(struct ib_ah *ah)
> {
> - return idr_read_obj(&ib_uverbs_ah_idr, ah_handle, context, 0);
> + put_uobj_read(ah->uobject);
> }
>
> -static void put_ah_read(struct ib_ah *ah)
> +static struct ib_ah *idr_read_ah(int ah_handle, struct ib_ucontext *context)
> {
> - put_uobj_read(ah->uobject);
> + return idr_read_obj(ah_handle, context, 0);
> }
These two functions got reordered, makes the diff ugly.
> + struct idr idr;
> + /* Global lock in use to safely release device IDR */
> + spinlock_t idr_lock;
That comment doesn't make sense, has nothing to do with
release... 'spinlock protects idr'
Otherwise this patch looks fine to me.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
