linux-kernel - Re: [patch 16/18] x86/apic: Convert 32bit to IPI shorthand static key

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:   Wed, 3 Jul 2019 21:14:48 +0000
From:   Nadav Amit <namit@...are.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
        Stephane Eranian <eranian@...gle.com>,
        Feng Tang <feng.tang@...el.com>
Subject: Re: [patch 16/18] x86/apic: Convert 32bit to IPI shorthand static key

> On Jul 3, 2019, at 1:34 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
> 
> Nadav,
> 
> On Wed, 3 Jul 2019, Nadav Amit wrote:
>>> On Jul 3, 2019, at 3:54 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
>>> void default_send_IPI_all(int vector)
>>> {
>>> -	if (apic_ipi_shorthand_off || vector == NMI_VECTOR) {
>>> +	if (static_branch_likely(&apic_use_ipi_shorthand)) {
>>> 		apic->send_IPI_mask(cpu_online_mask, vector);
>>> 	} else {
>>> 		__default_send_IPI_shortcut(APIC_DEST_ALLINC, vector);
>> 
>> It may be better to check the static-key in native_send_call_func_ipi() (and
>> other callers if there are any), and remove all the other checks in
>> default_send_IPI_all(), x2apic_send_IPI_mask_allbutself(), etc.
> 
> That makes sense. Should have thought about that myself, but hunting that
> APIC emulation issue was affecting my brain obviously :)

Well, if you used VMware and not KVM... ;-)

(allow me to reemphasize that I am joking and save myself from spam)

>> void native_send_call_func_ipi(const struct cpumask *mask)
>> {
>> -	cpumask_var_t allbutself;
>> -
>> -	if (!alloc_cpumask_var(&allbutself, GFP_ATOMIC)) {
>> -		apic->send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
>> -		return;
>> +	int cpu, this_cpu = smp_processor_id();
>> +	bool allbutself = true;
>> +	bool self = false;
>> +
>> +	for_each_cpu_and_not(cpu, cpu_online_mask, mask) {
>> +
>> +		if (cpu != this_cpu) {
>> +			allbutself = false;
>> +			break;
>> +		}
>> +		self = true;
> 
> That accumulates to a large iteration in the worst case. 

I don’t understand why. There should be at most two iterations - one for
self and one for another core. So _find_next_bit() will be called at most
twice. _find_next_bit() has its own loop, but I don’t think overall it is as
bad as calling alloc_cpumask_var(), cpumask_copy() and cpumask_equal(),
which also have loops.

I don’t have numbers (and I doubt they are very significant), but the cpumask
allocation showed when I was profiling my microbenchmark.