linux-kernel - Re: [PATCH v5 3/8] x86/mm/tlb: Open-code on_each_cpu_cond_mask() for tlb_is_not

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]

Message-ID: <29878BDA-1B02-4EAB-A7D3-5FEFCC73B2BE@vmware.com>
Date:   Thu, 18 Feb 2021 08:24:23 +0000
From:   Nadav Amit <namit@...are.com>
To:     Christoph Hellwig <hch@...radead.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Rik van Riel <riel@...riel.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH v5 3/8] x86/mm/tlb: Open-code on_each_cpu_cond_mask() for
 tlb_is_not_lazy()

> On Feb 18, 2021, at 12:16 AM, Christoph Hellwig <hch@...radead.org> wrote:
> 
> On Tue, Feb 09, 2021 at 02:16:48PM -0800, Nadav Amit wrote:
>> +		/*
>> +		 * Although we could have used on_each_cpu_cond_mask(),
>> +		 * open-coding it has performance advantages, as it eliminates
>> +		 * the need for indirect calls or retpolines. In addition, it
>> +		 * allows to use a designated cpumask for evaluating the
>> +		 * condition, instead of allocating one.
>> +		 *
>> +		 * This code works under the assumption that there are no nested
>> +		 * TLB flushes, an assumption that is already made in
>> +		 * flush_tlb_mm_range().
>> +		 *
>> +		 * cond_cpumask is logically a stack-local variable, but it is
>> +		 * more efficient to have it off the stack and not to allocate
>> +		 * it on demand. Preemption is disabled and this code is
>> +		 * non-reentrant.
>> +		 */
>> +		struct cpumask *cond_cpumask = this_cpu_ptr(&flush_tlb_mask);
>> +		int cpu;
>> +
>> +		cpumask_clear(cond_cpumask);
>> +
>> +		for_each_cpu(cpu, cpumask) {
>> +			if (tlb_is_not_lazy(cpu))
>> +				__cpumask_set_cpu(cpu, cond_cpumask);
>> +		}
>> +		smp_call_function_many(cond_cpumask, flush_tlb_func, (void *)info, 1);
> 
> No need for the cast here, which would also avoid the pointlessly
> overly long line.

Actually, there is - to remove the const qualifier. You might argue it is
ugly, but that’s the way it is also how it is done right now.

In general, thanks for the feedback (I will reply after I follow your
feedback). I do have a general question - I thought it was decided that
clarity should be preferred over following the 80-column limit. Please let
me know if I misunderstood.