Re: [PATCH v2] um: add a UML specific futex implementation

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

On 10/12/2020 22:38, Richard Weinberger wrote:
> On Thu, Nov 12, 2020 at 8:57 PM <anton.ivanov@cambridgegreys.com> wrote:
>>
>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>>
>> The generic asm futex implementation emulates atomic access to
>> memory by doing a get_user followed by put_user. These translate
>> to two mapping operations on UML with paging enabled in the
>> meantime. This, in turn may end up changing interrupts,
>> invoking the signal loop, etc.
>>
>> This replaces the generic implementation by a mapping followed
>> by an operation on the mapped segment. It is for now limited
>> to 64 bit as the 32 bit may also need to take care of highmem,
>> etc.
>>
>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
>> ---
>>   arch/um/include/asm/futex.h   |  20 ++++++
>>   arch/um/kernel/skas/uaccess.c | 119 ++++++++++++++++++++++++++++++++++
>>   2 files changed, 139 insertions(+)
>>   create mode 100644 arch/um/include/asm/futex.h
>>
>> diff --git a/arch/um/include/asm/futex.h b/arch/um/include/asm/futex.h
>> new file mode 100644
>> index 000000000000..9af35ab66b6e
>> --- /dev/null
>> +++ b/arch/um/include/asm/futex.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_GENERIC_FUTEX_H
>> +#define _ASM_GENERIC_FUTEX_H
>> +
>> +#ifdef CONFIG_64BIT
>> +
>> +#include <linux/futex.h>
>> +#include <linux/uaccess.h>
>> +#include <asm/errno.h>
>> +
>> +
>> +extern int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr);
>> +extern int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>> +                             u32 oldval, u32 newval);
>> +
>> +#else
>> +#include <"asm-generic/futex.h">
>> +#endif /* CONFIG_64BIT */
>> +
>> +#endif
>> diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c
>> index 2dec915abe6f..21a8f2b283bb 100644
>> --- a/arch/um/kernel/skas/uaccess.c
>> +++ b/arch/um/kernel/skas/uaccess.c
>> @@ -11,6 +11,7 @@
>>   #include <asm/current.h>
>>   #include <asm/page.h>
>>   #include <kern_util.h>
>> +#include <asm/futex.h>
>>   #include <os.h>
>>
>>   pte_t *virt_to_pte(struct mm_struct *mm, unsigned long addr)
>> @@ -248,3 +249,121 @@ long __strnlen_user(const void __user *str, long len)
>>          return 0;
>>   }
>>   EXPORT_SYMBOL(__strnlen_user);
>> +
>> +/**
>> + * arch_futex_atomic_op_inuser() - Atomic arithmetic operation with constant
>> + *                       argument and comparison of the previous
>> + *                       futex value with another constant.
>> + *
>> + * @encoded_op:        encoded operation to execute
>> + * @uaddr:     pointer to user space address
>> + *
>> + * Return:
>> + * 0 - On success
>> + * -EFAULT - User access resulted in a page fault
>> + * -EAGAIN - Atomic operation was unable to complete due to contention
>> + * -ENOSYS - Operation not supported
>> + */
>> +int arch_futex_atomic_op_inuser(int op, u32 oparg, int *oval, u32 __user *uaddr)
> 
> Does this even build in 32bits mode?
> AFAICT the generic futex implementation has
> arch_futex_atomic_op_inuser() as static inline...
> 

It's an inline, but it includes two very slow non-inline calls:

https://elixir.bootlin.com/linux/latest/source/include/asm-generic/futex.h#L39

if (unlikely(get_user(oldval, uaddr) != 0))

and

https://elixir.bootlin.com/linux/latest/source/include/asm-generic/futex.h#L65

if (ret == 0 && unlikely(put_user(tmp, uaddr) != 0))

So better to have not inline, but actually fast and done in a single atomic operation on the target memory location.

I will fix all 32 bit and resubmit (and promise to test it on 32 next time :)

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