Re: [PATCH v2 01/24] mlx4-ib: Use coherent memory for priv pages

On Thu, Jun 16, 2016 at 05:58:29PM -0400, Chuck Lever wrote:

quoted

On Jun 16, 2016, at 5:10 PM, Sagi Grimberg [off-list ref] wrote:

quoted

quoted

quoted

On Jun 15, 2016, at 12:28 AM, Leon Romanovsky [off-list ref] wrote:

On Tue, Jun 14, 2016 at 11:15:25PM -0400, Chuck Lever wrote:

quoted

From: Sagi Grimberg <redacted>

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 [off-list ref]

Thanks, I appreciate it.

quoted

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.

quoted

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.

quoted

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