| Message ID | 5487a5f7d1a4be1bb13e7d1f392281d18c0e935e.1503319573.git.lukas@wunner.de |
|---|---|
| State | New |
| Headers | show |
On Mon, 2017-08-21 at 15:12 +0200, Lukas Wunner wrote: > [ ... ] > Cc: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Alasdair Kergon <agk@redhat.com> > Cc: Mike Snitzer <snitzer@redhat.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> This Cc-list is incomplete. Previous <linux/bitops.h> patches went in through Andrew Morton's tree so I think an ack from Andrew Morton is needed before this patch can be sent to Linus Torvalds. Please also Cc other frequent contributors to this header file, e.g. Ingo Molnar and Peter Zijlstra. Please also consider to Cc the LKML for this patch or even for the entire series. > +/** > + * assign_bit - Assign value to a bit in memory > + * @value: the value to assign > + * @nr: the bit to set > + * @addr: the address to start counting from > + */ > +static __always_inline void assign_bit(bool value, long nr, > + volatile unsigned long *addr) > +{ > + if (value) > + set_bit(nr, addr); > + else > + clear_bit(nr, addr); > +} > + > +static __always_inline void __assign_bit(bool value, long nr, > + volatile unsigned long *addr) > +{ > + if (value) > + __set_bit(nr, addr); > + else > + __clear_bit(nr, addr); > +} Why has __always_inline been specified? What makes you think that you know better than the compiler whether or not these functions should be inlined? Thanks, Bart.
On Mon, Aug 21, 2017 at 3:12 PM, Lukas Wunner <lukas@wunner.de> wrote: > A common idiom is to assign a value to a bit with: > > if (value) > set_bit(nr, addr); > else > clear_bit(nr, addr); > > Likewise common is the one-line expression variant: > > value ? set_bit(nr, addr) : clear_bit(nr, addr); > > Commit 9a8ac3ae682e ("dm mpath: cleanup QUEUE_IF_NO_PATH bit > manipulation by introducing assign_bit()") introduced assign_bit() > to the md subsystem for brevity. > > Make it available to others, in particular gpiolib and the upcoming > driver for Maxim MAX3191x industrial serializer chips. > > Cc: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Alasdair Kergon <agk@redhat.com> > Cc: Mike Snitzer <snitzer@redhat.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> Looks reasonable. > +static __always_inline void assign_bit(bool value, long nr, > + volatile unsigned long *addr) > +{ > + if (value) > + set_bit(nr, addr); > + else > + clear_bit(nr, addr); > +} > + > +static __always_inline void __assign_bit(bool value, long nr, > + volatile unsigned long *addr) > +{ > + if (value) > + __set_bit(nr, addr); > + else > + __clear_bit(nr, addr); > +} What I have never understood is the semantic difference between bitop_foo() and __bitop_foo(). And I even use them quite a lot. Can someone explain when I use the bitop_foo() and when I use the __bitop_foo(). I am asking so I can understand patch 2/4 in this series. I guess this is why I generally detest the __annotation, since it is so unspecific and unclear, but that is not something I expect you to solve in this patch. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-08-23 at 09:32 +0200, Linus Walleij wrote: > Can someone explain when I use the bitop_foo() and when I > use the __bitop_foo(). I am asking so I can understand patch > 2/4 in this series. That's easy. From Documentation/core-api/atomic_ops.rst: ---------------------------------------------------------------------- Finally, there are non-atomic versions of the bitmask operations provided. They are used in contexts where some other higher-level SMP locking scheme is being used to protect the bitmask, and thus less expensive non-atomic operations may be used in the implementation. They have names similar to the above bitmask operation interfaces, except that two underscores are prefixed to the interface name. :: void __set_bit(unsigned long nr, volatile unsigned long *addr); void __clear_bit(unsigned long nr, volatile unsigned long *addr); void __change_bit(unsigned long nr, volatile unsigned long *addr); int __test_and_set_bit(unsigned long nr, volatile unsigned long *addr); int __test_and_clear_bit(unsigned long nr, volatile unsigned long *addr); int __test_and_change_bit(unsigned long nr, volatile unsigned long *addr); These non-atomic variants also do not require any special memory barrier semantics. ---------------------------------------------------------------------- Bart.
On Wed, Aug 23, 2017 at 7:09 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Wed, 2017-08-23 at 09:32 +0200, Linus Walleij wrote: >> Can someone explain when I use the bitop_foo() and when I >> use the __bitop_foo(). I am asking so I can understand patch >> 2/4 in this series. > > That's easy. From Documentation/core-api/atomic_ops.rst: > > ---------------------------------------------------------------------- > Finally, there are non-atomic versions of the bitmask operations > provided. They are used in contexts where some other higher-level SMP > locking scheme is being used to protect the bitmask, and thus less > expensive non-atomic operations may be used in the implementation. > They have names similar to the above bitmask operation interfaces, > except that two underscores are prefixed to the interface name. :: > > void __set_bit(unsigned long nr, volatile unsigned long *addr); > void __clear_bit(unsigned long nr, volatile unsigned long *addr); > void __change_bit(unsigned long nr, volatile unsigned long *addr); > int __test_and_set_bit(unsigned long nr, volatile unsigned long *addr); > int __test_and_clear_bit(unsigned long nr, volatile unsigned long *addr); > int __test_and_change_bit(unsigned long nr, volatile unsigned long *addr); > > These non-atomic variants also do not require any special memory > barrier semantics. All right that makes sense. On Rusty Russell's API ladder http://sweng.the-davies.net/Home/rustys-api-design-manifesto this scores 3/10 "read the documentation and you will get it right". We could raise it to 6-7 by renaming them all set_bit_nonatomic() etc (or even set_bit_na()), I almost feel like sending a script to Torvalds to do that after the merge window, but I fear some people would beat me up for it. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index 0e8ab5bb3575..c79c113b7e7d 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -638,14 +638,6 @@ static void process_queued_bios(struct work_struct *work) blk_finish_plug(&plug); } -static void assign_bit(bool value, long nr, unsigned long *addr) -{ - if (value) - set_bit(nr, addr); - else - clear_bit(nr, addr); -} - /* * If we run out of usable paths, should we queue I/O or error it? */ diff --git a/include/linux/bitops.h b/include/linux/bitops.h index a83c822c35c2..097af36887c0 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -226,6 +226,30 @@ static inline unsigned long __ffs64(u64 word) return __ffs((unsigned long)word); } +/** + * assign_bit - Assign value to a bit in memory + * @value: the value to assign + * @nr: the bit to set + * @addr: the address to start counting from + */ +static __always_inline void assign_bit(bool value, long nr, + volatile unsigned long *addr) +{ + if (value) + set_bit(nr, addr); + else + clear_bit(nr, addr); +} + +static __always_inline void __assign_bit(bool value, long nr, + volatile unsigned long *addr) +{ + if (value) + __set_bit(nr, addr); + else + __clear_bit(nr, addr); +} + #ifdef __KERNEL__ #ifndef set_mask_bits
A common idiom is to assign a value to a bit with: if (value) set_bit(nr, addr); else clear_bit(nr, addr); Likewise common is the one-line expression variant: value ? set_bit(nr, addr) : clear_bit(nr, addr); Commit 9a8ac3ae682e ("dm mpath: cleanup QUEUE_IF_NO_PATH bit manipulation by introducing assign_bit()") introduced assign_bit() to the md subsystem for brevity. Make it available to others, in particular gpiolib and the upcoming driver for Maxim MAX3191x industrial serializer chips. Cc: Bart Van Assche <bart.vanassche@wdc.com> Cc: Alasdair Kergon <agk@redhat.com> Cc: Mike Snitzer <snitzer@redhat.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/md/dm-mpath.c | 8 -------- include/linux/bitops.h | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-)