| Message ID | eb7458ed51b800f035976e879530ab1e90d02858.1450383740.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 17, 2015 at 08:50:59PM +0000, Robin Murphy wrote: > In certain unmapping situations it is quite possible to end up issuing > back-to-back TLB synchronisations, which at best is a waste of time and > effort, and at worst causes some hardware to get rather confused. Whilst > the pagetable implementations, or the IOMMU drivers, or both, could keep > track of things to avoid this happening, it seems to make the most sense > to prevent code duplication and add some simple state tracking in the > common interface between the two. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/io-pgtable.h | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h > index 95c5565..d06219b 100644 > --- a/drivers/iommu/io-pgtable.h > +++ b/drivers/iommu/io-pgtable.h > @@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops); > * @fmt: The page table format. > * @cookie: An opaque token provided by the IOMMU driver and passed back to > * any callback routines. > + * @sync_flag: Private flag for optimising out redundant syncs. It makes sense to factor this out like you're proposing, but maybe we can think of a better name? How about "tlb_sync_pending", to follow "tlb_flush_pending" in the core code? Will
On 12/01/16 18:27, Will Deacon wrote: > On Thu, Dec 17, 2015 at 08:50:59PM +0000, Robin Murphy wrote: >> In certain unmapping situations it is quite possible to end up issuing >> back-to-back TLB synchronisations, which at best is a waste of time and >> effort, and at worst causes some hardware to get rather confused. Whilst >> the pagetable implementations, or the IOMMU drivers, or both, could keep >> track of things to avoid this happening, it seems to make the most sense >> to prevent code duplication and add some simple state tracking in the >> common interface between the two. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/iommu/io-pgtable.h | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h >> index 95c5565..d06219b 100644 >> --- a/drivers/iommu/io-pgtable.h >> +++ b/drivers/iommu/io-pgtable.h >> @@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops); >> * @fmt: The page table format. >> * @cookie: An opaque token provided by the IOMMU driver and passed back to >> * any callback routines. >> + * @sync_flag: Private flag for optimising out redundant syncs. > > It makes sense to factor this out like you're proposing, but maybe we > can think of a better name? How about "tlb_sync_pending", to follow > "tlb_flush_pending" in the core code? Ooh, tlb_flush_pending is a much nicer name indeed. It's almost as if I threw this together in a pre-holiday rush and put very little thought into it... I've fixed it up locally, but I'll save the repost until after -rc1, especially in case Yong has any further comments in the meantime. Thanks, Robin.
On 13/01/16 11:18, Robin Murphy wrote: > On 12/01/16 18:27, Will Deacon wrote: >> On Thu, Dec 17, 2015 at 08:50:59PM +0000, Robin Murphy wrote: >>> In certain unmapping situations it is quite possible to end up issuing >>> back-to-back TLB synchronisations, which at best is a waste of time and >>> effort, and at worst causes some hardware to get rather confused. Whilst >>> the pagetable implementations, or the IOMMU drivers, or both, could keep >>> track of things to avoid this happening, it seems to make the most sense >>> to prevent code duplication and add some simple state tracking in the >>> common interface between the two. >>> >>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>> --- >>> drivers/iommu/io-pgtable.h | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h >>> index 95c5565..d06219b 100644 >>> --- a/drivers/iommu/io-pgtable.h >>> +++ b/drivers/iommu/io-pgtable.h >>> @@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops >>> *ops); >>> * @fmt: The page table format. >>> * @cookie: An opaque token provided by the IOMMU driver and passed >>> back to >>> * any callback routines. >>> + * @sync_flag: Private flag for optimising out redundant syncs. >> >> It makes sense to factor this out like you're proposing, but maybe we >> can think of a better name? How about "tlb_sync_pending", to follow >> "tlb_flush_pending" in the core code? > > Ooh, tlb_flush_pending is a much nicer name indeed. It's almost as if I Or tlb_sync_pending, even. Bah, mornings... > threw this together in a pre-holiday rush and put very little thought > into it... > > I've fixed it up locally, but I'll save the repost until after -rc1, > especially in case Yong has any further comments in the meantime. > > Thanks, > Robin. > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu >
Hi Robin, Thank you for the patch. On Tuesday 12 January 2016 18:27:55 Will Deacon wrote: > On Thu, Dec 17, 2015 at 08:50:59PM +0000, Robin Murphy wrote: > > In certain unmapping situations it is quite possible to end up issuing > > back-to-back TLB synchronisations, which at best is a waste of time and > > effort, and at worst causes some hardware to get rather confused. Whilst > > the pagetable implementations, or the IOMMU drivers, or both, could keep > > track of things to avoid this happening, it seems to make the most sense > > to prevent code duplication and add some simple state tracking in the > > common interface between the two. > > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > --- > > > > drivers/iommu/io-pgtable.h | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h > > index 95c5565..d06219b 100644 > > --- a/drivers/iommu/io-pgtable.h > > +++ b/drivers/iommu/io-pgtable.h > > @@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops > > *ops); > > > > * @fmt: The page table format. > > * @cookie: An opaque token provided by the IOMMU driver and passed back > > to > > * any callback routines. > > > > + * @sync_flag: Private flag for optimising out redundant syncs. > > It makes sense to factor this out like you're proposing, but maybe we > can think of a better name? How about "tlb_sync_pending", to follow > "tlb_flush_pending" in the core code? With tlb_sync_pending, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h index 95c5565..d06219b 100644 --- a/drivers/iommu/io-pgtable.h +++ b/drivers/iommu/io-pgtable.h @@ -132,12 +132,14 @@ void free_io_pgtable_ops(struct io_pgtable_ops *ops); * @fmt: The page table format. * @cookie: An opaque token provided by the IOMMU driver and passed back to * any callback routines. + * @sync_flag: Private flag for optimising out redundant syncs. * @cfg: A copy of the page table configuration. * @ops: The page table operations in use for this set of page tables. */ struct io_pgtable { enum io_pgtable_fmt fmt; void *cookie; + bool sync_flag; struct io_pgtable_cfg cfg; struct io_pgtable_ops ops; }; @@ -147,17 +149,22 @@ struct io_pgtable { static inline void io_pgtable_tlb_flush_all(struct io_pgtable *iop) { iop->cfg.tlb->tlb_flush_all(iop->cookie); + iop->sync_flag = true; } static inline void io_pgtable_tlb_add_flush(struct io_pgtable *iop, unsigned long iova, size_t size, size_t granule, bool leaf) { iop->cfg.tlb->tlb_add_flush(iova, size, granule, leaf, iop->cookie); + iop->sync_flag = true; } static inline void io_pgtable_tlb_sync(struct io_pgtable *iop) { - iop->cfg.tlb->tlb_sync(iop->cookie); + if (iop->sync_flag) { + iop->cfg.tlb->tlb_sync(iop->cookie); + iop->sync_flag = false; + } } /**
In certain unmapping situations it is quite possible to end up issuing back-to-back TLB synchronisations, which at best is a waste of time and effort, and at worst causes some hardware to get rather confused. Whilst the pagetable implementations, or the IOMMU drivers, or both, could keep track of things to avoid this happening, it seems to make the most sense to prevent code duplication and add some simple state tracking in the common interface between the two. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/io-pgtable.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)