| Message ID | Zv0kudA9xyGdaA4g@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] blk_iocost: remove some duplicate irq disable/enables | expand |
On Wed, 02 Oct 2024 13:47:21 +0300, Dan Carpenter wrote: > These are called from blkcg_print_blkgs() which already disables IRQs so > disabling it again is wrong. It means that IRQs will be enabled slightly > earlier than intended, however, so far as I can see, this bug is harmless. > > Applied, thanks! [1/1] blk_iocost: remove some duplicate irq disable/enables commit: 14d57ec3b86369d0037567f12caae0c9e9eaad9e Best regards,
On 10/2/24 06:47, Dan Carpenter wrote: > These are called from blkcg_print_blkgs() which already disables IRQs so > disabling it again is wrong. It means that IRQs will be enabled slightly > earlier than intended, however, so far as I can see, this bug is harmless. > > Fixes: 35198e323001 ("blk-iocost: read params inside lock in sysfs apis") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > v2: Fix typo in the subject > > block/blk-iocost.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/block/blk-iocost.c b/block/blk-iocost.c > index 9dc9323f84ac..384aa15e8260 100644 > --- a/block/blk-iocost.c > +++ b/block/blk-iocost.c > @@ -3166,7 +3166,7 @@ static u64 ioc_qos_prfill(struct seq_file *sf, struct blkg_policy_data *pd, > if (!dname) > return 0; > > - spin_lock_irq(&ioc->lock); > + spin_lock(&ioc->lock); > seq_printf(sf, "%s enable=%d ctrl=%s rpct=%u.%02u rlat=%u wpct=%u.%02u wlat=%u min=%u.%02u max=%u.%02u\n", > dname, ioc->enabled, ioc->user_qos_params ? "user" : "auto", > ioc->params.qos[QOS_RPPM] / 10000, > @@ -3179,7 +3179,7 @@ static u64 ioc_qos_prfill(struct seq_file *sf, struct blkg_policy_data *pd, > ioc->params.qos[QOS_MIN] % 10000 / 100, > ioc->params.qos[QOS_MAX] / 10000, > ioc->params.qos[QOS_MAX] % 10000 / 100); > - spin_unlock_irq(&ioc->lock); > + spin_unlock(&ioc->lock); > return 0; > } > > @@ -3366,14 +3366,14 @@ static u64 ioc_cost_model_prfill(struct seq_file *sf, > if (!dname) > return 0; > > - spin_lock_irq(&ioc->lock); > + spin_lock(&ioc->lock); > seq_printf(sf, "%s ctrl=%s model=linear " > "rbps=%llu rseqiops=%llu rrandiops=%llu " > "wbps=%llu wseqiops=%llu wrandiops=%llu\n", > dname, ioc->user_cost_model ? "user" : "auto", > u[I_LCOEF_RBPS], u[I_LCOEF_RSEQIOPS], u[I_LCOEF_RRANDIOPS], > u[I_LCOEF_WBPS], u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS]); > - spin_unlock_irq(&ioc->lock); > + spin_unlock(&ioc->lock); > return 0; > } > I would suggest adding a "lockdep_assert_irqs_disabled()" call before spin_lock() to confirm that irq is indeed disabled just in case the callers are changed in the future. Cheers, Longman
On Wed, Oct 02, 2024 at 01:49:48PM -0400, Waiman Long wrote: > > - spin_unlock_irq(&ioc->lock); > > + spin_unlock(&ioc->lock); > > return 0; > > } > > I would suggest adding a "lockdep_assert_irqs_disabled()" call before > spin_lock() to confirm that irq is indeed disabled just in case the callers > are changed in the future. It's really hard to predict future bugs. I doubt we'll add new callers. Outputting this information to a struct seq_file *sf is pretty specific. If there were a bug related to this, then wouldn't it be caught by lockdep? The other idea is that we could catch bugs like this using static analysis. Like every time we take the &ioc->lock, either IRQs should already be disabled or we disable it ourselves. I could write a Smatch check like this. KTODO: add Smatch check to ensure IRQs are disabled for &ioc->lock regards, dan carpenter
On 10/2/24 14:10, Dan Carpenter wrote: > On Wed, Oct 02, 2024 at 01:49:48PM -0400, Waiman Long wrote: >>> - spin_unlock_irq(&ioc->lock); >>> + spin_unlock(&ioc->lock); >>> return 0; >>> } >> I would suggest adding a "lockdep_assert_irqs_disabled()" call before >> spin_lock() to confirm that irq is indeed disabled just in case the callers >> are changed in the future. > It's really hard to predict future bugs. I doubt we'll add new callers. > Outputting this information to a struct seq_file *sf is pretty specific. > > If there were a bug related to this, then wouldn't it be caught by lockdep? > > The other idea is that we could catch bugs like this using static analysis. > Like every time we take the &ioc->lock, either IRQs should already be disabled > or we disable it ourselves. I could write a Smatch check like this. > > KTODO: add Smatch check to ensure IRQs are disabled for &ioc->lock This is just a suggestion and it is fine if you don't think it is necessary. The call can also serve as a comment that irq should have been disabled at this point. Cheers, Longman
diff --git a/block/blk-iocost.c b/block/blk-iocost.c index 9dc9323f84ac..384aa15e8260 100644 --- a/block/blk-iocost.c +++ b/block/blk-iocost.c @@ -3166,7 +3166,7 @@ static u64 ioc_qos_prfill(struct seq_file *sf, struct blkg_policy_data *pd, if (!dname) return 0; - spin_lock_irq(&ioc->lock); + spin_lock(&ioc->lock); seq_printf(sf, "%s enable=%d ctrl=%s rpct=%u.%02u rlat=%u wpct=%u.%02u wlat=%u min=%u.%02u max=%u.%02u\n", dname, ioc->enabled, ioc->user_qos_params ? "user" : "auto", ioc->params.qos[QOS_RPPM] / 10000, @@ -3179,7 +3179,7 @@ static u64 ioc_qos_prfill(struct seq_file *sf, struct blkg_policy_data *pd, ioc->params.qos[QOS_MIN] % 10000 / 100, ioc->params.qos[QOS_MAX] / 10000, ioc->params.qos[QOS_MAX] % 10000 / 100); - spin_unlock_irq(&ioc->lock); + spin_unlock(&ioc->lock); return 0; } @@ -3366,14 +3366,14 @@ static u64 ioc_cost_model_prfill(struct seq_file *sf, if (!dname) return 0; - spin_lock_irq(&ioc->lock); + spin_lock(&ioc->lock); seq_printf(sf, "%s ctrl=%s model=linear " "rbps=%llu rseqiops=%llu rrandiops=%llu " "wbps=%llu wseqiops=%llu wrandiops=%llu\n", dname, ioc->user_cost_model ? "user" : "auto", u[I_LCOEF_RBPS], u[I_LCOEF_RSEQIOPS], u[I_LCOEF_RRANDIOPS], u[I_LCOEF_WBPS], u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS]); - spin_unlock_irq(&ioc->lock); + spin_unlock(&ioc->lock); return 0; }
These are called from blkcg_print_blkgs() which already disables IRQs so disabling it again is wrong. It means that IRQs will be enabled slightly earlier than intended, however, so far as I can see, this bug is harmless. Fixes: 35198e323001 ("blk-iocost: read params inside lock in sysfs apis") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Christoph Hellwig <hch@lst.de> --- v2: Fix typo in the subject block/blk-iocost.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)