Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH] kernel/watch_queue: Make pipe NULL while clearing watch_queue | Date | Wed, 27 Jul 2022 15:46:40 +0100 |
| |
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
| |