On Thu, Jun 16, 2016 at 05:58:29PM -0400, Chuck Lever wrote: > > > On Jun 16, 2016, at 5:10 PM, Sagi Grimberg <sagigrim@xxxxxxxxx> wrote: > > > > > >>>> On Jun 15, 2016, at 12:28 AM, Leon Romanovsky <leon@xxxxxxxxxx> wrote: > >>>> > >>>> On Tue, Jun 14, 2016 at 11:15:25PM -0400, Chuck Lever wrote: > >>>>> From: Sagi Grimberg <sagi@xxxxxxxxxxx> > >>>>> > >>>>> kmalloc doesn't guarantee the returned memory is all on one page. > >>>> > >>>> IMHO, the patch posted by Christoph at that thread is best way to go, > >>>> because you changed streaming DMA mappings to be coherent DMA mappings [1]. > >>>> > >>>> "The kernel developers recommend the use of streaming mappings over > >>>> coherent mappings whenever possible" [1]. > >>>> > >>>> [1] http://www.makelinux.net/ldd3/chp-15-sect-4 > >>> > >>> Hi Leon- > >>> > >>> I'll happily drop this patch from my 4.8 series as soon > >>> as an official mlx4/mlx5 fix is merged. > >>> > >>> Meanwhile, I notice some unexplained instability (driver > >>> resets, list corruption, and so on) when I test NFS/RDMA > >>> without this patch included. So it is attached to the > >>> series for anyone with mlx4 who wants to pull my topic > >>> branch and try it out. > >> > >> hi Chuck, > >> > >> We plan to send attached patch during our second round of fixes for > >> mlx4/mlx5 and would be grateful to you if you could provide your > >> Tested-by tag before. > > Fwiw, Tested-by: Chuck Lever <chuck.lever@xxxxxxxxxx> Thanks, I appreciate it. > > > > First of all, IIRC the patch author was Christoph wasn't he. > > > > Plus, you do realize that this patch makes the pages allocation > > in granularity of pages. In systems with a large page size this > > is completely redundant, it might even be harmful as the storage > > ULPs need lots of MRs. > > I agree that the official fix should take a conservative > approach to allocating this resource; there will be lots > of MRs in an active system. This fix doesn't seem too > careful. In mlx5 system, we always added 2048 bytes to such allocations, for reasons unknown to me. And it doesn't seem as a conservative approach either. > > > > Also, I don't see how that solves the issue, I'm not sure I even > > understand the issue. Do you? Were you able to reproduce it? > > The issue is that dma_map_single() does not seem to DMA map > portions of a memory region that are past the end of the first > page of that region. Maybe that's a bug? No, I didn't find support for that. Function dma_map_single expects contiguous memory aligned to cache line, there is no limitation to be page bounded. > > This patch works around that behavior by guaranteeing that > > a) the memory region starts at the beginning of a page, and > b) the memory region is never larger than a page b) the memory region ends on cache line. > > This patch is not sufficient to repair mlx5, because b) > cannot be satisfied in that case; the array of __be64's can > be larger than 512 entries. > > > > IFF the pages buffer end not being aligned to a cacheline is problematic > > then why not extent it to end in a cacheline? Why in the next full page? > > I think the patch description justifies the choice of > solution, but does not describe the original issue at > all. The original issue had nothing to do with cacheline > alignment. I disagree, kmalloc with supplied flags will return contiguous memory which is enough for dma_map_single. It is cache line alignment. > > Lastly, this patch should remove the definition of > MLX4_MR_PAGES_ALIGN. Thanks, I missed it. > > > -- > Chuck Lever > > >
Attachment:
signature.asc
Description: Digital signature
