| Message ID | 20210622202835.1151230-3-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Generic XDP improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 8 maintainers not CCed: yhs@fb.com akpm@linux-foundation.org kpsingh@kernel.org andriy.shevchenko@linux.intel.com geert+renesas@glider.be songliubraving@fb.com richard.weiyang@linux.alibaba.com yury.norov@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 19248 this patch: 19248 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 45 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 18715 this patch: 18715 |
| netdev/header_inline | success | Link |
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? :) -Toke
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 really have no preference here though... > -Toke > -- Kartikeya
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? -Toke
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. So such a function modifying in place doesn't allow seeing through what the type of *addr is, it would have to take void * which would work with any pointer. It's just a little more safe (so we can be sure casting to unsigned long * is ok by inspecting the typeof(*addr) ). > -Toke > -- Kartikeya
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(+)