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

[Robin Murphy]

On 04/03/2024 4:10 pm, Michael Kelley 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.

In principle it might be more logical to fudge this into 
iotlb_align_mask rather than alloc_align_mask - since that's really the 
effective behaviour to preserve for streaming mappings - and then pass 
an explicit alloc_align_mask from swiotlb_alloc() to honour the 
dma-coherent requirements. However I also wouldn't really object to not 
going that far and instead just making the comment a bit clearer that 
this is still serving both purposes.

Cheers,
Robin.

> 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;