subject:"\[libvirt\] \[PATCH\] util\: make it more robust to calculate timeout value"

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

2015-05-18 Thread Michal Privoznik

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.wangyu...@huawei.com
 Signed-off-by: Zhang Bo oscar.zhan...@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)  0x7FFF;

 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

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

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

2015-05-17 Thread Wang Yufei

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.wangyu...@huawei.com
 Signed-off-by: Zhang Bo oscar.zhan...@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)  0x7FFF;
 
 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
 
 .
 



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

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

2015-05-17 Thread Wang Yufei

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.wangyu...@huawei.com
 Signed-off-by: Zhang Bo oscar.zhan...@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)  0x7FFF;

 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



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

[libvirt] [PATCH] util: make it more robust to calculate timeout value

2015-05-15 Thread zhang bo

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.wangyu...@huawei.com
Signed-off-by: Zhang Bo oscar.zhan...@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)  0x7FFF;
 } else {
 *timeout = -1;
 }
-- 
1.7.12.4


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

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

2015-05-15 Thread Daniel P. Berrange

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.wangyu...@huawei.com
  Signed-off-by: Zhang Bo oscar.zhan...@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)  0x7FFF;
 
 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 :|

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

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

2015-05-15 Thread Michal Privoznik

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.wangyu...@huawei.com
 Signed-off-by: Zhang Bo oscar.zhan...@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)  0x7FFF;

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

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

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

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

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

[libvirt] [PATCH] util: make it more robust to calculate timeout value

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

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

6 matches

Site Navigation

Mail list logo

Footer information