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

[Anton Ivanov]

On 21/09/2023 10:42, Anton Ivanov wrote:

On 21/09/2023 10:35, Johannes Berg wrote:

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.

We can if we want. We can just cut-off feature reflection into the kernel in the
setup. Presently it reflects all features. That can be filtered if we want to.
For example - x86 has boot flags to disable xrestore and xrestoreopt

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

This is just for the saving. AFAIK no linux module touches _TILE and the other 
recent
fancy stuff. The host kernel source looks that way.

This brings back the question of

1. Alternatives (this has become even more nightmarish since I last looked at 
it)
2. Static branch by key - this actually looks doable and is used by nearly all
crypto, I may look at it when I have some free time.

+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 :)

Yes. I was going to ask you to double check if they all work as intended.

Basic use seems without tt seems OK.

Thanks,
johannes

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

--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/

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