Date:   Mon, 6 Nov 2017 13:07:26 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Joerg Roedel <joro@...tes.org>,
        iommu@...ts.linux-foundation.org, vinadhy@...il.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of
 get_cpu_ptr() for ->fq

On Mon, 6 Nov 2017 16:45:11 +0000
Robin Murphy <robin.murphy@....com> wrote:

> On 06/11/17 15:43, Alex Williamson wrote:
> > [cc +robin]
> > 
> > On Thu, 2 Nov 2017 18:33:50 +0100
> > Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> >   
> >> On 2017-09-21 17:21:40 [+0200], Sebastian Andrzej Siewior wrote:  
> >>> get_cpu_ptr() disabled preemption and returns the ->fq object of the
> >>> current CPU. raw_cpu_ptr() does the same except that it not disable
> >>> preemption which means the scheduler can move it to another CPU after it
> >>> obtained the per-CPU object.
> >>> In this case this is not bad because the data structure itself is
> >>> protected with a spin_lock. This change shouldn't matter however on RT
> >>> it does because the sleeping lock can't be accessed with disabled
> >>> preemption.    
> >>
> >> Did this make to your tree Jörg?  
> > 
> > Hi Sebastian,
> > 
> > Joerg is out on paternity leave through the end of the year, I'm
> > filling in in the interim.  I hadn't looked for patches this far back,
> > so thanks for pointing it out.  Robin, any comments?  Thanks,  
> 
> The reasoning seems sound - assuming we can't get preempted once the
> lock is actually taken, the worst side-effect of getting moved to
> another CPU in that slim window looks to be a little bit of lock
> contention. Operating on "someone else's" queue should have no
> correctness impact, and the cmpxchg after the lock is released isn't
> even working on percpu data so doesn't really care anyway.
> 
> AFAICS it's more or less the direct equivalent of aaffaa8a3b59
> ("iommu/iova: Don't disable preempt around this_cpu_ptr()"), which seems
> to have been working out OK.

Great, thanks Robin.

Applied to iommu/iova for v4.15.  Thanks,

Alex
  
> >>> Cc: Joerg Roedel <joro@...tes.org>
> >>> Cc: iommu@...ts.linux-foundation.org
> >>> Reported-by: vinadhy@...il.com
> >>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> >>> ---
> >>> On 2017-09-19 11:41:19 [+0200], Joerg Roedel wrote:    
> >>>> Hi Sebastian,    
> >>> Hi Jörg,
> >>>     
> >>>> I moved the flushing to driver/iommu/iova.c to share it with the Intel
> >>>> IOMMU and possibly other drivers too, so this patch does no longer apply
> >>>> to v4.14-rc1. Can you update the patch to these changes?    
> >>>
> >>> Sure.
> >>>
> >>> v1…v2: move the change from amd_iommu.c to iova.c
> >>>
> >>>  drivers/iommu/iova.c | 4 +---
> >>>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> >>> index 33edfa794ae9..b30900025c62 100644
> >>> --- a/drivers/iommu/iova.c
> >>> +++ b/drivers/iommu/iova.c
> >>> @@ -570,7 +570,7 @@ void queue_iova(struct iova_domain *iovad,
> >>>  		unsigned long pfn, unsigned long pages,
> >>>  		unsigned long data)
> >>>  {
> >>> -	struct iova_fq *fq = get_cpu_ptr(iovad->fq);
> >>> +	struct iova_fq *fq = raw_cpu_ptr(iovad->fq);
> >>>  	unsigned long flags;
> >>>  	unsigned idx;
> >>>  
> >>> @@ -600,8 +600,6 @@ void queue_iova(struct iova_domain *iovad,
> >>>  	if (atomic_cmpxchg(&iovad->fq_timer_on, 0, 1) == 0)
> >>>  		mod_timer(&iovad->fq_timer,
> >>>  			  jiffies + msecs_to_jiffies(IOVA_FQ_TIMEOUT));
> >>> -
> >>> -	put_cpu_ptr(iovad->fq);
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(queue_iova);
> >>>  
> >>> -- 
> >>> 2.14.1
> >>>     
> >>
> >> Sebastian
> >> _______________________________________________
> >> iommu mailing list
> >> iommu@...ts.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/iommu  
> 

Powered by blists - more mailing lists