| Message ID | 20221104023938.2346986-5-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-iocost: random patches to improve configuration | expand |
On Fri, Nov 04, 2022 at 10:39:37AM +0800, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > match_u64() is called inside ioc->lock, which causes smatch static > checker warnings: > > block/blk-iocost.c:3211 ioc_qos_write() warn: sleeping in atomic context > block/blk-iocost.c:3240 ioc_qos_write() warn: sleeping in atomic context > block/blk-iocost.c:3407 ioc_cost_model_write() warn: sleeping in atomic > context > > Fix the problem by introducing a mutex and using it while prasing input > params. s/prasing/parsing/ Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, Nov 04, 2022 at 10:39:37AM +0800, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > match_u64() is called inside ioc->lock, which causes smatch static > checker warnings: > > block/blk-iocost.c:3211 ioc_qos_write() warn: sleeping in atomic context > block/blk-iocost.c:3240 ioc_qos_write() warn: sleeping in atomic context > block/blk-iocost.c:3407 ioc_cost_model_write() warn: sleeping in atomic > context > > Fix the problem by introducing a mutex and using it while prasing input > params. It bothers me that parsing an u64 string requires a GFP_KERNEL memory allocation. > @@ -2801,9 +2806,11 @@ static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos) > { > struct ioc *ioc = rqos_to_ioc(rqos); > > + mutex_lock(&ioc->params_mutex); > spin_lock_irq(&ioc->lock); > ioc_refresh_params(ioc, false); > spin_unlock_irq(&ioc->lock); > + mutex_unlock(&ioc->params_mutex); > } Aren't the params still protected by ioc->lock? Why do we need to grab both? Any chance I can persuade you into updating match_NUMBER() helpers to not use match_strdup()? They can easily disable irq/preemption and use percpu buffers and we won't need most of this patchset. Thanks.
Hi, 在 2022/11/15 6:07, Tejun Heo 写道: > On Fri, Nov 04, 2022 at 10:39:37AM +0800, Yu Kuai wrote: >> From: Yu Kuai <yukuai3@huawei.com> >> >> match_u64() is called inside ioc->lock, which causes smatch static >> checker warnings: >> >> block/blk-iocost.c:3211 ioc_qos_write() warn: sleeping in atomic context >> block/blk-iocost.c:3240 ioc_qos_write() warn: sleeping in atomic context >> block/blk-iocost.c:3407 ioc_cost_model_write() warn: sleeping in atomic >> context >> >> Fix the problem by introducing a mutex and using it while prasing input >> params. > > It bothers me that parsing an u64 string requires a GFP_KERNEL memory > allocation. > >> @@ -2801,9 +2806,11 @@ static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos) >> { >> struct ioc *ioc = rqos_to_ioc(rqos); >> >> + mutex_lock(&ioc->params_mutex); >> spin_lock_irq(&ioc->lock); >> ioc_refresh_params(ioc, false); >> spin_unlock_irq(&ioc->lock); >> + mutex_unlock(&ioc->params_mutex); >> } > > Aren't the params still protected by ioc->lock? Why do we need to grab both? Yes, the params is updated inside ioc->lock, but they can be read without the lock before updating them, which is protected by mutex instead. > > Any chance I can persuade you into updating match_NUMBER() helpers to not > use match_strdup()? They can easily disable irq/preemption and use percpu > buffers and we won't need most of this patchset. Do you mean preallocated percpu buffer? Is there any example I can learn? Anyway, replace match_strdup() to avoid memory allocation sounds good. Thanks, Kuai > > Thanks. >
Hi, Tejun! 在 2022/11/15 6:07, Tejun Heo 写道: > > Any chance I can persuade you into updating match_NUMBER() helpers to not > use match_strdup()? They can easily disable irq/preemption and use percpu > buffers and we won't need most of this patchset. Does the following patch match your proposal? diff --git a/lib/parser.c b/lib/parser.c index bcb23484100e..ded652471919 100644 --- a/lib/parser.c +++ b/lib/parser.c @@ -11,6 +11,24 @@ #include <linux/slab.h> #include <linux/string.h> +#define U64_MAX_SIZE 20 + +static DEFINE_PER_CPU(char, buffer[U64_MAX_SIZE]); + +static char *get_buffer(void) +{ + preempt_disable(); + local_irq_disable(); + + return this_cpu_ptr(buffer); +} + +static void put_buffer(void) +{ + local_irq_enable(); + preempt_enable(); +} + Then match_strdup() and kfree() in match_NUMBER() can be replaced with get_buffer() and put_buffer(). Thanks, Kuai > > Thanks. >
On Thu, Nov 17, 2022 at 07:28:50PM +0800, Yu Kuai wrote: > Hi, Tejun! > > 在 2022/11/15 6:07, Tejun Heo 写道: > > > > > Any chance I can persuade you into updating match_NUMBER() helpers to not > > use match_strdup()? They can easily disable irq/preemption and use percpu > > buffers and we won't need most of this patchset. > > Does the following patch match your proposal? > > diff --git a/lib/parser.c b/lib/parser.c > index bcb23484100e..ded652471919 100644 > --- a/lib/parser.c > +++ b/lib/parser.c > @@ -11,6 +11,24 @@ > #include <linux/slab.h> > #include <linux/string.h> > > +#define U64_MAX_SIZE 20 > + > +static DEFINE_PER_CPU(char, buffer[U64_MAX_SIZE]); > + > +static char *get_buffer(void) > +{ > + preempt_disable(); > + local_irq_disable(); > + > + return this_cpu_ptr(buffer); > +} > + > +static void put_buffer(void) > +{ > + local_irq_enable(); > + preempt_enable(); > +} > + > > Then match_strdup() and kfree() in match_NUMBER() can be replaced with > get_buffer() and put_buffer(). Sorry about the late reply. Yeah, something like this. Thanks.
On 11/22/22 2:10 PM, Tejun Heo wrote: > On Thu, Nov 17, 2022 at 07:28:50PM +0800, Yu Kuai wrote: >> Hi, Tejun! >> >> 在 2022/11/15 6:07, Tejun Heo 写道: >> >>> >>> Any chance I can persuade you into updating match_NUMBER() helpers to not >>> use match_strdup()? They can easily disable irq/preemption and use percpu >>> buffers and we won't need most of this patchset. >> >> Does the following patch match your proposal? >> >> diff --git a/lib/parser.c b/lib/parser.c >> index bcb23484100e..ded652471919 100644 >> --- a/lib/parser.c >> +++ b/lib/parser.c >> @@ -11,6 +11,24 @@ >> #include <linux/slab.h> >> #include <linux/string.h> >> >> +#define U64_MAX_SIZE 20 >> + >> +static DEFINE_PER_CPU(char, buffer[U64_MAX_SIZE]); >> + >> +static char *get_buffer(void) >> +{ >> + preempt_disable(); >> + local_irq_disable(); >> + >> + return this_cpu_ptr(buffer); >> +} >> + >> +static void put_buffer(void) >> +{ >> + local_irq_enable(); >> + preempt_enable(); >> +} >> + >> >> Then match_strdup() and kfree() in match_NUMBER() can be replaced with >> get_buffer() and put_buffer(). > > Sorry about the late reply. Yeah, something like this. Doesn't local_irq_disable() imply preemption disable as well?
On Tue, Nov 22, 2022 at 05:14:29PM -0700, Jens Axboe wrote: > >> Then match_strdup() and kfree() in match_NUMBER() can be replaced with > >> get_buffer() and put_buffer(). > > > > Sorry about the late reply. Yeah, something like this. > > Doesn't local_irq_disable() imply preemption disable as well? Right, I was thinking about spin_lock_irq() which doesn't imply disabling preemption in PREEMPT_RT. local_irq_disable() is actual irq disable even on RT. It should be fine on its own. Thanks.
Hi, Tejun 在 2022/11/23 8:42, Tejun Heo 写道: > On Tue, Nov 22, 2022 at 05:14:29PM -0700, Jens Axboe wrote: >>>> Then match_strdup() and kfree() in match_NUMBER() can be replaced with >>>> get_buffer() and put_buffer(). >>> >>> Sorry about the late reply. Yeah, something like this. Thanks for the feedback. I'll remove patch 4 from this seies and send a new patch separately soon. Thanks, Kuai >> >> Doesn't local_irq_disable() imply preemption disable as well? > > Right, I was thinking about spin_lock_irq() which doesn't imply disabling > preemption in PREEMPT_RT. local_irq_disable() is actual irq disable even on > RT. It should be fine on its own. > > Thanks. >
Hi, Tejun 在 2022/11/23 18:22, Yu Kuai 写道: > Hi, Tejun > > 在 2022/11/23 8:42, Tejun Heo 写道: >> On Tue, Nov 22, 2022 at 05:14:29PM -0700, Jens Axboe wrote: >>>>> Then match_strdup() and kfree() in match_NUMBER() can be replaced with >>>>> get_buffer() and put_buffer(). >>>> >>>> Sorry about the late reply. Yeah, something like this. > I wonder can we just use arary directly in stack? The max size is just 24 bytes, which should be fine: HEX: "0xFFFFFFFFFFFFFFFF" --> 18 DEC: "18446744073709551615" --> 20 OCT: "01777777777777777777777" --> 23 Something like: #define U64_MAX_SIZE 23 static int match_strdup_local(const substring_t *s, char *buf) { size_t len = s->to - s->from; if (len > U64_MAX_SIZE) return -ERANGE; if (!s->from) return -EINVAL; memcpy(buf, s->from, len); buf[len] = '\0'; return 0; } static int match_u64int(substring_t *s, u64 *result, int base) { char buf[U64_MAX_SIZE + 1]; int ret; u64 val; ret = match_strdup_local(s, buf); if (ret) return ret; ret = kstrtoull(buf, base, &val); if (!ret) *result = val;; return ret; }
On Mon, Dec 05, 2022 at 05:39:33PM +0800, Yu Kuai wrote: > Hi, Tejun > > 在 2022/11/23 18:22, Yu Kuai 写道: > > Hi, Tejun > > > > 在 2022/11/23 8:42, Tejun Heo 写道: > > > On Tue, Nov 22, 2022 at 05:14:29PM -0700, Jens Axboe wrote: > > > > > > Then match_strdup() and kfree() in match_NUMBER() can be replaced with > > > > > > get_buffer() and put_buffer(). > > > > > > > > > > Sorry about the late reply. Yeah, something like this. > > > > I wonder can we just use arary directly in stack? The max size is just > 24 bytes, which should be fine: > > HEX: "0xFFFFFFFFFFFFFFFF" --> 18 > DEC: "18446744073709551615" --> 20 > OCT: "01777777777777777777777" --> 23 > > Something like: > #define U64_MAX_SIZE 23 > static int match_strdup_local(const substring_t *s, char *buf) > { > size_t len = s->to - s->from; > > if (len > U64_MAX_SIZE) > return -ERANGE; > > if (!s->from) > return -EINVAL; > > memcpy(buf, s->from, len); > buf[len] = '\0'; > return 0; > } > > static int match_u64int(substring_t *s, u64 *result, int base) > { > char buf[U64_MAX_SIZE + 1]; > int ret; > u64 val; > > ret = match_strdup_local(s, buf); > if (ret) > return ret; > ret = kstrtoull(buf, base, &val); > if (!ret) > *result = val;; > return ret; > } Oh yeah, absolutely. That's much better. Thanks.
diff --git a/block/blk-iocost.c b/block/blk-iocost.c index 2bfecc511dd9..192ad4e0cfc6 100644 --- a/block/blk-iocost.c +++ b/block/blk-iocost.c @@ -404,6 +404,7 @@ struct ioc { bool enabled; + struct mutex params_mutex; struct ioc_params params; struct ioc_margins margins; u32 period_us; @@ -2212,6 +2213,8 @@ static void ioc_timer_fn(struct timer_list *timer) /* how were the latencies during the period? */ ioc_lat_stat(ioc, missed_ppm, &rq_wait_pct); + mutex_lock(&ioc->params_mutex); + /* take care of active iocgs */ spin_lock_irq(&ioc->lock); @@ -2222,6 +2225,7 @@ static void ioc_timer_fn(struct timer_list *timer) period_vtime = now.vnow - ioc->period_at_vtime; if (WARN_ON_ONCE(!period_vtime)) { spin_unlock_irq(&ioc->lock); + mutex_unlock(&ioc->params_mutex); return; } @@ -2419,6 +2423,7 @@ static void ioc_timer_fn(struct timer_list *timer) } spin_unlock_irq(&ioc->lock); + mutex_unlock(&ioc->params_mutex); } static u64 adjust_inuse_and_calc_cost(struct ioc_gq *iocg, u64 vtime, @@ -2801,9 +2806,11 @@ static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos) { struct ioc *ioc = rqos_to_ioc(rqos); + mutex_lock(&ioc->params_mutex); spin_lock_irq(&ioc->lock); ioc_refresh_params(ioc, false); spin_unlock_irq(&ioc->lock); + mutex_unlock(&ioc->params_mutex); } static void ioc_rqos_exit(struct rq_qos *rqos) @@ -2862,6 +2869,7 @@ static int blk_iocost_init(struct gendisk *disk) rqos->ops = &ioc_rqos_ops; rqos->q = q; + mutex_init(&ioc->params_mutex); spin_lock_init(&ioc->lock); timer_setup(&ioc->timer, ioc_timer_fn, 0); INIT_LIST_HEAD(&ioc->active_iocgs); @@ -2874,10 +2882,12 @@ static int blk_iocost_init(struct gendisk *disk) atomic64_set(&ioc->cur_period, 0); atomic_set(&ioc->hweight_gen, 0); + mutex_lock(&ioc->params_mutex); spin_lock_irq(&ioc->lock); ioc->autop_idx = AUTOP_INVALID; ioc_refresh_params(ioc, true); spin_unlock_irq(&ioc->lock); + mutex_unlock(&ioc->params_mutex); /* * rqos must be added before activation to allow iocg_pd_init() to @@ -3197,7 +3207,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input, blk_mq_freeze_queue(disk->queue); blk_mq_quiesce_queue(disk->queue); - spin_lock_irq(&ioc->lock); + mutex_lock(&ioc->params_mutex); memcpy(qos, ioc->params.qos, sizeof(qos)); enable = ioc->enabled; user = ioc->user_qos_params; @@ -3278,6 +3288,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input, if (qos[QOS_MIN] > qos[QOS_MAX]) goto out_unlock; + spin_lock_irq(&ioc->lock); if (enable) { blk_stat_enable_accounting(disk->queue); blk_queue_flag_set(QUEUE_FLAG_RQ_ALLOC_TIME, disk->queue); @@ -3298,9 +3309,10 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input, ioc_refresh_params(ioc, true); ret = nbytes; + spin_unlock_irq(&ioc->lock); out_unlock: - spin_unlock_irq(&ioc->lock); + mutex_unlock(&ioc->params_mutex); blk_mq_unquiesce_queue(disk->queue); blk_mq_unfreeze_queue(disk->queue); @@ -3385,7 +3397,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input, blk_mq_freeze_queue(q); blk_mq_quiesce_queue(q); - spin_lock_irq(&ioc->lock); + mutex_lock(&ioc->params_mutex); memcpy(u, ioc->params.i_lcoefs, sizeof(u)); user = ioc->user_cost_model; @@ -3431,6 +3443,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input, user = true; } + spin_lock_irq(&ioc->lock); if (user) { memcpy(ioc->params.i_lcoefs, u, sizeof(u)); ioc->user_cost_model = true; @@ -3440,9 +3453,10 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input, ioc_refresh_params(ioc, true); ret = nbytes; + spin_unlock_irq(&ioc->lock); out_unlock: - spin_unlock_irq(&ioc->lock); + mutex_unlock(&ioc->params_mutex); blk_mq_unquiesce_queue(q); blk_mq_unfreeze_queue(q);