Re: [libvirt] [PATCH v3 1/2] Implement the asynchronous suspend and RTC wakeup

2011-11-22 Thread Srivatsa S. Bhat 
On 11/22/2011 06:18 AM, Eric Blake wrote:
 On 11/09/2011 05:05 AM, Srivatsa S. Bhat wrote:
 Add the core functions that implement the functionality of the API.
 Suspend is done by using an asynchronous mechanism so that we can return
 the status to the caller successfully before the host gets suspended. This
 asynchronous operation is achieved by suspending the host in a separate
 thread of execution.

 To resume the host, an RTC alarm is set up (based on how long we want
 to suspend) before suspending the host. When this alarm fires, the host
 gets woken up.

 Signed-off-by: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
 ---


[...]

 +
 +#define MAX_SUSPEND_DURATION 365*24*60*60/* 1 year, a reasonable max? */
 
 Any time you impose an arbitrary limit (and 1 year can be deemed
 arbitrary), you are risking problems.  There should be a real technical
 reason why (and if) we have to impose a limit, not an arbitrary time
 duration; otherwise, I would favor letting the user sleep as long as the
 RTC clock supports (even if the sleep amount seems ridiculous to me).
 
 This is as far as I got on my review today; I'll pick up here tomorrow.
 

Hi Eric,
Thanks a lot for the review comments! I'll address them in my next
iteration of the patchset and post out soon.

Thanks,
Srivatsa S. Bhat
IBM Linux Technology Center

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

Re: [libvirt] [PATCH v3 1/2] Implement the asynchronous suspend and RTC wakeup

2011-11-22 Thread Eric Blake 
On 11/09/2011 05:05 AM, Srivatsa S. Bhat wrote:
 Add the core functions that implement the functionality of the API.
 Suspend is done by using an asynchronous mechanism so that we can return
 the status to the caller successfully before the host gets suspended. This
 asynchronous operation is achieved by suspending the host in a separate
 thread of execution.
 
 +
 +#define MAX_SUSPEND_DURATION 365*24*60*60/* 1 year, a reasonable max? */

Resuming where I left off...

 +
 +/**
 + * setNodeWakeup:
 + * @alarmTime : time in seconds from now, at which the RTC alarm has to be 
 set.
 + *
 + * Set up the RTC alarm to the specified time.
 + * Return 0 on success, -1 on failure.
 + */
 +
 +int setNodeWakeup(unsigned long long alarmTime)
 +{
 +virCommandPtr setAlarmCmd;
 +int ret = -1;
 +
 +if (alarmTime = SUSPEND_DELAY || alarmTime  MAX_SUSPEND_DURATION)

Why is SUSPEND_DELAY in nodeinfo.h but MAX_SUSPEND_DURATION in the .c?
They might as well be next to one another.

 +return -1;
 +
 +setAlarmCmd = virCommandNewArgList(rtcwake, -m, no, -s, NULL);
 +virCommandAddArgFormat(setAlarmCmd, %lld, alarmTime);

'man rtcwake' says that not all systems support RTC wake from S4;
systems that have a functioning nvram-wakeup will succeed, but that is
not all systems.  Do we need to be more cautious about allowing S4
suspension if we can't prove that RTC will wake up the system from S4?

On the other hand, you are using -m no to just set the wakeup time,
which ought to fail if the system does not support the requested delay
or the ability to wake up, so that you never actually suspend until
after you know the wakeup was successfully scheduled.

Hmm, does that mean the public API should provide a way to schedule the
wakeup without also scheduling a suspend?

 +++ b/src/util/threads-pthread.c
 @@ -81,10 +81,25 @@ void virMutexDestroy(virMutexPtr m)
  pthread_mutex_destroy(m-lock);
  }
  
 -void virMutexLock(virMutexPtr m){
 +void virMutexLock(virMutexPtr m)
 +{
  pthread_mutex_lock(m-lock);
  }
  
 +/**
 + * virMutexTryLock:

I'm not convinced we need this.  As I understand it, your code does:

thread 1:  thread 2: thread 3:
request suspend
grab lock
spawn helper
   sleep 10 sec
return success
 request suspend
 lock not available
 return failure
   suspend
   resume
 request suspend
 lock not available
 return failure
   release lock

But we don't need a try-lock operation, if we do:

thread 1:  thread 2: thread 3:
request suspend
grab lock
 request suspend
mark flag to true
release lock
 grab lock
 flag already true
 release lock
 return failure
spawn helper
   sleep 10 sec
return success
   suspend
   resume
 grab lock
 flag already true
 release lock
 return failure
   grab lock
   clear flag
   release lock

That is, instead of holding the lock for the entire duration of the
suspend, just hold the lock long enough to mark a volatile variable;
then you no longer need a non-blocking try-lock primitive, because the
lock will never starve anyone else long enough to be an issue.

But if you can still convince me that we need a try-lock operation, then
it should probably be added as a separate commit, alongside an
implementation for mingw in the same commit (without a mingw
implementation, you will cause a build failure for mingw32-libvirt).

 + * This is same as virMutexLock() except that
 + * if the mutex is unavailable (already locked),
 + * this fails and returns an error.
 + *
 + * Returns 1 if the lock was acquired, 0 if there was
 + * contention or error.
 + */
 +int virMutexTryLock(virMutexPtr m)
 +{
 +   return !pthread_mutex_trylock(m-lock);

Either return a bool (if we don't care about distinguishing why the lock
failed) or keep the return as an int but make it tri-state (1 for
grabbed, 0 for EBUSY, and -1 for all other failures (such as EINVAL).

 +++ b/src/util/threads.h
 @@ -81,6 +81,7 @@ int virMutexInitRecursive(virMutexPtr m) 
 ATTRIBUTE_RETURN_CHECK;
  void virMutexDestroy(virMutexPtr m);
  
  void virMutexLock(virMutexPtr m);
 +int virMutexTryLock(virMutexPtr m);

And if you convince me we need this, then mark it ATTRIBUTE_RETURN_CHECK.

-- 
Eric

Re: [libvirt] [PATCH v3 1/2] Implement the asynchronous suspend and RTC wakeup

2011-11-22 Thread Srivatsa S. Bhat 
On 11/22/2011 11:33 PM, Eric Blake wrote:
 On 11/09/2011 05:05 AM, Srivatsa S. Bhat wrote:
 Add the core functions that implement the functionality of the API.
 Suspend is done by using an asynchronous mechanism so that we can return
 the status to the caller successfully before the host gets suspended. This
 asynchronous operation is achieved by suspending the host in a separate
 thread of execution.

 +
[...]
 
 +return -1;
 +
 +setAlarmCmd = virCommandNewArgList(rtcwake, -m, no, -s, NULL);
 +virCommandAddArgFormat(setAlarmCmd, %lld, alarmTime);
 
 'man rtcwake' says that not all systems support RTC wake from S4;
 systems that have a functioning nvram-wakeup will succeed, but that is
 not all systems.  Do we need to be more cautious about allowing S4
 suspension if we can't prove that RTC will wake up the system from S4?
 
 On the other hand, you are using -m no to just set the wakeup time,
 which ought to fail if the system does not support the requested delay
 or the ability to wake up, so that you never actually suspend until
 after you know the wakeup was successfully scheduled.
 
 Hmm, does that mean the public API should provide a way to schedule the
 wakeup without also scheduling a suspend?

But how would that help? The aim of having this API is to suspend and
resume the system.. and hence I don't see why it has to expose a
functionality to only schedule a wakeup..

 
 +++ b/src/util/threads-pthread.c
 @@ -81,10 +81,25 @@ void virMutexDestroy(virMutexPtr m)
  pthread_mutex_destroy(m-lock);
  }
  
 -void virMutexLock(virMutexPtr m){
 +void virMutexLock(virMutexPtr m)
 +{
  pthread_mutex_lock(m-lock);
  }
  
 +/**
 + * virMutexTryLock:
 
 I'm not convinced we need this.  As I understand it, your code does:
 
 thread 1:  thread 2: thread 3:
 request suspend
 grab lock
 spawn helper
sleep 10 sec
 return success
  request suspend
  lock not available
  return failure
suspend
resume
  request suspend
  lock not available
  return failure
release lock
 
 But we don't need a try-lock operation, if we do:
 
 thread 1:  thread 2: thread 3:
 request suspend
 grab lock
  request suspend
 mark flag to true
 release lock
  grab lock
  flag already true
  release lock
  return failure
 spawn helper
sleep 10 sec
 return success
suspend
resume
  grab lock
  flag already true
  release lock
  return failure
grab lock
clear flag
release lock
 
 That is, instead of holding the lock for the entire duration of the
 suspend, just hold the lock long enough to mark a volatile variable;
 then you no longer need a non-blocking try-lock primitive, because the
 lock will never starve anyone else long enough to be an issue.
 

Yes, that would work. (In fact, that was the way I first developed the
code. But then I felt trylock() was a fairly popular primitive to use
in such cases and hence I thought I might as well add it to libvirt).

Anyways, I am fine with going with the method you suggested above.

Thanks,
Srivatsa S. Bhat
IBM Linux Technology Center

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

Re: [libvirt] [PATCH v3 1/2] Implement the asynchronous suspend and RTC wakeup

2011-11-21 Thread Eric Blake 
On 11/09/2011 05:05 AM, Srivatsa S. Bhat wrote:
 Add the core functions that implement the functionality of the API.
 Suspend is done by using an asynchronous mechanism so that we can return
 the status to the caller successfully before the host gets suspended. This
 asynchronous operation is achieved by suspending the host in a separate
 thread of execution.
 
 To resume the host, an RTC alarm is set up (based on how long we want
 to suspend) before suspending the host. When this alarm fires, the host
 gets woken up.
 
 Signed-off-by: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
 ---
 
  include/libvirt/libvirt.h.in |5 +
  src/libvirt_private.syms |7 +
  src/nodeinfo.c   |  220 
 ++
  src/nodeinfo.h   |   14 +++
  src/qemu/qemu_driver.c   |5 +
  src/util/threads-pthread.c   |   17 +++
  src/util/threads.h   |1 

It looks weird seeing a public change (include/libvirt/libvirt.h.in)
mixed in with a driver-specific change (src/qemu/qemu_driver.c).  I
would favor re-ordering the patches, by splitting patch 2/2 into two
parts (public API, then src/remote/remote_protocol.x implementation of
remote driver), then putting this patch 1/2 as the third patch.   We
also need a fourth patch, to tools/virsh.{pod,c} to expose the new
feature through virsh.

  7 files changed, 268 insertions(+), 1 deletions(-)
 
 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index aa320b6..25f1c9b 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -3357,6 +3357,11 @@ typedef struct _virTypedParameter virMemoryParameter;
   */
  typedef virMemoryParameter *virMemoryParameterPtr;
  
 +typedef enum {
 +VIR_S3 = 1, /* Suspend-to-RAM */
 +VIR_S4 = 2 /* Suspend-to-disk */

We tend to use trailing commas in enum decls (we require C99, after
all), so that if we later add VIR_... = 3, we don't have to edit the
VIR_S4 = 2, /* Suspend-to-disk */ line.

 +} virSuspendState;
 +

This hunk should be moved alongside the half of 2/2 that deals with the
public API.  Also, float it up to appear earlier in the file;
virMemoryParameter is in the section of deprecated names at the bottom
for a reason, but virSuspendState is not deprecated, so it should appear
sooner.  I wonder if we should name this VIR_NODE_S3 and
virNodeSuspendState, since it a different concept of 'suspend' than
virDomainSuspend.

 +++ b/src/libvirt_private.syms
 @@ -844,6 +844,13 @@ nodeGetCellsFreeMemory;
  nodeGetFreeMemory;
  nodeGetInfo;
  nodeGetMemoryStats;
 +virSuspendLock;
 +virSuspendUnlock;
 +virSuspendInit;
 +nodeSuspendForDuration;
 +setNodeWakeup;
 +nodeSuspend;
 +virSuspendCleanup;

Sort these lines.

 @@ -897,3 +905,215 @@ unsigned long long nodeGetFreeMemory(virConnectPtr conn 
 ATTRIBUTE_UNUSED)
  return 0;
  }
  #endif
 +
 +
 +static int initialized;
 +virMutex virSuspendMutex;
 +
 +int virSuspendLock(void)
 +{
 +return virMutexTryLock(virSuspendMutex);
 +}
 +
 +void virSuspendUnlock(void)
 +{
 +virMutexUnlock(virSuspendMutex);
 +}
 +
 +/**
 + * virSuspendInit:
 + *
 + * Get the low power states supported by the host, such as Suspend-to-RAM 
 (S3)
 + * or Suspend-to-Disk (S4), so that a request to suspend/hibernate the host
 + * can be handled appropriately based on this information.
 + *
 + * Returns 0 if successful, and -1 in case of error.
 + */
 +int virSuspendInit(void)
 +{
 +
 +if (virMutexInit(virSuspendMutex)  0)
 +return -1;
 +
 +/* Get the power management capabilities supported by the host.
 + * Ensure that this is done only once, by using the 'initialized'
 + * variable.

Should this function be marked static?  If it should only be called
once, and 'initialized' is static, then no one outside of this file
should be calling it.

 + */
 +if (virGetPMCapabilities(hostPMFeatures)  0) {
 +VIR_ERROR(_(Failed to get host power management features));
 +return -1;
 +}
 +
 +return 0;
 +}
 +
 +
 +/**
 + * nodeSuspendForDuration:
 + * @conn: pointer to the hypervisor connection
 + * @state: the state to which the host must be suspended to -
 + * VIR_HOST_PM_S3 (Suspend-to-RAM)
 + * or VIR_HOST_PM_S4 (Suspend-to-disk)
 + * @duration: the time duration in seconds, for which the host
 + *must be suspended
 + *
 + * Suspend the node (host machine) for the given duration of time
 + * in the specified state (such as S3 or S4). Resume the node
 + * after the time duration is complete.
 + *
 + * An RTC alarm is set appropriately to wake up the node from
 + * its sleep state. Then the actual node suspend is carried out
 + * asynchronously in another thread, after a short time delay
 + * so as to give enough time for this function to return status
 + * to its caller through the connection.
 + *
 + * Returns 0 in case the node is going to be suspended after a short
 + * delay, -1 if suspending the node is not supported,

[libvirt] [PATCH v3 1/2] Implement the asynchronous suspend and RTC wakeup

2011-11-09 Thread Srivatsa S. Bhat 
Add the core functions that implement the functionality of the API.
Suspend is done by using an asynchronous mechanism so that we can return
the status to the caller successfully before the host gets suspended. This
asynchronous operation is achieved by suspending the host in a separate
thread of execution.

To resume the host, an RTC alarm is set up (based on how long we want
to suspend) before suspending the host. When this alarm fires, the host
gets woken up.

Signed-off-by: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
---

 include/libvirt/libvirt.h.in |5 +
 src/libvirt_private.syms |7 +
 src/nodeinfo.c   |  220 ++
 src/nodeinfo.h   |   14 +++
 src/qemu/qemu_driver.c   |5 +
 src/util/threads-pthread.c   |   17 +++
 src/util/threads.h   |1 
 7 files changed, 268 insertions(+), 1 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index aa320b6..25f1c9b 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3357,6 +3357,11 @@ typedef struct _virTypedParameter virMemoryParameter;
  */
 typedef virMemoryParameter *virMemoryParameterPtr;
 
+typedef enum {
+VIR_S3 = 1, /* Suspend-to-RAM */
+VIR_S4 = 2 /* Suspend-to-disk */
+} virSuspendState;
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6a1562e..2fa84e0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -844,6 +844,13 @@ nodeGetCellsFreeMemory;
 nodeGetFreeMemory;
 nodeGetInfo;
 nodeGetMemoryStats;
+virSuspendLock;
+virSuspendUnlock;
+virSuspendInit;
+nodeSuspendForDuration;
+setNodeWakeup;
+nodeSuspend;
+virSuspendCleanup;
 
 
 # nwfilter_conf.h
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 6448b79..3c67fe6 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -46,6 +46,9 @@
 #include count-one-bits.h
 #include intprops.h
 #include virfile.h
+#include command.h
+#include threads.h
+#include datatypes.h
 
 
 #define VIR_FROM_THIS VIR_FROM_NONE
@@ -65,6 +68,11 @@
 # define LINUX_NB_MEMORY_STATS_ALL 4
 # define LINUX_NB_MEMORY_STATS_CELL 2
 
+/* Bitmask to hold the Power Management features supported by the host,
+ * such as Suspend-to-RAM (S3), Suspend-to-Disk (S4) etc.
+ */
+static unsigned int hostPMFeatures;
+
 /* NB, this is not static as we need to call it from the testsuite */
 int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
  virNodeInfoPtr nodeinfo,
@@ -897,3 +905,215 @@ unsigned long long nodeGetFreeMemory(virConnectPtr conn 
ATTRIBUTE_UNUSED)
 return 0;
 }
 #endif
+
+
+static int initialized;
+virMutex virSuspendMutex;
+
+int virSuspendLock(void)
+{
+return virMutexTryLock(virSuspendMutex);
+}
+
+void virSuspendUnlock(void)
+{
+virMutexUnlock(virSuspendMutex);
+}
+
+/**
+ * virSuspendInit:
+ *
+ * Get the low power states supported by the host, such as Suspend-to-RAM (S3)
+ * or Suspend-to-Disk (S4), so that a request to suspend/hibernate the host
+ * can be handled appropriately based on this information.
+ *
+ * Returns 0 if successful, and -1 in case of error.
+ */
+int virSuspendInit(void)
+{
+
+if (virMutexInit(virSuspendMutex)  0)
+return -1;
+
+/* Get the power management capabilities supported by the host.
+ * Ensure that this is done only once, by using the 'initialized'
+ * variable.
+ */
+if (virGetPMCapabilities(hostPMFeatures)  0) {
+VIR_ERROR(_(Failed to get host power management features));
+return -1;
+}
+
+return 0;
+}
+
+
+/**
+ * nodeSuspendForDuration:
+ * @conn: pointer to the hypervisor connection
+ * @state: the state to which the host must be suspended to -
+ * VIR_HOST_PM_S3 (Suspend-to-RAM)
+ * or VIR_HOST_PM_S4 (Suspend-to-disk)
+ * @duration: the time duration in seconds, for which the host
+ *must be suspended
+ *
+ * Suspend the node (host machine) for the given duration of time
+ * in the specified state (such as S3 or S4). Resume the node
+ * after the time duration is complete.
+ *
+ * An RTC alarm is set appropriately to wake up the node from
+ * its sleep state. Then the actual node suspend is carried out
+ * asynchronously in another thread, after a short time delay
+ * so as to give enough time for this function to return status
+ * to its caller through the connection.
+ *
+ * Returns 0 in case the node is going to be suspended after a short
+ * delay, -1 if suspending the node is not supported, or if a
+ * previous suspend operation is still in progress.
+ */
+int nodeSuspendForDuration(virConnectPtr conn ATTRIBUTE_UNUSED,
+   int state,
+   unsigned long long duration)
+{
+static virThread thread;
+char *cmdString;
+
+if (!initialized) {
+if(virSuspendInit()  0)
+return -1;
+initialized = 1;
+}
+
+/*
+ * Ensure that we are the only ones trying