On 1/10/20 9:49 am, Singh, Balbir wrote:
> On 1/10/20 7:38 am, Thomas Gleixner wrote:
>
>>
>>
>>
>> On Wed, Sep 30 2020 at 20:35, Peter Zijlstra wrote:
>>> On Wed, Sep 30, 2020 at 08:00:59PM +0200, Thomas Gleixner wrote:
>>>> On Wed, Sep 30 2020 at 19:03, Peter Zijlstra wrote:
>>>>> On Wed, Sep 30, 2020 at 05:40:08PM +0200, Thomas Gleixner wrote:
>>>>> Also, that preempt_disable() in there doesn't actually do anything.
>>>>> Worse, preempt_disable(); for_each_cpu(); is an anti-pattern. It mixes
>>>>> static_cpu_has() and boot_cpu_has() in the same bloody condition and has
>>>>> a pointless ret variable.
>>>
>>> Also, I forgot to add, it accesses ->cpus_mask without the proper
>>> locking, so it could be reading intermediate state from whatever cpumask
>>> operation that's in progress.
>>
>> Yes. I saw that after hitting send. :(
>>
>>>> I absolutely agree and I really missed it when looking at it before
>>>> merging. cpus_read_lock()/unlock() is the right thing to do if at all.
>>>>
>>>>> It's shoddy code, that only works if you align the planets right. We
>>>>> really shouldn't provide interfaces that are this bad.
>>>>>
>>>>> It's correct operation is only by accident.
>>>>
>>>> True :(
>>>>
>>>> I understand Balbirs problem and it makes some sense to provide a
>>>> solution. We can:
>>>>
>>>> 1) reject set_affinity() if the task has that flush muck enabled
>>>> and user space tries to move it to a SMT enabled core
>>>>
>>>> 2) disable the muck if if detects that it is runs on a SMT enabled
>>>> core suddenly (hotplug says hello)
>>>>
>>>> This one is nasty because there is no feedback to user space
>>>> about the wreckage.
>>>
>>> That's and, right, not or. because 1) deals with sched_setffinity()
>>> and 2) deals wit hotplug.
>>
>> It was meant as AND of course.
>>
>>> Now 1) requires an arch hook in sched_setaffinity(), something I'm not
>>> keen on providing, once we provide it, who knows what strange and
>>> wonderful things archs will dream up.
>>
>> I don't think so. We can have that magic in core:
>>
>> #ifdef CONFIG_HAS_PARANOID_L1D_FLUSH
>> static bool paranoid_l1d_valid(struct task_struct *tsk,
>> const struct cpumask *msk)
>> {
>> if (!test_tsk_thread_flag(tsk, TIF_SPEC_L1D_FLUSH))
>> return true;
>> /* Do magic stuff */
>> return res;
>> }
>> #else
>> static bool paranoid_l1d_valid(struct task_struct *tsk,
>> const struct cpumask *msk)
>> {
>> return true;
>> }
>> #endif
>>
>> It's a pretty well defined problem and having the magic in core code
>> prevents an arch hook which allows abuse of all sorts.
>>
>> And the same applies to enable_l1d_flush_for_task(). The only
>> architecture specific nonsense are the checks whether the CPU bug is
>> there and whether the hardware supports L1D flushing.
>>
>> So we can have:
>>
>> #ifdef CONFIG_HAS_PARANOID_L1D_FLUSH
>> int paranoid_l1d_enable(struct task_struct *tsk)
>> {
>> /* Do the SMT validation under the proper locks */
>> if (!res)
>> set_task_thread_flag(tsk, TIF_SPEC_L1D_FLUSH);
>> return res;
>> }
>> #endif
>>
>>> And 2) also happens on hot-un-plug, when the task's affinity gets
>>> forced because it became empty. No user feedback there either, and
>>> information is lost.
>>
>> Of course. It's both that suddenly SMT gets enabled on a core which was
>> isolated and when the last isolated core in the tasks CPU mask goes
>> offline.
>>
>>> I suppose we can do 2) but send a signal. That would cover all cases and
>>> keep it in arch code. But yes, that's pretty terrible too.
>>
>> Bah. I just looked at the condition to flush:
>>
>> if (sched_smt_active() && !this_cpu_read(cpu_info.smt_active) &&
>> (prev_mm & LAST_USER_MM_L1D_FLUSH))
>> l1d_flush_hw();
>>
>> That fails to flush when SMT is disabled globally. Balbir?
>>
>> Of course this should be:
>>
>> if (!this_cpu_read(cpu_info.smt_active) && (prev_mm & LAST_USER_MM_L1D_FLUSH))
>> l1d_flush_hw();
>>
>> Now we can make this:
>>
>> if (unlikely(prev_mm & LAST_USER_MM_L1D_FLUSH)) {
>> if (!this_cpu_read(cpu_info.smt_active))
>> l1d_flush_hw();
>> else
>> task_work_add(...);
>>
>> And that task work clears the flag and sends a signal. We're not going
>> to send a signal from switch_mm() ....
>>
>> Thanks,
>>
>
>
> So this is the change I am playing with, I don't like the idea of killing the task, but it's better than silently not flushing, I guess system administrators will learn with time not to correctly the affinity of tasks flushing
> L1D. For the affinity bits, not being able to change the affinity is better, but not being able to provide feedback on as to why is a bit weird as well, but I wonder if there are other cases where we might want to lock the affinity of a task for it's lifetime.
>
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 6b0f4c88b07c..6b0d0a9cd447 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -320,26 +320,15 @@ int enable_l1d_flush_for_task(struct task_struct *tsk)
>
> /*
> * Do not enable L1D_FLUSH_OUT if
> - * b. The CPU is not affected by the L1TF bug
> - * c. The CPU does not have L1D FLUSH feature support
> - * c. The task's affinity is on cores with SMT on.
> + * a. The CPU is not affected by the L1TF bug
> + * b. The CPU does not have L1D FLUSH feature support
> */
>
> if (!boot_cpu_has_bug(X86_BUG_L1TF) ||
> - !static_cpu_has(X86_FEATURE_FLUSH_L1D))
> + !boot_cpu_has(X86_FEATURE_FLUSH_L1D))
> return -EINVAL;
>
> - cpu = get_cpu();
> -
> - for_each_cpu(i, &tsk->cpus_mask) {
> - if (cpu_data(i).smt_active == true) {
> - put_cpu();
> - return -EINVAL;
> - }
> - }
> -
> set_ti_thread_flag(&tsk->thread_info, TIF_SPEC_L1D_FLUSH);
> - put_cpu();
> return ret;
> }
>
> @@ -349,6 +338,12 @@ int disable_l1d_flush_for_task(struct task_struct *tsk)
> return 0;
> }
>
> +static void l1d_flush_kill(struct callback_head *ch)
> +{
> + clear_ti_thread_flag(¤t->thread_info, TIF_SPEC_L1D_FLUSH);
> + force_signal(SIGBUS);
> +}
> +
> void switch_mm(struct mm_struct *prev, struct mm_struct *next,
> struct task_struct *tsk)
> {
> @@ -443,12 +438,14 @@ static void cond_mitigation(struct task_struct *next)
> }
>
> /*
> - * Flush only if SMT is disabled as per the contract, which is checked
> - * when the feature is enabled.
> + * Flush only if SMT is disabled, if flushing is enabled
> + * and we are on an SMT enabled core, kill the task
> */
> - if (sched_smt_active() && !this_cpu_read(cpu_info.smt_active) &&
> - (prev_mm & LAST_USER_MM_L1D_FLUSH))
> - l1d_flush_hw();
> + if (unlikely(prev_mm & LAST_USER_MM_L1D_FLUSH)) {
> + if (!this_cpu_read(cpu_info.smt_active))
> + l1d_flush_hw();
> + else
> + task_work_add(prev, l1d_flush_kill, true);
We have no access the to the previous task and mm->owner depends on MEMCG :)
We can do the magic in mm_mangle_tif_spec_bits(), I suppose
Balbir
