Re: [PATCH for-next 1/7] IB/core: Refactor IDR to be per-device

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Wed, Jan 11, 2017 at 12:53:47PM +0200, Matan Barak wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html