[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
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.
+ * @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.
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?
--
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
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
