Re: [PATCH] parisc: use per-pagetable spinlock

From: Mikulas Patocka <mpatocka@redhat.com>
To: John David Anglin <dave.anglin@bell.net>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>,
	Helge Deller <deller@gmx.de>,
	linux-parisc@vger.kernel.org
Subject: Re: [PATCH] parisc: use per-pagetable spinlock
Date: Wed, 10 Apr 2019 12:09:04 -0400 (EDT)	[thread overview]
Message-ID: <alpine.LRH.2.02.1904101205480.13246@file01.intranet.prod.int.rdu2.redhat.com> (raw)
In-Reply-To: <5028ebb4-e1eb-6c77-5466-86838e938aa2@bell.net>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2289 bytes --]

On Wed, 10 Apr 2019, John David Anglin wrote:

> On 2019-04-06 4:40 p.m., Mikulas Patocka wrote:
> >
> > On Sat, 6 Apr 2019, James Bottomley wrote:
> >
> >> On Sat, 2019-04-06 at 16:13 -0400, Mikulas Patocka wrote:
> >>>> Of course, on systems without a merced bus, we don't need the lock
> >>> at 
> >>>> all, so runtime patching might be usable to fix that case.
> >>>>
> >>>> James
> >>> The lock is still needed to synchronize TLB fault handlers with the
> >>> code that modifies the pagetables - but we could have per-process
> >>> lock for this purpose.
> >> It is?  I don't think we need any per-arch sync for that.  The purge
> >> should happen after all modifications are done so the next page fault
> >> inserts the new TLB entry ... so if there is a place where the purge
> >> lock matters to the page table updates, we're doing something wrong.
> >>
> >> James
> > Suppose that this sequence happens:
> >
> > CPU1:
> > (inside the TLB miss handler)
> > read the value XXX from the pagetables to the register
> The value is read holding TLB lock and the value is updated after insert.  Then, lock is released.
> Only concern is whether TLB inserts are strongly ordered.
> 
> >
> > CPU2:
> > modify the value in the pagetables to YYY
> > broadcast a TLB purge
> CPU2 needs to acquire TLB lock before it can modify value and broadcast TLB purge (see set_pte_at).  Lock is then released.
> TLB purges are strongly ordered.
> 
> So, I don't think this scenario can happen.

It can't happen in the current code. This is hypothetical scenario that 
could happen if we removed the TLB lock as suggested by James. But James 
claims that it can't happen because the purge tlb instruction will wait 
until all the other CPUs finish executing a high-priority interruption.

What do you think? Could the TLB lock go away?

Having one lock shared by all four cores degrades performance 
significantly (30%).

Mikulas

> >
> > CPU1:
> > receives the TLB purge broadcast and flushes the TLB
> > ... continues executing the TLB handler and inserts the value XXX from the register into the TLB
> >
> > And now, CPU1 is running with stale entry in the TLB. We need the lock to 
> > prevent this situation.
> >
> > Mikulas
> >
> 
> Dave
> 
> -- 
> John David Anglin  dave.anglin@bell.net
> 
> 