RE: [PATCH net-next v3 2/5] bitops: add non-atomic bitops for pointers

From: Toke Høiland-Jørgensen

Sent: 23 June 2021 12:09
Kumar Kartikeya Dwivedi [off-list ref] writes:

quoted

On Wed, Jun 23, 2021 at 04:03:06AM IST, Toke Høiland-Jørgensen wrote:

quoted

Kumar Kartikeya Dwivedi [off-list ref] writes:

quoted

On Wed, Jun 23, 2021 at 03:22:51AM IST, Toke Høiland-Jørgensen wrote:

quoted

Kumar Kartikeya Dwivedi [off-list ref] writes:

quoted

cpumap needs to set, clear, and test the lowest bit in skb pointer in
various places. To make these checks less noisy, add pointer friendly
bitop macros that also do some typechecking to sanitize the argument.

These wrap the non-atomic bitops __set_bit, __clear_bit, and test_bit
but for pointer arguments. Pointer's address has to be passed in and it
is treated as an unsigned long *, since width and representation of
pointer and unsigned long match on targets Linux supports. They are
prefixed with double underscore to indicate lack of atomicity.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bitops.h    | 19 +++++++++++++++++++
 include/linux/typecheck.h | 10 ++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 26bf15e6cd35..a9e336b9fa4d 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h

@@ -4,6 +4,7 @@

 #include <asm/types.h>
 #include <linux/bits.h>
+#include <linux/typecheck.h>

 #include <uapi/linux/kernel.h>

@@ -253,6 +254,24 @@ static __always_inline void __assign_bit(long nr, volatile unsigned long

*addr,

quoted

quoted

quoted

quoted

quoted

 		__clear_bit(nr, addr);
 }

+#define __ptr_set_bit(nr, addr)                         \
+	({                                              \
+		typecheck_pointer(*(addr));             \
+		__set_bit(nr, (unsigned long *)(addr)); \
+	})
+
+#define __ptr_clear_bit(nr, addr)                         \
+	({                                                \
+		typecheck_pointer(*(addr));               \
+		__clear_bit(nr, (unsigned long *)(addr)); \
+	})
+
+#define __ptr_test_bit(nr, addr)                       \
+	({                                             \
+		typecheck_pointer(*(addr));            \
+		test_bit(nr, (unsigned long *)(addr)); \
+	})
+

Before these were functions that returned the modified values, now they
are macros that modify in-place. Why the change? :)

Given that we're exporting this to all kernel users now, it felt more
appropriate to follow the existing convention/argument order for the
functions/ops they are wrapping.

I wasn't talking about the order of the arguments; swapping those is
fine. But before, you had:

static void *__ptr_set_bit(void *ptr, int bit)

with usage (function return is the modified value):
ret = ptr_ring_produce(rcpu->queue, __ptr_set_bit(skb, 0));

now you have:
#define __ptr_set_bit(nr, addr)

with usage (modifies argument in-place):
__ptr_set_bit(0, &skb);
ret = ptr_ring_produce(rcpu->queue, skb);

why change from function to macro?

Earlier it just took the pointer value and returned one with the bit set. I
changed it to work similar to __set_bit.

Hmm, okay, fair enough I suppose there's something to be said for
consistency, even though I personally prefer the function style. Let's
keep it as macros, then :)

Passing the address of the pointer will trash a lot of optimisations.
You do really want to use the return address.
Or, even better, get the whole thing inlined.

So something like:
#define ptr_set_bit(ptr, val) ((typeof (ptr))((unsigned long)(ptr) | (1 << (val))))

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)