Messages in this thread | | | Date | Sun, 4 Sep 2016 12:39:27 +0200 (CEST) | From | Thomas Gleixner <> | Subject | RE: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation. |
| |
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
| |