Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700833 > unrolled thread

Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-01 12:40 +0200
    Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-01 14:20 +0200
      Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-01 18:40 +0200
        Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-01 18:50 +0200
          Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 01:00 +0200
            Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-02 04:10 +0200
              Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-02 10:20 +0200
                Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-02 10:50 +0200
                  Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 11:00 +0200
                    Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-02 11:10 +0200
                      Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-03 04:40 +0200
                Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 10:50 +0200
                  Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-02 11:10 +0200
                    Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 11:20 +0200
              Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 10:20 +0200
                Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-02 17:50 +0200
                  Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Peter Zijlstra <peterz@infradead.org> - 2017-08-02 17:50 +0200
          Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-02 03:10 +0200
        Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Will Deacon <will.deacon@arm.com> - 2017-08-01 18:50 +0200
      Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-02 04:00 +0200
    Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending() Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-01 15:40 +0200

Page 1 of 2  [1] 2  Next page →

#1700833 — Re: [RFC][PATCH 1/5] mm: Rework {set,clear,mm}_tlb_flush_pending()

On Fri, Jul 28, 2017 at 07:45:33PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 09, 2017 at 03:45:54PM +0100, Will Deacon wrote:
> > On Wed, Jun 07, 2017 at 06:15:02PM +0200, Peter Zijlstra wrote:
> > > Commit:
> > > 
> > >   af2c1401e6f9 ("mm: numa: guarantee that tlb_flush_pending updates are visible before page table updates")
> > > 
> > > added smp_mb__before_spinlock() to set_tlb_flush_pending(). I think we
> > > can solve the same problem without this barrier.
> > > 
> > > If instead we mandate that mm_tlb_flush_pending() is used while
> > > holding the PTL we're guaranteed to observe prior
> > > set_tlb_flush_pending() instances.
> > > 
> > > For this to work we need to rework migrate_misplaced_transhuge_page()
> > > a little and move the test up into do_huge_pmd_numa_page().
> > > 
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Cc: Rik van Riel <riel@redhat.com>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > > --- a/include/linux/mm_types.h
> > > +++ b/include/linux/mm_types.h
> > > @@ -527,18 +527,16 @@ static inline cpumask_t *mm_cpumask(stru
> > >   */
> > >  static inline bool mm_tlb_flush_pending(struct mm_struct *mm)
> > >  {
> > > -	barrier();
> > > +	/*
> > > +	 * Must be called with PTL held; such that our PTL acquire will have
> > > +	 * observed the store from set_tlb_flush_pending().
> > > +	 */
> > >  	return mm->tlb_flush_pending;
> > >  }
> > >  static inline void set_tlb_flush_pending(struct mm_struct *mm)
> > >  {
> > >  	mm->tlb_flush_pending = true;
> > > -
> > > -	/*
> > > -	 * Guarantee that the tlb_flush_pending store does not leak into the
> > > -	 * critical section updating the page tables
> > > -	 */
> > > -	smp_mb__before_spinlock();
> > > +	barrier();
> > 
> > Why do you need the barrier() here? Isn't the ptl unlock sufficient?
> 
> So I was going through these here patches again, and wrote the
> following comment:
> 
> static inline void set_tlb_flush_pending(struct mm_struct *mm)
> {
> 	mm->tlb_flush_pending = true;
> 	/*
> 	 * The only time this value is relevant is when there are indeed pages
> 	 * to flush. And we'll only flush pages after changing them, which
> 	 * requires the PTL.
> 	 *
> 	 * So the ordering here is:
> 	 *
> 	 * 	mm->tlb_flush_pending = true;
> 	 * 	spin_lock(&ptl);
> 	 *	...
> 	 * 	set_pte_at();
> 	 * 	spin_unlock(&ptl);
> 	 *
> 	 *
> 	 * 				spin_lock(&ptl)
> 	 * 				mm_tlb_flush_pending();
> 	 * 				....
> 	 * 				spin_unlock(&ptl);
> 	 *
> 	 * 	flush_tlb_range();
> 	 * 	mm->tlb_flush_pending = false;
> 	 */
> }
> 
> And while the ptl locks are indeed sufficient to constrain the true
> assignment, what constrains the false assignment? As in the above there
> is nothing stopping the false from ending up visible at
> mm_tlb_flush_pending().
> 
> Or does flush_tlb_range() have implicit ordering? It does on x86, but is
> this generally so?

Looks like that's what's currently relied upon:

  /* Clearing is done after a TLB flush, which also provides a barrier. */

It also provides barrier semantics on arm/arm64. In reality, I suspect
all archs have to provide some order between set_pte_at and flush_tlb_range
which is sufficient to hold up clearing the flag. :/

Will

[toc] | [next] | [standalone]

#1700966

On Tue, Aug 01, 2017 at 10:02:45PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2017-08-01 at 11:31 +0100, Will Deacon wrote:
> > Looks like that's what's currently relied upon:
> > 
> >   /* Clearing is done after a TLB flush, which also provides a barrier. */
> > 
> > It also provides barrier semantics on arm/arm64. In reality, I suspect
> > all archs have to provide some order between set_pte_at and flush_tlb_range
> > which is sufficient to hold up clearing the flag. :/
> 
> Hrm... not explicitely.
> 
> Most archs (powerpc among them) have set_pte_at be just a dumb store,
> so the only barrier it has is the surrounding PTL.
> 
> Now flush_tlb_range() I assume has some internal strong barriers but
> none of that is well defined or documented at all, so I suspect all
> bets are off.

Right.. but seeing how we're in fact relying on things here it might be
time to go figure this out and document bits.

*sigh*, I suppose its going to be me doing this.. :-)

[toc] | [prev] | [next] | [standalone]

#1701222

On Tue, Aug 01, 2017 at 02:14:19PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 01, 2017 at 10:02:45PM +1000, Benjamin Herrenschmidt wrote:
> > On Tue, 2017-08-01 at 11:31 +0100, Will Deacon wrote:
> > > Looks like that's what's currently relied upon:
> > > 
> > >   /* Clearing is done after a TLB flush, which also provides a barrier. */
> > > 
> > > It also provides barrier semantics on arm/arm64. In reality, I suspect
> > > all archs have to provide some order between set_pte_at and flush_tlb_range
> > > which is sufficient to hold up clearing the flag. :/
> > 
> > Hrm... not explicitely.
> > 
> > Most archs (powerpc among them) have set_pte_at be just a dumb store,
> > so the only barrier it has is the surrounding PTL.
> > 
> > Now flush_tlb_range() I assume has some internal strong barriers but
> > none of that is well defined or documented at all, so I suspect all
> > bets are off.
> 
> Right.. but seeing how we're in fact relying on things here it might be
> time to go figure this out and document bits.
> 
> *sigh*, I suppose its going to be me doing this.. :-)


So on the related question; does on_each_cpu() provide a full smp_mb(),
I think we can answer: yes.

on_each_cpu() does IPIs to all _other_ CPUs, and those IPIs are using
llist_add() which is cmpxchg() which implies smp_mb().

After that it runs the local function.

So we can see on_each_cpu() as doing a smp_mb() before running @func.


xtensa - it uses on_each_cpu() for TLB invalidates.

x86 - we use either on_each_cpu() (flush_tlb_all(),
flush_tlb_kernel_range()) or we use flush_tlb_mm_range() which does an
atomic_inc_return() at the very start. Not to mention that actually
flushing TLBs itself is a barrier. Arguably flush_tlb_mm_range() should
first do _others* and then self, because others will use
smp_call_function_many() and see above.

(TODO look into paravirt)

Tile - does mb() in flush_remote()

sparc32-smp !?

sparc64 -- nope, no-op functions, TLB flushes are contained inside the PTL.

sh - yes, per smp_call_function

s390 - has atomics when it flushes. ptep_modify_prot_start() can set
mm->flush_mm = 1, at which point flush_tlb_range() will actually do
something, in that case there will be a smp_mb as per the atomics.
Otherwise the TLB invalidate is contained inside the PTL.

powerpc - radix - PTESYNC
	  hash - flush inside PTL


parisc - has all PTE and TLB operations serialized using a global lock

nm10300 - *ugh* but yes, smp_call_function() for remote CPUs

mips - smp_call_function for remote CPUs

metag - mmio write

m32r - doesn't seem to have smp_mb()

ia64 - smp_call_function_*()

hexagon - HVM trap, no smp_mb()

blackfin - nommu

arm - dsb ish

arm64  - dsb ish

arc - no barrier

alpha - no barrier


Now the architectures that do not have a barrier, like alpha, arc,
metag, the  PTL spin_unlock has a smp_mb, however I don't think that is
enough, because then the flush_tlb_range() might still be pending. That
said, these architectures probably don't have transparant huge pages so
it doesn't matter.


Still this is all rather unsatisfactory. Either we should define
flush_tlb*() to imply a barrier when its not a no-op (sparc64/ppc-hash)
or simply make clear_tlb_flush_pending() an smp_store_release().

I prefer the latter option.

Opinions?

[toc] | [prev] | [next] | [standalone]

#1701233

On Tue, Aug 01, 2017 at 05:44:14PM +0100, Will Deacon wrote:
> On Tue, Aug 01, 2017 at 06:39:03PM +0200, Peter Zijlstra wrote:
> > Still this is all rather unsatisfactory. Either we should define
> > flush_tlb*() to imply a barrier when its not a no-op (sparc64/ppc-hash)
> > or simply make clear_tlb_flush_pending() an smp_store_release().
> > 
> > I prefer the latter option.
> > 
> > Opinions?
> 
> I prefer the latter option too, since I'd like to relax the arm64 TLB
> flushing to have weaker barriers for the local case. Granted, that doesn't
> break the NUMA migration code, but it would make the barrier semantics of
> the TLB invalidation routines even more subtle if we were to define them
> generally.

Another 'fun' question, is smp_mb() strong enough to order against the
TLB invalidate? Because we really want to clear this flag _after_.

PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
work? Ben?

[toc] | [prev] | [next] | [standalone]

#1701568

On Tue, Aug 01, 2017 at 06:48:20PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 01, 2017 at 05:44:14PM +0100, Will Deacon wrote:
> > On Tue, Aug 01, 2017 at 06:39:03PM +0200, Peter Zijlstra wrote:
> > > Still this is all rather unsatisfactory. Either we should define
> > > flush_tlb*() to imply a barrier when its not a no-op (sparc64/ppc-hash)
> > > or simply make clear_tlb_flush_pending() an smp_store_release().
> > > 
> > > I prefer the latter option.
> > > 
> > > Opinions?
> > 
> > I prefer the latter option too, since I'd like to relax the arm64 TLB
> > flushing to have weaker barriers for the local case. Granted, that doesn't
> > break the NUMA migration code, but it would make the barrier semantics of
> > the TLB invalidation routines even more subtle if we were to define them
> > generally.
> 
> Another 'fun' question, is smp_mb() strong enough to order against the
> TLB invalidate? Because we really want to clear this flag _after_.
> 
> PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
> work? Ben?

From what I gather it is not. You have TLBSYNC for it. So the good news
is that PPC-radix does all that and is fully serialized on the tlb
flush. Not sure for the PPC-hash case.

At the same time, smp_mb() is not sufficient on ARM either, they need a
DSB barrier on both ends.

So are we going to mandate tlb flush implementations are completely
ordered ?

[toc] | [prev] | [next] | [standalone]

#1701626

On Wed, 2017-08-02 at 00:59 +0200, Peter Zijlstra wrote:
> > PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
> > work? Ben?
> > From what I gather it is not. You have TLBSYNC for it. So the good news

tlbsync is pretty much a nop these days. ptesync is a strict superset
of sync and we have it after every tlbie.

> is that PPC-radix does all that and is fully serialized on the tlb
> flush. Not sure for the PPC-hash case.
> 
> At the same time, smp_mb() is not sufficient on ARM either, they need a
> DSB barrier on both ends.
> 
> So are we going to mandate tlb flush implementations are completely
> ordered ?

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1701834

On Wed, Aug 02, 2017 at 10:11:06AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 02, 2017 at 11:23:12AM +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2017-08-02 at 00:59 +0200, Peter Zijlstra wrote:
> > > > PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
> > > > work? Ben?
> > > > From what I gather it is not. You have TLBSYNC for it. So the good news
> > 
> > tlbsync is pretty much a nop these days. ptesync is a strict superset
> > of sync and we have it after every tlbie.
> 
> In the radix code, yes. I got lost going through the hash code, and I
> didn't look at the 32bit code at all.
> 
> So the radix code does:
> 
>  PTESYNC
>  TLBIE
>  EIEIO; TLBSYNC; PTESYNC
> 
> which should be completely ordered against anything prior and anything
> following, and is I think the behaviour we want from TLB flushes in
> general, but is very much not provided by a number of architectures
> afaict.
> 
> Ah, found the hash-64 code, yes that's good too. The hash32 code lives
> in asm and confuses me, it has a bunch of SYNC, SYNC_601 and isync in.
> The nohash variant seems to do a isync after tlbwe, but again no clue.
> 
> 
> Now, do I go and attempt fixing all that needs fixing?
> 
> 
> x86 is good, our CR3 writes or INVLPG stuff is fully serializing.
> 
> arm is good, it does DSB ISH before and after
> 
> arm64 looks good too, although it plays silly games with the first
> barrier, but I trust that to be sufficient.

The first barrier only orders prior stores for us, because page table
updates are made using stores. A prior load could be reordered past the
invalidation, but can't make it past the second barrier.

I really think we should avoid defining TLB invalidation in terms of
smp_mb() because it's a lot more subtle than that.

Will

[toc] | [prev] | [next] | [standalone]

#1701895

On Wed, Aug 02, 2017 at 09:15:23AM +0100, Will Deacon wrote:
> On Wed, Aug 02, 2017 at 10:11:06AM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 02, 2017 at 11:23:12AM +1000, Benjamin Herrenschmidt wrote:
> > > On Wed, 2017-08-02 at 00:59 +0200, Peter Zijlstra wrote:
> > > > > PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
> > > > > work? Ben?
> > > > > From what I gather it is not. You have TLBSYNC for it. So the good news
> > > 
> > > tlbsync is pretty much a nop these days. ptesync is a strict superset
> > > of sync and we have it after every tlbie.
> > 
> > In the radix code, yes. I got lost going through the hash code, and I
> > didn't look at the 32bit code at all.
> > 
> > So the radix code does:
> > 
> >  PTESYNC
> >  TLBIE
> >  EIEIO; TLBSYNC; PTESYNC
> > 
> > which should be completely ordered against anything prior and anything
> > following, and is I think the behaviour we want from TLB flushes in
> > general, but is very much not provided by a number of architectures
> > afaict.
> > 
> > Ah, found the hash-64 code, yes that's good too. The hash32 code lives
> > in asm and confuses me, it has a bunch of SYNC, SYNC_601 and isync in.
> > The nohash variant seems to do a isync after tlbwe, but again no clue.
> > 
> > 
> > Now, do I go and attempt fixing all that needs fixing?
> > 
> > 
> > x86 is good, our CR3 writes or INVLPG stuff is fully serializing.
> > 
> > arm is good, it does DSB ISH before and after
> > 
> > arm64 looks good too, although it plays silly games with the first
> > barrier, but I trust that to be sufficient.
> 
> The first barrier only orders prior stores for us, because page table
> updates are made using stores. A prior load could be reordered past the
> invalidation, but can't make it past the second barrier.
> 
> I really think we should avoid defining TLB invalidation in terms of
> smp_mb() because it's a lot more subtle than that.

Another worry I have here is with architectures that can optimise the
"only need to flush the local TLB" case. For example, this version of 'R':


P0:
WRITE_ONCE(x, 1);
smp_mb();
WRITE_ONCE(y, 1);

P1:
WRITE_ONCE(y, 2);
flush_tlb_range(...);  // Only needs to flush the local TLB
r0 = READ_ONCE(x);


It doesn't seem unreasonable to me for y==2 && r0==0 if the
flush_tlb_range(...) ends up only doing local invalidation. As a concrete
example, imagine a CPU with a page table walker that can snoop the local
store-buffer. Then, the local flush_tlb_range in P1 only needs to progress
the write to y as far as the store-buffer before it can invalidate the local
TLB. Once the TLB is invalidated, it can read x knowing that the translation
is up-to-date wrt the page table, but that read doesn't need to wait for
write to y to become visible to other CPUs.

So flush_tlb_range is actually weaker than smp_mb in some respects, yet the
flush_tlb_pending stuff will still work correctly.

Will

[toc] | [prev] | [next] | [standalone]

#1701928

On Wed, Aug 02, 2017 at 09:43:50AM +0100, Will Deacon wrote:
> On Wed, Aug 02, 2017 at 09:15:23AM +0100, Will Deacon wrote:

> > I really think we should avoid defining TLB invalidation in terms of
> > smp_mb() because it's a lot more subtle than that.
> 
> Another worry I have here is with architectures that can optimise the
> "only need to flush the local TLB" case. For example, this version of 'R':
> 
> 
> P0:
> WRITE_ONCE(x, 1);
> smp_mb();
> WRITE_ONCE(y, 1);
> 
> P1:
> WRITE_ONCE(y, 2);
> flush_tlb_range(...);  // Only needs to flush the local TLB
> r0 = READ_ONCE(x);
> 
> 
> It doesn't seem unreasonable to me for y==2 && r0==0 if the
> flush_tlb_range(...) ends up only doing local invalidation. As a concrete
> example, imagine a CPU with a page table walker that can snoop the local
> store-buffer. Then, the local flush_tlb_range in P1 only needs to progress
> the write to y as far as the store-buffer before it can invalidate the local
> TLB. Once the TLB is invalidated, it can read x knowing that the translation
> is up-to-date wrt the page table, but that read doesn't need to wait for
> write to y to become visible to other CPUs.
> 
> So flush_tlb_range is actually weaker than smp_mb in some respects, yet the
> flush_tlb_pending stuff will still work correctly.

So while I think you're right, and we could live with this, after all,
if we know the mm is CPU local, there shouldn't be any SMP concerns wrt
its page tables. Do you really want to make this more complicated?

[toc] | [prev] | [next] | [standalone]

#1701938

On Wed, Aug 02, 2017 at 10:51:11AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 02, 2017 at 09:43:50AM +0100, Will Deacon wrote:
> > On Wed, Aug 02, 2017 at 09:15:23AM +0100, Will Deacon wrote:
> 
> > > I really think we should avoid defining TLB invalidation in terms of
> > > smp_mb() because it's a lot more subtle than that.
> > 
> > Another worry I have here is with architectures that can optimise the
> > "only need to flush the local TLB" case. For example, this version of 'R':
> > 
> > 
> > P0:
> > WRITE_ONCE(x, 1);
> > smp_mb();
> > WRITE_ONCE(y, 1);
> > 
> > P1:
> > WRITE_ONCE(y, 2);
> > flush_tlb_range(...);  // Only needs to flush the local TLB
> > r0 = READ_ONCE(x);
> > 
> > 
> > It doesn't seem unreasonable to me for y==2 && r0==0 if the
> > flush_tlb_range(...) ends up only doing local invalidation. As a concrete
> > example, imagine a CPU with a page table walker that can snoop the local
> > store-buffer. Then, the local flush_tlb_range in P1 only needs to progress
> > the write to y as far as the store-buffer before it can invalidate the local
> > TLB. Once the TLB is invalidated, it can read x knowing that the translation
> > is up-to-date wrt the page table, but that read doesn't need to wait for
> > write to y to become visible to other CPUs.
> > 
> > So flush_tlb_range is actually weaker than smp_mb in some respects, yet the
> > flush_tlb_pending stuff will still work correctly.
> 
> So while I think you're right, and we could live with this, after all,
> if we know the mm is CPU local, there shouldn't be any SMP concerns wrt
> its page tables. Do you really want to make this more complicated?

It gives us a nice performance lift on arm64 and I have a patch...[1]

Will

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/commit/?h=aarch64/devel&id=1c7cf53658f0fa16338d1f8406285ae28fd5f616

[toc] | [prev] | [next] | [standalone]

#1702627

On Wed, 2017-08-02 at 10:02 +0100, Will Deacon wrote:
> > > So flush_tlb_range is actually weaker than smp_mb in some respects, yet the
> > > flush_tlb_pending stuff will still work correctly.
> > 
> > So while I think you're right, and we could live with this, after all,
> > if we know the mm is CPU local, there shouldn't be any SMP concerns wrt
> > its page tables. Do you really want to make this more complicated?
> 
> It gives us a nice performance lift on arm64 and I have a patch...[1]

We do that on powerpc too, though there are ongoing questions a to
whether an smp_mb() after setting the mask bit in switch_mm is
sufficient vs. prefetch brining entries in the TLB after the context is
switched. But that's a powerpc specific issue. Nick Piggin is working
on sorting that out.

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1701903

On Wed, Aug 02, 2017 at 09:15:23AM +0100, Will Deacon wrote:
> On Wed, Aug 02, 2017 at 10:11:06AM +0200, Peter Zijlstra wrote:

> > arm64 looks good too, although it plays silly games with the first
> > barrier, but I trust that to be sufficient.
> 
> The first barrier only orders prior stores for us, because page table
> updates are made using stores. A prior load could be reordered past the
> invalidation, but can't make it past the second barrier.

So then you rely on the program not having any loads pending to the
address you're about to invalidate, right? Otherwise we can do the TLBI
and then the load to insta-repopulate the TLB entry you just wanted
dead.

That later DSB ISH is too late for that.

Isn't that somewhat fragile?

> I really think we should avoid defining TLB invalidation in terms of
> smp_mb() because it's a lot more subtle than that.

I'm tempted to say stronger, smp_mb() only provides order, we want full
serialization. Everything before stays before and _completes_ before.
Everything after happens after (if the primitives actually do something
at all of course, sparc64 for instance has no-op flush_tlb*).

While such semantics might be slightly too strong for what we currently
need, it is what powerpc, x86 and arm currently implement and are fairly
easy to reason about. If we weaken it, stuff gets confusing again.

[toc] | [prev] | [next] | [standalone]

#1701940

On Wed, Aug 02, 2017 at 10:45:51AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 02, 2017 at 09:15:23AM +0100, Will Deacon wrote:
> > On Wed, Aug 02, 2017 at 10:11:06AM +0200, Peter Zijlstra wrote:
> 
> > > arm64 looks good too, although it plays silly games with the first
> > > barrier, but I trust that to be sufficient.
> > 
> > The first barrier only orders prior stores for us, because page table
> > updates are made using stores. A prior load could be reordered past the
> > invalidation, but can't make it past the second barrier.
> 
> So then you rely on the program not having any loads pending to the
> address you're about to invalidate, right? Otherwise we can do the TLBI
> and then the load to insta-repopulate the TLB entry you just wanted
> dead.
> 
> That later DSB ISH is too late for that.
> 
> Isn't that somewhat fragile?

We only initiate the TLB invalidation after the page table update is
observable to the page table walker, so any repopulation will cause a fill
using the new page table entry.

> > I really think we should avoid defining TLB invalidation in terms of
> > smp_mb() because it's a lot more subtle than that.
> 
> I'm tempted to say stronger, smp_mb() only provides order, we want full
> serialization. Everything before stays before and _completes_ before.
> Everything after happens after (if the primitives actually do something
> at all of course, sparc64 for instance has no-op flush_tlb*).
> 
> While such semantics might be slightly too strong for what we currently
> need, it is what powerpc, x86 and arm currently implement and are fairly
> easy to reason about. If we weaken it, stuff gets confusing again.

My problem with this is that we're strengthening the semantics for no actual
use-case, but at the same time this will have a real performance impact.

Will

[toc] | [prev] | [next] | [standalone]

#1701945

On Wed, Aug 02, 2017 at 10:02:28AM +0100, Will Deacon wrote:
> On Wed, Aug 02, 2017 at 10:45:51AM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 02, 2017 at 09:15:23AM +0100, Will Deacon wrote:
> > > On Wed, Aug 02, 2017 at 10:11:06AM +0200, Peter Zijlstra wrote:
> > 
> > > > arm64 looks good too, although it plays silly games with the first
> > > > barrier, but I trust that to be sufficient.
> > > 
> > > The first barrier only orders prior stores for us, because page table
> > > updates are made using stores. A prior load could be reordered past the
> > > invalidation, but can't make it past the second barrier.
> > 
> > So then you rely on the program not having any loads pending to the
> > address you're about to invalidate, right? Otherwise we can do the TLBI
> > and then the load to insta-repopulate the TLB entry you just wanted
> > dead.
> > 
> > That later DSB ISH is too late for that.
> > 
> > Isn't that somewhat fragile?
> 
> We only initiate the TLB invalidation after the page table update is
> observable to the page table walker, so any repopulation will cause a fill
> using the new page table entry.

Ah, indeed. Might be worth a comment tho.

> > > I really think we should avoid defining TLB invalidation in terms of
> > > smp_mb() because it's a lot more subtle than that.
> > 
> > I'm tempted to say stronger, smp_mb() only provides order, we want full
> > serialization. Everything before stays before and _completes_ before.
> > Everything after happens after (if the primitives actually do something
> > at all of course, sparc64 for instance has no-op flush_tlb*).
> > 
> > While such semantics might be slightly too strong for what we currently
> > need, it is what powerpc, x86 and arm currently implement and are fairly
> > easy to reason about. If we weaken it, stuff gets confusing again.
> 
> My problem with this is that we're strengthening the semantics for no actual
> use-case, but at the same time this will have a real performance impact.

Well, you could put in a dmb(ish) in the local case, that's loads
cheaper than the dsb(ish) you need for the !local case. But OK..

Back to staring at dodgy arch code..

[toc] | [prev] | [next] | [standalone]

#1701839

On Wed, Aug 02, 2017 at 11:23:12AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2017-08-02 at 00:59 +0200, Peter Zijlstra wrote:
> > > PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
> > > work? Ben?
> > > From what I gather it is not. You have TLBSYNC for it. So the good news
> 
> tlbsync is pretty much a nop these days. ptesync is a strict superset
> of sync and we have it after every tlbie.

In the radix code, yes. I got lost going through the hash code, and I
didn't look at the 32bit code at all.

So the radix code does:

 PTESYNC
 TLBIE
 EIEIO; TLBSYNC; PTESYNC

which should be completely ordered against anything prior and anything
following, and is I think the behaviour we want from TLB flushes in
general, but is very much not provided by a number of architectures
afaict.

Ah, found the hash-64 code, yes that's good too. The hash32 code lives
in asm and confuses me, it has a bunch of SYNC, SYNC_601 and isync in.
The nohash variant seems to do a isync after tlbwe, but again no clue.


Now, do I go and attempt fixing all that needs fixing?


x86 is good, our CR3 writes or INVLPG stuff is fully serializing.

arm is good, it does DSB ISH before and after

arm64 looks good too, although it plays silly games with the first
barrier, but I trust that to be sufficient.

But I'll have to go dig up arch manuals for the rest, if they include
the relevant information at all of course :/

[toc] | [prev] | [next] | [standalone]

#1702200

On Wed, 2017-08-02 at 10:11 +0200, Peter Zijlstra wrote:

> which should be completely ordered against anything prior and anything
> following, and is I think the behaviour we want from TLB flushes in
> general, but is very much not provided by a number of architectures
> afaict.
> 
> Ah, found the hash-64 code, yes that's good too. The hash32 code lives
> in asm and confuses me, it has a bunch of SYNC, SYNC_601 and isync in.
> The nohash variant seems to do a isync after tlbwe, but again no clue.

Doing some archeology ? :-)

In the hash32 days ptesync didn't exist, sync had all the needed
semantics. tlbew isn't a proper invalidate per-se, but isync will flush
the shadow TLBs, but I wouldn't bother too much about these, if needed
I can go fix them.

> Now, do I go and attempt fixing all that needs fixing?
> 
> 
> x86 is good, our CR3 writes or INVLPG stuff is fully serializing.
> 
> arm is good, it does DSB ISH before and after
> 
> arm64 looks good too, although it plays silly games with the first
> barrier, but I trust that to be sufficient.
> 
> But I'll have to go dig up arch manuals for the rest, if they include
> the relevant information at all of course :/

Ben.

[toc] | [prev] | [next] | [standalone]

#1702206

On Wed, Aug 02, 2017 at 11:57:04PM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2017-08-02 at 10:11 +0200, Peter Zijlstra wrote:
> 
> > which should be completely ordered against anything prior and anything
> > following, and is I think the behaviour we want from TLB flushes in
> > general, but is very much not provided by a number of architectures
> > afaict.
> > 
> > Ah, found the hash-64 code, yes that's good too. The hash32 code lives
> > in asm and confuses me, it has a bunch of SYNC, SYNC_601 and isync in.
> > The nohash variant seems to do a isync after tlbwe, but again no clue.
> 
> Doing some archeology ? :-)

I thought ppc32 is still a popular platform for embedded, and not
actually knowing what kind of mmu those sport (if one at all of course),
I just looked at all of them.

Also, I'd been looking at all arch tlb invalidate code in any case :-)
(and yes my head hurts because of it)

[toc] | [prev] | [next] | [standalone]

#1701615

On Tue, 2017-08-01 at 18:48 +0200, Peter Zijlstra wrote:
> On Tue, Aug 01, 2017 at 05:44:14PM +0100, Will Deacon wrote:
> > On Tue, Aug 01, 2017 at 06:39:03PM +0200, Peter Zijlstra wrote:
> > > Still this is all rather unsatisfactory. Either we should define
> > > flush_tlb*() to imply a barrier when its not a no-op (sparc64/ppc-hash)
> > > or simply make clear_tlb_flush_pending() an smp_store_release().
> > > 
> > > I prefer the latter option.
> > > 
> > > Opinions?
> > 
> > I prefer the latter option too, since I'd like to relax the arm64 TLB
> > flushing to have weaker barriers for the local case. Granted, that doesn't
> > break the NUMA migration code, but it would make the barrier semantics of
> > the TLB invalidation routines even more subtle if we were to define them
> > generally.
> 
> Another 'fun' question, is smp_mb() strong enough to order against the
> TLB invalidate? Because we really want to clear this flag _after_.
> 
> PowerPC for example uses PTESYNC before the TBLIE, so does a SYNC after
> work? Ben?

I have no idea. But then our tlbie has a ptesync after too no ? And
afaik a ptesync is a superset of sync.

Cheers,
Ben.

[toc] | [prev] | [next] | [standalone]

#1701238

On Tue, Aug 01, 2017 at 06:39:03PM +0200, Peter Zijlstra wrote:
> Still this is all rather unsatisfactory. Either we should define
> flush_tlb*() to imply a barrier when its not a no-op (sparc64/ppc-hash)
> or simply make clear_tlb_flush_pending() an smp_store_release().
> 
> I prefer the latter option.
> 
> Opinions?

I prefer the latter option too, since I'd like to relax the arm64 TLB
flushing to have weaker barriers for the local case. Granted, that doesn't
break the NUMA migration code, but it would make the barrier semantics of
the TLB invalidation routines even more subtle if we were to define them
generally.

Will

[toc] | [prev] | [next] | [standalone]

#1701624

On Tue, 2017-08-01 at 14:14 +0200, Peter Zijlstra wrote:
> Right.. but seeing how we're in fact relying on things here it might be
> time to go figure this out and document bits.
> 
> *sigh*, I suppose its going to be me doing this.. :-)

Thanks mate ! :-)

Cheers
Ben.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web