Re: [PATCH][next] bcache: Use 64-bit arithmetic instead of 32-bit

[Coly Li <colyli@suse.de>]

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

> 
> I hope BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW is zero :-)

Why BCH_WRITEBACK_FRAGMENT_THRESHOLD_LOW being zero can be helpful to
avoid the overflow ? Could you please to provide more detailed information.

I am not challenging you, I just want to extend my knowledge by learning
from you. Thanks in advance.

Coly Li