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

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2023-09-22 at 07:52 +0100, anton.ivanov@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.

> +#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
 [<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?

johannes

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