| Message ID | 20230105212432.289569-2-tj@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] blkcg: Drop unnecessary RCU read [un]locks from blkg_conf_prep/finish() | expand |
On Thu, Jan 05, 2023 at 11:24:29AM -1000, Tejun Heo wrote: > Holding the queue lock now implies RCU read lock, so no need to use > rcu_read_[un]lock() explicitly. This shouldn't cause any behavior changes. How so? > While at it, drop __acquires() annotation on the queue lock too. The > __acquires() part was already out of sync and it doesn't catch anything that > lockdep can't. This makes sparse even more unhappy than it was before. For now please keep the annotation.
Hello, Christoph. On Sun, Jan 08, 2023 at 06:02:40PM +0100, Christoph Hellwig wrote: > On Thu, Jan 05, 2023 at 11:24:29AM -1000, Tejun Heo wrote: > > Holding the queue lock now implies RCU read lock, so no need to use > > rcu_read_[un]lock() explicitly. This shouldn't cause any behavior changes. > > How so? Now that all RCU flavors have been combined, holding a spin lock, disabling irq, disabling preemption all imply RCU read lock. > > While at it, drop __acquires() annotation on the queue lock too. The > > __acquires() part was already out of sync and it doesn't catch anything that > > lockdep can't. > > This makes sparse even more unhappy than it was before. For now > please keep the annotation. I can drop the changes but this actually bothers me. The annotation has been broken for a *long* time and nobody noticed. Furthermore, I can't remember a time when __acquires/__releases notation caught anything that lockdep couldn't trivially and can't even think of a way how it could. AFAICS, these annotations don't contribute anything other than preservation of themselves. I don't see why we would want to keep them. Thanks.
On Mon, Jan 09, 2023 at 10:48:55AM -1000, Tejun Heo wrote: > Now that all RCU flavors have been combined, holding a spin lock, disabling > irq, disabling preemption all imply RCU read lock. Can you write it like this in the commit log, please? > I can drop the changes but this actually bothers me. The annotation has been > broken for a *long* time and nobody noticed. Furthermore, I can't remember a > time when __acquires/__releases notation caught anything that lockdep > couldn't trivially and can't even think of a way how it could. AFAICS, these > annotations don't contribute anything other than preservation of themselves. > I don't see why we would want to keep them. People have noticed it. It just hasn't been a priority as there are lots of even more problematic things.
Hello, On Tue, Jan 10, 2023 at 07:49:00AM +0100, Christoph Hellwig wrote: > On Mon, Jan 09, 2023 at 10:48:55AM -1000, Tejun Heo wrote: > > Now that all RCU flavors have been combined, holding a spin lock, disabling > > irq, disabling preemption all imply RCU read lock. > > Can you write it like this in the commit log, please? Sure, will do. > > I can drop the changes but this actually bothers me. The annotation has been > > broken for a *long* time and nobody noticed. Furthermore, I can't remember a > > time when __acquires/__releases notation caught anything that lockdep > > couldn't trivially and can't even think of a way how it could. AFAICS, these > > annotations don't contribute anything other than preservation of themselves. > > I don't see why we would want to keep them. > > People have noticed it. It just hasn't been a priority as there are > lots of even more problematic things. That doesn't really shed a positive light on them, does it? I'll drop this part but can you think of actual reasons to keep these around other than to keep sparse happy? I'm genuninely curious and have asked several people. Nobody had a good answer. Thanks.
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index ce6a2b7d3dfb..99674e23cf88 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -672,12 +672,11 @@ struct block_device *blkcg_conf_open_bdev(char **inputp) * * Parse per-blkg config update from @input and initialize @ctx with the * result. @ctx->blkg points to the blkg to be updated and @ctx->body the - * part of @input following MAJ:MIN. This function returns with RCU read - * lock and queue lock held and must be paired with blkg_conf_finish(). + * part of @input following MAJ:MIN. This function returns with queue lock + * held and must be paired with blkg_conf_finish(). */ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol, char *input, struct blkg_conf_ctx *ctx) - __acquires(rcu) __acquires(&bdev->bd_queue->queue_lock) { struct block_device *bdev; struct gendisk *disk; @@ -699,7 +698,6 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol, if (ret) goto fail; - rcu_read_lock(); spin_lock_irq(&q->queue_lock); if (!blkcg_policy_enabled(q, pol)) { @@ -728,7 +726,6 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol, /* Drop locks to do new blkg allocation with GFP_KERNEL. */ spin_unlock_irq(&q->queue_lock); - rcu_read_unlock(); new_blkg = blkg_alloc(pos, disk, GFP_KERNEL); if (unlikely(!new_blkg)) { @@ -742,7 +739,6 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol, goto fail_exit_queue; } - rcu_read_lock(); spin_lock_irq(&q->queue_lock); if (!blkcg_policy_enabled(q, pol)) { @@ -778,7 +774,6 @@ int blkg_conf_prep(struct blkcg *blkcg, const struct blkcg_policy *pol, radix_tree_preload_end(); fail_unlock: spin_unlock_irq(&q->queue_lock); - rcu_read_unlock(); fail_exit_queue: blk_queue_exit(q); fail: @@ -805,10 +800,8 @@ EXPORT_SYMBOL_GPL(blkg_conf_prep); * with blkg_conf_prep(). */ void blkg_conf_finish(struct blkg_conf_ctx *ctx) - __releases(&ctx->bdev->bd_queue->queue_lock) __releases(rcu) { spin_unlock_irq(&bdev_get_queue(ctx->bdev)->queue_lock); - rcu_read_unlock(); blkdev_put_no_open(ctx->bdev); } EXPORT_SYMBOL_GPL(blkg_conf_finish);
Holding the queue lock now implies RCU read lock, so no need to use rcu_read_[un]lock() explicitly. This shouldn't cause any behavior changes. While at it, drop __acquires() annotation on the queue lock too. The __acquires() part was already out of sync and it doesn't catch anything that lockdep can't. Signed-off-by: Tejun Heo <tj@kernel.org> --- block/blk-cgroup.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)