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

[Johannes Berg]

Hi,

Haven't chimed in here before, but thanks for looking at this! I
actually really wanted at least the things you get for debug from this
in the past, so much appreciated.

I think I actually tried a simpler approach and it kind of even worked,
but my userspace likely has very little FPU usage.


> +/* UML knows about 387 features up to and including AVX512, tile, etc are 
> not yet
> + * supported.
> + */

Do you ensure that somehow? Warn if it's there at least? I may have
missed that.

Or is it just for the saving? But what if you don't save enough and the
host CPU has more?


> +void kernel_fpu_begin(void)
> +{
> +#if defined(PREEMPT) || defined(PREEMPT_VOLUNTRARY)

maybe leave it as an inline in case it'd be empty, i.e. neither of these
are defined?

> +     preempt_disable();
> +
> +     WARN_ON(this_cpu_read(in_kernel_fpu));
> +
> +     this_cpu_write(in_kernel_fpu, true);
> +
> +#ifdef CONFIG_64BIT
> +     if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT)))
> +             __builtin_ia32_xsaveopt64(&current->thread.fpu, 
> KNOWN_387_FEATURES);
> +     else {
> +             if (cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))
> +                     __builtin_ia32_xsave64(&current->thread.fpu, 
> KNOWN_387_FEATURES);
> +             else
> +                     __builtin_ia32_fxsave64(&current->thread.fpu);
> +     }

nit: you could make all of these else if chains, and avoid the braces:

if (likely(...))
        ...
else if (cpu_has(...))
        ...
else
        ...


> +void kernel_fpu_end(void)

same comments as above apply here.


>  void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs 
> *regs)
>  {
> +     preempt_disable();
>       _sigio_handler(regs, irqs_suspended);
> +     preempt_enable();
> 

I may have to look at those time-travel bits here :)

Thanks,
johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um