[PATCH] iommu/amd: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->flush_queue

[PATCH] iommu/amd: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->flush_queue

[Sebastian Andrzej Siewior]

get_cpu_ptr() disables preemption and returns the ->flush_queue 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 in general
but on RT it does because the sleeping lock can't be accessed with
disabled preemption.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org
Reported-by: vinadhy@gmail.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iommu/amd_iommu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 4ad7e5e31943..943efbc08128 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1911,7 +1911,7 @@ static void queue_add(struct dma_ops_domain *dom,
 	pages     = __roundup_pow_of_two(pages);
 	address >>= PAGE_SHIFT;
 
-	queue = get_cpu_ptr(dom->flush_queue);
+	queue = raw_cpu_ptr(dom->flush_queue);
 	spin_lock_irqsave(&queue->lock, flags);
 
 	/*
@@ -1940,8 +1940,6 @@ static void queue_add(struct dma_ops_domain *dom,
 
 	if (atomic_cmpxchg(&dom->flush_timer_on, 0, 1) == 0)
 		mod_timer(&dom->flush_timer, jiffies + msecs_to_jiffies(10));
-
-	put_cpu_ptr(dom->flush_queue);
 }
 
 static void queue_flush_timeout(unsigned long data)
-- 
2.14.1

Re: [PATCH] iommu/amd: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->flush_queue

[Joerg Roedel]

Hi Sebastian,

On Wed, Sep 06, 2017 at 12:34:59PM +0200, Sebastian Andrzej Siewior wrote:
> get_cpu_ptr() disables preemption and returns the ->flush_queue 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 in general
> but on RT it does because the sleeping lock can't be accessed with
> disabled preemption.

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?

Regards,

	Joerg

[PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

[Sebastian Andrzej Siewior]

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.

Cc: Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux-foundation.org
Reported-by: vinadhy@gmail.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.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

Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

[Sebastian Andrzej Siewior]

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?

> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: iommu@lists.linux-foundation.org
> Reported-by: vinadhy@gmail.com
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.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

Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

[Alex Williamson]

[cc +robin]

On Thu, 2 Nov 2017 18:33:50 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.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,

Alex

> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: iommu@lists.linux-foundation.org
> > Reported-by: vinadhy@gmail.com
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.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@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

[Robin Murphy]

On 06/11/17 15:43, Alex Williamson wrote:
> [cc +robin]
> 
> On Thu, 2 Nov 2017 18:33:50 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.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@8bytes.org>
>>> Cc: iommu@lists.linux-foundation.org
>>> Reported-by: vinadhy@gmail.com
>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.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@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/iova: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->fq

[Alex Williamson]

On Mon, 6 Nov 2017 16:45:11 +0000
Robin Murphy <robin.murphy@arm.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@linutronix.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@8bytes.org>
> >>> Cc: iommu@lists.linux-foundation.org
> >>> Reported-by: vinadhy@gmail.com
> >>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.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@lists.linux-foundation.org
> >> https://lists.linuxfoundation.org/mailman/listinfo/iommu  
> 

Re: [PATCH] iommu/amd: Use raw_cpu_ptr() instead of get_cpu_ptr() for ->flush_queue

[Sebastian Andrzej Siewior]

On 2017-09-11 22:22:11 [-0400], Vinod Adhikary wrote:
> Dear all,
Hi,

> Thank you for the great community support and support from Sebastian to
> provide me this patch. I wanted to send this email to inform you and
> perhaps get some information on how I could keep myself updated on updates
> in regards to linux-rt. I apologize if I'm spamming you with this email.

There is a rt-users mailing list, see
	https://wiki.linuxfoundation.org/realtime/communication/mailinglists

> I noticed `Bug: Scheduling while atomic` on ahci after building 4.9 rt and
> reported the issue on oftc channel. Sebastian quickly helped me get a patch
> and my `journalctl -f` was clean without any such messages. Yesterday I
> decided to build 4.11 without the patch but I saw a whole bunch of similar
> bug logs not just on ahci but on my nic card as well among a few others.
> There were a few instances where I had to hard shutdown my machine. I
> patched 4.11 with Sebastian's patch again and so far it's been okay for
> about 24 hours and going.

It sounds like the same issue. I taking this for v4.11 RT and v4.9 will
pick it at some point.

> My concern is, how I can provide more information to resolve this issue,
> perhaps in the next release. Please let me know how I can be of help.

usually providing a backtrace like you did to the rt-usrs list is
enough.

> Thank you

Sebastian