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

[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 Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111122/ceb50e1b/attachment-0001.sig>