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] [day] [month] [year] [list]

Date:   Wed, 3 Jul 2019 23:30:31 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Nadav Amit <namit@...are.com>
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 Wed, 3 Jul 2019, Nadav Amit wrote:
> > On Jul 3, 2019, at 1:34 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
> > 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... ;-)

Then I would have hunted some other bug probably :)
 
> >> 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

Indeed, misread the code and right the bit search should be fast.

> 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.

Yes, that alloc/free part is completely bogus.

Thanks,

	tglx