Re: [PATCH v7] um: Enable preemption in UML

[Johannes Berg]

> @@ -23,7 +23,7 @@ struct thread_info {
>       int                     preempt_count;  /* 0 => preemptable,
>                                                  <0 => BUG */
>       struct thread_info      *real_thread;    /* Points to non-IRQ stack */
> -     unsigned long aux_fp_regs[FP_SIZE];     /* auxiliary fp_regs to 
> save/restore
> +     unsigned long aux_fp_regs[FP_SIZE] __aligned(64);       /* auxiliary 
> fp_regs to save/restore
>                                                  them out-of-band */

nit: that comment looks strange now, maybe pull it up before the member?

 /* auxiliary ... out-of-band */
 unsigned long aux_fp_regs[...] __aligned(64);


> +#ifdef CONFIG_64BIT
> +     if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT)))
> +             __builtin_ia32_xsaveopt64(&current_thread_info()->aux_fp_regs, 
> KNOWN_387_FEATURES);
> +     else {
> +             if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE)))
> +                     
> __builtin_ia32_xsave64(&current_thread_info()->aux_fp_regs, 
> KNOWN_387_FEATURES);
> +             else
> +                     
> __builtin_ia32_fxsave64(&current_thread_info()->aux_fp_regs);
> +     }

Why not write this as a chain?

        if (likely(cpu_has(...))
                __builtin_ia32_xsaveopt64(...);
        else if (likely(cpu_has(...)))
                __builtin_ia32_xave64(...);
        else
                __builtin_ia32_fxsave64(...);


and IMHO pulling the "&current_thread_info()->aux_fp_regs" that appears
on all three of them into a local variable would make that more readable
too.

> +#else
> +     if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT)))
> +             __builtin_ia32_xsaveopt(&current->aux_fp_regs, 
> KNOWN_387_FEATURES);
> +     else {
> +             if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE)))
> +                     __builtin_ia32_xsave(&current->aux_fp_regs, 
> KNOWN_387_FEATURES);
> +             else
> +                     __builtin_ia32_fxsave(&current->aux_fp_regs);
> +     }
> +#endif

And all of the above also applies for 32-bit,

> +void kernel_fpu_end(void)

and this as well.

johannes