linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: Linux-MM <linux-mm@kvack.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	 Michael Ellerman <mpe@ellerman.id.au>,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	 Kalesh Singh <kaleshsingh@google.com>,
	Nick Piggin <npiggin@gmail.com>,
	 Joel Fernandes <joel@joelfernandes.org>,
	Christophe Leroy <christophe.leroy@csgroup.eu>
Subject: Re: [PATCH v6 07/11] mm/mremap: Use range flush that does TLB and page walk cache flush
Date: Mon, 24 May 2021 07:02:55 -1000	[thread overview]
Message-ID: <CAHk-=wimLWeWmsdkGetYzaASqxdzHmZGXJ51_3qjqyXBAYaw6g@mail.gmail.com> (raw)
In-Reply-To: <20210524090114.63446-8-aneesh.kumar@linux.ibm.com>

On Sun, May 23, 2021 at 11:04 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Add new helper flush_pte_tlb_pwc_range() which invalidates both TLB and
> page walk cache where TLB entries are mapped with page size PAGE_SIZE.

So I dislike this patch for two reasons:

 (a) naming.

If the ppc people want to use crazy TLA's that have no meaning outside
of the powerpc community, that's fine. But only in powerpc code.

"pwc" makes no sense to me, or to anybody else that isn't intimately
involved in low-level powerpc stuff. I assume it's "page walk cache",
but honestly, outside of this area, PWC is mostly used for a specific
type of webcam.

So there's no way I'd accept this as-is, simply because of that.
flush_pte_tlb_pwc_range() is simply not an acceptable name. You would
have to spell it out, not use an obscure TLA.

But I think you don't even want to do that, because of

 (b) is this even worth it as a public interface?

Why doesn't the powerpc radix TLB flushing code just always flush the
page table walking cache when the range is larger than a PMD?

Once you have big flush ranges like that, I don't believe it makes any
sense not to flush the walking cache too.

NOTE! This is particularly true as "flush the walking cache" isn't a
well-defined operation anyway. Which _levels_ of the walking cache?
Again, the size (and alignment) of the flush would actually tell you.
A new boolean "flush" parameter does *NOT* tell that at all.

So I think this new interface is mis-named, but I also think it's
pointless. Just DTRT automatically when somebody asks for a flush that
covers a PMD range (or a PUD range).

              Linus


  reply	other threads:[~2021-05-24 17:03 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-24  9:01 [PATCH v6 00/11] Speedup mremap on ppc64 Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 01/11] selftest/mremap_test: Update the test to handle pagesize other than 4K Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 02/11] selftest/mremap_test: Avoid crash with static build Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 03/11] mm/mremap: Convert huge PUD move to separate helper Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 04/11] mm/mremap: Use pmd/pud_poplulate to update page table entries Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 05/11] powerpc/mm/book3s64: Fix possible build error Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 06/11] powerpc/mm/book3s64: Update tlb flush routines to take a page walk cache flush argument Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 07/11] mm/mremap: Use range flush that does TLB and page walk cache flush Aneesh Kumar K.V
2021-05-24 17:02   ` Linus Torvalds [this message]
2021-05-25 13:27     ` Aneesh Kumar K.V
2021-05-25 17:08       ` Linus Torvalds
2021-05-24  9:01 ` [PATCH v6 08/11] mm/mremap: properly flush the TLB on mremap Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 09/11] mm/mremap: Fix race between mremap and pageout Aneesh Kumar K.V
2021-05-24 13:38   ` [PATCH v6 updated 9/11] " Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 10/11] mm/mremap: Allow arch runtime override Aneesh Kumar K.V
2021-05-24  9:01 ` [PATCH v6 11/11] powerpc/mm: Enable HAVE_MOVE_PMD support Aneesh Kumar K.V

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='CAHk-=wimLWeWmsdkGetYzaASqxdzHmZGXJ51_3qjqyXBAYaw6g@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=akpm@linux-foundation.org \
    --cc=aneesh.kumar@linux.ibm.com \
    --cc=christophe.leroy@csgroup.eu \
    --cc=joel@joelfernandes.org \
    --cc=kaleshsingh@google.com \
    --cc=linux-mm@kvack.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    --cc=npiggin@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox