On Wed, Oct 02, 2024 at 02:40:52PM -0400, Waiman Long wrote: > > 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. I mean it's good to think about preventing future bugs. I just feel like when it comes to adding asserts probably that's more useful when there are a lot of call paths. Meanwhile if we add a static checker rule then we're probably going to find bugs. Boom, maybe I've found one already?: block/blk-iocost.c:3144 ioc_weight_write() warn: expected irq_disable for '&iocg->ioc->lock' block/blk-iocost.c 3090 static ssize_t ioc_weight_write(struct kernfs_open_file *of, char *buf, 3091 size_t nbytes, loff_t off) 3092 { 3093 struct blkcg *blkcg = css_to_blkcg(of_css(of)); 3094 struct ioc_cgrp *iocc = blkcg_to_iocc(blkcg); 3095 struct blkg_conf_ctx ctx; 3096 struct ioc_now now; 3097 struct ioc_gq *iocg; 3098 u32 v; 3099 int ret; 3100 3101 if (!strchr(buf, ':')) { 3102 struct blkcg_gq *blkg; 3103 3104 if (!sscanf(buf, "default %u", &v) && !sscanf(buf, "%u", &v)) 3105 return -EINVAL; 3106 3107 if (v < CGROUP_WEIGHT_MIN || v > CGROUP_WEIGHT_MAX) 3108 return -EINVAL; 3109 3110 spin_lock_irq(&blkcg->lock); Here we disable IRQs. 3111 iocc->dfl_weight = v * WEIGHT_ONE; 3112 hlist_for_each_entry(blkg, &blkcg->blkg_list, blkcg_node) { 3113 struct ioc_gq *iocg = blkg_to_iocg(blkg); 3114 3115 if (iocg) { 3116 spin_lock(&iocg->ioc->lock); So this is fine. 3117 ioc_now(iocg->ioc, &now); 3118 weight_updated(iocg, &now); 3119 spin_unlock(&iocg->ioc->lock); 3120 } 3121 } 3122 spin_unlock_irq(&blkcg->lock); 3123 3124 return nbytes; 3125 } 3126 3127 blkg_conf_init(&ctx, buf); 3128 3129 ret = blkg_conf_prep(blkcg, &blkcg_policy_iocost, &ctx); 3130 if (ret) 3131 goto err; 3132 3133 iocg = blkg_to_iocg(ctx.blkg); 3134 3135 if (!strncmp(ctx.body, "default", 7)) { 3136 v = 0; 3137 } else { 3138 if (!sscanf(ctx.body, "%u", &v)) 3139 goto einval; 3140 if (v < CGROUP_WEIGHT_MIN || v > CGROUP_WEIGHT_MAX) 3141 goto einval; 3142 } 3143 3144 spin_lock(&iocg->ioc->lock); But why is this not spin_lock_irq()? I haven't analyzed this so maybe it's fine. 3145 iocg->cfg_weight = v * WEIGHT_ONE; 3146 ioc_now(iocg->ioc, &now); 3147 weight_updated(iocg, &now); 3148 spin_unlock(&iocg->ioc->lock); 3149 3150 blkg_conf_exit(&ctx); 3151 return nbytes; 3152 3153 einval: 3154 ret = -EINVAL; 3155 err: 3156 blkg_conf_exit(&ctx); 3157 return ret; 3158 } regards, dan carpenter