Re: [PATCH v5 2/2] iommu/iova: Free global iova rcache on iova alloc failure

[Robin Murphy]

On 2020-11-03 14:31, John Garry wrote:
> On 03/11/2020 12:35, Robin Murphy wrote:
> > On 2020-09-30 08:44, vjitta@xxxxxxxxxxxxxx wrote:
> > > From: Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx>
> > >
> > > When ever an iova alloc request fails we free the iova
> > > ranges present in the percpu iova rcaches and then retry
> > > but the global iova rcache is not freed as a result we could
> > > still see iova alloc failure even after retry as global
> > > rcache is holding the iova's which can cause fragmentation.
> > > So, free the global iova rcache as well and then go for the
> > > retry.
>
> If we do clear all the CPU rcaches, it would nice to have something 
> immediately available to replenish, i.e. use the global rcache, instead 
> of flushing it, if that is not required...

If we've reached the point of clearing *any* caches, though, I think any 
hope of maintaining performance is already long gone. We've walked the 
rbtree for the entire address space and found that it's still too full 
to allocate from; we're teetering on the brink of hard failure and this 
is a last-ditch attempt to claw back as much as possible in the hope 
that it gives us a usable space.

TBH I'm not entirely sure what allocation pattern was expected by the 
original code such that purging only some of the caches made sense, nor 
what kind of pattern leads to lots of smaller IOVAs being allocated, 
freed, and never reused to the point of blocking larger allocations, but 
either way the reasoning does at least seem to hold up in abstract.

> > This looks reasonable to me - it's mildly annoying that we end up with 
> > so many similar-looking functions,
>
> Well I did add a function to clear all CPU rcaches here, if you would 
> like to check:
>
> https://lore.kernel.org/linux-iommu/1603733501-211004-2-git-send-email-john.garry@xxxxxxxxxx/ 

I was thinking more of the way free_iova_rcaches(), 
free_cpu_cached_iovas(), and free_global_cached_iovas() all look pretty 
much the same shape at a glance.

> > but the necessary differences are right down in the middle of the 
> > loops so nothing can reasonably be factored out :(
> >
> > Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>
> >
> > > Signed-off-by: Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx>
> > > ---
> > >   drivers/iommu/iova.c | 23 +++++++++++++++++++++++
> > >   1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> > > index c3a1a8e..faf9b13 100644
> > > --- a/drivers/iommu/iova.c
> > > +++ b/drivers/iommu/iova.c
> > > @@ -25,6 +25,7 @@ static void init_iova_rcaches(struct iova_domain 
> > > *iovad);
> > >   static void free_iova_rcaches(struct iova_domain *iovad);
> > >   static void fq_destroy_all_entries(struct iova_domain *iovad);
> > >   static void fq_flush_timeout(struct timer_list *t);
> > > +static void free_global_cached_iovas(struct iova_domain *iovad);
>
> a thought: It would be great if the file could be rearranged at some 
> point where we don't require so many forward declarations.
>
> > >   void
> > >   init_iova_domain(struct iova_domain *iovad, unsigned long granule,
> > > @@ -442,6 +443,7 @@ alloc_iova_fast(struct iova_domain *iovad, 
> > > unsigned long size,
> > >           flush_rcache = false;
> > >           for_each_online_cpu(cpu)
> > >               free_cpu_cached_iovas(cpu, iovad);
> > > +        free_global_cached_iovas(iovad);
> > >           goto retry;
> > >       }
> > > @@ -1057,5 +1059,26 @@ void free_cpu_cached_iovas(unsigned int cpu, 
> > > struct iova_domain *iovad)
> > >       }
> > >   }
> > > +/*
> > > + * free all the IOVA ranges of global cache
> > > + */
> > > +static void free_global_cached_iovas(struct iova_domain *iovad)
> > > +{
> > > +    struct iova_rcache *rcache;
> > > +    unsigned long flags;
> > > +    int i, j;
> > > +
> > > +    for (i = 0; i < IOVA_RANGE_CACHE_MAX_SIZE; ++i) {
> > > +        rcache = &iovad->rcaches[i];
> > > +        spin_lock_irqsave(&rcache->lock, flags);
> > > +        for (j = 0; j < rcache->depot_size; ++j) {
> > > +            iova_magazine_free_pfns(rcache->depot[j], iovad);
> > > +            iova_magazine_free(rcache->depot[j]);
> > > +            rcache->depot[j] = NULL;
>
> I don't think that NULLify is strictly necessary

True, we don't explicitly clear depot entries in __iova_rcache_get() for 
normal operation, so there's not much point in doing so here.

Robin.

> > > +        }
> > > +        rcache->depot_size = 0;
> > > +        spin_unlock_irqrestore(&rcache->lock, flags);
> > > +    }
> > > +}
> > >   MODULE_AUTHOR("Anil S Keshavamurthy 
> > > <anil.s.keshavamurthy@xxxxxxxxx>");
> > >   MODULE_LICENSE("GPL");
> > _______________________________________________
> > iommu mailing list
> > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> > .