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

[permalink] [raw]

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

From: Yu Kuai <[email protected]>

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

Signed-off-by: Yu Kuai <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
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

2022-11-04 02:35:01

[permalink] [raw]

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

From: Yu Kuai <[email protected]>

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

Signed-off-by: Yu Kuai <[email protected]>
---
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

2022-11-04 02:45:45

[permalink] [raw]

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

From: Yu Kuai <[email protected]>

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 <[email protected]>
Signed-off-by: Yu Kuai <[email protected]>
---
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

2022-11-04 02:57:48

[permalink] [raw]

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

From: Yu Kuai <[email protected]>

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 <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
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

2022-11-04 05:40:33

[permalink] [raw]

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

Looks good:

Reviewed-by: Christoph Hellwig <[email protected]>

2022-11-04 05:42:41

[permalink] [raw]

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

2022-11-07 06:27:34

[permalink] [raw]

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

Looks good:

Reviewed-by: Christoph Hellwig <[email protected]>

2022-11-07 07:06:34

[permalink] [raw]

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

Looks good:

Reviewed-by: Christoph Hellwig <[email protected]>

2022-11-12 07:03:38

[permalink] [raw]

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

Hi, Jens

Can you apply this patchset?

Thanks!
Kuai

?? 2022/11/04 10:39, Yu Kuai д??:
> From: Yu Kuai <[email protected]>
>
> 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(-)
>

2022-11-14 22:34:48

[permalink] [raw]

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

On Fri, Nov 04, 2022 at 10:39:37AM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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

2022-11-15 01:59:01

[permalink] [raw]

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

Hi,

?? 2022/11/15 6:07, Tejun Heo д??:
> On Fri, Nov 04, 2022 at 10:39:37AM +0800, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> 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.
>

2022-11-17 12:27:30

[permalink] [raw]

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

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

2022-11-22 21:46:37

[permalink] [raw]

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

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

2022-11-23 00:59:39

by Jens Axboe

[permalink] [raw]

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

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

2022-11-23 01:13:48

[permalink] [raw]

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

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

2022-11-23 10:52:31

[permalink] [raw]

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

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

2022-12-05 09:53:53

[permalink] [raw]

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

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

2022-12-08 17:32:35