linux-kernel - Re: [v2 PATCH] RISC-V: Optimize tlb flush path.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Aug 2019 08:42:19 +0000
From:   Atish Patra <Atish.Patra@....com>
To:     "schwab@...ux-m68k.org" <schwab@...ux-m68k.org>
CC:     "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
        "paul.walmsley@...ive.com" <paul.walmsley@...ive.com>,
        "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
        "allison@...utok.net" <allison@...utok.net>,
        "anup@...infault.org" <anup@...infault.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hch@...radead.org" <hch@...radead.org>,
        "palmer@...ive.com" <palmer@...ive.com>
Subject: Re: [v2 PATCH] RISC-V: Optimize tlb flush path.

On Tue, 2019-08-20 at 09:46 +0200, Andreas Schwab wrote:
> On Aug 19 2019, Atish Patra <atish.patra@....com> wrote:
> 
> > @@ -42,20 +43,44 @@ static inline void flush_tlb_range(struct
> > vm_area_struct *vma,
> >  
> >  #include <asm/sbi.h>
> >  
> > -static inline void remote_sfence_vma(struct cpumask *cmask,
> > unsigned long start,
> > -				     unsigned long size)
> > +static void __riscv_flush_tlb(struct cpumask *cmask, unsigned long
> > start,
> > +			      unsigned long size)
> 
> cmask can be const.
> 

Sure. Will update it.

> >  {
> >  	struct cpumask hmask;
> > +	unsigned int hartid;
> > +	unsigned int cpuid;
> >  
> >  	cpumask_clear(&hmask);
> > +
> > +	if (!cmask) {
> > +		riscv_cpuid_to_hartid_mask(cpu_online_mask, &hmask);
> > +		goto issue_sfence;
> > +	}
> 
> Wouldn't it make sense to fall through to doing a local flush here?
> 
> 	if (!cmask)
> 		cmask = cpu_online_mask;
> 

cmask NULL is pretty common case and we would  be unnecessarily
executing bunch of instructions everytime while not saving much. Kernel
still have to make an SBI call and OpenSBI is doing a local flush
anyways.

Looking at the code again, I think we can just use cpumask_weight and
do local tlb flush only if local cpu is the only cpu present. 

Otherwise, it will just fall through and call sbi_remote_sfence_vma().

....
....
+
+       cpuid = get_cpu();
+	if (!cmask) {
+               riscv_cpuid_to_hartid_mask(cpu_online_mask, &hmask);
+               goto issue_sfence;
+       }
+
+       
+       if (cpumask_test_cpu(cpuid, cmask) && cpumask_weight(cmask) ==
1) {
+               /* Save trap cost by issuing a local tlb flush here */
+               if ((start == 0 && size == -1) || (size > PAGE_SIZE))
+                       local_flush_tlb_all();
+               else if (size == PAGE_SIZE)
+                       local_flush_tlb_page(start);
+               goto done;
+       }
+
        riscv_cpuid_to_hartid_mask(cmask, &hmask);
+
+issue_sfence:
        sbi_remote_sfence_vma(hmask.bits, start, size);
+done:
+       put_cpu();
 }

This is much simpler than what I had done in v2. I will address the if
condition around size as well.

Regards,
Atish
> Andreas.
>