On 2/13/21 12:42 AM, David Laight wrote:
> From: Coly Li <colyli@xxxxxxx>
>> Sent: 12 February 2021 16:02
>>
>> On 2/12/21 11:31 PM, David Laight wrote:
>>>>> if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID) {
>>>>> - fp_term = dc->writeback_rate_fp_term_low *
>>>>> + fp_term = (int64_t)dc->writeback_rate_fp_term_low *
>>>>> (c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW);
>>>>> } else if (c->gc_stats.in_use <= BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) {
>>>>> - fp_term = dc->writeback_rate_fp_term_mid *
>>>>> + fp_term = (int64_t)dc->writeback_rate_fp_term_mid *
>>>>> (c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_MID);
>>>>> } else {
>>>>> - fp_term = dc->writeback_rate_fp_term_high *
>>>>> + fp_term = (int64_t)dc->writeback_rate_fp_term_high *
>>>>> (c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH);
>>>>> }
>>>>> fps = div_s64(dirty, dirty_buckets) * fp_term;
>>>>>
>>>>
>>>> Hmm, should such thing be handled by compiler ? Otherwise this kind of
>>>> potential overflow issue will be endless time to time.
>>>>
>>>> I am not a compiler expert, should we have to do such explicit type cast
>>>> all the time ?
>>>
>>
>> Hi David,
>>
>> I add Dongdong Tao Cced, who is author of this patch.
>>
>> Could you please offer me more information about the following lines?
>> Let me ask more for my questions.
>>
>>> We do to get a 64bit product from two 32bit values.
>>> An alternative for the above would be:
>>> fp_term = c->gc_stats.in_use - BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH;
>>> fp_term *= dc->writeback_rate_fp_term_high;
>>
>> The original line is,
>> fp_term = dc->writeback_rate_fp_term_high * (c->gc_stats.in_use -
>> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH)
>>
>> The first value dc->writeback_rate_fp_term_high is unsigned int (32bit),
>> and the second value (c->gc_stats.in_use -
>> BCH_WRITEBACK_FRAGMENT_THRESHOLD_HIGH) is unsigned int (32bit) too. And
>> fp_term is 64bit, if the product is larger than 32bits, the compiler
>> should know fp_term is 64bit and upgrade the product to 64bit.
>>
>> The above is just my guess, because I feel compiling should have the
>> clue for the product upgrade to avoid overflow. But I almost know
>> nothing about compiler internal ....
>
> No, the expression is evaluated as 32bit and then extended for the assignment.
I do some test, and you are right. I am so surprised that the product
does not extended and the overflowed result is 0. Thank you for letting
me know this, and I correct my mistaken concept not too late :-)
If you want me to take this patch, it is OK to me. I will have it in
v5.12. If you want to handle this patch to upstream in your track, you
may have my
Acked-by: Coly Li <colyli@xxxxxxx>
Thanks for your patient explaining.
>
> Or, more correctly, integer variables smaller than int (usually short and char)
> are extended to int prior to any arithmetic.
> If one argument to an operator is larger than int it is extended.
> If there is a sign v unsigned mismatch the signed value is cast to unsigned
> (same bit pattern on 2's compliment systems).
>
> There are some oddities:
> - 'unsigned char/short' gets converted to 'signed int' unless
> char/short and int are the same size (which is allowed).
> (Although if char and int are the same size there are issues
> with the value for EOF.)
> - (1 << 31) is a signed integer, it will get sign extended if used
> to mask a 64bit value.
>
> K&R C converted 'unsigned char' to 'unsigned int' - but the ANSI
> standards body decided otherwise.
>
> The compiler is allowed to use an 'as if' rule to use the 8bit and
> 16bit arithmetic/registers on x86.
> But on almost everything else arithmetic on char/short local variables
> requires the compiler repeatedly mask the value back to 8/16 bits.
>
> The C language has some other oddities - that are allowed but never done.
> (Except for 'thought-machines' in comp.lang.c)
I know the above things, but I DO NOT know product of two 32bit values
multiplying does not extend to 64bit. Good to know the compiling is not
that smart.
Thank you!
Coly Li
