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

[Anton Ivanov]

On 22/09/2023 08:30, Johannes Berg wrote:

On Fri, 2023-09-22 at 07:52 +0100, anton.iva...@cambridgegreys.com
wrote:

+++ b/arch/um/include/asm/processor-generic.h
@@ -44,6 +44,9 @@ struct thread_struct {
                        } cb;
                } u;
        } request;
+#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY)
+    u8 fpu[2048] __aligned(64); /* Intel docs require xsave/xrestore area to 
be aligned to 64 bytes */
+#endif

Looks like you used spaces instead of tabs in a few places such as here.

Ack. I am not sure how they got there.

My environment is configured to use tabs when working on the kernel tree.

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

Still think the else if chains would look better, but it also doesn't
matter much.

        mm = &init_mm;
        hvc = INIT_HVC(mm, force, userspace);
+
+    preempt_disable();

Also here spaces instead of tabs. Interesting you display tabs as 4
spaces when the kernel really does everything with tabs being 8 spaces
wide :)

But anyway that's all nitpicking, the real problem I found when running
this now was this:

BUG: sleeping function called from invalid context at 
kernel/locking/rwsem.c:1519
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 282, name: startup.sh
preempt_count: 2, expected: 0
no locks held by startup.sh/282.
irq event stamp: 0
hardirqs last  enabled at (0): [<0000000000000000>] 0x0
hardirqs last disabled at (0): [<0000000060044b82>] copy_process+0xa02/0x244e
softirqs last  enabled at (0): [<0000000060044b82>] copy_process+0xa02/0x244e
softirqs last disabled at (0): [<0000000000000000>] 0x0
CPU: 0 PID: 282 Comm: startup.sh Not tainted 6.6.0-rc1 #147
Stack:
  7229be60 60500273 00000002 6003cfc9
  606bd782 00000000 60b3e968 00000000
  7229bea0 60526312 00000081 00000000
Call Trace:
  [<6051cbaa>] ? _printk+0x0/0x94
  [<6002a5b4>] show_stack+0x13d/0x14c
  [<60500273>] ? dump_stack_print_info+0xde/0xed
  [<6003cfc9>] ? um_set_signals+0x0/0x3f
  [<60526312>] dump_stack_lvl+0x62/0x96
  [<6051cbaa>] ? _printk+0x0/0x94
  [<6052729b>] ? debug_lockdep_rcu_enabled+0x0/0x3b
  [<60526360>] dump_stack+0x1a/0x1c
  [<60073561>] __might_resched+0x2bb/0x2d9
  [<60073640>] __might_sleep+0xc1/0xcb
  [<6052bad8>] down_read+0x32/0x1c3
  [<6002c94e>] force_flush_all+0x74/0x105

TLB once again by the look of it.

  [<6002926e>] fork_handler+0x14/0x96


I had enabled CONFIG_DEBUG_ATOMIC_SLEEP because that's actually
something I'd really like to have in our testing.

But with that issue I don't even know how we get there really. It
doesn't even happen every time we fork?

I'll dig a little bit, but did you try enabling
CONFIG_DEBUG_ATOMIC_SLEEP also?

Will do. I have no crashes over here so I need to trigger this one first.

Though, frankly, if it is a race in a tlb flush it may be subject to local 
conditions. So it will be difficult to reproduce.

johannes

--
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