From: Guo Ren <guoren@kernel.org> To: Christoph Hellwig <hch@lst.de> Cc: Anup Patel <anup.patel@wdc.com>, Palmer Dabbelt <palmerdabbelt@google.com>, Arnd Bergmann <arnd@arndb.de>, linux-riscv <linux-riscv@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-arch <linux-arch@vger.kernel.org>, linux-sunxi@lists.linux.dev, Guo Ren <guoren@linux.alibaba.com> Subject: Re: [PATCH V3 2/2] riscv: Use use_asid_allocator flush TLB Date: Wed, 26 May 2021 11:12:40 +0800 [thread overview] Message-ID: <CAJF2gTSa3LJKTRBv6NOvg0HtoKPL-5YyP6wY2=AJhQtAZwBBzg@mail.gmail.com> (raw) In-Reply-To: <20210525123556.GB4842@lst.de> On Tue, May 25, 2021 at 8:35 PM Christoph Hellwig <hch@lst.de> wrote: > > On Tue, May 25, 2021 at 12:24:07PM +0000, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Use static_branch_unlikely(&use_asid_allocator) to keep the origin > > tlb flush style, so it's no effect on the existing machine. Here > > are the optimized functions: > > - flush_tlb_mm > > - flush_tlb_page > > - flush_tlb_range > > > > All above are based on the below new implement functions: > > - __sbi_tlb_flush_range_asid > > - local_flush_tlb_range_asid > > > This mentiones what functions you're changing, but not what the > substantial change is, and more importantly why you change it. > > > +static inline void local_flush_tlb_range_asid(unsigned long start, unsigned long size, > > + unsigned long asid) > > Crazy long line. Should be: > > static inline void local_flush_tlb_range_asid(unsigned long start, > unsigned long size, unsigned long asid) > > > +{ > > + unsigned long tmp = start & PAGE_MASK; > > + unsigned long end = ALIGN(start + size, PAGE_SIZE); > > + > > + if (size == -1) { > > + __asm__ __volatile__ ("sfence.vma x0, %0" : : "r" (asid) : "memory"); > > + return; > > Please split the global (size == -1) case into separate helpers. Do you mean: if (size == -1) { __asm__ __volatile__ ("sfence.vma x0, %0" : : "r" (asid) : "memory"); } else { for (tmp = start & PAGE_MASK; tmp < end; tmp += PAGE_SIZE) { __asm__ __volatile__ ("sfence.vma %0, %1" : : "r" (tmp), "r" (asid) : "memory"); tmp += PAGE_SIZE; } } > > > + while(tmp < end) { > > Missing whitespace befre the (. > > Also I think this would read nicer as: > > for (tmp = start & PAGE_MASK; tmp < end; tmp += PAGE_SIZE) > > > +static void __sbi_tlb_flush_range_asid(struct cpumask *cmask, unsigned long start, > > + unsigned long size, unsigned long asid) > > Another overly long line. > > Also for all thee __sbi_* functions, why the __ prefix? I just follow the previous coding convention by __sbi_tlb_flush_range. If you don't like it, I think it should be another coding convention patchset. This patchset is only to add the functions of tlb_flush_with_asid. > > > + if (cpumask_any_but(cmask, cpuid) >= nr_cpu_ids) { > > + local_flush_tlb_range_asid(start, size, asid); > > + } else { > > + riscv_cpuid_to_hartid_mask(cmask, &hmask); > > + sbi_remote_sfence_vma_asid(cpumask_bits(&hmask), start, size, asid); > > Another long line (and a few more later). -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
WARNING: multiple messages have this Message-ID (diff)
From: Guo Ren <guoren@kernel.org> To: Christoph Hellwig <hch@lst.de> Cc: Anup Patel <anup.patel@wdc.com>, Palmer Dabbelt <palmerdabbelt@google.com>, Arnd Bergmann <arnd@arndb.de>, linux-riscv <linux-riscv@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-arch <linux-arch@vger.kernel.org>, linux-sunxi@lists.linux.dev, Guo Ren <guoren@linux.alibaba.com> Subject: Re: [PATCH V3 2/2] riscv: Use use_asid_allocator flush TLB Date: Wed, 26 May 2021 11:12:40 +0800 [thread overview] Message-ID: <CAJF2gTSa3LJKTRBv6NOvg0HtoKPL-5YyP6wY2=AJhQtAZwBBzg@mail.gmail.com> (raw) In-Reply-To: <20210525123556.GB4842@lst.de> On Tue, May 25, 2021 at 8:35 PM Christoph Hellwig <hch@lst.de> wrote: > > On Tue, May 25, 2021 at 12:24:07PM +0000, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Use static_branch_unlikely(&use_asid_allocator) to keep the origin > > tlb flush style, so it's no effect on the existing machine. Here > > are the optimized functions: > > - flush_tlb_mm > > - flush_tlb_page > > - flush_tlb_range > > > > All above are based on the below new implement functions: > > - __sbi_tlb_flush_range_asid > > - local_flush_tlb_range_asid > > > This mentiones what functions you're changing, but not what the > substantial change is, and more importantly why you change it. > > > +static inline void local_flush_tlb_range_asid(unsigned long start, unsigned long size, > > + unsigned long asid) > > Crazy long line. Should be: > > static inline void local_flush_tlb_range_asid(unsigned long start, > unsigned long size, unsigned long asid) > > > +{ > > + unsigned long tmp = start & PAGE_MASK; > > + unsigned long end = ALIGN(start + size, PAGE_SIZE); > > + > > + if (size == -1) { > > + __asm__ __volatile__ ("sfence.vma x0, %0" : : "r" (asid) : "memory"); > > + return; > > Please split the global (size == -1) case into separate helpers. Do you mean: if (size == -1) { __asm__ __volatile__ ("sfence.vma x0, %0" : : "r" (asid) : "memory"); } else { for (tmp = start & PAGE_MASK; tmp < end; tmp += PAGE_SIZE) { __asm__ __volatile__ ("sfence.vma %0, %1" : : "r" (tmp), "r" (asid) : "memory"); tmp += PAGE_SIZE; } } > > > + while(tmp < end) { > > Missing whitespace befre the (. > > Also I think this would read nicer as: > > for (tmp = start & PAGE_MASK; tmp < end; tmp += PAGE_SIZE) > > > +static void __sbi_tlb_flush_range_asid(struct cpumask *cmask, unsigned long start, > > + unsigned long size, unsigned long asid) > > Another overly long line. > > Also for all thee __sbi_* functions, why the __ prefix? I just follow the previous coding convention by __sbi_tlb_flush_range. If you don't like it, I think it should be another coding convention patchset. This patchset is only to add the functions of tlb_flush_with_asid. > > > + if (cpumask_any_but(cmask, cpuid) >= nr_cpu_ids) { > > + local_flush_tlb_range_asid(start, size, asid); > > + } else { > > + riscv_cpuid_to_hartid_mask(cmask, &hmask); > > + sbi_remote_sfence_vma_asid(cpumask_bits(&hmask), start, size, asid); > > Another long line (and a few more later). -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2021-05-26 3:12 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-25 12:24 [PATCH V3 0/2] riscv: Fixup asid_allocator remaining issues guoren 2021-05-25 12:24 ` guoren 2021-05-25 12:24 ` [PATCH V3 1/2] riscv: Fixup _PAGE_GLOBAL in _PAGE_KERNEL guoren 2021-05-25 12:24 ` guoren 2021-05-25 12:28 ` Christoph Hellwig 2021-05-25 12:28 ` Christoph Hellwig 2021-05-25 12:24 ` [PATCH V3 2/2] riscv: Use use_asid_allocator flush TLB guoren 2021-05-25 12:24 ` guoren 2021-05-25 12:35 ` Christoph Hellwig 2021-05-25 12:35 ` Christoph Hellwig 2021-05-26 3:12 ` Guo Ren [this message] 2021-05-26 3:12 ` Guo Ren 2021-05-26 3:12 ` Guo Ren 2021-05-26 5:23 ` Christoph Hellwig 2021-05-26 5:23 ` Christoph Hellwig
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='CAJF2gTSa3LJKTRBv6NOvg0HtoKPL-5YyP6wY2=AJhQtAZwBBzg@mail.gmail.com' \ --to=guoren@kernel.org \ --cc=anup.patel@wdc.com \ --cc=arnd@arndb.de \ --cc=guoren@linux.alibaba.com \ --cc=hch@lst.de \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=linux-sunxi@lists.linux.dev \ --cc=palmerdabbelt@google.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.