diff mbox series

[v2,4/5] blk-iocost: fix sleeping in atomic context warnning

Message ID 20221104023938.2346986-5-yukuai1@huaweicloud.com
State New
Headers
Series blk-iocost: random patches to improve configuration |

Commit Message

Yu Kuai Nov. 4, 2022, 2:39 a.m. UTC 
  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.

Fixes: 2c0647988433 ("blk-iocost: don't release 'ioc->lock' while updating params")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Comments

Christoph Hellwig Nov. 4, 2022, 5:17 a.m. UTC | #1 
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>
Christoph Hellwig Nov. 7, 2022, 5:56 a.m. UTC | #2 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Tejun Heo Nov. 14, 2022, 10:07 p.m. UTC | #3 
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.
Yu Kuai Nov. 15, 2022, 1:16 a.m. UTC | #4 
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.
>
Yu Kuai Nov. 17, 2022, 11:28 a.m. UTC | #5 
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.
>
Tejun Heo Nov. 22, 2022, 9:10 p.m. UTC | #6 
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.
Jens Axboe Nov. 23, 2022, 12:14 a.m. UTC | #7 
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?
Tejun Heo Nov. 23, 2022, 12:42 a.m. UTC | #8 
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.
Yu Kuai Nov. 23, 2022, 10:22 a.m. UTC | #9 
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.
>
Yu Kuai Dec. 5, 2022, 9:39 a.m. UTC | #10 
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;
  }
Tejun Heo Dec. 8, 2022, 3:59 p.m. UTC | #11 
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 mbox series

Patch

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);