[libvirt] [PATCH] util: make it more robust to calculate timeout value
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@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@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..5f5a149 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) & 0x7FFFFFFF; } else { *timeout = -1; } -- 1.7.12.4
On 15.05.2015 08:26, zhang bo wrote:
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@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@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..5f5a149 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) & 0x7FFFFFFF;
You're trying to make this an unsigned value. What's wrong with plain typecast?
} else { *timeout = -1; }
I must say this is ugly. If the system clock is changed, all the timeouts should fire, shouldn't they? Otherwise important events can be missed. Michal
On Fri, May 15, 2015 at 01:09:09PM +0200, Michal Privoznik wrote:
On 15.05.2015 08:26, zhang bo wrote:
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@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@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..5f5a149 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) & 0x7FFFFFFF;
You're trying to make this an unsigned value. What's wrong with plain typecast?
} else { *timeout = -1; }
I must say this is ugly. If the system clock is changed, all the timeouts should fire, shouldn't they? Otherwise important events can be missed.
As I said in previous thread, I think that this is really just papering over one specific issue, and you are still going to have a multitude of problems with every app on the system when you change the system clock in this kind of way. I'm just not convinced we should be trying to hack around it in this one case, as it is just giving us a false illusion that things are going to continue to work, when in reality they'll just break somewhere else instead. eg the pthread_cond_wait() timeouts. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 2015/5/15 19:16, Daniel P. Berrange wrote:
On Fri, May 15, 2015 at 01:09:09PM +0200, Michal Privoznik wrote:
On 15.05.2015 08:26, zhang bo wrote:
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@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@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..5f5a149 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) & 0x7FFFFFFF;
You're trying to make this an unsigned value. What's wrong with plain typecast?
} else { *timeout = -1; }
I must say this is ugly. If the system clock is changed, all the timeouts should fire, shouldn't they? Otherwise important events can be missed.
As I said in previous thread, I think that this is really just papering over one specific issue, and you are still going to have a multitude of problems with every app on the system when you change the system clock in this kind of way. I'm just not convinced we should be trying to hack around it in this one case, as it is just giving us a false illusion that things are going to continue to work, when in reality they'll just break somewhere else instead. eg the pthread_cond_wait() timeouts.
You're right, this patch can not fix system clock changed problem. I'm trying to fix the bug that assigning an unsigned long long value to an int variable, and it can fix cpu up to 100% bug. What I do is decreasing the bad effect to the whole OS. At least we can do something right.
Regards, Daniel
On 18.05.2015 03:11, Wang Yufei wrote:
On 2015/5/15 19:16, Daniel P. Berrange wrote:
On Fri, May 15, 2015 at 01:09:09PM +0200, Michal Privoznik wrote:
On 15.05.2015 08:26, zhang bo wrote:
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@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@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..5f5a149 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) & 0x7FFFFFFF;
You're trying to make this an unsigned value. What's wrong with plain typecast?
} else { *timeout = -1; }
I must say this is ugly. If the system clock is changed, all the timeouts should fire, shouldn't they? Otherwise important events can be missed.
As I said in previous thread, I think that this is really just papering over one specific issue, and you are still going to have a multitude of problems with every app on the system when you change the system clock in this kind of way. I'm just not convinced we should be trying to hack around it in this one case, as it is just giving us a false illusion that things are going to continue to work, when in reality they'll just break somewhere else instead. eg the pthread_cond_wait() timeouts.
You're right, this patch can not fix system clock changed problem. I'm trying to fix the bug that assigning an unsigned long long value to an int variable, and it can fix cpu up to 100% bug. What I do is decreasing the bad effect to the whole OS. At least we can do something right.
That's why I told it's ugly. Libvirt it meant to run on many platforms, even there where an integer is not 4 bytes long. Therefore we use plain typecast when needed instead of masking sign bit. For instance, on platforms where int is 2bytes, this patch will not work at all. Michal
On 2015/5/15 19:09, Michal Privoznik wrote:
On 15.05.2015 08:26, zhang bo wrote:
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@huawei.com> Signed-off-by: Zhang Bo <oscar.zhangbo@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..5f5a149 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) & 0x7FFFFFFF;
You're trying to make this an unsigned value. What's wrong with plain typecast?
No, I'm trying to give an positive value that an int variable can express,
} else { *timeout = -1; }
I must say this is ugly. If the system clock is changed, all the timeouts should fire, shouldn't they? Otherwise important events can be missed.
In fact, the events will not be handled. In virEventPollDispatchTimeouts if (eventLoop.timeouts[i].expiresAt <= (now+20)) { ... (cb)(timer, opaque); 'expiresAt' is much bigger than 'now' because the system clock has been changed to long long ago. Assign an unsigned long long value to an int value is out of control, we don't know whether it be negative or positive because of overflow of int. I know this patch can not fix system clock changed problem, but at least, the 'timeout' will be under control, and the cpu up to 100% situation can be fixed, that's my point.
Michal
.
participants (4)
-
Daniel P. Berrange -
Michal Privoznik -
Wang Yufei -
zhang bo