> On Jun 18, 2016, at 6:56 AM, Leon Romanovsky <leon@xxxxxxxxxx> wrote: > > On Fri, Jun 17, 2016 at 03:55:56PM -0400, Chuck Lever wrote: >> >>> On Jun 17, 2016, at 5:20 AM, Leon Romanovsky <leon@xxxxxxxxxx> wrote: >>> >>> On Thu, Jun 16, 2016 at 05:58:29PM -0400, Chuck Lever wrote: >>>> >>>> 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. >> >> The reason I find this hard to believe is that there is >> no end alignment guarantee at all in this code, but it >> works without issue when SLUB debugging is not enabled. >> >> xprtrdma allocates 256 elements in this array on x86. >> The code makes the array start on an 0x40 byte boundary. >> I'm pretty sure that means the end of that array will >> also be on at least an 0x40 byte boundary, and thus >> aligned to the DMA cacheline, whether or not SLUB >> debugging is enabled. >> >> Notice that in the current code, if the consumer requests >> an odd number of SGs, that array can't possibly end on >> an alignment boundary. But we've never had a complaint. >> >> SLUB debugging changes the alignment of lots of things, >> but mlx4_alloc_priv_pages is the only breakage that has >> been reported. > > I think it is related to custom logic which is in this function only. > I posted grep output earlier to emphasize it. > > For example adds 2K region after the actual data and it will ensure > alignment. > >> >> DMA-API.txt says: >> >>> [T]he mapped region must begin exactly on a cache line >>> boundary and end exactly on one (to prevent two separately >>> mapped regions from sharing a single cache line) >> >> The way I read this, cacheline alignment shouldn't be >> an issue at all, as long as DMA cachelines aren't >> shared between mappings. >> >> If I simply increase the memory allocation size a little >> and ensure the end of the mapping is aligned, that should >> be enough to prevent DMA cacheline sharing with another >> memory allocation on the same page. But I still see Local >> Protection Errors when SLUB debugging is enabled, on my >> system (with patches to allocate more pages per MR). >> >> I'm not convinced this has anything to do with DMA >> cacheline alignment. The reason your patch fixes this >> issue is because it keeps the entire array on one page. > > If you don't mind, we can do an experiment. > Let's add padding which will prevent alignment issue and > for sure will cross the page boundary. > > diff --git a/drivers/infiniband/hw/mlx4/mr.c > b/drivers/infiniband/hw/mlx4/mr.c > index 6312721..41e277e 100644 > --- a/drivers/infiniband/hw/mlx4/mr.c > +++ b/drivers/infiniband/hw/mlx4/mr.c > @@ -280,8 +280,10 @@ mlx4_alloc_priv_pages(struct ib_device *device, > int size = max_pages * sizeof(u64); > int add_size; > int ret; > - > +/* > add_size = max_t(int, MLX4_MR_PAGES_ALIGN - ARCH_KMALLOC_MINALIGN, 0); > +*/ > + add_size = 2048; > > mr->pages_alloc = kzalloc(size + add_size, GFP_KERNEL); > if (!mr->pages_alloc) Hi Leon- I created a different patch, see attachment. It aligns the start _and_ end of the DMA mapped region, places large arrays so they encounter a page boundary, and leaves slack space around each array so there is no possibility of a shared DMA cacheline or other activity in that memory. I am able to reproduce the Local Protection Errors with this patch applied and SLUB debugging disabled. -- Chuck Lever
Attachment:
fix-mlx4-alloc-priv-pages.diff
Description: Binary data
