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

[Anton Ivanov <anton.ivanov@cambridgegreys.com>]

On 22/09/2023 08:30, Johannes Berg wrote:
> 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.

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