Messages in this thread | | | Date | Wed, 27 Jul 2022 21:50:52 +0530 | From | Siddh Raman Pant <> | Subject | Re: [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch_queue |
| |
On Wed, 27 Jul 2022 20:16:40 +0530 David Howells <dhowells@redhat.com> wrote: > Siddh Raman Pant <code@siddh.me> wrote: > > > Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > - spin_unlock_bh(&wqueue->lock); > > > > rcu_read_unlock(); > > > > > > Also you now have a spinlock held when calling rcu_read_unlock(), are > > > you sure that's ok? > > Worse, we have softirqs disabled still, which might cause problems for > rcu_read_unlock()? > > > We logically should not do write operations in a read critical section, so the > > nulling of `wqueue->pipe->watch_queue` should happen after rcu_read_unlock(). > > Also, since we already have a spinlock, we can use it to ensure the nulling. > > So I think it is okay. > > Read/write locks are perhaps misnamed in this sense; they perhaps should be > shared/exclusive. But, yes, we *can* do certain write operations with the > lock held - if we're careful. Locks are required if we need to pairs of > related memory accesses; if we're only making a single non-dependent write, > then we don't necessarily need a write lock. > > However, you're referring to RCU read lock. That's a very special lock that > has to do with maintenance of persistence of objects without taking any other > lock. The moment you drop that lock, anything you accessed under RCU protocol > rules should be considered to have evaporated. > > Think of it more as a way to have a deferred destructor/deallocator. > > So I would do: > > + > + /* Clearing the watch queue, so we should clean the associated pipe. */ > + if (wqueue->pipe) { > + wqueue->pipe->watch_queue = NULL; > + wqueue->pipe = NULL; > + } > + > spin_unlock_bh(&wqueue->lock); > rcu_read_unlock(); > } > > However, since you're now changing wqueue->pipe whilst a notification is being > posted, you need a barrier in post_one_notification() to prevent the compiler > from reloading the value: > > struct pipe_inode_info *pipe = READ_ONCE(wqueue->pipe); > > David >
Thank you for explaining it!
I will send a v3. Should I add a Suggested-by tag mentioning you?
Thanks, Siddh
| |