Date:   Mon, 6 Nov 2017 16:45:11 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     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 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.

Robin.

> 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