| Message ID | 20210622202835.1151230-3-memxor@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Generic XDP improvements | expand |
Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: > On Wed, Jun 23, 2021 at 04:03:06AM IST, Toke Høiland-Jørgensen wrote: >> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: >> >> > On Wed, Jun 23, 2021 at 03:22:51AM IST, Toke Høiland-Jørgensen wrote: >> >> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: >> >> >> >> > 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, >> >> > __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 :) -Toke
From: Toke Høiland-Jørgensen > Sent: 23 June 2021 12:09 > Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: > > > On Wed, Jun 23, 2021 at 04:03:06AM IST, Toke Høiland-Jørgensen wrote: > >> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: > >> > >> > On Wed, Jun 23, 2021 at 03:22:51AM IST, Toke Høiland-Jørgensen wrote: > >> >> Kumar Kartikeya Dwivedi <memxor@gmail.com> writes: > >> >> > >> >> > 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, > >> >> > __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)
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, __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)); \ + }) + #ifdef __KERNEL__ #ifndef set_mask_bits diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h index 20d310331eb5..33c78f27147a 100644 --- a/include/linux/typecheck.h +++ b/include/linux/typecheck.h @@ -22,4 +22,14 @@ (void)__tmp; \ }) +/* + * Check at compile that something is a pointer type. + * Always evaluates to 1 so you may use it easily in comparisons. + */ +#define typecheck_pointer(x) \ +({ typeof(x) __dummy; \ + (void)sizeof(*__dummy); \ + 1; \ +}) + #endif /* TYPECHECK_H_INCLUDED */
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(+)