Re: powerpc64 head -r344018 stuck sleeping problems: th->th_scale * tc_delta(th) overflows unsigned 64 bits sometimes [patched failed]

On Thu, 14 Mar 2019, Konstantin Belousov wrote:

> On Thu, Mar 07, 2019 at 05:29:51PM -0800, Mark Millard wrote:
>> A basic question and a small note.
>>
>> Question's context for it tc->tc_get_timecount(tc) values:
>>
>> In the powerpc64 context tc->tc_get_timecount(tc) is the lower
>> 32 bits of the tbr, in my context having a 33,333,333 MHz or so
>> increment rate for a machine with a 2.5 GHz or so clock rate.
>> The truncated 32 bit tbr value wraps every 128 seconds or so.
>> 2 sockets, 2 cores per socket, so 4 separate tbr values.
>>
>> The question is . . .
>>
>> In tc_delta's:
>>
>>     tc->tc_get_timecount(tc) - th->th_offset_count
>>
>> is observing tc->tc_get_timecount(tc) < th->th_offset_count
>> ever supposed to be possible in correct operation, other than
>> tc->tc_get_timecount(tc) having wrapped around (and so being
>> newly 0 or "near" 0, no evidence of of having it having been
>> near 128 seconds or more for my context)?
> I think yes, there is no reason for current get_timecount() value
> to have any arithmetic relation to th_offset_count.  Look at tc_windup()
> on how the th_offset_count is calculated.  The final value is clamped
> by the tc_counter_mask, so only lower bits are important (higher bits
> are evacuated to th_offset or lost due to overflow if tc_windup()
> was not called soon enough).

Yes, it is a standard method to calculate time differences from a possibly-
wrapped counter as (finish - start) & mask in unsigned arithmetic, where
the counter must be checked before it wraps relative to 'start'.

>> The note:
>>
>> On 2019-Mar-7, at 14:22, Konstantin Belousov <[email protected]> wrote:
>>
>>> . . .
>>> +
>>> +	if (__predict_false(delta < large_delta)) {
>>
>> I thought that delta<large_delta was the non-overflow context
>> for scale*delta and that the overflow case for the multiplication
>> was when delta>=large_delta .
> You are right, I fixed this in my repo.
>>
>>> +		/* Avoid overflow for scale * delta. */
>>> +		x = (scale >> 32) * delta;
>>> +		bt->sec += x >> 32;
>>> +		bintime_addx(bt, x << 32);
>>> +		bintime_addx(bt, (scale & 0xffffffff) * delta);
>>> +	} else {
>>> +		bintime_addx(bt, scale * delta);
>>> +	}
>>> . . .

Fixed in my version too.

I might have helped break this.  I reversed the condition to get the
unusual path executed (though not when it overflow), and forgot to
undo this.  At least the unusual path got checked more).

Bruce
_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-ppc
To unsubscribe, send any mail to "[email protected]"