On Wed, May 27, 2015 at 04:58:22PM +0200, Martin Kletzander wrote:
On Mon, May 25, 2015 at 02:22:42PM +0800, Wang Yufei wrote:
>From: Zhang Bo <oscar.zhangbo(a)huawei.com>
>
>When we change system clock to years ago, a certain CPU may use up 100% cputime.
>The reason is that in function virEventPollCalculateTimeout(), we assign the
>unsigned long long result to an INT variable,
> *timeout = then - now; // timeout is INT, and then/now are long long
> if (*timeout < 0)
> *timeout = 0;
>there's a chance that variable @then minus variable @now may be a very large
number
>that overflows INT value expression, then *timeout will be negative and be assigned to
0.
>Next the 'poll' in function virEventPollRunOnce() will get into an
'endless' while loop there.
>thus, the cpu that virEventPollRunOnce() thread runs on will go up to 100%.
>
>Although as we discussed before in
https://www.redhat.com/archives/libvir-list/2015-May/msg00400.html
>it should be prohibited to set-time while other applications are running, but it does
>seems to have no harm to make the codes more robust.
>
>Signed-off-by: Wang Yufei <james.wangyufei(a)huawei.com>
>Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com>
>---
>src/util/vireventpoll.c | 5 +++--
>1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
>index ffda206..4e4f407 100644
>--- a/src/util/vireventpoll.c
>+++ b/src/util/vireventpoll.c
>@@ -357,9 +357,10 @@ static int virEventPollCalculateTimeout(int *timeout)
> return -1;
>
> EVENT_DEBUG("Schedule timeout then=%llu now=%llu", then, now);
>- *timeout = then - now;
>- if (*timeout < 0)
>+ if (then <= now)
> *timeout = 0;
>+ else
>+ *timeout = ((then-now) > INT_MAX) ? INT_MAX : (then-now);
I added spaces around the minus sign.
This will work, although the following would be nicer to read since
there were such problems with the calculation in earlier versions:
if (then <= now) {
*timeout = 0;
} else {
long long tmp = then - now;
And of course this would be unsigned.
if (tmp > INT_MAX)
*timeout = INT_MAX
else
*timeout = tmp;
}
But that, of course, has the same meaning as your version, which is
fine. ACK to that, I'll push it in a while.
> } else {
> *timeout = -1;
> }
>--
>1.7.12.4
>
>
>--
>libvir-list mailing list
>libvir-list(a)redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list