|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 4/4] include/uk: merge two bitops functions
Florian Schmidt <Florian.Schmidt@xxxxxxxxx> writes:
> Hi Yuri,
>
> some comments:
>
> On 10/05/2018 10:21 AM, Yuri Volchkov wrote:
>> Unikraft currently has two sets of bit operations. This patch merges
>> them together
>>
>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>> include/uk/arch/atomic.h | 107 -----------------------------------
>> include/uk/bitops.h | 117 +++++++++++++++++++++++++++++++++------
>> 2 files changed, 100 insertions(+), 124 deletions(-)
>>
>> diff --git a/include/uk/arch/atomic.h b/include/uk/arch/atomic.h
>> index e6444c0..ce8f6e5 100644
>> --- a/include/uk/arch/atomic.h
>> +++ b/include/uk/arch/atomic.h
>> @@ -83,113 +83,6 @@ extern "C" {
>> : old; \
>> })
>>
>> -/**
>> - * test_and_clear_bit - Clear a bit and return its old value
>> - * @nr: Bit to clear
>> - * @addr: Address to count from
>> - *
>> - * Note that @nr may be almost arbitrarily large; this function is not
>> - * restricted to acting on a single-word quantity.
>> - *
>> - * This operation is atomic.
>> - * If you need a memory barrier, use synch_test_and_clear_bit instead.
>> - */
>> -static inline int ukarch_test_and_clr_bit(unsigned int nr, volatile void
>> *byte)
>> -{
>> - volatile __u8 *addr = ((__u8 *)byte) + (nr >> 3);
>> - __u8 bit = 1 << (nr & 7);
>> - __u8 orig;
>> -
>> - orig = __atomic_fetch_and(addr, ~bit, __ATOMIC_RELAXED);
>> -
>> - return (orig & bit) != 0;
>> -}
>> -
>> -/**
>> - * Atomically set a bit and return the old value.
>> - * Similar to test_and_clear_bit.
>> - */
>> -static inline int ukarch_test_and_set_bit(unsigned int nr, volatile void
>> *byte)
>> -{
>> - volatile __u8 *addr = ((__u8 *)byte) + (nr >> 3);
>> - __u8 bit = 1 << (nr & 7);
>> - __u8 orig;
>> -
>> - orig = __atomic_fetch_or(addr, bit, __ATOMIC_RELAXED);
>> -
>> - return (orig & bit) != 0;
>> -}
>> -
>> -/**
>> - * Test whether a bit is set.
>> - */
>> -static inline int ukarch_test_bit(unsigned int nr,
>> - const volatile unsigned long *byte)
>> -{
>> - const volatile __u8 *ptr = (const __u8 *)byte;
>> - int ret = ((1 << (nr & 7)) & (ptr[nr >> 3])) != 0;
>> -
>> - barrier();
>> - return ret;
>> -}
>> -
>> -/**
>> - * Atomically set a bit in memory (like test_and_set_bit but discards
>> result).
>> - */
>> -static inline void ukarch_set_bit(unsigned int nr,
>> - volatile unsigned long *byte)
>> -{
>> - ukarch_test_and_set_bit(nr, byte);
>> -}
>> -
>> -/**
>> - * Atomically clear a bit in memory (like test_and_clear_bit but discards
>> - * result).
>> - */
>> -static inline void ukarch_clr_bit(unsigned int nr,
>> - volatile unsigned long *byte)
>> -{
>> - ukarch_test_and_clr_bit(nr, byte);
>> -}
>> -
>> -/* As test_and_clear_bit, but using __ATOMIC_SEQ_CST */
>> -static inline int ukarch_test_and_clr_bit_sync(unsigned int nr,
>> - volatile void *byte)
>> -{
>> - volatile __u8 *addr = ((__u8 *)byte) + (nr >> 3);
>> - __u8 bit = 1 << (nr & 7);
>> - __u8 orig;
>> -
>> - orig = __atomic_fetch_and(addr, ~bit, __ATOMIC_SEQ_CST);
>> -
>> - return (orig & bit) != 0;
>> -}
>> -
>> -/* As test_and_set_bit, but using __ATOMIC_SEQ_CST */
>> -static inline int ukarch_test_and_set_bit_sync(unsigned int nr,
>> - volatile void *byte)
>> -{
>> - volatile __u8 *addr = ((__u8 *)byte) + (nr >> 3);
>> - __u8 bit = 1 << (nr & 7);
>> - __u8 orig;
>> -
>> - orig = __atomic_fetch_or(addr, bit, __ATOMIC_SEQ_CST);
>> -
>> - return (orig & bit) != 0;
>> -}
>> -
>> -/* As set_bit, but using __ATOMIC_SEQ_CST */
>> -static inline void ukarch_set_bit_sync(unsigned int nr, volatile void *byte)
>> -{
>> - ukarch_test_and_set_bit_sync(nr, byte);
>> -}
>> -
>> -/* As clear_bit, but using __ATOMIC_SEQ_CST */
>> -static inline void ukarch_clr_bit_sync(unsigned int nr, volatile void *byte)
>> -{
>> - ukarch_test_and_clr_bit_sync(nr, byte);
>> -}
>> -
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/include/uk/bitops.h b/include/uk/bitops.h
>> index 5a28410..373dcd3 100644
>> --- a/include/uk/bitops.h
>> +++ b/include/uk/bitops.h
>> @@ -242,38 +242,88 @@ uk_find_next_zero_bit(const unsigned long *addr,
>> unsigned long size,
>> return (bit);
>> }
>>
>> -/* uk_set_bit and uk_clear_bit are atomic and protected against
>> - * reordering (do barriers), while the underscored (__*) versions of
>> - * them don't (not atomic).
>> +/**
>> + * uk_test_and_clear_bit - Atomically clear a bit and return its old value
>> + * @nr: Bit to clear
>> + * @addr: Address to count from
>> + *
>> + * Note that @nr may be almost arbitrarily large; this function is not
>> + * restricted to acting on a single-word quantity.
>> */
>> -#define __uk_set_bit(i, a) ukarch_set_bit(i, a)
>> -#define uk_set_bit(i, a) ukarch_set_bit_sync(i, a)
>> -#define __uk_clear_bit(i, a) ukarch_clr_bit(i, a)
>> -#define uk_clear_bit(i, a) ukarch_clr_bit_sync(i, a)
>> -#define uk_test_bit(i, a) ukarch_test_bit(i, a)
>> -
>> static inline int
>> -uk_test_and_clear_bit(long bit, volatile unsigned long *var)
>> +uk_test_and_clear_bit(long nr, volatile unsigned long *addr)
>> {
>> - return ukarch_test_and_clr_bit_sync(bit, (volatile void *) var);
>> + volatile __u8 *ptr = ((__u8 *) addr) + (nr >> 3);
>> + __u8 mask = 1 << (nr & 7);
>> + __u8 orig;
>> +
>> + orig = __atomic_fetch_and(ptr, ~mask, __ATOMIC_SEQ_CST);
>> +
>> + return (orig & mask) != 0;
>> }
>>
>> +/**
>> + * __uk_test_and_clear_bit - Clear a bit and return its old value
>> + * @nr: Bit to clear
>> + * @addr: Address to count from
>> + *
>> + * Note that @nr may be almost arbitrarily large; this function is not
>> + * restricted to acting on a single-word quantity.
>> + *
>> + * This operation is not atomic and can be reordered
>> + */
>> static inline int
>> -__uk_test_and_clear_bit(long bit, volatile unsigned long *var)
>> +__uk_test_and_clear_bit(long nr, volatile unsigned long *addr)
>> {
>> - return ukarch_test_and_clr_bit(bit, (volatile void *) var);
>> + volatile __u8 *ptr = ((__u8 *) addr) + (nr >> 3);
>> + __u8 mask = 1 << (nr & 7);
>> + __u8 orig;
>> +
>> + orig = __atomic_fetch_and(ptr, ~mask, __ATOMIC_RELAXED);
>> +
>> + return (orig & mask) != 0;
>> }
>>
>> +/**
>> + * __uk_test_and_set_bit - Atomically set a bit and return its old value
>
> This is a typo: the comment actually describes the non-underscore
> version of the function.
Oops. Thanks for pointing out
>
>> + * @nr: Bit to clear
>> + * @addr: Address to count from
>> + *
>> + * Note that @nr may be almost arbitrarily large; this function is not
>> + * restricted to acting on a single-word quantity.
>> + */
>> static inline int
>> -uk_test_and_set_bit(long bit, volatile unsigned long *var)
>> +uk_test_and_set_bit(long nr, volatile unsigned long *addr)
>> {
>> - return ukarch_test_and_set_bit_sync(bit, (volatile void *) var);
>> + volatile __u8 *ptr = ((__u8 *) addr) + (nr >> 3);
>> + __u8 mask = 1 << (nr & 7);
>> + __u8 orig;
>> +
>> + orig = __atomic_fetch_or(ptr, mask, __ATOMIC_SEQ_CST);
>> +
>> + return (orig & mask) != 0;
>> }
>>
>> +/**
>> + * __uk_test_and_set_bit - Set a bit and return its old value
>> + * @nr: Bit to clear
>> + * @addr: Address to count from
>> + *
>> + * Note that @nr may be almost arbitrarily large; this function is not
>> + * restricted to acting on a single-word quantity.
>> + *
>> + * This operation is not atomic and can be reordered
>> + */
>> static inline int
>> -__uk_test_and_set_bit(long bit, volatile unsigned long *var)
>> +__uk_test_and_set_bit(long nr, volatile unsigned long *addr)
>> {
>> - return ukarch_test_and_set_bit(bit, (volatile void *) var);
>> + volatile __u8 *ptr = ((__u8 *) addr) + (nr >> 3);
>> + __u8 mask = 1 << (nr & 7);
>> + __u8 orig;
>> +
>> + orig = __atomic_fetch_or(ptr, mask, __ATOMIC_RELAXED);
>> +
>> + return (orig & mask) != 0;
>> }
>>
>> enum {
>> @@ -282,6 +332,39 @@ enum {
>> REG_OP_RELEASE,
>> };
>>
>> +/* uk_set_bit and uk_clear_bit are atomic and protected against
>> + * reordering (do barriers), while the underscored (__*) versions of
>> + * them don't (not atomic).
>
> I know you only copied that comment from a previous location in the
> other file, but this should say "aren't" instead of "don't", so we might
> as well fix that now that we touch the comment line.
>
> Also, come to think of it, isn't that wrong? The underscore versions
> also do atomic bit operations, they're just not synchronized.
Actually it is. That is not only about reordering. There are also
caches. And if 2 __uk_set_bit will be executing simultaneously on 2
different cpu it might be that only one of them will be successful. I
will reflect this in the comment too.
>
> Isn't this both more correct and clearer:
> "The following uk_* functions are synchronized versions that guarantee
> memory ordering, while their __uk_* counterparts do not."
>
>
>> + */
>> +static inline void uk_set_bit(long nr, volatile unsigned long *addr)
>> +{
>> + uk_test_and_set_bit(nr, addr);
>> +}
>> +
>> +static inline void __uk_set_bit(long nr, volatile unsigned long *addr)
>> +{
>> + __uk_test_and_set_bit(nr, addr);
>> +}
>> +
>> +static inline void uk_clear_bit(long nr, volatile unsigned long *addr)
>> +{
>> + uk_test_and_clear_bit(nr, addr);
>> +}
>> +
>> +static inline void __uk_clear_bit(long nr, volatile unsigned long *addr)
>> +{
>> + __uk_test_and_clear_bit(nr, addr);
>> +}
>> +
>> +static inline int uk_test_bit(int nr, const volatile unsigned long *addr)
>> +{
>> + const volatile __u8 *ptr = (const __u8 *) addr;
>> + int ret = ((1 << (nr & 7)) & (ptr[nr >> 3])) != 0;
>> +
>> + barrier();
>> + return ret;
>> +}
>
> This test_bit isn't synced, though. So to stay with the naming scheme,
> shouldn't this be __uk_test_bit? And then we create a synced version
> with __atomic_thread_fence or something like that?
The idea is to stay in sync with the linux API (since it is a BSD
version of linux bit operations). It provides only one version of the
test_bit.
>
>
> --
> Dr. Florian Schmidt
> フローリアン・シュミット
> Research Scientist,
> Systems and Machine Learning Group
> NEC Laboratories Europe
> Kurfürsten-Anlage 36, D-69115 Heidelberg
> Tel. +49 (0)6221 4342-265
> Fax: +49 (0)6221 4342-155
> e-mail: florian.schmidt@xxxxxxxxx
> ============================================================
> Registered at Amtsgericht Mannheim, Germany, HRB728558
--
Yuri Volchkov
Software Specialist
NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |