Re: [PATCH v3 25/25] IB/mlx4: Workaround for mlx4_alloc_priv_pages() array allocator

[Leon Romanovsky <leon@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

On Wed, Jun 22, 2016 at 10:47:27AM -0400, Chuck Lever wrote:
> 
> > On Jun 22, 2016, at 10:04 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
> > 
> > 
> >> +    /* This is overkill, but hardware requires that the
> >> +     * PBL array begins at a properly aligned address and
> >> +     * never occupies the last 8 bytes of a page.
> >> +     */
> >> +    mr->pages = (__be64 *)get_zeroed_page(GFP_KERNEL);
> >> +    if (!mr->pages)
> >>          return -ENOMEM;
> > 
> > Again, I'm not convinced that this is a better choice then allocating
> > the exact needed size as dma coherent, but given that the dma coherent
> > allocations are always page aligned I wander if it's not the same
> > effect...
> 
> My concerns with DMA coherent were:
> 
> 1. That pool may be a somewhat limited resource?
> 
> 2. IMO DMA-API.txt suggests DMA coherent will perform less
> well in some cases. Macro benchmarks I ran seemed to show
> there was a slight performance hit with that approach, though
> it was nearly in the noise.
> 
> I agree that the over-allocation in the streaming solution is a
> concern. But as you say, there may be little we can do about it.

According to [1] dma_alloc_coherent doesn't allocate from pool, but
calls to the __get_free_page().

"A DMA pool is an allocation mechanism for small, coherent DMA mappings.
Mappings obtained from dma_alloc_coherent may have a minimum size of one
page."

> 
> Wrt to Or's comment, the device's maximum page list depth
> is advertised to consumers via the device's attributes. However,
> it would be defensive if there was a sanity check added in
> mlx4_alloc_priv_pages to ensure that the max_pages argument
> is a reasonable value (ie, that the calculated array size does
> indeed fit into a page).
> 
> > In any event, we can move forward with this for now:
> > 
> > Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> 
> Thanks, I'll add that! Though as before, I'm happy to drop this
> patch if there is a different preferred official fix.

We submitted your version of patch with minor changes in
comments and commit message together with Sagi's ROB tag [2].

[1] http://www.makelinux.net/ldd3/chp-15-sect-4
[2] https://patchwork.kernel.org/patch/9193075/

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]