On Sun, 4 Sep 2016, Liav Rehana wrote:
> >> The root of the problem is that in case the multiplication of delta
> >> and
> >> tkr->mult in the line that I've changed is too big that the MSB of the
> >> result is set, then the shift will cause an unwanted sign extension.
>
> > I completely understand that, but as I said before:
>
> > > > This typecast is just a baindaid. What happens if you double the
> > > > suspend time? The multiplication will simply overflow. So the
> > > > proper fix is to sanity check delta and do multiple conversions if
> > > > delta is big enough. Preferrably this happens somewhere at the call
> > > > site and not in this hotpath function.
>
> > > That sign extension will be avoided completely if the variable nsec
> > > was unsigned (u64 instead of s64), so I think the correct solution for
> > > this is to change the type of nsec to u64.
>
> > That's a different story and its not a solution for the general problem of
>
> > delta * mult >= (1 << 31) or delta * mult >= (1 << 32)
>
> The case that delta * mult >= 1 << 31 is not a problem by itself, but it
> causes
> an unwanted sign extension since the type of nsec is signed. That sign
> extension is what causes the loop to take too long, and not the overflow.
> I understand that the typecast is not a general solution, so as I've said, I
> think that changing the type of nsec to u64 instead of s64 will be a good and
> general solution, as it will indeed solve the problem of the unwanted sign
> extension.
>
> To summarize: a sign extension occurs if the nsec variable is signed, and so
> I ask if you think it will be a good solution to change its type to unsigned.
Do you actually read what I write? I asked John before:
> John, why is that stuff signed at all? Shouldn't we use u64 for all of this?
So to summarize:
- Yes, we can use u64 if there is nothing which I missed, but John will
have the last word on this
- No, making it u64 does not solve the general problem. It just papers
over the problem you observe. And we don't add 'paper over' fixes,
period.
Thanks,
tglx
