Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] sched/fair: Use 'unsigned long' for
 group_shares,group_runnable
To:     Peter Zijlstra <peterz@infradead.org>
CC:     <huawei.libin@huawei.com>, <xiexiuqi@huawei.com>,
        <yangyingliang@huawei.com>, <mingo@redhat.com>,
        <linux-kernel@vger.kernel.org>,
        "chengjian (D)" <cj.chengjian@huawei.com>,
        Yang Yingliang <yangyingliang@huawei.com>
References: <20190413033234.30002-1-cj.chengjian@huawei.com>
 <20190415124617.GP11158@hirez.programming.kicks-ass.net>
From:   "chengjian (D)" <cj.chengjian@huawei.com>
Message-ID: <eb7dede4-9668-9252-b95a-e20e313c5890@huawei.com>
Date:   Mon, 15 Apr 2019 23:20:31 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.2
MIME-Version: 1.0
In-Reply-To: <20190415124617.GP11158@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.