Re: [libvirt] [resend v2 1/7] Resctrl: Add some utils functions

[Daniel P. Berrange]

On Tue, Feb 07, 2017 at 02:43:17PM +0800, Eli Qiao wrote:
> On Tuesday, 7 February 2017 at 12:11 AM, Daniel P. Berrange wrote:
> 
> > On Mon, Feb 06, 2017 at 10:23:36AM +0800, Eli Qiao wrote:
> > > This patch adds some utils struct and functions to expose resctrl
> > > information.
> > > 
> > > virResCtrlAvailable: If resctrl interface exist on host
> > > virResCtrlGet: get specify type resource contral information
> > > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > > ResCtrlAll[]: an array to maintain resource control information.
> > > 
> > > Signed-off-by: Eli Qiao  > > (mailto:liyong.q...@intel.com)>
> > > ---
> > > +};
> > > +
> > > +
> > > +typedef struct _virResCacheBank virResCacheBank;
> > > +typedef virResCacheBank *virResCacheBankPtr;
> > > +struct _virResCacheBank {
> > > + unsigned int host_id;
> > > + unsigned long long cache_size;
> > > + unsigned long long cache_left;
> > > + unsigned long long cache_min;
> > > + virBitmapPtr cpu_mask;
> > > +};
> > > 
> > 
> > 
> > > +typedef struct _virResCtrl virResCtrl;
> > > +typedef virResCtrl *virResCtrlPtr;
> > > +struct _virResCtrl {
> > > + bool enabled;
> > > + const char *name;
> > > + int num_closid;
> > > + int cbm_len;
> > > + int min_cbm_bits;
> > > + const char* cache_level;
> > > + int num_banks;
> > > + virResCacheBankPtr cache_banks;
> > > +};
> > > 
> > 
> > 
> > Can any of these fields change at runtime, or are they all
> > immutable once populated.
> 
> Only cache_banks will be change at runtime, such as cache_left
> on each socket will be updated if libvirt allocates cache to domains.

Ok, so the API is returning a direct point to the virResCtrlPtr
struct and there's no locking here. So if cache_banks struct
contents changes while some caller is reading the virResCtrlPtr
struct we have race problems.

I'd suggest that best option is probably to take the mutable
cache_left field *out* of the struct. That lets the callers
treat the entire struct as immutable.

Then add a separate API to explicitly query the cache_left
value - only that api needs to do locking to return a consistent
point-in-time view of the cache_left value.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

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

Re: [libvirt] [resend v2 1/7] Resctrl: Add some utils functions

[Eli Qiao]

-- 
Eli Qiao
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)


On Tuesday, 7 February 2017 at 12:11 AM, Daniel P. Berrange wrote:

> On Mon, Feb 06, 2017 at 10:23:36AM +0800, Eli Qiao wrote:
> > This patch adds some utils struct and functions to expose resctrl
> > information.
> > 
> > virResCtrlAvailable: If resctrl interface exist on host
> > virResCtrlGet: get specify type resource contral information
> > virResCtrlInit: initialize resctrl struct from the host's sys fs.
> > ResCtrlAll[]: an array to maintain resource control information.
> > 
> > Signed-off-by: Eli Qiao  > (mailto:liyong.q...@intel.com)>
> > ---
> > include/libvirt/virterror.h | 1 +
> > src/Makefile.am (http://Makefile.am) | 1 +
> > src/libvirt_private.syms | 4 +
> > src/util/virerror.c | 1 +
> > src/util/virresctrl.c | 330 
> > src/util/virresctrl.h | 89 
> > 6 files changed, 426 insertions(+)
> > create mode 100644 src/util/virresctrl.c
> > create mode 100644 src/util/virresctrl.h
> > 
> 
> 
> > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> > new file mode 100644
> > index 000..63bc808
> > --- /dev/null
> > +++ b/src/util/virresctrl.c
> > @@ -0,0 +1,330 @@
> > +/*
> > + * virresctrl.c: methods for managing resource contral
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * .
> > + *
> > + * Authors:
> > + * Eli Qiao 
> > + */
> > +#include 
> > +
> > +#include 
> > +#if defined HAVE_SYS_SYSCALL_H
> > +# include 
> > +#endif
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "virresctrl.h"
> > +#include "viralloc.h"
> > +#include "virerror.h"
> > +#include "virfile.h"
> > +#include "virhostcpu.h"
> > +#include "virlog.h"
> > +#include "virstring.h"
> > +#include "nodeinfo.h"
> > +
> > +VIR_LOG_INIT("util.resctrl");
> > +
> > +#define VIR_FROM_THIS VIR_FROM_RESCTRL
> > +
> > +static unsigned int host_id = 0;
> > +
> > +static virResCtrl ResCtrlAll[] = {
> > 
> 
> 
> Lowercase for global variable names please.
> 
 
> > + {
> > + .name = "L3",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L3DATA",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L3CODE",
> > + .cache_level = "l3",
> > + },
> > + {
> > + .name = "L2",
> > + .cache_level = "l2",
> > + },
> > +};
> > +
> > +static int virResCtrlGetInfoStr(const int type, const char *item, char 
> > **str)
> > +{
> > + int ret = 0;
> > + char *tmp;
> > + char *path;
> > +
> > + if (asprintf(, "%s/%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[type].name, 
> > item) < 0)
> > + return -1;
> > 
> 
> 
> Use of asprintf is forbidden in libvirt - use virAsprintf.
> 
> Please make sure to run 'make check' and 'make syntax-check' on each
> patch to catch this kind of policy error. There's quite a few other
> style rules missed in these patches - i won't list them all since
> 'make syntax-check' can tell you.
> 
> 
> 


okay, thanks Daniel. 
> 
> > + if (virFileReadAll(path, 10, str) < 0) {
> > + ret = -1;
> > + goto cleanup;
> > + }
> > +
> > + if ((tmp = strchr(*str, '\n'))) {
> > + *tmp = '\0';
> > + }
> > +
> > +cleanup:
> > + VIR_FREE(path);
> > + return ret;
> > +}
> > +
> > +
> > +static int virResCtrlGetCPUValue(const char* path, char** value)
> > +{
> > + int ret = -1;
> > + char* tmp;
> > 
> 
> 
> The '*' should be next to the variable name, not the type name.
> Multiple other cases follow
> 
okay 
> > +
> > + if(virFileReadAll(path, 10, value) < 0) {
> > + goto cleanup;
> > + }
> > + if ((tmp = strchr(*value, '\n'))) {
> > + *tmp = '\0';
> > + }
> > + ret = 0;
> > +cleanup:
> > + return ret;
> > +}
> > +
> > 
> 
> 
> 
> > +int virResCtrlInit(void) {
> > + int i = 0;
> > + char *tmp;
> > + int rc = 0;
> > +
> > + for(i = 0; i < RDT_NUM_RESOURCES; i++) {
> > + if ((rc = asprintf(, "%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[i].name)) 
> > < 0) {
> > + continue;
> > 
> 
> 
> Silently ignoring OOM is not good - please return a proper error - using
> virAsprintf would do so.
> 
okay. 
> > + }
> > + if (virFileExists(tmp)) {
> > + if ((rc = virResCtrlGetConfig(i)) < 0 )
> > + VIR_WARN("Ignor error while get config for %d", i);
> > 
> 
> 
> Again don't ignore errors like this - this should be fatal.
> 
sure 
> > + }
> > +
> > + 

Re: [libvirt] [resend v2 1/7] Resctrl: Add some utils functions

[Daniel P. Berrange]

On Mon, Feb 06, 2017 at 10:23:36AM +0800, Eli Qiao wrote:
> This patch adds some utils struct and functions to expose resctrl
> information.
> 
> virResCtrlAvailable: If resctrl interface exist on host
> virResCtrlGet: get specify type resource contral information
> virResCtrlInit: initialize resctrl struct from the host's sys fs.
> ResCtrlAll[]: an array to maintain resource control information.
> 
> Signed-off-by: Eli Qiao 
> ---
>  include/libvirt/virterror.h |   1 +
>  src/Makefile.am |   1 +
>  src/libvirt_private.syms|   4 +
>  src/util/virerror.c |   1 +
>  src/util/virresctrl.c   | 330 
> 
>  src/util/virresctrl.h   |  89 
>  6 files changed, 426 insertions(+)
>  create mode 100644 src/util/virresctrl.c
>  create mode 100644 src/util/virresctrl.h

> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> new file mode 100644
> index 000..63bc808
> --- /dev/null
> +++ b/src/util/virresctrl.c
> @@ -0,0 +1,330 @@
> +/*
> + * virresctrl.c: methods for managing resource contral
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + * Authors:
> + *  Eli Qiao 
> + */
> +#include 
> +
> +#include 
> +#if defined HAVE_SYS_SYSCALL_H
> +# include 
> +#endif
> +#include 
> +#include 
> +#include 
> +
> +#include "virresctrl.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virfile.h"
> +#include "virhostcpu.h"
> +#include "virlog.h"
> +#include "virstring.h"
> +#include "nodeinfo.h"
> +
> +VIR_LOG_INIT("util.resctrl");
> +
> +#define VIR_FROM_THIS VIR_FROM_RESCTRL
> +
> +static unsigned int host_id = 0;
> +
> +static virResCtrl ResCtrlAll[] = {

Lowercase for global variable names please.

> +{
> +.name = "L3",
> +.cache_level = "l3",
> +},
> +{
> +.name = "L3DATA",
> +.cache_level = "l3",
> +},
> +{
> +.name = "L3CODE",
> +.cache_level = "l3",
> +},
> +{
> +.name = "L2",
> +.cache_level = "l2",
> +},
> +};
> +
> +static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
> +{
> +int ret = 0;
> +char *tmp;
> +char *path;
> +
> +if (asprintf(, "%s/%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[type].name, 
> item) < 0)
> +return -1;

Use of asprintf is forbidden in libvirt - use virAsprintf.

Please make sure to run 'make check' and 'make syntax-check' on each
patch to catch this kind of policy error. There's quite a few other
style rules missed in these patches - i won't list them all since
'make syntax-check' can tell you.

> +if (virFileReadAll(path, 10, str) < 0) {
> +ret = -1;
> +goto cleanup;
> +}
> +
> +if ((tmp = strchr(*str, '\n'))) {
> +*tmp = '\0';
> +}
> +
> +cleanup:
> +VIR_FREE(path);
> +return ret;
> +}
> +
> +
> +static int virResCtrlGetCPUValue(const char* path, char** value)
> +{
> +int ret = -1;
> +char* tmp;

The '*' should be next to the variable name, not the type name.
Multiple other cases follow

> +
> +if(virFileReadAll(path, 10, value) < 0) {
> +goto cleanup;
> +}
> +if ((tmp = strchr(*value, '\n'))) {
> +*tmp = '\0';
> +}
> +ret = 0;
> +cleanup:
> +return ret;
> +}
> +


> +int virResCtrlInit(void) {
> +int i = 0;
> +char *tmp;
> +int rc = 0;
> +
> +for(i = 0; i < RDT_NUM_RESOURCES; i++) {
> +if ((rc = asprintf(, "%s/%s", RESCTRL_INFO_DIR, 
> ResCtrlAll[i].name)) < 0) {
> +continue;

Silently ignoring OOM is not good - please return a proper error - using
virAsprintf would do so.

> +}
> +if (virFileExists(tmp)) {
> +if ((rc = virResCtrlGetConfig(i)) < 0 )
> +VIR_WARN("Ignor error while get config for %d", i);

Again don't ignore errors like this - this should be fatal.

> +}
> +
> +VIR_FREE(tmp);
> +}
> +return rc;
> +}
> +
> +bool virResCtrlAvailable(void) {
> +if (!virFileExists(RESCTRL_INFO_DIR))
> +return false;
> +return true;
> +}
> +
> +virResCtrlPtr virResCtrlGet(int type) {
> +return [type];
> +}


> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
> new file mode 100644
> index 000..f713e66
> --- 

[libvirt] [resend v2 1/7] Resctrl: Add some utils functions

[Eli Qiao]

This patch adds some utils struct and functions to expose resctrl
information.

virResCtrlAvailable: If resctrl interface exist on host
virResCtrlGet: get specify type resource contral information
virResCtrlInit: initialize resctrl struct from the host's sys fs.
ResCtrlAll[]: an array to maintain resource control information.

Signed-off-by: Eli Qiao 
---
 include/libvirt/virterror.h |   1 +
 src/Makefile.am |   1 +
 src/libvirt_private.syms|   4 +
 src/util/virerror.c |   1 +
 src/util/virresctrl.c   | 330 
 src/util/virresctrl.h   |  89 
 6 files changed, 426 insertions(+)
 create mode 100644 src/util/virresctrl.c
 create mode 100644 src/util/virresctrl.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 2efee8f..3dd2d08 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -132,6 +132,7 @@ typedef enum {
 
 VIR_FROM_PERF = 65, /* Error from perf */
 VIR_FROM_LIBSSH = 66,   /* Error from libssh connection transport */
+VIR_FROM_RESCTRL = 67,  /* Error from resource control */
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_ERR_DOMAIN_LAST
diff --git a/src/Makefile.am b/src/Makefile.am
index 2f32d41..b626f29 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -162,6 +162,7 @@ UTIL_SOURCES =  
\
util/virprocess.c util/virprocess.h \
util/virqemu.c util/virqemu.h   \
util/virrandom.h util/virrandom.c   \
+   util/virresctrl.h util/virresctrl.c \
util/virrotatingfile.h util/virrotatingfile.c   \
util/virscsi.c util/virscsi.h   \
util/virscsivhost.c util/virscsivhost.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8e994c7..743e5ac 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2313,6 +2313,10 @@ virRandomGenerateWWN;
 virRandomInt;
 
 
+# util/virresctrl.h
+virResCtrlAvailable;
+virResCtrlInit;
+
 # util/virrotatingfile.h
 virRotatingFileReaderConsume;
 virRotatingFileReaderFree;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index ef17fb5..93dfd4f 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -139,6 +139,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
 
   "Perf", /* 65 */
   "Libssh transport layer",
+  "Rescouce Control",
 )
 
 
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
new file mode 100644
index 000..63bc808
--- /dev/null
+++ b/src/util/virresctrl.c
@@ -0,0 +1,330 @@
+/*
+ * virresctrl.c: methods for managing resource contral
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *  Eli Qiao 
+ */
+#include 
+
+#include 
+#if defined HAVE_SYS_SYSCALL_H
+# include 
+#endif
+#include 
+#include 
+#include 
+
+#include "virresctrl.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virfile.h"
+#include "virhostcpu.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "nodeinfo.h"
+
+VIR_LOG_INIT("util.resctrl");
+
+#define VIR_FROM_THIS VIR_FROM_RESCTRL
+
+static unsigned int host_id = 0;
+
+static virResCtrl ResCtrlAll[] = {
+{
+.name = "L3",
+.cache_level = "l3",
+},
+{
+.name = "L3DATA",
+.cache_level = "l3",
+},
+{
+.name = "L3CODE",
+.cache_level = "l3",
+},
+{
+.name = "L2",
+.cache_level = "l2",
+},
+};
+
+static int virResCtrlGetInfoStr(const int type, const char *item, char **str)
+{
+int ret = 0;
+char *tmp;
+char *path;
+
+if (asprintf(, "%s/%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[type].name, 
item) < 0)
+return -1;
+if (virFileReadAll(path, 10, str) < 0) {
+ret = -1;
+goto cleanup;
+}
+
+if ((tmp = strchr(*str, '\n'))) {
+*tmp = '\0';
+}
+
+cleanup:
+VIR_FREE(path);
+return ret;
+}
+
+
+static int virResCtrlGetCPUValue(const char* path, char** value)
+{
+int ret = -1;
+char* tmp;
+
+if(virFileReadAll(path, 10, value) < 0) {
+goto cleanup;
+}
+if ((tmp = strchr(*value, '\n')))