Re: [PATCH RFC 2/2] IB/mlx5: Implement Fast Indirect Memory Registration Feature

[Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On 10/14/2014 8:41 AM, Bart Van Assche wrote:
> On 10/07/14 16:48, Sagi Grimberg wrote:
>> -static __be64 *mr_align(__be64 *ptr, int align)
>> +static void *mr_align(void *ptr, int align)
>>   {
>>       unsigned long mask = align - 1;
>>
>> -    return (__be64 *)(((unsigned long)ptr + mask) & ~mask);
>> +    return (void *)(((unsigned long)ptr + mask) & ~mask);
>>   }
>
> Maybe I'm missing something, but why doesn't this function use the
> ALIGN() macro defined in <linux/kernel.h> ? Are you aware that a type
> with name uintptr_t has been defined in <linux/types.h> that is intended
> for casting a pointer to an int ?

Heh, you are right obviously.

I'll fix that to use ALIGN(ptr, mask)

>
>  > +struct ib_indir_reg_list *
>  > +mlx5_ib_alloc_indir_reg_list(struct ib_device *device,
>  > +                 unsigned int max_indir_list_len)
>
> There are three k[zc]alloc() calls in this function. Can these be
> combined into one without increasing the chance too much that this will
> increase the order of the allocation ?

Well, I have 3 allocations:
1. mlx5_indir_reg_list container
2. sg_list: buffer of sg elements for the user to pass
3. mapped_ilist: buffer for HW representation of sg elements.

mapped_ilist buffer has some constraints on it (see below) so I can't
combine it with the other two. I assume I can rearrange the structure to
combine the fist 2 allocations together, but it can trigger a higher
order allocation (unless I use vzalloc, but I'm not sure what is the
benefit here...).

>
>  > +    dsize = sizeof(*mirl->klms) * max_indir_list_len;
>  > +    mirl->mapped_ilist = kzalloc(dsize + MLX5_UMR_ALIGN - 1,
>  > +                      GFP_KERNEL);
>
> The value of the constant MLX5_UMR_ALIGN is 2048. Does that mean this
> code will always allocate 2 KB more than needed ? Is this really the
> minimum alignment required for this data structure ? How likely is it
> that this code will trigger a higher order allocation ?

Well, here I need a variable size physically contiguous allocation with
a start address aligned to 2K (HW requirement). So yes, with this code
given the user allocate more than PAGE_SIZE-2K ib_sges, a higher order
allocation will be triggered.

I allocate 2K more to ensure I can align to 2K, then I set mirl->klms to
point at the aligned address. Any idea on how can I do this without
allocating 2K more?

>
>  > +    if (unlikely((*seg == qp->sq.qend)))
>
> A minor comment: one pair of parentheses can be left out from the above
> code.

Thanks, will fix.

>
>  > +                    mlx5_ib_warn(dev, "\n");
>
> Will the warning generated by this code allow a user to find out which
> code triggered that warning ?

Not at all - and it should be fixed. will do.

Sagi.
--
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