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

Re: [libvirt] [RFC v3] Export KVM Host Power Management capabilities



On 08/08/2011 09:18 AM, Srivatsa S. Bhat wrote:
Open issues:
-----------
1. Design new APIs in libvirt to actually exploit the host power management
    features instead of relying on external programs. This was discussed in
    [4].

As pointed out in [4], until we have additional use for this feature, then this feature in isolation is unlikely to be pushed. That is not meant to discourage you from developing further patches, rather it is just stating that this patch will probably be deferred until it can be pushed as part of a larger series.

2. Decide on whether to include "pm-utils" package in the libvirt.spec
    file considering the fact that the package name (pm-utils) may differ
    from one Linux distribution to another.

No decision to make - libvirt.spec is solely for Fedora, where the package is always named pm-utils. Other distros use other packaging files, and can adjust accordingly, it's just that none of those other packaging files have been contributed for inclusion in upstream libvirt.git.

+  <define name='power_management'>
+    <choice>

No need for a <choice> here.

+      <element name='power_management'>
+        <optional>
+          <element name='S3'>
+            <empty/>
+          </element>
+        </optional>
+        <optional>
+          <element name='S4'>
+            <empty/>
+          </element>
+        </optional>
+      </element>

The two <optional> blocks are sufficient for allowing <power_management/> as an empty element, without any further rng grammar.


+/**
+ * virCapabilitiesAddHostPowerManagement:
+ * @caps: capabilities to extend
+ * @feature: the power management feature to be added
+ *
+ * Registers a new host power management feature, eg: 'S3' or 'S4'
+ */
+int
+virCapabilitiesAddHostPowerManagement(virCapsPtr caps,
+                                      int feature)
+{
+    if(VIR_RESIZE_N(caps->host.powerMgmt, caps->host.npowerMgmt_max,
+                   caps->host.npowerMgmt, 1)<  0) {
+        virReportOOMError();
+        return -1;
+    }
+
+    caps->host.powerMgmt[caps->host.npowerMgmt] = feature;
+    caps->host.npowerMgmt++;

After thinking about this more, I'm wondering if it would be smarter to represent power management as a bitmask:

caps->hst.powerMgmt |= 1U << feature;

at which point, you still need isPMQuerySuccess (but see below on naming), but you don't need npowerMgmt. Instead, output would be:

@@ -686,6 +717,25 @@ virCapabilitiesFormatXML(virCapsPtr caps)

      virBufferAddLit(&xml, "</cpu>\n");

+    if(caps->host.isPMQuerySuccess) {

if (caps->host.powerMgmt) {
  unsigned int pm = caps->host.powerMgmt;
  virBufferAddLit(&xml, "<power_management>\n");
  while (pm) {
    int bit = ffs(pm) - 1;
    virBufferAsprintf(&xml, "<%s/>\n",
      virHostPMCapabilityTypeToString(bit);
    pm &= ~bit;
  }
  virBufferAddLit(&xml, "</power_management>\n");
} else {
  virBufferAddLit(&xml, "<power_management/>\n");
}

+++ b/src/conf/capabilities.h
@@ -105,6 +105,10 @@ struct _virCapsHost {
      size_t nfeatures;
      size_t nfeatures_max;
      char **features;
+    bool isPMQuerySuccess;

This isn't typical naming.  For consistency, I'm thinking:

bool powerMgmt_valid;

defaulting to false (powerMgmt is irrelavant), but set to true when powerMgmt contains valid data (where 0 can include valid data).

+    size_t npowerMgmt;
+    size_t npowerMgmt_max;
+    int *powerMgmt;    /* enum virHostPMCapability */

See above about just using 'unsigned int powerMgmt' as a bit-mask of valid capabilities, rather than a dynamically allocated array of ints.

@@ -824,6 +825,32 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps)
          old_caps->host.cpu = NULL;
      }

+    /* Add the power management features of the host */
+
+    /* Check for Suspend-to-RAM support (S3) */
+    status = virCheckPMCapability(VIR_HOST_PM_S3);
+    if(status<  0) {
+        caps->host.isPMQuerySuccess = false;
+        VIR_WARN("Failed to get host power management features");
+    } else {
+        /* The PM Query succeeded */
+        caps->host.isPMQuerySuccess = true;
+        if(status == 1)    /* S3 is supported */
+            virCapabilitiesAddHostPowerManagement(caps, VIR_HOST_PM_S3);
+    }
+
+    /* Check for Suspend-to-Disk support (S4) */
+    status = virCheckPMCapability(VIR_HOST_PM_S4);
+    if(status<  0) {
+        caps->host.isPMQuerySuccess = false;
+        VIR_WARN("Failed to get host power management features");
+    } else {
+        /* The PM Query succeeded */
+        caps->host.isPMQuerySuccess = true;
+        if(status == 1)    /* S4 is supported */
+            virCapabilitiesAddHostPowerManagement(caps, VIR_HOST_PM_S4);
+    }

You don't want to set caps->host.isPMQuerySuccess (by whatever name) to true unless both commands succeeded. If the first command succeeds but the second fails, then you are better off treating the entire operation as failed.

Hmm, maybe virCheckPMCapability should take no arguments, and return the bitmask directly, rather than making qemuCapsInit have to compute the right bitmask.

  }
+
+/**
+ * Check the Power Management Capabilities of the host system.
+ * The script 'pm-is-supported' (from the pm-utils package) is run
+ * to find out if the capability is supported by the host.
+ *
+ * @capability: capability to check for
+ * VIR_HOST_PM_S3: Check for Suspend-to-RAM support
+ * VIR_HOST_PM_S4: Check for Suspend-to-Disk support
+ *
+ * Return values:
+ * 1 if the capability is supported.
+ * 0 if the query was successful but the capability is
+ *   not supported by the host.
+ * -1 on error like 'pm-is-supported' is not found.
+ */
+int
+virCheckPMCapability(int capability)
+{

As I mentioned above, it would be nicer to have this return the bitmask up front, rather than to make each caller have to repeatedly call this function for as many capabilities as are currently defined. -1 for unknown, 0 for success but no support, or positive for a bitmask of 1<<bit for each supported bit.

+++ b/src/util/virterror.c
@@ -148,6 +148,9 @@ static const char *virErrorDomainName(virErrorDomain domain) {
          case VIR_FROM_CPU:
              dom = "CPU ";
              break;
+        case VIR_FROM_CAPABILITIES:
+            dom = "Capabilities ";
+            break;
          case VIR_FROM_NWFILTER:
              dom = "Network Filter ";
              break;

Better to make these case statements line up with declaration order (VIR_FROM_CAPABILITIES is new, so it should be at the end of the list just before default:).

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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