Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1600830 > unrolled thread

Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID

Started by"H. Peter Anvin" <hpa@zytor.com>
First post2017-03-14 20:10 +0100
Last post2017-03-15 10:20 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID "H. Peter Anvin" <hpa@zytor.com> - 2017-03-14 20:10 +0100
    Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID Kyle Huey <me@kylehuey.com> - 2017-03-14 20:10 +0100
      Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID "H. Peter Anvin" <hpa@zytor.com> - 2017-03-14 21:20 +0100
    Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID "H. Peter Anvin" <hpa@zytor.com> - 2017-03-14 20:20 +0100
    Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID Andy Lutomirski <luto@amacapital.net> - 2017-03-14 20:30 +0100
      Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID "H. Peter Anvin" <hpa@zytor.com> - 2017-03-15 10:20 +0100

#1600830 — Re: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID

From"H. Peter Anvin" <hpa@zytor.com>
Date2017-03-14 20:10 +0100
SubjectRe: [PATCH v10 6/7] x86/arch_prctl: Add ARCH_[GET|SET]_CPUID
Message-ID<tkZZv-4D5-15@gated-at.bofh.it>
On 11/08/16 10:39, Kyle Huey wrote:
>  	}
>  
> +	if (test_tsk_thread_flag(prev_p, TIF_NOCPUID) ^
> +	    test_tsk_thread_flag(next_p, TIF_NOCPUID)) {
> +		set_cpuid_faulting(test_tsk_thread_flag(next_p, TIF_NOCPUID));
> +	}
> +
>  	if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^
>  	    test_tsk_thread_flag(next_p, TIF_NOTSC)) {
>  		/* prev and next are different */
>  		if (test_tsk_thread_flag(next_p, TIF_NOTSC))
>  			hard_disable_TSC();
>  		else
>  			hard_enable_TSC();
>  	}

I'm unhappy about this part: we already do two XORs on these after bit
extraction, which is quite inefficient; and at least theoretically we
could be indirecting though the ->stack pointer for every one if gcc
can't tell it won't have changed (we really need to get thread_info
moved into the task_struct allocation and away from the kernel stack,
especially since on x86 the pointer is the same size as the vestigial
structure it points to.)

It would be so much saner to do one xor and then go onto a common slow path:

	struct thread_info *prev_ti = task_thread_info(prev_p);
	struct thread_info *next_ti = task_thread_info(next_p);

	tif_flipped = prev_ti->flags ^ next_ti->flags;

	if (unlikely(tif_flipped &
		(_TIF_BLOCKSTEP | _TIF_NOTSC | _TIF_NOCPUID))) {
		if (tif_flipped & _TIF_BLOCKSTEP) {
			...
		}
		if (tif_flipped & _TIF_NOTSC) {
			...
		}
		if (tif_flipped & _TIF_NOCPUID) {
			...
		}
	}

Then we can also replace test_tsk_thread_flag() with
test_ti_thread_flag() in other places in this function.

	-hpa

[toc] | [next] | [standalone]

#1600833

FromKyle Huey <me@kylehuey.com>
Date2017-03-14 20:10 +0100
Message-ID<tkZZw-4D5-19@gated-at.bofh.it>
In reply to#1600830
On Tue, Mar 14, 2017 at 12:01 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 11/08/16 10:39, Kyle Huey wrote:
>>       }
>>
>> +     if (test_tsk_thread_flag(prev_p, TIF_NOCPUID) ^
>> +         test_tsk_thread_flag(next_p, TIF_NOCPUID)) {
>> +             set_cpuid_faulting(test_tsk_thread_flag(next_p, TIF_NOCPUID));
>> +     }
>> +
>>       if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^
>>           test_tsk_thread_flag(next_p, TIF_NOTSC)) {
>>               /* prev and next are different */
>>               if (test_tsk_thread_flag(next_p, TIF_NOTSC))
>>                       hard_disable_TSC();
>>               else
>>                       hard_enable_TSC();
>>       }
>
> I'm unhappy about this part: we already do two XORs on these after bit
> extraction, which is quite inefficient; and at least theoretically we
> could be indirecting though the ->stack pointer for every one if gcc
> can't tell it won't have changed (we really need to get thread_info
> moved into the task_struct allocation and away from the kernel stack,
> especially since on x86 the pointer is the same size as the vestigial
> structure it points to.)
>
> It would be so much saner to do one xor and then go onto a common slow path:
>
>         struct thread_info *prev_ti = task_thread_info(prev_p);
>         struct thread_info *next_ti = task_thread_info(next_p);
>
>         tif_flipped = prev_ti->flags ^ next_ti->flags;
>
>         if (unlikely(tif_flipped &
>                 (_TIF_BLOCKSTEP | _TIF_NOTSC | _TIF_NOCPUID))) {
>                 if (tif_flipped & _TIF_BLOCKSTEP) {
>                         ...
>                 }
>                 if (tif_flipped & _TIF_NOTSC) {
>                         ...
>                 }
>                 if (tif_flipped & _TIF_NOCPUID) {
>                         ...
>                 }
>         }
>
> Then we can also replace test_tsk_thread_flag() with
> test_ti_thread_flag() in other places in this function.

That's largely what we ended up doing.  See
https://lkml.org/lkml/2017/2/14/80 and the latest version of this
patch, https://lkml.org/lkml/2017/3/11/197.

- Kyle

[toc] | [prev] | [next] | [standalone]

#1600873

From"H. Peter Anvin" <hpa@zytor.com>
Date2017-03-14 21:20 +0100
Message-ID<tl15g-5mY-33@gated-at.bofh.it>
In reply to#1600833
On 03/14/17 12:08, Kyle Huey wrote:
> 
> That's largely what we ended up doing.  See
> https://lkml.org/lkml/2017/2/14/80 and the latest version of this
> patch, https://lkml.org/lkml/2017/3/11/197.
> 

Yes, as I said, my mistake.

I would still like to see an early-out when none of these flags are set
(I just discussed this with tglx on IRC):

if (likely(!((tifp|tifn) &
	(_TIF_BLOCKSTEP|_TIF_NOTSC|_TIF_IO_BITMAP|
         _TIF_NOCPUID|_TIF_USER_RETURN_NOTIFY))))
	return;

The USER_RETURN_NOTIFY could really use some sanity: it is a notifier
chain with a single in-kernel user, which is KVM on x86 only, but we
most likely will need to propagate the flag even if it ends up getting
specialized.

	-hpa

[toc] | [prev] | [next] | [standalone]

#1600835

From"H. Peter Anvin" <hpa@zytor.com>
Date2017-03-14 20:20 +0100
Message-ID<tl09c-4Iz-7@gated-at.bofh.it>
In reply to#1600830
On 03/14/17 12:01, H. Peter Anvin wrote:
> On 11/08/16 10:39, Kyle Huey wrote:
>>  	}
>>  
>> +	if (test_tsk_thread_flag(prev_p, TIF_NOCPUID) ^
>> +	    test_tsk_thread_flag(next_p, TIF_NOCPUID)) {
>> +		set_cpuid_faulting(test_tsk_thread_flag(next_p, TIF_NOCPUID));
>> +	}
>> +
>>  	if (test_tsk_thread_flag(prev_p, TIF_NOTSC) ^
>>  	    test_tsk_thread_flag(next_p, TIF_NOTSC)) {
>>  		/* prev and next are different */
>>  		if (test_tsk_thread_flag(next_p, TIF_NOTSC))
>>  			hard_disable_TSC();
>>  		else
>>  			hard_enable_TSC();
>>  	}
> 
> I'm unhappy about this part: we already do two XORs on these after bit
> extraction, which is quite inefficient; and at least theoretically we
> could be indirecting though the ->stack pointer for every one if gcc
> can't tell it won't have changed (we really need to get thread_info
> moved into the task_struct allocation and away from the kernel stack,
> especially since on x86 the pointer is the same size as the vestigial
> structure it points to.)
> 

Nevermind, I was accidentally looking at v10 not v15 of this patchset.
My bad.

	-hpa

[toc] | [prev] | [next] | [standalone]

#1600846

FromAndy Lutomirski <luto@amacapital.net>
Date2017-03-14 20:30 +0100
Message-ID<tl0iS-4M7-15@gated-at.bofh.it>
In reply to#1600830
On Tue, Mar 14, 2017 at 12:01 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> and at least theoretically we
> could be indirecting though the ->stack pointer for every one if gcc
> can't tell it won't have changed (we really need to get thread_info
> moved into the task_struct allocation and away from the kernel stack,
> especially since on x86 the pointer is the same size as the vestigial
> structure it points to.)

Solved by use of time machine:

commit 15f4eae70d365bba26854c90b6002aaabb18c8aa
Author: Andy Lutomirski <luto@kernel.org>
Date:   Tue Sep 13 14:29:25 2016 -0700

    x86: Move thread_info into task_struct


:)

[toc] | [prev] | [next] | [standalone]

#1601162

From"H. Peter Anvin" <hpa@zytor.com>
Date2017-03-15 10:20 +0100
Message-ID<tldg5-5yN-5@gated-at.bofh.it>
In reply to#1600846
<linux-fsdevel@vger.kernel.org>,"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>,kvm list <kvm@vger.kernel.org>
From: hpa@zytor.com
Message-ID: <E47D06A9-AFDA-4A9D-8539-9CC5AB19B395@zytor.com>

On March 14, 2017 12:23:40 PM PDT, Andy Lutomirski <luto@amacapital.net> wrote:
>On Tue, Mar 14, 2017 at 12:01 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> and at least theoretically we
>> could be indirecting though the ->stack pointer for every one if gcc
>> can't tell it won't have changed (we really need to get thread_info
>> moved into the task_struct allocation and away from the kernel stack,
>> especially since on x86 the pointer is the same size as the vestigial
>> structure it points to.)
>
>Solved by use of time machine:
>
>commit 15f4eae70d365bba26854c90b6002aaabb18c8aa
>Author: Andy Lutomirski <luto@kernel.org>
>Date:   Tue Sep 13 14:29:25 2016 -0700
>
>    x86: Move thread_info into task_struct
>
>
>:)

My apologies, -ESTALEBRAINCACHE...
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web