Re: [PATCH] RISC-V: Issue a local tlb flush if possible.

[Atish Patra]

On Tue, 2019-08-13 at 07:30 -0700, hch@xxxxxxxxxxxxx wrote:
> On Tue, Aug 13, 2019 at 12:15:15AM +0000, Atish Patra wrote:
> > I thought if it recieves an empty cpumask, then it should at least
> > do a
> > local flush is the expected behavior. Are you saying that we should
> > just skip all and return ? 
> 
> How could we ever receive an empty cpu mask?  I think it could only
> be empty without the current cpu.  At least that is my reading of
> the callers and a few other implementations.
> 

We get ton of them. Here is the stack dump.

[   16.735814] [<ffffffe000035498>] walk_stackframe+0x0/0xa0^M
298436 [   16.819037] [<ffffffe0000355f8>] show_stack+0x2a/0x34^M
298437 [   16.899648] [<ffffffe00067b54c>] dump_stack+0x62/0x7c^M
298438 [   16.977402] [<ffffffe0000ef6f6>] tlb_flush_mmu+0x14a/0x150^M
298439 [   17.054197] [<ffffffe0000ef7a4>] tlb_finish_mmu+0x42/0x72^M
298440 [   17.129986] [<ffffffe0000ede7c>] exit_mmap+0x8e/0xfa^M
298441 [   17.203669] [<ffffffe000037d54>] mmput.part.3+0x1a/0xc4^M
298442 [   17.275985] [<ffffffe000037e1e>] mmput+0x20/0x28^M
298443 [   17.345248] [<ffffffe0001143c2>] flush_old_exec+0x418/0x5f8^M
298444 [   17.415370] [<ffffffe000158408>]
load_elf_binary+0x262/0xc84^M
298445 [   17.483641] [<ffffffe000114614>]
search_binary_handler.part.7+0x72/0x172^M
298446 [   17.552078] [<ffffffe000114bb2>]
__do_execve_file+0x40c/0x56a^M
298447 [   17.617932] [<ffffffe00011503e>] sys_execve+0x26/0x32^M
298448 [   17.682164] [<ffffffe00003437e>] ret_from_syscall+0x0/0xe^M

It looks like it is in the path of clearing the old traces of already
run script or program.  I am not sure if the cpumask supposed to be
empty in this path.

Probably we should just issue tlb flush all for all CPUs instead of
just the local CPU.

> > > 	if (!cpumask || cpumask_test_cpu(cpu, cpumask) {
> > > 		if ((start == 0 && size == -1) || size > PAGE_SIZE)
> > > 			local_flush_tlb_all();
> > > 		else if (size == PAGE_SIZE)
> > > 			local_flush_tlb_page(start);
> > > 		cpumask_clear_cpu(cpuid, cpumask);
> > 
> > This will modify the original cpumask which is not correct. clear
> > part
> > has to be done on hmask to avoid a copy.
> 
> Indeed.  But looking at the x86 tlbflush implementation it seems like
> we
> could use cpumask_any_but() to avoid having to modify the passed in
> cpumask.

Looking at the x86 code, it uses cpumask_any_but to just test if there
any other cpu present apart from the current one.

If yes, it calls smp_call_function_many which ignores the current cpu
and execute tlb flush code on all other cpus.

For RISC-V, it has to still send the cpumask containing local cpu and
M-mode software may do a local tlb flush the tlbs again for no reason.


Regards,
Atish