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=-8.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, 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 2D6F4C2D0B1 for ; Thu, 6 Feb 2020 06:06:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BF56120720 for ; Thu, 6 Feb 2020 06:06:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=c-s.fr header.i=@c-s.fr header.b="f4UQnclN" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BF56120720 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=c-s.fr Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 68A526B0007; Thu, 6 Feb 2020 01:06:12 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 63BD76B0008; Thu, 6 Feb 2020 01:06:12 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 529C46B000A; Thu, 6 Feb 2020 01:06:12 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0060.hostedemail.com [216.40.44.60]) by kanga.kvack.org (Postfix) with ESMTP id 373566B0007 for ; Thu, 6 Feb 2020 01:06:12 -0500 (EST) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id D3CED33C4 for ; Thu, 6 Feb 2020 06:06:11 +0000 (UTC) X-FDA: 76458666942.15.way26_18ecc0bf78113 X-HE-Tag: way26_18ecc0bf78113 X-Filterd-Recvd-Size: 10100 Received: from pegase1.c-s.fr (pegase1.c-s.fr [93.17.236.30]) by imf31.hostedemail.com (Postfix) with ESMTP for ; Thu, 6 Feb 2020 06:06:11 +0000 (UTC) Received: from localhost (mailhub1-int [192.168.12.234]) by localhost (Postfix) with ESMTP id 48CnxF0SxrzB09b9; Thu, 6 Feb 2020 07:06:09 +0100 (CET) Authentication-Results: localhost; dkim=pass reason="1024-bit key; insecure key" header.d=c-s.fr header.i=@c-s.fr header.b=f4UQnclN; dkim-adsp=pass; dkim-atps=neutral X-Virus-Scanned: Debian amavisd-new at c-s.fr Received: from pegase1.c-s.fr ([192.168.12.234]) by localhost (pegase1.c-s.fr [192.168.12.234]) (amavisd-new, port 10024) with ESMTP id j05g7TitYtnk; Thu, 6 Feb 2020 07:06:08 +0100 (CET) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase1.c-s.fr (Postfix) with ESMTP id 48CnxD6KPhzB09b8; Thu, 6 Feb 2020 07:06:08 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=c-s.fr; s=mail; t=1580969168; bh=C0dWiGjQX9kyjwbOBhkXPvb2XOD9IYbC4c03jrpHs1A=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=f4UQnclNil89sNzepQ/KrKVg4NcfszxJfzrlLkehSfaKFRPw2BPkZeM4Daaao4PY1 udG6nlPaVH1TJKx72EWbFpUQIyfttZfM8/fTkLc+YYXOSMzrpAnseVxvpbJTdJtDTK MlxAQvTsJrDmzoSqG8nty6MaUGWBUyilk1Mkh+yA= Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id A1D498B78A; Thu, 6 Feb 2020 07:06:09 +0100 (CET) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id zGNEq76t7uhU; Thu, 6 Feb 2020 07:06:09 +0100 (CET) Received: from [192.168.4.90] (unknown [192.168.4.90]) by messagerie.si.c-s.fr (Postfix) with ESMTP id DC8588B776; Thu, 6 Feb 2020 07:06:07 +0100 (CET) Subject: Re: [PATCH v6 06/11] powerpc/mm/book3s64/hash: Use functions to track lockless pgtbl walks To: Leonardo Bras , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Arnd Bergmann , Andrew Morton , "Aneesh Kumar K.V" , Nicholas Piggin , Steven Price , Robin Murphy , Mahesh Salgaonkar , Balbir Singh , Reza Arbab , Thomas Gleixner , Allison Randal , Greg Kroah-Hartman , Mike Rapoport , Michal Suchanek Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-arch@vger.kernel.org, linux-mm@kvack.org References: <20200206030900.147032-1-leonardo@linux.ibm.com> <20200206030900.147032-7-leonardo@linux.ibm.com> From: Christophe Leroy Message-ID: <1b65f1a8-42a8-6ffc-3a06-08fbb34edab5@c-s.fr> Date: Thu, 6 Feb 2020 07:06:07 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: <20200206030900.147032-7-leonardo@linux.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: fr Content-Transfer-Encoding: quoted-printable 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: Le 06/02/2020 =C3=A0 04:08, Leonardo Bras a =C3=A9crit=C2=A0: > Applies the new tracking functions to all hash-related functions that d= o > lockless pagetable walks. >=20 > hash_page_mm: Adds comment that explain that there is no need to > local_int_disable/save given that it is only called from DataAccess > interrupt, so interrupts are already disabled. >=20 > local_irq_{save,restore} is already inside {begin,end}_lockless_pgtbl_w= alk, > so there is no need to repeat it here. >=20 > Variable that saves the irq mask was renamed from flags to irq_mask so = it > doesn't lose meaning now it's not directly passed to local_irq_* functi= ons. >=20 > Signed-off-by: Leonardo Bras > --- > arch/powerpc/mm/book3s64/hash_tlb.c | 6 +++--- > arch/powerpc/mm/book3s64/hash_utils.c | 27 +++++++++++++++++---------= - > 2 files changed, 20 insertions(+), 13 deletions(-) >=20 > diff --git a/arch/powerpc/mm/book3s64/hash_tlb.c b/arch/powerpc/mm/book= 3s64/hash_tlb.c > index 4a70d8dd39cd..86547c4151f6 100644 > --- a/arch/powerpc/mm/book3s64/hash_tlb.c > +++ b/arch/powerpc/mm/book3s64/hash_tlb.c > @@ -194,7 +194,7 @@ void __flush_hash_table_range(struct mm_struct *mm,= unsigned long start, > { > bool is_thp; > int hugepage_shift; > - unsigned long flags; > + unsigned long irq_mask; > =20 > start =3D _ALIGN_DOWN(start, PAGE_SIZE); > end =3D _ALIGN_UP(end, PAGE_SIZE); > @@ -209,7 +209,7 @@ void __flush_hash_table_range(struct mm_struct *mm,= unsigned long start, > * to being hashed). This is not the most performance oriented > * way to do things but is fine for our needs here. > */ > - local_irq_save(flags); > + irq_mask =3D begin_lockless_pgtbl_walk(); > arch_enter_lazy_mmu_mode(); > for (; start < end; start +=3D PAGE_SIZE) { > pte_t *ptep =3D find_current_mm_pte(mm->pgd, start, &is_thp, > @@ -229,7 +229,7 @@ void __flush_hash_table_range(struct mm_struct *mm,= unsigned long start, > hpte_need_flush(mm, start, ptep, pte, hugepage_shift); > } > arch_leave_lazy_mmu_mode(); > - local_irq_restore(flags); > + end_lockless_pgtbl_walk(irq_mask); > } > =20 > void flush_tlb_pmd_range(struct mm_struct *mm, pmd_t *pmd, unsigned l= ong addr) > diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/bo= ok3s64/hash_utils.c > index 523d4d39d11e..e6d4ab42173b 100644 > --- a/arch/powerpc/mm/book3s64/hash_utils.c > +++ b/arch/powerpc/mm/book3s64/hash_utils.c > @@ -1341,12 +1341,16 @@ int hash_page_mm(struct mm_struct *mm, unsigned= long ea, > ea &=3D ~((1ul << mmu_psize_defs[psize].shift) - 1); > #endif /* CONFIG_PPC_64K_PAGES */ > =20 > - /* Get PTE and page size from page tables */ > + /* Get PTE and page size from page tables : > + * Called in from DataAccess interrupt (data_access_common: 0x300), > + * interrupts are disabled here. > + */ Comments formatting is not in line with Linux kernel rules. Should be no=20 text on the first /* line. > + __begin_lockless_pgtbl_walk(false); I think it would be better to not use __begin_lockless_pgtbl_walk()=20 directly but keep it in a single place, and define something like=20 begin_lockless_pgtbl_walk_noirq() similar to begin_lockless_pgtbl_walk() > ptep =3D find_linux_pte(pgdir, ea, &is_thp, &hugeshift); > if (ptep =3D=3D NULL || !pte_present(*ptep)) { > DBG_LOW(" no PTE !\n"); > rc =3D 1; > - goto bail; > + goto bail_pgtbl_walk; What's the point in changing the name of this label ? There is only one=20 label, why polute the function with so huge name ? For me, everyone understand what 'bail' means. Unneccessary changes=20 should be avoided. If you really really want to do it, it should be=20 another patch. See kernel codying style, chapter 'naming': "LOCAL variable names should be short". This also applies to labels. "C is a Spartan language, and so should your naming be. Unlike Modula-2=20 and Pascal programmers, C programmers do not use cute names like=20 ThisVariableIsATemporaryCounter. A C programmer would call that variable=20 tmp, which is much easier to write, and not the least more difficult to=20 understand." > } > =20 > /* Add _PAGE_PRESENT to the required access perm */ > @@ -1359,7 +1363,7 @@ int hash_page_mm(struct mm_struct *mm, unsigned l= ong ea, > if (!check_pte_access(access, pte_val(*ptep))) { > DBG_LOW(" no access !\n"); > rc =3D 1; > - goto bail; > + goto bail_pgtbl_walk; > } > =20 > if (hugeshift) { > @@ -1383,7 +1387,7 @@ int hash_page_mm(struct mm_struct *mm, unsigned l= ong ea, > if (current->mm =3D=3D mm) > check_paca_psize(ea, mm, psize, user_region); > =20 > - goto bail; > + goto bail_pgtbl_walk; > } > =20 > #ifndef CONFIG_PPC_64K_PAGES > @@ -1457,6 +1461,8 @@ int hash_page_mm(struct mm_struct *mm, unsigned l= ong ea, > #endif > DBG_LOW(" -> rc=3D%d\n", rc); > =20 > +bail_pgtbl_walk: > + __end_lockless_pgtbl_walk(0, false); > bail: > exception_exit(prev_state); > return rc; > @@ -1545,7 +1551,7 @@ static void hash_preload(struct mm_struct *mm, un= signed long ea, > unsigned long vsid; > pgd_t *pgdir; > pte_t *ptep; > - unsigned long flags; > + unsigned long irq_mask; > int rc, ssize, update_flags =3D 0; > unsigned long access =3D _PAGE_PRESENT | _PAGE_READ | (is_exec ? _PA= GE_EXEC : 0); > =20 > @@ -1567,11 +1573,12 @@ static void hash_preload(struct mm_struct *mm, = unsigned long ea, > vsid =3D get_user_vsid(&mm->context, ea, ssize); > if (!vsid) > return; > + Is this new line related to the patch ? > /* > * Hash doesn't like irqs. Walking linux page table with irq disable= d > * saves us from holding multiple locks. > */ > - local_irq_save(flags); > + irq_mask =3D begin_lockless_pgtbl_walk(); > =20 > /* > * THP pages use update_mmu_cache_pmd. We don't do > @@ -1616,7 +1623,7 @@ static void hash_preload(struct mm_struct *mm, un= signed long ea, > mm_ctx_user_psize(&mm->context), > pte_val(*ptep)); > out_exit: > - local_irq_restore(flags); > + end_lockless_pgtbl_walk(irq_mask); > } > =20 > /* > @@ -1679,16 +1686,16 @@ u16 get_mm_addr_key(struct mm_struct *mm, unsig= ned long address) > { > pte_t *ptep; > u16 pkey =3D 0; > - unsigned long flags; > + unsigned long irq_mask; > =20 > if (!mm || !mm->pgd) > return 0; > =20 > - local_irq_save(flags); > + irq_mask =3D begin_lockless_pgtbl_walk(); > ptep =3D find_linux_pte(mm->pgd, address, NULL, NULL); > if (ptep) > pkey =3D pte_to_pkey_bits(pte_val(READ_ONCE(*ptep))); > - local_irq_restore(flags); > + end_lockless_pgtbl_walk(irq_mask); > =20 > return pkey; > } >=20 Christophe