RE: [Linuxarm] Re: [PATCH] Documentation/features: mark BATCHED_UNMAP_TLB_FLUSH doesn't apply to ARM64

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Anshuman Khandual [mailto:anshuman.khandual@xxxxxxx]
> Sent: Tuesday, February 23, 2021 7:10 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; corbet@xxxxxxx;
> linux-doc@xxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linuxarm@xxxxxxxxxxxxx; Mel Gorman <mgorman@xxxxxxx>; Andy Lutomirski
> <luto@xxxxxxxxxx>; Catalin Marinas <catalin.marinas@xxxxxxx>; Will Deacon
> <will@xxxxxxxxxx>
> Subject: [Linuxarm] Re: [PATCH] Documentation/features: mark
> BATCHED_UNMAP_TLB_FLUSH doesn't apply to ARM64
> 
> 
> 
> On 2/23/21 6:02 AM, Barry Song wrote:
> > BATCHED_UNMAP_TLB_FLUSH is used on x86 to do batched tlb shootdown by
> > sending one IPI to TLB flush all entries after unmapping pages rather
> > than sending an IPI to flush each individual entry.
> > On arm64, tlb shootdown is done by hardware. Flush instructions are
> > innershareable. The local flushes are limited to the boot (1 per CPU)
> > and when a task is getting a new ASID.
> 
> Is there any previous discussion around this ?

I copied the declaration of local flushes from:

"ARM64 Linux kernel is SMP-aware (no possibility to build only for UP).
Most of the flush instructions are innershareable. The local flushes are
limited to the boot (1 per CPU) and when a task is getting a new ASIC."

https://patchwork.kernel.org/project/xen-devel/patch/1461756173-10300-1-git-send-email-julien.grall@xxxxxxx/

I am not sure if getting a new asid and the boot are the only two
cases of local flushes while I think this is probably true.

But even we find more corner cases, hardly the trend arm64 doesn't
need BATCHED_UNMAP_TLB_FLUSH will be changed.

> 
> > So marking this feature as "TODO" is not proper. ".." isn't good as
> > well. So this patch adds a "N/A" for this kind of features which are
> > not needed on some architectures.
> >
> > Cc: Mel Gorman <mgorman@xxxxxxx>
> > Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Signed-off-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx>
> > ---
> >  Documentation/features/arch-support.txt        | 1 +
> >  Documentation/features/vm/TLB/arch-support.txt | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/features/arch-support.txt
> b/Documentation/features/arch-support.txt
> > index d22a1095e661..118ae031840b 100644
> > --- a/Documentation/features/arch-support.txt
> > +++ b/Documentation/features/arch-support.txt
> > @@ -8,4 +8,5 @@ The meaning of entries in the tables is:
> >      | ok |  # feature supported by the architecture
> >      |TODO|  # feature not yet supported by the architecture
> >      | .. |  # feature cannot be supported by the hardware
> > +    | N/A|  # feature doesn't apply to the architecture
> 
> NA might be better here. s/doesn't apply/not applicable/ in order to match NA.
> Still wondering if NA is really needed when there is already ".." ? Regardless
> either way should be fine.

I don't think ".." is proper here. ".." means hardware doesn't support
the feature. But here it is just opposite, arm64 has the hardware
support of tlb shootdown rather than depending on a software IPI.

> 
> >
> > diff --git a/Documentation/features/vm/TLB/arch-support.txt
> b/Documentation/features/vm/TLB/arch-support.txt
> > index 30f75a79ce01..0d070f9f98d8 100644
> > --- a/Documentation/features/vm/TLB/arch-support.txt
> > +++ b/Documentation/features/vm/TLB/arch-support.txt
> > @@ -9,7 +9,7 @@
> >      |       alpha: | TODO |
> >      |         arc: | TODO |
> >      |         arm: | TODO |
> > -    |       arm64: | TODO |
> > +    |       arm64: | N/A  |
> >      |         c6x: |  ..  |
> >      |        csky: | TODO |
> >      |       h8300: |  ..  |
> >
Thanks
Barry