Messages in this thread | | | Subject | Re: [PATCH] sched/fair: Use 'unsigned long' for group_shares,group_runnable | From | "chengjian (D)" <> | Date | Mon, 15 Apr 2019 23:20:31 +0800 |
| |
Hi, Peter
On 2019/4/15 20:46, Peter Zijlstra wrote:
I write a demo about this, which I described it as overflow.
#cat test.c
//test.c #include <stdio.h> #include <stdlib.h> #include <limits.h> #include <assert.h>
int main(void) { long a = 1048576 * 9144968455305; /* shares = tg_shares * load */ unsigned long b = a; //unsigned long e = 1048576 * 9144968455305;
printf("LONG_MAX = %ld, 0x%lx\n", LONG_MAX, LONG_MAX); printf("ULONG_MAX = %lu, 0x%lx\n", ULONG_MAX, ULONG_MAX);
if (b > LONG_MAX) printf("==overflow!!!==\n"); // OVERFLOW printf("a = %20ld,0x%016lx\n", a, a); printf("b = %20lu,0x%016lx\n", b, b);
/* shares /= tg_weight */ printf("a/3 = 0x%016lx\n", (a / 3)); // WRONG printf("a/3 = 0x%016lx\n", (((unsigned long)a) / 3)); // unsigned printf("a/3 = 0x%016lx\n", (a / (unsigned long)3)); // unsigned printf("b/3 = 0x%016lx\n", b / 3); // unsigned
return EXIT_SUCCESS; }
#./test
LONG_MAX = 9223372036854775807, 0x7fffffffffffffff ULONG_MAX = 18446744073709551615, 0xffffffffffffffff ==overflow!!!== a = -8857549630719655936,0x8513a98a48900000 b = 9589194442989895680,0x8513a98a48900000 a/3 = 0xd7068dd8c2daaaab // WRONG a/3 = 0x2c5be32e18300000 a/3 = 0x2c5be32e18300000 b/3 = 0x2c5be32e18300000
> On Sat, Apr 13, 2019 at 03:32:34AM +0000, Cheng Jian wrote: >> group_share and group_runnable are tracked as 'unsigned long', >> however some functions using them as 'long' which is ultimately >> assigned back to 'unsigned long' variables in reweight_entity. >> >> Since there is not scope on using a different and signed type, >> this change improves code consistency and avoids further type >> conversions. More important, to prevent undefined behavior >> caused by overflow. > There is no undefined behaviour due to overflow.UBSAN is broken, > upgrade to GCC8 or later. > > .
So, function calc_group_shares will return the wrong value.
```cpp
static long calc_group_shares(struct cfs_rq *cfs_rq) {
// ......
shares = (tg_shares * load); if (tg_weight) shares /= tg_weight; }
```
The same to calc_group_runnable.
Thanks.
CHENG Jian.
| |