From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sagi Grimberg Subject: Re: [PATCH RFC 2/2] IB/mlx5: Implement Fast Indirect Memory Registration Feature Date: Sun, 19 Oct 2014 22:34:04 +0300 Message-ID: <5444122C.6070804@dev.mellanox.co.il> References: <1412693281-6161-1-git-send-email-sagig@mellanox.com> <1412693281-6161-3-git-send-email-sagig@mellanox.com> <543CB79B.6050400@acm.org> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <543CB79B.6050400-HInyCGIudOg@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Bart Van Assche , Sagi Grimberg , linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: roland-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org, ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org, oren-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org, sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org List-Id: linux-rdma@vger.kernel.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 ? Are you aware that a type > with name uintptr_t has been defined in 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