[PATCH -next v2 0/2] bugfix for blk-throttle

[PATCH -next v2 0/2] bugfix for blk-throttle

[Yu Kuai]

Changes in v2:
 - use a new solution suggested by Ming
 - change the title of patch 1
 - add patch 2

Patch 1 fix that blk-throttle can't work if multiple bios are throttle,
Patch 2 fix io hung due to configuration updates.

Previous version:
v1 : https://lore.kernel.org/all/20220517134909.2910251-1-yukuai3@huawei.com/

Yu Kuai (2):
  blk-throttle: fix that io throttle can only work for single bio
  blk-throttle: fix io hung due to configuration updates

 block/blk-throttle.c | 88 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 19 deletions(-)

-- 
2.31.1

[PATCH -next v2 1/2] blk-throttle: fix that io throttle can only work for single bio

[Yu Kuai]

commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
introduce a new problem, for example:

[root@localhost ~]# echo "8:0 1024" > /sys/fs/cgroup/blkio/blkio.throttle.write_bps_device
[root@localhost ~]# echo $$ > /sys/fs/cgroup/blkio/cgroup.procs
[root@localhost ~]# dd if=/dev/zero of=/dev/sda bs=10k count=1 oflag=direct &
[1] 620
[root@localhost ~]# dd if=/dev/zero of=/dev/sda bs=10k count=1 oflag=direct &
[2] 626
[root@localhost ~]# 1+0 records in
1+0 records out
10240 bytes (10 kB, 10 KiB) copied, 10.0038 s, 1.0 kB/s1+0 records in
1+0 records out

10240 bytes (10 kB, 10 KiB) copied, 9.23076 s, 1.1 kB/s
-> the second bio is issued after 10s instead of 20s.

This is because if some bios are already queued, current bio is queued
directly and the flag 'BIO_THROTTLED' is set. And later, when former
bios are dispatched, this bio will be dispatched without waiting at all,
this is due to tg_with_in_bps_limit() return 0 for this bio.

In order to fix the problem, don't skip flaged bio in
tg_with_in_bps_limit(), and for the problem that split bio can be
double accounted, compensate the over-accounting in __blk_throtl_bio().

Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 447e1b8722f7..6f69859eae23 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
 	unsigned int bio_size = throtl_bio_data_size(bio);
 
 	/* no need to throttle if this bio's bytes have been accounted */
-	if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
+	if (bps_limit == U64_MAX) {
 		if (wait)
 			*wait = 0;
 		return true;
@@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
 	unsigned int bio_size = throtl_bio_data_size(bio);
 
 	/* Charge the bio to the group */
-	if (!bio_flagged(bio, BIO_THROTTLED)) {
-		tg->bytes_disp[rw] += bio_size;
-		tg->last_bytes_disp[rw] += bio_size;
-	}
-
+	tg->bytes_disp[rw] += bio_size;
+	tg->last_bytes_disp[rw] += bio_size;
 	tg->io_disp[rw]++;
 	tg->last_io_disp[rw]++;
 
@@ -2121,6 +2118,21 @@ bool __blk_throtl_bio(struct bio *bio)
 			tg->last_low_overflow_time[rw] = jiffies;
 		throtl_downgrade_check(tg);
 		throtl_upgrade_check(tg);
+
+		/*
+		 * re-entered bio has accounted bytes already, so try to
+		 * compensate previous over-accounting. However, if new
+		 * slice is started, just forget it.
+		 */
+		if (bio_flagged(bio, BIO_THROTTLED)) {
+			unsigned int bio_size = throtl_bio_data_size(bio);
+
+			if (tg->bytes_disp[rw] >= bio_size)
+				tg->bytes_disp[rw] -= bio_size;
+			if (tg->last_bytes_disp[rw] > bio_size)
+				tg->last_bytes_disp[rw] -= bio_size;
+		}
+
 		/* throtl is FIFO - if bios are already queued, should queue */
 		if (sq->nr_queued[rw])
 			break;
-- 
2.31.1

Re: [PATCH -next v2 1/2] blk-throttle: fix that io throttle can only work for single bio

[Ming Lei]

On Wed, May 18, 2022 at 03:27:50PM +0800, Yu Kuai wrote:
> commit 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
> introduce a new problem, for example:
> 
> [root@localhost ~]# echo "8:0 1024" > /sys/fs/cgroup/blkio/blkio.throttle.write_bps_device
> [root@localhost ~]# echo $$ > /sys/fs/cgroup/blkio/cgroup.procs
> [root@localhost ~]# dd if=/dev/zero of=/dev/sda bs=10k count=1 oflag=direct &
> [1] 620
> [root@localhost ~]# dd if=/dev/zero of=/dev/sda bs=10k count=1 oflag=direct &
> [2] 626
> [root@localhost ~]# 1+0 records in
> 1+0 records out
> 10240 bytes (10 kB, 10 KiB) copied, 10.0038 s, 1.0 kB/s1+0 records in
> 1+0 records out
> 
> 10240 bytes (10 kB, 10 KiB) copied, 9.23076 s, 1.1 kB/s
> -> the second bio is issued after 10s instead of 20s.
> 
> This is because if some bios are already queued, current bio is queued
> directly and the flag 'BIO_THROTTLED' is set. And later, when former
> bios are dispatched, this bio will be dispatched without waiting at all,
> this is due to tg_with_in_bps_limit() return 0 for this bio.
> 
> In order to fix the problem, don't skip flaged bio in
> tg_with_in_bps_limit(), and for the problem that split bio can be
> double accounted, compensate the over-accounting in __blk_throtl_bio().
> 
> Fixes: 9f5ede3c01f9 ("block: throttle split bio in case of iops limit")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  block/blk-throttle.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 447e1b8722f7..6f69859eae23 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -811,7 +811,7 @@ static bool tg_with_in_bps_limit(struct throtl_grp *tg, struct bio *bio,
>  	unsigned int bio_size = throtl_bio_data_size(bio);
>  
>  	/* no need to throttle if this bio's bytes have been accounted */
> -	if (bps_limit == U64_MAX || bio_flagged(bio, BIO_THROTTLED)) {
> +	if (bps_limit == U64_MAX) {
>  		if (wait)
>  			*wait = 0;
>  		return true;
> @@ -921,11 +921,8 @@ static void throtl_charge_bio(struct throtl_grp *tg, struct bio *bio)
>  	unsigned int bio_size = throtl_bio_data_size(bio);
>  
>  	/* Charge the bio to the group */
> -	if (!bio_flagged(bio, BIO_THROTTLED)) {
> -		tg->bytes_disp[rw] += bio_size;
> -		tg->last_bytes_disp[rw] += bio_size;
> -	}
> -
> +	tg->bytes_disp[rw] += bio_size;
> +	tg->last_bytes_disp[rw] += bio_size;
>  	tg->io_disp[rw]++;
>  	tg->last_io_disp[rw]++;
>  
> @@ -2121,6 +2118,21 @@ bool __blk_throtl_bio(struct bio *bio)
>  			tg->last_low_overflow_time[rw] = jiffies;
>  		throtl_downgrade_check(tg);
>  		throtl_upgrade_check(tg);
> +
> +		/*
> +		 * re-entered bio has accounted bytes already, so try to
> +		 * compensate previous over-accounting. However, if new
> +		 * slice is started, just forget it.
> +		 */
> +		if (bio_flagged(bio, BIO_THROTTLED)) {
> +			unsigned int bio_size = throtl_bio_data_size(bio);
> +
> +			if (tg->bytes_disp[rw] >= bio_size)
> +				tg->bytes_disp[rw] -= bio_size;
> +			if (tg->last_bytes_disp[rw] > bio_size)
> +				tg->last_bytes_disp[rw] -= bio_size;

The above check should be:
			if (tg->last_bytes_disp[rw] >= bio_size)

Otherwise, this patch looks fine for me.


Thanks,
Ming

[PATCH -next v2 2/2] blk-throttle: fix io hung due to configuration updates

[Yu Kuai]

If new configuration is submitted while a bio is throttled, then new
waiting time is recaculated regardless that the bio might aready wait
for some time:

tg_conf_updated
 throtl_start_new_slice
  tg_update_disptime
  throtl_schedule_next_dispatch

Then io hung can be triggered by always submmiting new configuration
before the throttled bio is dispatched.

Fix the problem by respecting the time that throttled bio aready waited.
In order to do that, instead of start new slice in tg_conf_updated(),
just update 'bytes_disp' and 'io_disp' based on the new configuration.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-throttle.c | 64 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 13 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 6f69859eae23..1c3dfd3d3d9a 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1271,7 +1271,42 @@ static int tg_print_conf_uint(struct seq_file *sf, void *v)
 	return 0;
 }
 
-static void tg_conf_updated(struct throtl_grp *tg, bool global)
+static u64 throtl_update_bytes_disp(u64 dispatched, u64 new_limit,
+				    u64 old_limit)
+{
+	if (new_limit == old_limit)
+		return dispatched;
+
+	if (new_limit == U64_MAX)
+		return 0;
+
+	return dispatched * new_limit / old_limit;
+}
+
+static u32 throtl_update_io_disp(u32 dispatched, u32 new_limit, u32 old_limit)
+{
+	if (new_limit == old_limit)
+		return dispatched;
+
+	if (new_limit == UINT_MAX)
+		return 0;
+
+	return dispatched * new_limit / old_limit;
+}
+
+static void throtl_update_slice(struct throtl_grp *tg, u64 *old_limits)
+{
+	tg->bytes_disp[READ] = throtl_update_bytes_disp(tg->bytes_disp[READ],
+			tg_bps_limit(tg, READ), old_limits[0]);
+	tg->bytes_disp[WRITE] = throtl_update_bytes_disp(tg->bytes_disp[WRITE],
+			tg_bps_limit(tg, WRITE), old_limits[1]);
+	tg->io_disp[READ] = throtl_update_io_disp(tg->io_disp[READ],
+			tg_iops_limit(tg, READ), (u32)old_limits[2]);
+	tg->io_disp[WRITE] = throtl_update_io_disp(tg->io_disp[WRITE],
+			tg_iops_limit(tg, WRITE), (u32)old_limits[3]);
+}
+
+static void tg_conf_updated(struct throtl_grp *tg, u64 *old_limits, bool global)
 {
 	struct throtl_service_queue *sq = &tg->service_queue;
 	struct cgroup_subsys_state *pos_css;
@@ -1310,16 +1345,7 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 				parent_tg->latency_target);
 	}
 
-	/*
-	 * We're already holding queue_lock and know @tg is valid.  Let's
-	 * apply the new config directly.
-	 *
-	 * Restart the slices for both READ and WRITES. It might happen
-	 * that a group's limit are dropped suddenly and we don't want to
-	 * account recently dispatched IO with new low rate.
-	 */
-	throtl_start_new_slice(tg, READ);
-	throtl_start_new_slice(tg, WRITE);
+	throtl_update_slice(tg, old_limits);
 
 	if (tg->flags & THROTL_TG_PENDING) {
 		tg_update_disptime(tg);
@@ -1327,6 +1353,14 @@ static void tg_conf_updated(struct throtl_grp *tg, bool global)
 	}
 }
 
+static void tg_get_limits(struct throtl_grp *tg, u64 *limits)
+{
+	limits[0] = tg_bps_limit(tg, READ);
+	limits[1] = tg_bps_limit(tg, WRITE);
+	limits[2] = tg_iops_limit(tg, READ);
+	limits[3] = tg_iops_limit(tg, WRITE);
+}
+
 static ssize_t tg_set_conf(struct kernfs_open_file *of,
 			   char *buf, size_t nbytes, loff_t off, bool is_u64)
 {
@@ -1335,6 +1369,7 @@ static ssize_t tg_set_conf(struct kernfs_open_file *of,
 	struct throtl_grp *tg;
 	int ret;
 	u64 v;
+	u64 old_limits[4];
 
 	ret = blkg_conf_prep(blkcg, &blkcg_policy_throtl, buf, &ctx);
 	if (ret)
@@ -1347,13 +1382,14 @@ static ssize_t tg_set_conf(struct kernfs_open_file *of,
 		v = U64_MAX;
 
 	tg = blkg_to_tg(ctx.blkg);
+	tg_get_limits(tg, old_limits);
 
 	if (is_u64)
 		*(u64 *)((void *)tg + of_cft(of)->private) = v;
 	else
 		*(unsigned int *)((void *)tg + of_cft(of)->private) = v;
 
-	tg_conf_updated(tg, false);
+	tg_conf_updated(tg, old_limits, false);
 	ret = 0;
 out_finish:
 	blkg_conf_finish(&ctx);
@@ -1523,6 +1559,7 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 	struct blkg_conf_ctx ctx;
 	struct throtl_grp *tg;
 	u64 v[4];
+	u64 old_limits[4];
 	unsigned long idle_time;
 	unsigned long latency_time;
 	int ret;
@@ -1533,6 +1570,7 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 		return ret;
 
 	tg = blkg_to_tg(ctx.blkg);
+	tg_get_limits(tg, old_limits);
 
 	v[0] = tg->bps_conf[READ][index];
 	v[1] = tg->bps_conf[WRITE][index];
@@ -1624,7 +1662,7 @@ static ssize_t tg_set_limit(struct kernfs_open_file *of,
 			tg->td->limit_index = LIMIT_LOW;
 	} else
 		tg->td->limit_index = LIMIT_MAX;
-	tg_conf_updated(tg, index == LIMIT_LOW &&
+	tg_conf_updated(tg, old_limits, index == LIMIT_LOW &&
 		tg->td->limit_valid[LIMIT_LOW]);
 	ret = 0;
 out_finish:
-- 
2.31.1

Re: [PATCH -next v2 2/2] blk-throttle: fix io hung due to configuration updates

[kernel test robot]

Hi Yu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20220517]

url:    https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
base:    47c1c54d1bcd0a69a56b49473bc20f17b70e5242
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220518/202205182347.tMOOqyfL-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f8345dbaf4ed491742aab29834aff66b4930c087
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
        git checkout f8345dbaf4ed491742aab29834aff66b4930c087
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: block/blk-throttle.o: in function `tg_conf_updated':
>> blk-throttle.c:(.text+0x25bc): undefined reference to `__udivdi3'
>> m68k-linux-ld: blk-throttle.c:(.text+0x2626): undefined reference to `__udivdi3'
   `.exit.text' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH -next v2 2/2] blk-throttle: fix io hung due to configuration updates

[yukuai (C)]

在 2022/05/18 23:52, kernel test robot 写道:
> Hi Yu,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on next-20220517]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
> base:    47c1c54d1bcd0a69a56b49473bc20f17b70e5242
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220518/202205182347.tMOOqyfL-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 11.3.0
> reproduce (this is a W=1 build):
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # https://github.com/intel-lab-lkp/linux/commit/f8345dbaf4ed491742aab29834aff66b4930c087
>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>          git fetch --no-tags linux-review Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
>          git checkout f8345dbaf4ed491742aab29834aff66b4930c087
>          # save the config file
>          mkdir build_dir && cp config build_dir/.config
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>     m68k-linux-ld: block/blk-throttle.o: in function `tg_conf_updated':
>>> blk-throttle.c:(.text+0x25bc): undefined reference to `__udivdi3'
>>> m68k-linux-ld: blk-throttle.c:(.text+0x2626): undefined reference to `__udivdi3'
Hi,

I'm confused here, the only place that I can relate to this:

	return dispatched * new_limit / old_limit;

However, I don't understand yet why this is problematic...
>     `.exit.text' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
> 

Re: [PATCH -next v2 2/2] blk-throttle: fix io hung due to configuration updates

[yukuai (C)]

在 2022/05/19 10:11, yukuai (C) 写道:
> 
> 
> 在 2022/05/18 23:52, kernel test robot 写道:
>> Hi Yu,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on next-20220517]
>>
>> url:    
>> https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/bugfix-for-blk-throttle/20220518-151713 
>>
>> base:    47c1c54d1bcd0a69a56b49473bc20f17b70e5242
>> config: m68k-allyesconfig 
>> (https://download.01.org/0day-ci/archive/20220518/202205182347.tMOOqyfL-lkp@intel.com/config) 
>>
>> compiler: m68k-linux-gcc (GCC) 11.3.0
>> reproduce (this is a W=1 build):
>>          wget 
>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross 
>> -O ~/bin/make.cross
>>          chmod +x ~/bin/make.cross
>>          # 
>> https://github.com/intel-lab-lkp/linux/commit/f8345dbaf4ed491742aab29834aff66b4930c087 
>>
>>          git remote add linux-review 
>> https://github.com/intel-lab-lkp/linux
>>          git fetch --no-tags linux-review 
>> Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
>>          git checkout f8345dbaf4ed491742aab29834aff66b4930c087
>>          # save the config file
>>          mkdir build_dir && cp config build_dir/.config
>>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 
>> make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>>     m68k-linux-ld: block/blk-throttle.o: in function `tg_conf_updated':
>>>> blk-throttle.c:(.text+0x25bc): undefined reference to `__udivdi3'
>>>> m68k-linux-ld: blk-throttle.c:(.text+0x2626): undefined reference to 
>>>> `__udivdi3'
> Hi,
> 
> I'm confused here, the only place that I can relate to this:
> 
>      return dispatched * new_limit / old_limit;
> 
I understand it now. I'm doing (u64 / u64), I should use div64_u64
> However, I don't understand yet why this is problematic...
>>     `.exit.text' referenced in section `.data' of 
>> sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section 
>> `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
>>

Re: [PATCH -next v2 2/2] blk-throttle: fix io hung due to configuration updates

[Geert Uytterhoeven]

Hi Yukuai,

On Thu, May 19, 2022 at 5:25 AM yukuai (C) <yukuai3@huawei.com> wrote:
> 在 2022/05/19 10:11, yukuai (C) 写道:
> > 在 2022/05/18 23:52, kernel test robot 写道:
> >> Thank you for the patch! Yet something to improve:
> >>
> >> [auto build test ERROR on next-20220517]
> >>
> >> url:
> >> https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
> >>
> >> base:    47c1c54d1bcd0a69a56b49473bc20f17b70e5242
> >> config: m68k-allyesconfig
> >> (https://download.01.org/0day-ci/archive/20220518/202205182347.tMOOqyfL-lkp@intel.com/config)
> >>
> >> compiler: m68k-linux-gcc (GCC) 11.3.0
> >> reproduce (this is a W=1 build):
> >>          wget
> >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> >> -O ~/bin/make.cross
> >>          chmod +x ~/bin/make.cross
> >>          #
> >> https://github.com/intel-lab-lkp/linux/commit/f8345dbaf4ed491742aab29834aff66b4930c087
> >>
> >>          git remote add linux-review
> >> https://github.com/intel-lab-lkp/linux
> >>          git fetch --no-tags linux-review
> >> Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
> >>          git checkout f8345dbaf4ed491742aab29834aff66b4930c087
> >>          # save the config file
> >>          mkdir build_dir && cp config build_dir/.config
> >>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0
> >> make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <lkp@intel.com>
> >>
> >> All errors (new ones prefixed by >>):
> >>
> >>     m68k-linux-ld: block/blk-throttle.o: in function `tg_conf_updated':
> >>>> blk-throttle.c:(.text+0x25bc): undefined reference to `__udivdi3'
> >>>> m68k-linux-ld: blk-throttle.c:(.text+0x2626): undefined reference to
> >>>> `__udivdi3'
> > Hi,
> >
> > I'm confused here, the only place that I can relate to this:
> >
> >      return dispatched * new_limit / old_limit;
> >
> > However, I don't understand yet why this is problematic...
> >>     `.exit.text' referenced in section `.data' of
> >> sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section
> >> `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
>
> + static u64 throtl_update_bytes_disp(u64 dispatched, u64 new_limit,
> +                                    u64 old_limit)
> + {
> +        if (new_limit == old_limit)
> +                return dispatched;
> +
> +        if (new_limit == U64_MAX)
> +                return 0;
> +
> +        return dispatched * new_limit / old_limit;
>
> I understand it now. I'm doing (u64 / u64), I should use div64_u64

Better, use mul_u64_u64_div_u64(), as "dispatched * new_limit"
may overflow?

> + }
> +
> + static u32 throtl_update_io_disp(u32 dispatched, u32 new_limit, u32 old_limit)
> + {
> +        if (new_limit == old_limit)
> +                return dispatched;
> +
> +        if (new_limit == UINT_MAX)
> +                return 0;
> +
> +        return dispatched * new_limit / old_limit;

This is the same as above, but now operating on u32s instead of u64s.
Likewise, can the multiplication overflow?

> + }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH -next v2 2/2] blk-throttle: fix io hung due to configuration updates

[yukuai (C)]

在 2022/05/19 15:01, Geert Uytterhoeven 写道:
> Hi Yukuai,
> 
> On Thu, May 19, 2022 at 5:25 AM yukuai (C) <yukuai3@huawei.com> wrote:
>> 在 2022/05/19 10:11, yukuai (C) 写道:
>>> 在 2022/05/18 23:52, kernel test robot 写道:
>>>> Thank you for the patch! Yet something to improve:
>>>>
>>>> [auto build test ERROR on next-20220517]
>>>>
>>>> url:
>>>> https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
>>>>
>>>> base:    47c1c54d1bcd0a69a56b49473bc20f17b70e5242
>>>> config: m68k-allyesconfig
>>>> (https://download.01.org/0day-ci/archive/20220518/202205182347.tMOOqyfL-lkp@intel.com/config)
>>>>
>>>> compiler: m68k-linux-gcc (GCC) 11.3.0
>>>> reproduce (this is a W=1 build):
>>>>           wget
>>>> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
>>>> -O ~/bin/make.cross
>>>>           chmod +x ~/bin/make.cross
>>>>           #
>>>> https://github.com/intel-lab-lkp/linux/commit/f8345dbaf4ed491742aab29834aff66b4930c087
>>>>
>>>>           git remote add linux-review
>>>> https://github.com/intel-lab-lkp/linux
>>>>           git fetch --no-tags linux-review
>>>> Yu-Kuai/bugfix-for-blk-throttle/20220518-151713
>>>>           git checkout f8345dbaf4ed491742aab29834aff66b4930c087
>>>>           # save the config file
>>>>           mkdir build_dir && cp config build_dir/.config
>>>>           COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0
>>>> make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
>>>>
>>>> If you fix the issue, kindly add following tag as appropriate
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>>
>>>> All errors (new ones prefixed by >>):
>>>>
>>>>      m68k-linux-ld: block/blk-throttle.o: in function `tg_conf_updated':
>>>>>> blk-throttle.c:(.text+0x25bc): undefined reference to `__udivdi3'
>>>>>> m68k-linux-ld: blk-throttle.c:(.text+0x2626): undefined reference to
>>>>>> `__udivdi3'
>>> Hi,
>>>
>>> I'm confused here, the only place that I can relate to this:
>>>
>>>       return dispatched * new_limit / old_limit;
>>>
>>> However, I don't understand yet why this is problematic...
>>>>      `.exit.text' referenced in section `.data' of
>>>> sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section
>>>> `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
>>
>> + static u64 throtl_update_bytes_disp(u64 dispatched, u64 new_limit,
>> +                                    u64 old_limit)
>> + {
>> +        if (new_limit == old_limit)
>> +                return dispatched;
>> +
>> +        if (new_limit == U64_MAX)
>> +                return 0;
>> +
>> +        return dispatched * new_limit / old_limit;
>>
>> I understand it now. I'm doing (u64 / u64), I should use div64_u64
> 
> Better, use mul_u64_u64_div_u64(), as "dispatched * new_limit"
> may overflow?
Hi,

It's right that it  can overflow, I'll handle such case in next version.
> 
>> + }
>> +
>> + static u32 throtl_update_io_disp(u32 dispatched, u32 new_limit, u32 old_limit)
>> + {
>> +        if (new_limit == old_limit)
>> +                return dispatched;
>> +
>> +        if (new_limit == UINT_MAX)
>> +                return 0;
>> +
>> +        return dispatched * new_limit / old_limit;
> 
> This is the same as above, but now operating on u32s instead of u64s.
> Likewise, can the multiplication overflow?
same as above.

Thanks,
Kuai
> 
>> + }
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
> .
>