Re: [PATCHv3] util: make it more robust to calculate timeout value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Zhang Bo <oscar.zhangbo@xxxxxxxxxx>
---
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]