LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
To: Hugh Dickins <hugh@veritas.com>
Cc: akpm@linux-foundation.org, peterz@infradead.org,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [patch 5/5] s390 tlb flush fix.
Date: Wed, 04 Jul 2007 09:34:34 +0200	[thread overview]
Message-ID: <1183534474.1208.20.camel@localhost> (raw)
In-Reply-To: <Pine.LNX.4.64.0707031921270.8155@blonde.wat.veritas.com>

On Tue, 2007-07-03 at 19:58 +0100, Hugh Dickins wrote:
> On Tue, 3 Jul 2007, Martin Schwidefsky wrote:
> > +
> > +static inline struct mmu_gather *tlb_gather_mmu(struct mm_struct *mm,
> > +						unsigned int full_mm_flush)
> > +{
> > +	struct mmu_gather *tlb = &get_cpu_var(mmu_gathers);
> > +
> > +	tlb->mm = mm;
> > +	tlb->fullmm = full_mm_flush || (num_online_cpus() == 1) ||
> > +		(atomic_read(&mm->mm_users) <= 1);
> > +	tlb->nr_ptes = 0;
> > +	tlb->nr_pmds = TLB_NR_PTRS;
> > +	if (tlb->fullmm)
> > +		__tlb_flush_mm(mm);
> > +	return tlb;
> > +}
> 
> I'm afraid that mm_users test (and probably some of your other
> mm_users tests) is not good: because this also gets called when
> a file is truncated while it is mapped - the active mm at that
> time is likely not to be one of the mm_users.  (Do any other
> arches use mm_users in that way?  No: that should be a warning.)

Good catch, that would have caused me some headache. So I need to add a
current->active_mm==mm check if mm_users==1.

> You might do better to make more use of cpu_vm_mask (though I
> didn't see where any bits get cleared from it on s390 at present).

We don't clear any of the bits in cpu_vm_mask. I though about it for a
while and got tangled in race conditions. The cpu_vm_mask optimization
works for short-lived processes which always executed on the same cpu.

> Though it seems sensible to aim for one TLB flush at the beginning
> as you're doing, that's not what other arches do (some have to
> worry about speculative execution, but you don't?), and it
> worries me that you're taking s390 further away into its own
> implementation: which you're surely entitled to do, but then
> we're more likely to screw you over by mistake in future.

We do not have to worry about speculative execution because s390 uses
special instruction to do user access (mvcs, mvcp and mvcos) and the
kernel has its own address space. The compiler doesn't know about these
instruction and cannot "accidentally" access a user space address over
the user page table when it shouldn't.

> Is there perhaps another architecture whose procedures you
> can copy?  Changing a pte while another cpu is accessing it
> is not a problem unique to s390.

No, I don't think so. s390 is quite unique with the restriction that you
may not do a set_pte_at .. flush_tlb_xxx while a pte might get accessed
by a different cpu.

> Patches 1-4 looked fine to me, but I believe this 5/5
> is the rationale behind all of them.

Yes, indeed the tlb flush fix and the 1K/2K page tables are my reasons
for all these patches.

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.



      reply	other threads:[~2007-07-04  7:32 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-07-03 11:18 [patch 0/5] some mm improvements + s390 tlb flush Martin Schwidefsky
2007-07-03 11:18 ` [patch 1/5] avoid tlb gather restarts Martin Schwidefsky
2007-07-03 17:42   ` Hugh Dickins
2007-07-04  7:37     ` Martin Schwidefsky
2007-07-16  6:20   ` Andrew Morton
2007-07-03 11:18 ` [patch 2/5] remove ptep_establish Martin Schwidefsky
2007-07-03 11:18 ` [patch 3/5] remove ptep_test_and_clear_dirty and ptep_clear_flush_dirty Martin Schwidefsky
2007-07-03 11:18 ` [patch 4/5] move mm_struct and vm_area_struct Martin Schwidefsky
2007-07-03 11:18 ` [patch 5/5] s390 tlb flush fix Martin Schwidefsky
2007-07-03 18:58   ` Hugh Dickins
2007-07-04  7:34     ` Martin Schwidefsky [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1183534474.1208.20.camel@localhost \
    --to=schwidefsky@de.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=hugh@veritas.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=peterz@infradead.org \
    --subject='Re: [patch 5/5] s390 tlb flush fix.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).