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