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

[Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>]

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 ?

 > +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 ?

 > +	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 ?

 > +	if (unlikely((*seg == qp->sq.qend)))

A minor comment: one pair of parentheses can be left out from the above 
code.

 > +					mlx5_ib_warn(dev, "\n");

Will the warning generated by this code allow a user to find out which 
code triggered that warning ?

Bart.

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