[PATCH v2 0/5] blk-iocost: random patches to improve configuration

[PATCH v2 0/5] blk-iocost: random patches to improve configuration

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v2:
 - add review tag from Christoph for patch 1-3
 - add a mutex to fix warnning as suggested by Christoph
 - add patch 5

Yu Kuai (5):
  blk-iocost: cleanup ioc_qos_write() and ioc_cost_model_write()
  blk-iocost: improve hanlder of match_u64()
  blk-iocost: don't allow to configure bio based device
  blk-iocost: fix sleeping in atomic context warnning
  blk-iocost: read params inside lock in sysfs apis

 block/blk-iocost.c | 116 +++++++++++++++++++++++++++++----------------
 1 file changed, 74 insertions(+), 42 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] blk-iocost: cleanup ioc_qos_write() and ioc_cost_model_write()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

There are no functional changes, just to make the code a litter cleaner
and follow up patches easier.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-iocost.c | 62 +++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 37 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index f01359906c83..fd495e823db2 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3185,7 +3185,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	if (!ioc) {
 		ret = blk_iocost_init(disk);
 		if (ret)
-			goto err;
+			goto out;
 		ioc = q_to_ioc(disk->queue);
 	}
 
@@ -3197,6 +3197,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	enable = ioc->enabled;
 	user = ioc->user_qos_params;
 
+	ret = -EINVAL;
 	while ((p = strsep(&input, " \t\n"))) {
 		substring_t args[MAX_OPT_ARGS];
 		char buf[32];
@@ -3218,7 +3219,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 			else if (!strcmp(buf, "user"))
 				user = true;
 			else
-				goto einval;
+				goto out_unlock;
 			continue;
 		}
 
@@ -3228,39 +3229,39 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 		case QOS_WPPM:
 			if (match_strlcpy(buf, &args[0], sizeof(buf)) >=
 			    sizeof(buf))
-				goto einval;
+				goto out_unlock;
 			if (cgroup_parse_float(buf, 2, &v))
-				goto einval;
+				goto out_unlock;
 			if (v < 0 || v > 10000)
-				goto einval;
+				goto out_unlock;
 			qos[tok] = v * 100;
 			break;
 		case QOS_RLAT:
 		case QOS_WLAT:
 			if (match_u64(&args[0], &v))
-				goto einval;
+				goto out_unlock;
 			qos[tok] = v;
 			break;
 		case QOS_MIN:
 		case QOS_MAX:
 			if (match_strlcpy(buf, &args[0], sizeof(buf)) >=
 			    sizeof(buf))
-				goto einval;
+				goto out_unlock;
 			if (cgroup_parse_float(buf, 2, &v))
-				goto einval;
+				goto out_unlock;
 			if (v < 0)
-				goto einval;
+				goto out_unlock;
 			qos[tok] = clamp_t(s64, v * 100,
 					   VRATE_MIN_PPM, VRATE_MAX_PPM);
 			break;
 		default:
-			goto einval;
+			goto out_unlock;
 		}
 		user = true;
 	}
 
 	if (qos[QOS_MIN] > qos[QOS_MAX])
-		goto einval;
+		goto out_unlock;
 
 	if (enable) {
 		blk_stat_enable_accounting(disk->queue);
@@ -3281,21 +3282,14 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	}
 
 	ioc_refresh_params(ioc, true);
-	spin_unlock_irq(&ioc->lock);
+	ret = nbytes;
 
-	blk_mq_unquiesce_queue(disk->queue);
-	blk_mq_unfreeze_queue(disk->queue);
-
-	blkdev_put_no_open(bdev);
-	return nbytes;
-einval:
+out_unlock:
 	spin_unlock_irq(&ioc->lock);
-
 	blk_mq_unquiesce_queue(disk->queue);
 	blk_mq_unfreeze_queue(disk->queue);
 
-	ret = -EINVAL;
-err:
+out:
 	blkdev_put_no_open(bdev);
 	return ret;
 }
@@ -3364,7 +3358,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	if (!ioc) {
 		ret = blk_iocost_init(bdev->bd_disk);
 		if (ret)
-			goto err;
+			goto out;
 		ioc = q_to_ioc(q);
 	}
 
@@ -3375,6 +3369,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	memcpy(u, ioc->params.i_lcoefs, sizeof(u));
 	user = ioc->user_cost_model;
 
+	ret = -EINVAL;
 	while ((p = strsep(&input, " \t\n"))) {
 		substring_t args[MAX_OPT_ARGS];
 		char buf[32];
@@ -3392,20 +3387,20 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 			else if (!strcmp(buf, "user"))
 				user = true;
 			else
-				goto einval;
+				goto out_unlock;
 			continue;
 		case COST_MODEL:
 			match_strlcpy(buf, &args[0], sizeof(buf));
 			if (strcmp(buf, "linear"))
-				goto einval;
+				goto out_unlock;
 			continue;
 		}
 
 		tok = match_token(p, i_lcoef_tokens, args);
 		if (tok == NR_I_LCOEFS)
-			goto einval;
+			goto out_unlock;
 		if (match_u64(&args[0], &v))
-			goto einval;
+			goto out_unlock;
 		u[tok] = v;
 		user = true;
 	}
@@ -3416,23 +3411,16 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 	} else {
 		ioc->user_cost_model = false;
 	}
-	ioc_refresh_params(ioc, true);
-	spin_unlock_irq(&ioc->lock);
 
-	blk_mq_unquiesce_queue(q);
-	blk_mq_unfreeze_queue(q);
-
-	blkdev_put_no_open(bdev);
-	return nbytes;
+	ioc_refresh_params(ioc, true);
+	ret = nbytes;
 
-einval:
+out_unlock:
 	spin_unlock_irq(&ioc->lock);
-
 	blk_mq_unquiesce_queue(q);
 	blk_mq_unfreeze_queue(q);
 
-	ret = -EINVAL;
-err:
+out:
 	blkdev_put_no_open(bdev);
 	return ret;
 }
-- 
2.31.1

[PATCH v2 2/5] blk-iocost: improve hanlder of match_u64()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

1) There are one place that return value of match_u64() is not checked.
2) If match_u64() failed, return value is set to -EINVAL despite that
   there are other possible errnos.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-iocost.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index fd495e823db2..c532129a1456 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3202,6 +3202,7 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 		substring_t args[MAX_OPT_ARGS];
 		char buf[32];
 		int tok;
+		int err;
 		s64 v;
 
 		if (!*p)
@@ -3209,7 +3210,12 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 
 		switch (match_token(p, qos_ctrl_tokens, args)) {
 		case QOS_ENABLE:
-			match_u64(&args[0], &v);
+			err = match_u64(&args[0], &v);
+			if (err) {
+				ret = err;
+				goto out_unlock;
+			}
+
 			enable = v;
 			continue;
 		case QOS_CTRL:
@@ -3238,8 +3244,12 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 			break;
 		case QOS_RLAT:
 		case QOS_WLAT:
-			if (match_u64(&args[0], &v))
+			err = match_u64(&args[0], &v);
+			if (err) {
+				ret = err;
 				goto out_unlock;
+			}
+
 			qos[tok] = v;
 			break;
 		case QOS_MIN:
@@ -3374,6 +3384,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 		substring_t args[MAX_OPT_ARGS];
 		char buf[32];
 		int tok;
+		int err;
 		u64 v;
 
 		if (!*p)
@@ -3399,8 +3410,13 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 		tok = match_token(p, i_lcoef_tokens, args);
 		if (tok == NR_I_LCOEFS)
 			goto out_unlock;
-		if (match_u64(&args[0], &v))
+
+		err = match_u64(&args[0], &v);
+		if (err) {
+			ret = err;
 			goto out_unlock;
+		}
+
 		u[tok] = v;
 		user = true;
 	}
-- 
2.31.1

[PATCH v2 3/5] blk-iocost: don't allow to configure bio based device

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

iocost is based on rq_qos, which can only work for request based device,
thus it doesn't make sense to configure iocost for bio based device.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-iocost.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index c532129a1456..2bfecc511dd9 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3181,6 +3181,11 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 		return PTR_ERR(bdev);
 
 	disk = bdev->bd_disk;
+	if (!queue_is_mq(disk->queue)) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	ioc = q_to_ioc(disk->queue);
 	if (!ioc) {
 		ret = blk_iocost_init(disk);
@@ -3364,6 +3369,11 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
 		return PTR_ERR(bdev);
 
 	q = bdev_get_queue(bdev);
+	if (!queue_is_mq(q)) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	ioc = q_to_ioc(q);
 	if (!ioc) {
 		ret = blk_iocost_init(bdev->bd_disk);
-- 
2.31.1

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

[Yu Kuai]

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

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

[PATCH v2 5/5] blk-iocost: read params inside lock in sysfs apis

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Otherwise, user might get abnormal values if params is updated
concurrently.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-iocost.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 192ad4e0cfc6..7d682ce0bee6 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3135,6 +3135,7 @@ static u64 ioc_qos_prfill(struct seq_file *sf, struct blkg_policy_data *pd,
 	if (!dname)
 		return 0;
 
+	mutex_lock(&ioc->params_mutex);
 	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,
@@ -3147,6 +3148,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);
+	mutex_unlock(&ioc->params_mutex);
 	return 0;
 }
 
@@ -3331,12 +3333,14 @@ static u64 ioc_cost_model_prfill(struct seq_file *sf,
 	if (!dname)
 		return 0;
 
+	mutex_lock(&ioc->params_mutex);
 	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]);
+	mutex_unlock(&ioc->params_mutex);
 	return 0;
 }
 
-- 
2.31.1

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

[Christoph Hellwig]

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>

Re: [PATCH v2 5/5] blk-iocost: read params inside lock in sysfs apis

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 5/5] blk-iocost: read params inside lock in sysfs apis

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 0/5] blk-iocost: random patches to improve configuration

[Yu Kuai]

Hi, Jens

Can you apply this patchset?

Thanks!
Kuai

在 2022/11/04 10:39, Yu Kuai 写道:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Changes in v2:
>   - add review tag from Christoph for patch 1-3
>   - add a mutex to fix warnning as suggested by Christoph
>   - add patch 5
> 
> Yu Kuai (5):
>    blk-iocost: cleanup ioc_qos_write() and ioc_cost_model_write()
>    blk-iocost: improve hanlder of match_u64()
>    blk-iocost: don't allow to configure bio based device
>    blk-iocost: fix sleeping in atomic context warnning
>    blk-iocost: read params inside lock in sysfs apis
> 
>   block/blk-iocost.c | 116 +++++++++++++++++++++++++++++----------------
>   1 file changed, 74 insertions(+), 42 deletions(-)
> 

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

[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?

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.

-- 
tejun

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

[Yu Kuai]

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.
> 

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

[Yu Kuai]

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.
> 

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

[Tejun Heo]

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.

-- 
tejun

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

[Jens Axboe]

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?

-- 
Jens Axboe

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

[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.
> 
> 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.

-- 
tejun

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

[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.

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.
> 

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

[Yu Kuai]

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

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

[Tejun Heo]

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.

-- 
tejun