[RFC,0/3] iommu/intel: Free empty page tables on unmaps

Message

Pasha Tatashin Dec. 21, 2023, 3:19 a.m. UTC

This series frees empty page tables on unmaps. It intends to be a
low overhead feature.

The read-writer lock is used to synchronize page table, but most of
time the lock is held is reader. It is held as a writer for short
period of time when unmapping a page that is bigger than the current
iova request. For all other cases this lock is read-only.

page->refcount is used in order to track number of entries at each page
table.

Microbenchmark data using iova_stress[1]:

Base:
yqbtg12:/home# ./iova_stress -s 16
dma_size:       4K iova space: 16T iommu: ~  32783M time:   22.297s

/iova_stress -s 16
dma_size:       4K iova space: 16T iommu: ~      0M time:   23.388s

The test maps/unmaps 4K pages and cycles through the IOVA space.
Base uses 32G of memory, and test completes in 22.3S.
Fix uses 0G of memory, and test completes in 23.4s.

I believe the proposed fix is a good compromize in terms of complexity/
scalability. A more scalable solution would be to spread read/writer
lock per-page table, and user page->private field to store the lock
itself.

However, since iommu already has some protection: i.e. no-one touches
the iova space of the request map/unmap we can avoid the extra complexity
and rely on a single per page table RW lock, and be in a reader mode
most of the time.

[1] https://github.com/soleen/iova_stress

Pasha Tatashin (3):
  iommu/intel: Use page->refcount to count number of entries in IOMMU
  iommu/intel: synchronize page table map and unmap operations
  iommu/intel: free empty page tables on unmaps

 drivers/iommu/intel/iommu.c | 153 ++++++++++++++++++++++++++++--------
 drivers/iommu/intel/iommu.h |  44 +++++++++--
 2 files changed, 158 insertions(+), 39 deletions(-)

Comments

Matthew Wilcox Dec. 21, 2023, 4:16 a.m. UTC | #1

On Thu, Dec 21, 2023 at 03:19:12AM +0000, Pasha Tatashin wrote:
> This series frees empty page tables on unmaps. It intends to be a
> low overhead feature.
> 
> The read-writer lock is used to synchronize page table, but most of
> time the lock is held is reader. It is held as a writer for short
> period of time when unmapping a page that is bigger than the current
> iova request. For all other cases this lock is read-only.
> 
> page->refcount is used in order to track number of entries at each page
> table.

Have I not put enough DANGER signs up around the page refcount?

 * If you want to use the refcount field, it must be used in such a way
 * that other CPUs temporarily incrementing and then decrementing the
 * refcount does not cause problems.  On receiving the page from
 * alloc_pages(), the refcount will be positive.

You can't use refcount for your purpose, and honestly I'm shocked you
haven't seen any of your WARNings trigger.

Pasha Tatashin Dec. 21, 2023, 5:13 a.m. UTC | #2

On Wed, Dec 20, 2023 at 11:16 PM Matthew Wilcox <[e-mail]> wrote:
>
> On Thu, Dec 21, 2023 at 03:19:12AM +0000, Pasha Tatashin wrote:
> > This series frees empty page tables on unmaps. It intends to be a
> > low overhead feature.
> >
> > The read-writer lock is used to synchronize page table, but most of
> > time the lock is held is reader. It is held as a writer for short
> > period of time when unmapping a page that is bigger than the current
> > iova request. For all other cases this lock is read-only.
> >
> > page->refcount is used in order to track number of entries at each page
> > table.
>
> Have I not put enough DANGER signs up around the page refcount?
>
>  * If you want to use the refcount field, it must be used in such a way
>  * that other CPUs temporarily incrementing and then decrementing the
>  * refcount does not cause problems.  On receiving the page from
>  * alloc_pages(), the refcount will be positive.
>
> You can't use refcount for your purpose, and honestly I'm shocked you
> haven't seen any of your WARNings trigger.

Hi Matthew,

Thank you for looking at this.

Could you please explain exactly why refcount can't be used like this?

After alloc_page() refcount is set to 1, we never reduce it to 0,
every new entry in a page table adds 1, so we get up-to 513, that is
why I added warn like this: WARN_ON_ONCE(rc > 513 || rc < 2); to
dma_set_pte() macro. When refcount == 1, we know that the page table
is empty, and can be added to a freelist for a delayed freeing.

What is wrong with using refcount for a scalable way of keeping the
track of number of entries in a iommu page table? Is there a better
way that I should use?

Thank you,
Pasha

Pasha Tatashin Dec. 21, 2023, 5:42 a.m. UTC | #3

On Thu, Dec 21, 2023 at 12:13 AM Pasha Tatashin
<[e-mail]> wrote:
>
> On Wed, Dec 20, 2023 at 11:16 PM Matthew Wilcox <[e-mail]> wrote:
> >
> > On Thu, Dec 21, 2023 at 03:19:12AM +0000, Pasha Tatashin wrote:
> > > This series frees empty page tables on unmaps. It intends to be a
> > > low overhead feature.
> > >
> > > The read-writer lock is used to synchronize page table, but most of
> > > time the lock is held is reader. It is held as a writer for short
> > > period of time when unmapping a page that is bigger than the current
> > > iova request. For all other cases this lock is read-only.
> > >
> > > page->refcount is used in order to track number of entries at each page
> > > table.
> >
> > Have I not put enough DANGER signs up around the page refcount?
> >
> >  * If you want to use the refcount field, it must be used in such a way
> >  * that other CPUs temporarily incrementing and then decrementing the
> >  * refcount does not cause problems.  On receiving the page from
> >  * alloc_pages(), the refcount will be positive.
> >
> > You can't use refcount for your purpose, and honestly I'm shocked you
> > haven't seen any of your WARNings trigger.
>
> Hi Matthew,
>
> Thank you for looking at this.
>
> Could you please explain exactly why refcount can't be used like this?
>
> After alloc_page() refcount is set to 1, we never reduce it to 0,
> every new entry in a page table adds 1, so we get up-to 513, that is
> why I added warn like this: WARN_ON_ONCE(rc > 513 || rc < 2); to

I guess, what you mean is that other CPUs could temporarily
increase/decrease refcount outside of IOMMU management, do you have an
example of why that would happen? I could remove the above warning,
and in the worst case we would miss an opportunity to free a page
table during unmap, not a big deal, it can be freed during another
map/unmap event. Still better than today, where we never free them
during unmaps.

Pasha

Matthew Wilcox Dec. 21, 2023, 2:06 p.m. UTC | #4

On Thu, Dec 21, 2023 at 12:42:41AM -0500, Pasha Tatashin wrote:
> On Thu, Dec 21, 2023 at 12:13 AM Pasha Tatashin
> <[e-mail]> wrote:
> >
> > On Wed, Dec 20, 2023 at 11:16 PM Matthew Wilcox <[e-mail]> wrote:
> > >
> > > On Thu, Dec 21, 2023 at 03:19:12AM +0000, Pasha Tatashin wrote:
> > > > This series frees empty page tables on unmaps. It intends to be a
> > > > low overhead feature.
> > > >
> > > > The read-writer lock is used to synchronize page table, but most of
> > > > time the lock is held is reader. It is held as a writer for short
> > > > period of time when unmapping a page that is bigger than the current
> > > > iova request. For all other cases this lock is read-only.
> > > >
> > > > page->refcount is used in order to track number of entries at each page
> > > > table.
> > >
> > > Have I not put enough DANGER signs up around the page refcount?
> > >
> > >  * If you want to use the refcount field, it must be used in such a way
> > >  * that other CPUs temporarily incrementing and then decrementing the
> > >  * refcount does not cause problems.  On receiving the page from
> > >  * alloc_pages(), the refcount will be positive.
> > >
> > > You can't use refcount for your purpose, and honestly I'm shocked you
> > > haven't seen any of your WARNings trigger.
> >
> > Hi Matthew,
> >
> > Thank you for looking at this.
> >
> > Could you please explain exactly why refcount can't be used like this?
> >
> > After alloc_page() refcount is set to 1, we never reduce it to 0,
> > every new entry in a page table adds 1, so we get up-to 513, that is
> > why I added warn like this: WARN_ON_ONCE(rc > 513 || rc < 2); to
> 
> I guess, what you mean is that other CPUs could temporarily
> increase/decrease refcount outside of IOMMU management, do you have an
> example of why that would happen? I could remove the above warning,
> and in the worst case we would miss an opportunity to free a page
> table during unmap, not a big deal, it can be freed during another
> map/unmap event. Still better than today, where we never free them
> during unmaps.

Both GUP-fast and the page cache will find a page under RCU protection,
inc it's refcount if not zero, check the page is still the one they were
looking for, and if not will dec the refcount again.  That means if a
page has been in the page cache or process page tables and you can't
guarantee that all CPUs have been through the requisite grace periods,
you might see the refcount increased.

I'm not prepared to make a guarantee that these are the only circumstances
under which you'll see a temporarily higher refcount than you expect.
Either currently or in the future.  If you use the refcount as anything
other than a refcount, you're living dangerously.  And if you think that
you'll be the one to do the last refcount put, you're not necessarily
correct (see the saga around __free_pages() which ended up as commit
e320d3012d25 fixed by 462a8e08e0e6 (which indicates the rare race does
actually happen)).

Now, it seems like from your further explanation that the consequence
of getting this wrong is simply that you fail to free the page early.
That seems OK, but I insist that you insert some comments explaining
what is going on and why it's safe so somebody auditing uses of refcount
doesn't have to reanalyse the whole thing for themself.  Or worse that
somebody working on the iommu sees this and thinks they can "improve"
on it.

Pasha Tatashin Dec. 21, 2023, 2:58 p.m. UTC | #5

On Thu, Dec 21, 2023 at 9:06 AM Matthew Wilcox <[e-mail]> wrote:
>
> On Thu, Dec 21, 2023 at 12:42:41AM -0500, Pasha Tatashin wrote:
> > On Thu, Dec 21, 2023 at 12:13 AM Pasha Tatashin
> > <[e-mail]> wrote:
> > >
> > > On Wed, Dec 20, 2023 at 11:16 PM Matthew Wilcox <[e-mail]> wrote:
> > > >
> > > > On Thu, Dec 21, 2023 at 03:19:12AM +0000, Pasha Tatashin wrote:
> > > > > This series frees empty page tables on unmaps. It intends to be a
> > > > > low overhead feature.
> > > > >
> > > > > The read-writer lock is used to synchronize page table, but most of
> > > > > time the lock is held is reader. It is held as a writer for short
> > > > > period of time when unmapping a page that is bigger than the current
> > > > > iova request. For all other cases this lock is read-only.
> > > > >
> > > > > page->refcount is used in order to track number of entries at each page
> > > > > table.
> > > >
> > > > Have I not put enough DANGER signs up around the page refcount?
> > > >
> > > >  * If you want to use the refcount field, it must be used in such a way
> > > >  * that other CPUs temporarily incrementing and then decrementing the
> > > >  * refcount does not cause problems.  On receiving the page from
> > > >  * alloc_pages(), the refcount will be positive.
> > > >
> > > > You can't use refcount for your purpose, and honestly I'm shocked you
> > > > haven't seen any of your WARNings trigger.
> > >
> > > Hi Matthew,
> > >
> > > Thank you for looking at this.
> > >
> > > Could you please explain exactly why refcount can't be used like this?
> > >
> > > After alloc_page() refcount is set to 1, we never reduce it to 0,
> > > every new entry in a page table adds 1, so we get up-to 513, that is
> > > why I added warn like this: WARN_ON_ONCE(rc > 513 || rc < 2); to
> >
> > I guess, what you mean is that other CPUs could temporarily
> > increase/decrease refcount outside of IOMMU management, do you have an
> > example of why that would happen? I could remove the above warning,
> > and in the worst case we would miss an opportunity to free a page
> > table during unmap, not a big deal, it can be freed during another
> > map/unmap event. Still better than today, where we never free them
> > during unmaps.
>
> Both GUP-fast and the page cache will find a page under RCU protection,
> inc it's refcount if not zero, check the page is still the one they were
> looking for, and if not will dec the refcount again.  That means if a
> page has been in the page cache or process page tables and you can't
> guarantee that all CPUs have been through the requisite grace periods,
> you might see the refcount increased.

Interesting scenario, it sounds like this could only happen for a
short period of time at the beginning of the life of a page in the
IOMMU Page Table.


> I'm not prepared to make a guarantee that these are the only circumstances
> under which you'll see a temporarily higher refcount than you expect.
> Either currently or in the future.  If you use the refcount as anything
> other than a refcount, you're living dangerously.  And if you think that
> you'll be the one to do the last refcount put, you're not necessarily
> correct (see the saga around __free_pages() which ended up as commit
> e320d3012d25 fixed by 462a8e08e0e6 (which indicates the rare race does
> actually happen)).
>
> Now, it seems like from your further explanation that the consequence
> of getting this wrong is simply that you fail to free the page early.
> That seems OK, but I insist that you insert some comments explaining
> what is going on and why it's safe so somebody auditing uses of refcount
> doesn't have to reanalyse the whole thing for themself.  Or worse that
> somebody working on the iommu sees this and thinks they can "improve"
> on it.

Yes, I can add detailed comments explaining how refcount is used here.

Alternatively, I was thinking of using mapcount:

From mm_types.h:
  * If your page will not be mapped to userspace, you can also use the
four
  * bytes in the mapcount union, but you must call
page_mapcount_reset()
  * before freeing it.

It sounds like we can safely use _mapcount for our needs, and do
page_mapcount_reset() before freeing pages.

Pasha