Re: [libvirt] [PATCH 08/12] libxl: Use atomic ops for driver-nactive

2013-09-03 Thread Jim Fehlig 
Michal Privoznik wrote:
 On 30.08.2013 23:46, Jim Fehlig wrote:
   
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 ---
  src/libxl/libxl_conf.h   |  2 +-
  src/libxl/libxl_driver.c | 10 --
  2 files changed, 5 insertions(+), 7 deletions(-)

 diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
 index e3875ba..83bb6b9 100644
 --- a/src/libxl/libxl_conf.h
 +++ b/src/libxl/libxl_conf.h
 @@ -90,7 +90,7 @@ struct _libxlDriverPrivate {
   * then lockless thereafter */
  libxlDriverConfigPtr config;
  
 -size_t nactive;
 +unsigned int nactive;
  
  virStateInhibitCallback inhibitCallback;
  void *inhibitOpaque;
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index e604899..7615cdd 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -50,6 +50,7 @@
  #include virstring.h
  #include virsysinfo.h
  #include viraccessapicheck.h
 +#include viratomic.h
  
  #define VIR_FROM_THIS VIR_FROM_LIBXL
  
 @@ -265,8 +266,7 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
  virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
  }
  
 -driver-nactive--;
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntDecAndTest(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(false, driver-inhibitOpaque);
  
  if ((vm-def-ngraphics == 1) 
 @@ -655,9 +655,8 @@ libxlVmStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  if (virDomainSaveStatus(driver-xmlopt, cfg-stateDir, vm)  0)
  goto error;
  
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(true, driver-inhibitOpaque);
 

 Not exactly the same as previous code. Previously, the callback was
 called iff nactive == 0; However, with your change the callback is
 invoked each time the control gets to the 'if' (as virAtomicIntInc()
 returns the *new* value after the increment). I think we want this to be:

 if (virAtomicIntInc(driver-nactive) == 1  driver-inhibitCallback)
   

Opps, thanks for catching that.  I've fixed both instances you pointed out.

Regards,
Jim

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

Re: [libvirt] [PATCH 08/12] libxl: Use atomic ops for driver-nactive

2013-09-02 Thread Michal Privoznik 
On 30.08.2013 23:46, Jim Fehlig wrote:
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 ---
  src/libxl/libxl_conf.h   |  2 +-
  src/libxl/libxl_driver.c | 10 --
  2 files changed, 5 insertions(+), 7 deletions(-)
 
 diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
 index e3875ba..83bb6b9 100644
 --- a/src/libxl/libxl_conf.h
 +++ b/src/libxl/libxl_conf.h
 @@ -90,7 +90,7 @@ struct _libxlDriverPrivate {
   * then lockless thereafter */
  libxlDriverConfigPtr config;
  
 -size_t nactive;
 +unsigned int nactive;
  
  virStateInhibitCallback inhibitCallback;
  void *inhibitOpaque;
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index e604899..7615cdd 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -50,6 +50,7 @@
  #include virstring.h
  #include virsysinfo.h
  #include viraccessapicheck.h
 +#include viratomic.h
  
  #define VIR_FROM_THIS VIR_FROM_LIBXL
  
 @@ -265,8 +266,7 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
  virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
  }
  
 -driver-nactive--;
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntDecAndTest(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(false, driver-inhibitOpaque);
  
  if ((vm-def-ngraphics == 1) 
 @@ -655,9 +655,8 @@ libxlVmStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  if (virDomainSaveStatus(driver-xmlopt, cfg-stateDir, vm)  0)
  goto error;
  
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(true, driver-inhibitOpaque);

Not exactly the same as previous code. Previously, the callback was
called iff nactive == 0; However, with your change the callback is
invoked each time the control gets to the 'if' (as virAtomicIntInc()
returns the *new* value after the increment). I think we want this to be:

if (virAtomicIntInc(driver-nactive) == 1  driver-inhibitCallback)

 -driver-nactive++;
  
  event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED,
   restore_fd  0 ?
 @@ -727,9 +726,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
  vm-def-id = d_info.domid;
  virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN);
  
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)

Same applies here.

  driver-inhibitCallback(true, driver-inhibitOpaque);
 -driver-nactive++;
  
  /* Recreate domain death et. al. events */
  libxlCreateDomEvents(vm);
 

ACK if you fix the issue.

Michal

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

Re: [libvirt] [PATCH 08/12] libxl: Use atomic ops for driver-nactive

2013-09-02 Thread Daniel P. Berrange 
On Fri, Aug 30, 2013 at 03:46:54PM -0600, Jim Fehlig wrote:
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 ---
  src/libxl/libxl_conf.h   |  2 +-
  src/libxl/libxl_driver.c | 10 --
  2 files changed, 5 insertions(+), 7 deletions(-)
 
 diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
 index e3875ba..83bb6b9 100644
 --- a/src/libxl/libxl_conf.h
 +++ b/src/libxl/libxl_conf.h
 @@ -90,7 +90,7 @@ struct _libxlDriverPrivate {
   * then lockless thereafter */
  libxlDriverConfigPtr config;
  
 -size_t nactive;
 +unsigned int nactive;
  
  virStateInhibitCallback inhibitCallback;
  void *inhibitOpaque;
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index e604899..7615cdd 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -50,6 +50,7 @@
  #include virstring.h
  #include virsysinfo.h
  #include viraccessapicheck.h
 +#include viratomic.h
  
  #define VIR_FROM_THIS VIR_FROM_LIBXL
  
 @@ -265,8 +266,7 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
  virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
  }
  
 -driver-nactive--;
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntDecAndTest(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(false, driver-inhibitOpaque);
  
  if ((vm-def-ngraphics == 1) 
 @@ -655,9 +655,8 @@ libxlVmStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  if (virDomainSaveStatus(driver-xmlopt, cfg-stateDir, vm)  0)
  goto error;
  
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(true, driver-inhibitOpaque);
 -driver-nactive++;
  
  event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED,
   restore_fd  0 ?
 @@ -727,9 +726,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
  vm-def-id = d_info.domid;
  virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN);
  
 -if (!driver-nactive  driver-inhibitCallback)
 +if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)
  driver-inhibitCallback(true, driver-inhibitOpaque);
 -driver-nactive++;
  
  /* Recreate domain death et. al. events */
  libxlCreateDomEvents(vm);

ACK if you fix Michael's comments


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

[libvirt] [PATCH 08/12] libxl: Use atomic ops for driver-nactive

2013-08-30 Thread Jim Fehlig 

Signed-off-by: Jim Fehlig jfeh...@suse.com
---
 src/libxl/libxl_conf.h   |  2 +-
 src/libxl/libxl_driver.c | 10 --
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index e3875ba..83bb6b9 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -90,7 +90,7 @@ struct _libxlDriverPrivate {
  * then lockless thereafter */
 libxlDriverConfigPtr config;
 
-size_t nactive;
+unsigned int nactive;
 
 virStateInhibitCallback inhibitCallback;
 void *inhibitOpaque;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index e604899..7615cdd 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -50,6 +50,7 @@
 #include virstring.h
 #include virsysinfo.h
 #include viraccessapicheck.h
+#include viratomic.h
 
 #define VIR_FROM_THIS VIR_FROM_LIBXL
 
@@ -265,8 +266,7 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
 virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
 }
 
-driver-nactive--;
-if (!driver-nactive  driver-inhibitCallback)
+if (virAtomicIntDecAndTest(driver-nactive)  driver-inhibitCallback)
 driver-inhibitCallback(false, driver-inhibitOpaque);
 
 if ((vm-def-ngraphics == 1) 
@@ -655,9 +655,8 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr 
vm,
 if (virDomainSaveStatus(driver-xmlopt, cfg-stateDir, vm)  0)
 goto error;
 
-if (!driver-nactive  driver-inhibitCallback)
+if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)
 driver-inhibitCallback(true, driver-inhibitOpaque);
-driver-nactive++;
 
 event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED,
  restore_fd  0 ?
@@ -727,9 +726,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
 vm-def-id = d_info.domid;
 virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN);
 
-if (!driver-nactive  driver-inhibitCallback)
+if (virAtomicIntInc(driver-nactive)  driver-inhibitCallback)
 driver-inhibitCallback(true, driver-inhibitOpaque);
-driver-nactive++;
 
 /* Recreate domain death et. al. events */
 libxlCreateDomEvents(vm);
-- 
1.8.1.4

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