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
