Messages in this thread | | | Subject | Re: [PATCH 4.19 17/32] iommu/vt-d: Dont queue_iova() if there is no flush queue | From | Dmitry Safonov <> | Date | Tue, 6 Aug 2019 23:47:44 +0100 |
| |
Hi Pavel,
On 8/3/19 10:34 PM, Pavel Machek wrote: > Hi! > >> --- a/drivers/iommu/intel-iommu.c >> +++ b/drivers/iommu/intel-iommu.c >> @@ -3721,7 +3721,7 @@ static void intel_unmap(struct device *d >> >> freelist = domain_unmap(domain, start_pfn, last_pfn); >> >> - if (intel_iommu_strict) { >> + if (intel_iommu_strict || !has_iova_flush_queue(&domain->iovad)) { >> iommu_flush_iotlb_psi(iommu, domain, start_pfn, >> nrpages, !freelist, 0); >> /* free iova */ >> --- a/drivers/iommu/iova.c >> +++ b/drivers/iommu/iova.c >> @@ -65,9 +65,14 @@ init_iova_domain(struct iova_domain *iov >> } >> EXPORT_SYMBOL_GPL(init_iova_domain); >> >> +bool has_iova_flush_queue(struct iova_domain *iovad) >> +{ >> + return !!iovad->fq; > > Should this be READ_ONCE()?
Why? Compiler can't anyhow assume that it's always true/false and there is a clear data dependency between this and: : queue_iova(&domain->iovad, iova_pfn, nrpages, : (unsigned long)freelist);
> >> @@ -100,13 +106,17 @@ int init_iova_flush_queue(struct iova_do >> for_each_possible_cpu(cpu) { >> struct iova_fq *fq; >> >> - fq = per_cpu_ptr(iovad->fq, cpu); >> + fq = per_cpu_ptr(queue, cpu); >> fq->head = 0; >> fq->tail = 0; >> >> spin_lock_init(&fq->lock); >> } >> >> + smp_wmb(); >> + >> + iovad->fq = queue; >> + > > Could we have a comment why the barrier is needed,
I'm up for the comment if you feel like it - in my POV it's quite obvious that we want finish initializing the queue's internals before assigning the queue. I didn't put the comment exactly because I felt like it would state something evident [in my POV].
> and perhaps there > should be oposing smp_rmb() somewhere? Does this need to be > WRITE_ONCE() as it is racing against reader?
I feel confused. I might have forgotten everything about barriers, but again if I'm not mistaken, one doesn't need a barrier in: : if (A->a != NULL) : use(A); /* dereferences A->a */ : else : /* don't use `a' */
Thanks, Dmitry
| |