On Mon, 6 Nov 2017 16:45:11 +0000 Robin Murphy <[email protected]> wrote:
> On 06/11/17 15:43, Alex Williamson wrote: > > [cc +robin] > > > > On Thu, 2 Nov 2017 18:33:50 +0100 > > Sebastian Andrzej Siewior <[email protected]> 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 <[email protected]> > >>> Cc: [email protected] > >>> Reported-by: [email protected] > >>> Signed-off-by: Sebastian Andrzej Siewior <[email protected]> > >>> --- > >>> 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 > >> [email protected] > >> https://lists.linuxfoundation.org/mailman/listinfo/iommu >
