[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dd01a9d2-4b9b-8edc-1195-40a3624f9a8a@arm.com>
Date: Wed, 30 Nov 2022 08:27:40 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Yicong Yang <yangyicong@...wei.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: yangyicong@...ilicon.com, linux-mm@...ck.org,
linux-arm-kernel@...ts.infradead.org, x86@...nel.org,
catalin.marinas@....com, will@...nel.org,
linux-doc@...r.kernel.org, corbet@....net, peterz@...radead.org,
arnd@...db.de, punit.agrawal@...edance.com,
linux-kernel@...r.kernel.org, darren@...amperecomputing.com,
huzhanyuan@...o.com, lipeifeng@...o.com, zhangshiming@...o.com,
guojian@...o.com, realmz6@...il.com, linux-mips@...r.kernel.org,
openrisc@...ts.librecores.org, linuxppc-dev@...ts.ozlabs.org,
linux-riscv@...ts.infradead.org, linux-s390@...r.kernel.org,
Barry Song <21cnbao@...il.com>, wangkefeng.wang@...wei.com,
xhao@...ux.alibaba.com, prime.zeng@...ilicon.com,
Anshuman Khandual <khandual@...ux.vnet.ibm.com>,
Barry Song <baohua@...nel.org>
Subject: Re: [PATCH v7 1/2] mm/tlbbatch: Introduce
arch_tlbbatch_should_defer()
On 11/30/22 07:53, Yicong Yang wrote:
> On 2022/11/30 7:23, Andrew Morton wrote:
>> On Thu, 17 Nov 2022 16:26:47 +0800 Yicong Yang <yangyicong@...wei.com> wrote:
>>
>>> From: Anshuman Khandual <khandual@...ux.vnet.ibm.com>
>>>
>>> The entire scheme of deferred TLB flush in reclaim path rests on the
>>> fact that the cost to refill TLB entries is less than flushing out
>>> individual entries by sending IPI to remote CPUs. But architecture
>>> can have different ways to evaluate that. Hence apart from checking
>>> TTU_BATCH_FLUSH in the TTU flags, rest of the decision should be
>>> architecture specific.
>>>
>>> ...
>>>
>>> --- a/arch/x86/include/asm/tlbflush.h
>>> +++ b/arch/x86/include/asm/tlbflush.h
>>> @@ -240,6 +240,18 @@ static inline void flush_tlb_page(struct vm_area_struct *vma, unsigned long a)
>>> flush_tlb_mm_range(vma->vm_mm, a, a + PAGE_SIZE, PAGE_SHIFT, false);
>>> }
>>>
>>> +static inline bool arch_tlbbatch_should_defer(struct mm_struct *mm)
>>> +{
>>> + bool should_defer = false;
>>> +
>>> + /* If remote CPUs need to be flushed then defer batch the flush */
>>> + if (cpumask_any_but(mm_cpumask(mm), get_cpu()) < nr_cpu_ids)
>>> + should_defer = true;
>>> + put_cpu();
>>> +
>>> + return should_defer;
>>> +}
>>> +
>>> static inline u64 inc_mm_tlb_gen(struct mm_struct *mm)
>>> {
>>> /*
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index 2ec925e5fa6a..a9ab10bc0144 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -685,17 +685,10 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
>>> */
>>> static bool should_defer_flush(struct mm_struct *mm, enum ttu_flags flags)
>>> {
>>> - bool should_defer = false;
>>> -
>>> if (!(flags & TTU_BATCH_FLUSH))
>>> return false;
>>>
>>> - /* If remote CPUs need to be flushed then defer batch the flush */
>>> - if (cpumask_any_but(mm_cpumask(mm), get_cpu()) < nr_cpu_ids)
>>> - should_defer = true;
>>> - put_cpu();
>>> -
>>> - return should_defer;
>>> + return arch_tlbbatch_should_defer(mm);
>>> }
>>
>> I think this conversion could have been done better.
>>
>> should_defer_flush() is compiled if
>> CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH. So the patch implicitly
>> assumes that only x86 implements
>> CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH. Presently true, but what
>> happens if sparc (for example) wants to set
>> CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH? Now sparc needs its private
>> version of arch_tlbbatch_should_defer(), even if that is identical to
>> x86's.
>>
>
> The current logic is if architecture want to enable batched TLB flush, they
> need to implement their own version of arch_tlbbatch_should_defer() (for the
> hint to defer the TLB flush) and arch_tlbbatch_add_mm() (for pending TLB flush)
> and select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH. That's what we do in Patch 2/2 for
> enabling this on arm64.
>
> Since it is architecture specific, we must rely on the architecture to implement
> these two functions. Only select the ARCH_HAS_ARCH_TLBBATCH_SHOULD_DEFER is not
> enough.
>
>> Wouldn't it be better to make should_defer_flush() a __weak
>> function in rmap.c, or a static inline inside #ifndef
>> ARCH_HAS_ARCH_TLBBATCH_SHOULD_DEFER, or whatever technique best fits?
>>
>
> When ARCH_HAS_ARCH_TLBBATCH_SHOULD_DEFER is not selected, should_defer_flush()
> is implemented to only return false. I think this match what you want already.
Right, platform needs to provide both the helpers arch_tlbbatch_should_defer() and
arch_tlbbatch_add_mm() before ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH can be selected.
Otherwise there is a fallback should_defer_flush() definition which always return
negative when ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH is not selected.
Powered by blists - more mailing lists