On Thu, Jan 12, 2017 at 12:39 AM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
>
Assuming we keep the current separate fds model (i.e, per-device fd
and another rdma-cm fd),
what is the actual benefit here?
Memory wise - we'll probably consume a lot more (every idr layer is >
2K on a 64bit architecture),
so sharing here might have some memory usage benefits.
> __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?
>
It's closer to the current approach (which could be refined later on)
and it's probably more optimized memory wise.
> 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.
>
We can, but except for dropping the (uobj->context == context)
condition and *maybe* getting some
negligible performance benefits (if the per-context idr tree is indeed
lower in height), what will we gain?
>> +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..
>
Agreed, they can be inlined.
>> @@ -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.
>
Yep, thanks.
>> + 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'
>
Yeah, it's only protecting the idr. I'll fix that.
> Otherwise this patch looks fine to me.
>
> Jason
Thanks for reviewing.
Matan
> --
> 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
--
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
