Re: [PATCH 5/5] riscv: vdso: Implement __vdso_futex_robust_try_unlock()

[Nam Cao <namcao@linutronix.de>]

André Almeida <andrealmeid@igalia.com> writes:
> Hi Nam Cao, thanks for your patch!

Hi,

> How have you tested it?

With a gdb breakpoint in the critical session.

> I believe this is missing the ifdef header guard
>
> #ifndef __ASM_VDSO_FUTEX_H
> #define __ASM_VDSO_FUTEX_H

Right, thanks!

>> +#if defined(CONFIG_RISCV_ISA_ZACAS) && defined(CONFIG_TOOLCHAIN_HAS_ZACAS)
>> +#define FUTEX_CAS_OVERWRITE_VDSO_CS_RANGE(vdso, fd, idx, xlen, symbol)				\
>> +{												\
>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS)) {				\
>> +		void *start = symbol(vdso, CONCAT3(futex_list, xlen, _try_unlock_cs_cas_start));\
>> +		void *end   = symbol(vdso, CONCAT3(futex_list, xlen, _try_unlock_cs_cas_end));	\
>> +												\
>> +		futex_set_vdso_cs_range(fd, idx, (uintptr_t)start, (uintptr_t)end, xlen == 32);	\
>> +	}											\
>> +}
>> +#else
>> +#define FUTEX_CAS_OVERWRITE_VDSO_CS_RANGE(...)
>> +#endif
>> +
>> +#define FUTEX_SET_VDSO_CS_RANGE(vdso, fd, idx, xlen, symbol)					\
>> +{												\
>> +	void *start = symbol(vdso, CONCAT3(futex_list, xlen, _try_unlock_cs_lrsc_start));	\
>> +	void *end   = symbol(vdso, CONCAT3(futex_list, xlen, _try_unlock_cs_lrsc_end));		\
>> +												\
>> +	futex_set_vdso_cs_range(fd, idx, (uintptr_t)start, (uintptr_t)end, xlen == 32);		\
>> +												\
>> +	FUTEX_CAS_OVERWRITE_VDSO_CS_RANGE(vdso, fd, idx, xlen, symbol);				\
>> +												\
>
> So if the build has support for CAS, the function overwrite what it had 
> just set. Why do it even writes it in the first place then, can't this 
> be an if/else?

Functionally speaking, an if/else makes more sense. However, I cannot
figure out how to do that without creating a complete mess, since
*_try_unlock_cs_cas_start and *_try_unlock_cs_cas_end are not always
available. If you have a suggestion, please do let me know. But doing
it this way only costs us a few more instructions.

Nam