From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B29D0C43331 for ; Thu, 2 Apr 2020 11:24:18 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6F2B8206F8 for ; Thu, 2 Apr 2020 11:24:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6F2B8206F8 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 1E6958E0009; Thu, 2 Apr 2020 07:24:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1970C8E0007; Thu, 2 Apr 2020 07:24:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0D4F28E0009; Thu, 2 Apr 2020 07:24:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0031.hostedemail.com [216.40.44.31]) by kanga.kvack.org (Postfix) with ESMTP id EBA898E0007 for ; Thu, 2 Apr 2020 07:24:17 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id AED5E52D6 for ; Thu, 2 Apr 2020 11:24:17 +0000 (UTC) X-FDA: 76662681354.01.curve24_5f9773c33f02b X-HE-Tag: curve24_5f9773c33f02b X-Filterd-Recvd-Size: 8777 Received: from huawei.com (szxga04-in.huawei.com [45.249.212.190]) by imf31.hostedemail.com (Postfix) with ESMTP for ; Thu, 2 Apr 2020 11:24:16 +0000 (UTC) Received: from DGGEMS409-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 30743A876D0C1A482D0C; Thu, 2 Apr 2020 19:24:13 +0800 (CST) Received: from [127.0.0.1] (10.173.220.25) by DGGEMS409-HUB.china.huawei.com (10.3.19.209) with Microsoft SMTP Server id 14.3.487.0; Thu, 2 Apr 2020 19:24:06 +0800 Subject: Re: [RFC PATCH v5 4/8] mm: tlb: Pass struct mmu_gather to flush_pmd_tlb_range To: Peter Zijlstra CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , References: <20200331142927.1237-1-yezhenyu2@huawei.com> <20200331142927.1237-5-yezhenyu2@huawei.com> <20200331151331.GS20730@hirez.programming.kicks-ass.net> <20200401122004.GE20713@hirez.programming.kicks-ass.net> From: Zhenyu Ye Message-ID: <53675fb9-21c7-5309-07b8-1bbc1e775f9b@huawei.com> Date: Thu, 2 Apr 2020 19:24:04 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: <20200401122004.GE20713@hirez.programming.kicks-ass.net> Content-Type: text/plain; charset="gbk" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.173.220.25] X-CFilter-Loop: Reflected X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi Peter, On 2020/4/1 20:20, Peter Zijlstra wrote: > On Wed, Apr 01, 2020 at 04:51:15PM +0800, Zhenyu Ye wrote: >> On 2020/3/31 23:13, Peter Zijlstra wrote: > >>> Instead of trying to retro-fit flush_*tlb_range() to take an mmu_gather >>> parameter, please replace them out-right. >>> >> >> I'm sorry that I'm not sure what "replace them out-right" means. Do you >> mean that I should define flush_*_tlb_range like this? >> >> #define flush_pmd_tlb_range(vma, addr, end) \ >> do { \ >> struct mmu_gather tlb; \ >> tlb_gather_mmu(&tlb, (vma)->vm_mm, addr, end); \ >> tlba.cleared_pmds = 1; \ >> flush_tlb_range(&tlb, vma, addr, end); \ >> tlb_finish_mmu(&tlb, addr, end); \ >> } while (0) >> > > I was thinking to remove flush_*tlb_range() entirely (from generic > code). > > And specifically to not use them like the above; instead extend the > mmu_gather API. > > Specifically, if you wanted to express flush_pmd_tlb_range() in mmu > gather, you'd write it like: > > static inline void flush_pmd_tlb_range(struct vm_area_struct *vma, unsigned long addr, unsigned long end) > { > struct mmu_gather tlb; > > tlb_gather_mmu(&tlb, vma->vm_mm, addr, end); > tlb_start_vma(&tlb, vma); > tlb.cleared_pmds = 1; > __tlb_adjust_range(addr, end - addr); > tlb_end_vma(&tlb, vma); > tlb_finish_mmu(&tlb, addr, end); > } > > Except of course, that the code between start_vma and end_vma is not a > proper mmu_gather API. > > So maybe add: > > tlb_flush_{pte,pmd,pud,p4d}_range() > > Then we can write: > > static inline void flush_XXX_tlb_range(struct vm_area_struct *vma, unsigned long addr, unsigned long end) > { > struct mmu_gather tlb; > > tlb_gather_mmu(&tlb, vma->vm_mm, addr, end); > tlb_start_vma(&tlb, vma); > tlb_flush_XXX_range(&tlb, addr, end - addr); > tlb_end_vma(&tlb, vma); > tlb_finish_mmu(&tlb, addr, end); > } > > But when I look at the output of: > > git grep flush_.*tlb_range -- :^arch/ > > I doubt it makes sense to provide wrappers like the above. > Thanks for your detailed explanation. I notice that you used `tlb_end_vma` replace `flush_tlb_range`, which will call `tlb_flush`, then finally call `flush_tlb_range` in generic code. However, some architectures define tlb_end_vma|tlb_flush|flush_tlb_range themselves, so this may cause problems. For example, in s390, it defines: #define tlb_end_vma(tlb, vma) do { } while (0) And it doesn't define it's own flush_pmd_tlb_range(). So there will be a mistake if we changed flush_pmd_tlb_range() using tlb_end_vma(). Is this really a problem or something I understand wrong ? If true, I think there are three ways to solve this problem: 1. use `flush_tlb_range` rather than `tlb_end_vma` in flush_XXX_tlb_range; In this way, we still need retro-fit `flush_tlb_range` to take an mmu_gather parameter. 2. use `tlb_flush` rather than `tlb_end_vma`. There is a constraint such like: #ifndef tlb_flush #if defined(tlb_start_vma) || defined(tlb_end_vma) #error Default tlb_flush() relies on default tlb_start_vma() and tlb_end_vma() #endif So all architectures that define tlb_{start|end}_vma have defined tlb_flush. Also, we can add a constraint to flush_XXX_tlb_range such like: #ifndef flush_XXX_tlb_range #if defined(tlb_start_vma) || defined(tlb_end_vma) #error Default flush_XXX_tlb_range() relies on default tlb_start/end_vma() #endif 3. Define flush_XXX_tlb_range() architecture-self, and keep original define in generic code, such as: In arm64: #define flush_XXX_tlb_range flush_XXX_tlb_range In generic: #ifndef flush_XXX_tlb_range #define flush_XXX_tlb_range flush_tlb_range Which do you think is more appropriate? > ( Also, we should probably remove the (addr, end) arguments from > tlb_finish_mmu(), Will? ) > This can be changed quickly. If you want I can do this with a separate patch. > --- > diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h > index f391f6b500b4..be5452a8efaa 100644 > --- a/include/asm-generic/tlb.h > +++ b/include/asm-generic/tlb.h > @@ -511,6 +511,34 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm > } > #endif > > +static inline void tlb_flush_pte_range(struct mmu_gather *tlb, > + unsigned long address, unsigned long size) > +{ > + __tlb_adjust_range(tlb, address, size); > + tlb->cleared_ptes = 1; > +} > + > +static inline void tlb_flush_pmd_range(struct mmu_gather *tlb, > + unsigned long address, unsigned long size) > +{ > + __tlb_adjust_range(tlb, address, size); > + tlb->cleared_pmds = 1; > +} > + > +static inline void tlb_flush_pud_range(struct mmu_gather *tlb, > + unsigned long address, unsigned long size) > +{ > + __tlb_adjust_range(tlb, address, size); > + tlb->cleared_puds = 1; > +} > + > +static inline void tlb_flush_p4d_range(struct mmu_gather *tlb, > + unsigned long address, unsigned long size) > +{ > + __tlb_adjust_range(tlb, address, size); > + tlb->cleared_p4ds = 1; > +} > + By the way, I think the name of tlb_set_XXX_range() is more suitable, because we don't do actual flush there. > #ifndef __tlb_remove_tlb_entry > #define __tlb_remove_tlb_entry(tlb, ptep, address) do { } while (0) > #endif > @@ -524,8 +552,7 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm > */ > #define tlb_remove_tlb_entry(tlb, ptep, address) \ > do { \ > - __tlb_adjust_range(tlb, address, PAGE_SIZE); \ > - tlb->cleared_ptes = 1; \ > + tlb_flush_pte_range(tlb, address, PAGE_SIZE); \ > __tlb_remove_tlb_entry(tlb, ptep, address); \ > } while (0) > > @@ -550,8 +577,7 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm > > #define tlb_remove_pmd_tlb_entry(tlb, pmdp, address) \ > do { \ > - __tlb_adjust_range(tlb, address, HPAGE_PMD_SIZE); \ > - tlb->cleared_pmds = 1; \ > + tlb_flush_pmd_range(tlb, address, HPAGE_PMD_SIZE); \ > __tlb_remove_pmd_tlb_entry(tlb, pmdp, address); \ > } while (0) > > @@ -565,8 +591,7 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm > > #define tlb_remove_pud_tlb_entry(tlb, pudp, address) \ > do { \ > - __tlb_adjust_range(tlb, address, HPAGE_PUD_SIZE); \ > - tlb->cleared_puds = 1; \ > + tlb_flush_pud_range(tlb, address, HPAGE_PUD_SIZE); \ > __tlb_remove_pud_tlb_entry(tlb, pudp, address); \ > } while (0) > > > . >