Re: [PATCH v5 6/6] swiotlb: Remove pointless stride adjustment for allocations >= PAGE_SIZE

[Petr Tesařík]

On Mon, 4 Mar 2024 16:10:46 +0000
Michael Kelley <mhklinux@xxxxxxxxxxx> wrote:

> From: Will Deacon <will@xxxxxxxxxx> Sent: Monday, March 4, 2024 8:02 AM
> > 
> > Hi folks,
> > 
> > On Mon, Mar 04, 2024 at 04:55:06PM +0100, Petr Tesařík wrote:  
> > > On Mon, 4 Mar 2024 13:37:56 +0000
> > > Robin Murphy <robin.murphy@xxxxxxx> wrote:  
> > > > On 04/03/2024 11:00 am, Petr Tesařík wrote:
> > > > [...]  
> > > > >> Here's my take on tying all the threads together. There are
> > > > >> four alignment combinations:
> > > > >>
> > > > >> 1. alloc_align_mask: zero; min_align_mask: zero  
> > 
> > Based on this ^^^ ...
> >   
> > > > >> xen_swiotlb_map_page() and dma_direct_map_page() are #1 or #2
> > > > >> via swiotlb_map() and swiotlb_tbl_map_single()
> > > > >>
> > > > >> iommu_dma_map_page() is #3 and #4 via swiotlb_tbl_map_single()
> > > > >>
> > > > >> swiotlb_alloc() is #3, directly to swiotlb_find_slots()
> > > > >>
> > > > >> For #1, the returned physical address has no constraints if
> > > > >> the requested size is less than a page. For page size or
> > > > >> greater, the discussed historical requirement for page
> > > > >> alignment applies.  
> > 
> > ... and this ^^^ ...
> > 
> >   
> > > I believe this patch series is now good as is, except the commit
> > > message should make it clear that alloc_align_mask and min_align_mask
> > > can both be zero, but that simply means no alignment constraints.  
> > 
> > ... my (possibly incorrect!) reading of the thread so far is that we
> > should preserve page-aligned allocation in this case if the allocation
> > size is >= PAGE_SIZE.
> > 
> > Something like the diff below, to replace this final patch?
> > 
> > Will
> >   
> > --->8  
> > 
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c381a7ed718f..67eac05728c0 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -992,6 +992,14 @@ static int swiotlb_search_pool_area(struct device
> > *dev, struct io_tlb_pool *pool
> >         BUG_ON(!nslots);
> >         BUG_ON(area_index >= pool->nareas);
> > 
> > +       /*
> > +        * Historically, allocations >= PAGE_SIZE were guaranteed to be
> > +        * page-aligned in the absence of any other alignment requirements.
> > +        * Since drivers may be relying on this, preserve the old behaviour.
> > +        */
> > +       if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
> > +               alloc_align_mask = PAGE_SIZE - 1;
> > +  
> 
> Yes, I think that should do it.

Sure, this will solve the allocations. But my understanding of this
same thread is that we don't need it here. The historical page order
alignment applies ONLY to allocations, NOT to mappings. It is
documented in Documentation/core-api/dma-api-howto.rst under Consistent
DMA mappings, for dma_alloc_coherent(). IIUC it does not apply to the
streaming DMA mappings. At least, it would explain why nobody
complained that the more strict guarantee for sizes greater than
PAGE_SIZE was not kept...

The SWIOTLB can be used for allocation if CONFIG_DMA_RESTRICTED_POOL=y,
but this case is handled by patch 3/6 of this patch series.

Do I miss something again?

Petr T

> Michael
> 
> >         /*
> >          * Ensure that the allocation is at least slot-aligned and update
> >          * 'iotlb_align_mask' to ignore bits that will be preserved when
> > @@ -1006,13 +1014,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >          */
> >         stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
> > 
> > -       /*
> > -        * For allocations of PAGE_SIZE or larger only look for page aligned
> > -        * allocations.
> > -        */
> > -       if (alloc_size >= PAGE_SIZE)
> > -               stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> > -
> >         spin_lock_irqsave(&area->lock, flags);
> >         if (unlikely(nslots > pool->area_nslabs - area->used))
> >                 goto not_found;  
>