| Message ID | 20240826063659.15327-3-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make wake_up_{bit,var} less fragile | expand |
On Mon, Aug 26, 2024 at 04:30:59PM +1000, NeilBrown wrote: > wake_up_bit() currently allows a "void *". While this isn't strictly a > problem as the address is never dereferenced, it is inconsistent with > the corresponding wait_var_event() which requires "unsigned long *" and > does dereference the pointer. I'm having trouble parsing this. The way I read it, you're contradicting yourself. Where does wait_var_event() require 'unsigned long *' ? > And code that needs to wait for a change in something other than an > unsigned long would be better served by wake_up_var(). This, afaict the whole var thing is size invariant. It only cares about the address.
On Mon, 16 Sep 2024, Peter Zijlstra wrote: > On Mon, Aug 26, 2024 at 04:30:59PM +1000, NeilBrown wrote: > > wake_up_bit() currently allows a "void *". While this isn't strictly a > > problem as the address is never dereferenced, it is inconsistent with > > the corresponding wait_var_event() which requires "unsigned long *" and > > does dereference the pointer. > > I'm having trouble parsing this. The way I read it, you're contradicting > yourself. Where does wait_var_event() require 'unsigned long *' ? Sorry, that is meant so as "the corresponding wait_on_bit()". > > > And code that needs to wait for a change in something other than an > > unsigned long would be better served by wake_up_var(). > > This, afaict the whole var thing is size invariant. It only cares about > the address. > Again - wake_up_bit(). Sorry - bits are vars were swimming around my brain and I didn't proof-read properly. This patch is all "bit", no "var". NeilBrown
On Mon, Sep 16, 2024 at 09:48:11PM +1000, NeilBrown wrote: > On Mon, 16 Sep 2024, Peter Zijlstra wrote: > > On Mon, Aug 26, 2024 at 04:30:59PM +1000, NeilBrown wrote: > > > wake_up_bit() currently allows a "void *". While this isn't strictly a > > > problem as the address is never dereferenced, it is inconsistent with > > > the corresponding wait_var_event() which requires "unsigned long *" and > > > does dereference the pointer. > > > > I'm having trouble parsing this. The way I read it, you're contradicting > > yourself. Where does wait_var_event() require 'unsigned long *' ? > > Sorry, that is meant so as "the corresponding wait_on_bit()". > > > > > > > And code that needs to wait for a change in something other than an > > > unsigned long would be better served by wake_up_var(). > > > > This, afaict the whole var thing is size invariant. It only cares about > > the address. > > > > Again - wake_up_bit(). Sorry - bits are vars were swimming around my > brain and I didn't proof-read properly. > > This patch is all "bit", no "var". OK :-) Anyway, other than that the patches look fine, but given we're somewhat in the middle of the merge window and all traveling to get into Vienna and have a few beers, I would much prefer merging these patches after -rc1, that okay?
On Tue, 17 Sep 2024, Peter Zijlstra wrote: > On Mon, Sep 16, 2024 at 09:48:11PM +1000, NeilBrown wrote: > > On Mon, 16 Sep 2024, Peter Zijlstra wrote: > > > On Mon, Aug 26, 2024 at 04:30:59PM +1000, NeilBrown wrote: > > > > wake_up_bit() currently allows a "void *". While this isn't strictly a > > > > problem as the address is never dereferenced, it is inconsistent with > > > > the corresponding wait_var_event() which requires "unsigned long *" and > > > > does dereference the pointer. > > > > > > I'm having trouble parsing this. The way I read it, you're contradicting > > > yourself. Where does wait_var_event() require 'unsigned long *' ? > > > > Sorry, that is meant so as "the corresponding wait_on_bit()". > > > > > > > > > > > And code that needs to wait for a change in something other than an > > > > unsigned long would be better served by wake_up_var(). > > > > > > This, afaict the whole var thing is size invariant. It only cares about > > > the address. > > > > > > > Again - wake_up_bit(). Sorry - bits are vars were swimming around my > > brain and I didn't proof-read properly. > > > > This patch is all "bit", no "var". > > OK :-) > > Anyway, other than that the patches look fine, but given we're somewhat > in the middle of the merge window and all traveling to get into Vienna > and have a few beers, I would much prefer merging these patches after > -rc1, that okay? > Yes, that's OK. Thanks for having a look. Have fun in Vienna. NeilBrown
diff --git a/include/linux/wait_bit.h b/include/linux/wait_bit.h index 7725b7579b78..48e123839892 100644 --- a/include/linux/wait_bit.h +++ b/include/linux/wait_bit.h @@ -8,7 +8,7 @@ #include <linux/wait.h> struct wait_bit_key { - void *flags; + unsigned long *flags; int bit_nr; unsigned long timeout; }; @@ -23,14 +23,14 @@ struct wait_bit_queue_entry { typedef int wait_bit_action_f(struct wait_bit_key *key, int mode); -void __wake_up_bit(struct wait_queue_head *wq_head, void *word, int bit); +void __wake_up_bit(struct wait_queue_head *wq_head, unsigned long *word, int bit); int __wait_on_bit(struct wait_queue_head *wq_head, struct wait_bit_queue_entry *wbq_entry, wait_bit_action_f *action, unsigned int mode); int __wait_on_bit_lock(struct wait_queue_head *wq_head, struct wait_bit_queue_entry *wbq_entry, wait_bit_action_f *action, unsigned int mode); -void wake_up_bit(void *word, int bit); -int out_of_line_wait_on_bit(void *word, int, wait_bit_action_f *action, unsigned int mode); -int out_of_line_wait_on_bit_timeout(void *word, int, wait_bit_action_f *action, unsigned int mode, unsigned long timeout); -int out_of_line_wait_on_bit_lock(void *word, int, wait_bit_action_f *action, unsigned int mode); -struct wait_queue_head *bit_waitqueue(void *word, int bit); +void wake_up_bit(unsigned long *word, int bit); +int out_of_line_wait_on_bit(unsigned long *word, int, wait_bit_action_f *action, unsigned int mode); +int out_of_line_wait_on_bit_timeout(unsigned long *word, int, wait_bit_action_f *action, unsigned int mode, unsigned long timeout); +int out_of_line_wait_on_bit_lock(unsigned long *word, int, wait_bit_action_f *action, unsigned int mode); +struct wait_queue_head *bit_waitqueue(unsigned long *word, int bit); extern void __init wait_bit_init(void); int wake_bit_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key); @@ -327,7 +327,7 @@ do { \ * You can use this helper if bitflags are manipulated atomically rather than * non-atomically under a lock. */ -static inline void clear_and_wake_up_bit(int bit, void *word) +static inline void clear_and_wake_up_bit(int bit, unsigned long *word) { clear_bit_unlock(bit, word); /* See wake_up_bit() for which memory barrier you need to use. */ diff --git a/kernel/sched/wait_bit.c b/kernel/sched/wait_bit.c index 134d7112ef71..058b0e18727e 100644 --- a/kernel/sched/wait_bit.c +++ b/kernel/sched/wait_bit.c @@ -9,7 +9,7 @@ static wait_queue_head_t bit_wait_table[WAIT_TABLE_SIZE] __cacheline_aligned; -wait_queue_head_t *bit_waitqueue(void *word, int bit) +wait_queue_head_t *bit_waitqueue(unsigned long *word, int bit) { const int shift = BITS_PER_LONG == 32 ? 5 : 6; unsigned long val = (unsigned long)word << shift | bit; @@ -55,7 +55,7 @@ __wait_on_bit(struct wait_queue_head *wq_head, struct wait_bit_queue_entry *wbq_ } EXPORT_SYMBOL(__wait_on_bit); -int __sched out_of_line_wait_on_bit(void *word, int bit, +int __sched out_of_line_wait_on_bit(unsigned long *word, int bit, wait_bit_action_f *action, unsigned mode) { struct wait_queue_head *wq_head = bit_waitqueue(word, bit); @@ -66,7 +66,7 @@ int __sched out_of_line_wait_on_bit(void *word, int bit, EXPORT_SYMBOL(out_of_line_wait_on_bit); int __sched out_of_line_wait_on_bit_timeout( - void *word, int bit, wait_bit_action_f *action, + unsigned long *word, int bit, wait_bit_action_f *action, unsigned mode, unsigned long timeout) { struct wait_queue_head *wq_head = bit_waitqueue(word, bit); @@ -108,7 +108,7 @@ __wait_on_bit_lock(struct wait_queue_head *wq_head, struct wait_bit_queue_entry } EXPORT_SYMBOL(__wait_on_bit_lock); -int __sched out_of_line_wait_on_bit_lock(void *word, int bit, +int __sched out_of_line_wait_on_bit_lock(unsigned long *word, int bit, wait_bit_action_f *action, unsigned mode) { struct wait_queue_head *wq_head = bit_waitqueue(word, bit); @@ -118,7 +118,7 @@ int __sched out_of_line_wait_on_bit_lock(void *word, int bit, } EXPORT_SYMBOL(out_of_line_wait_on_bit_lock); -void __wake_up_bit(struct wait_queue_head *wq_head, void *word, int bit) +void __wake_up_bit(struct wait_queue_head *wq_head, unsigned long *word, int bit) { struct wait_bit_key key = __WAIT_BIT_KEY_INITIALIZER(word, bit); @@ -144,7 +144,7 @@ EXPORT_SYMBOL(__wake_up_bit); * may need to use a less regular barrier, such fs/inode.c's smp_mb(), * because spin_unlock() does not guarantee a memory barrier. */ -void wake_up_bit(void *word, int bit) +void wake_up_bit(unsigned long *word, int bit) { __wake_up_bit(bit_waitqueue(word, bit), word, bit); }
wake_up_bit() currently allows a "void *". While this isn't strictly a problem as the address is never dereferenced, it is inconsistent with the corresponding wait_var_event() which requires "unsigned long *" and does dereference the pointer. And code that needs to wait for a change in something other than an unsigned long would be better served by wake_up_var(). This patch changes all related "void *" to "unsigned long *". Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: NeilBrown <neilb@suse.de> --- include/linux/wait_bit.h | 16 ++++++++-------- kernel/sched/wait_bit.c | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-)