Eli Qiao

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 <liyong.qiao@intel.com>
---
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 0000000..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
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ * Eli Qiao <liyong.qiao@intel.com>
+ */
+#include <config.h>
+
+#include <sys/ioctl.h>
+#if defined HAVE_SYS_SYSCALL_H
+# include <sys/syscall.h>
+#endif
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#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(&path, "%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

+
+ 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(&tmp, "%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 &ResCtrlAll[type];
+}

diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
new file mode 100644
index 0000000..f713e66
--- /dev/null
+++ b/src/util/virresctrl.h
@@ -0,0 +1,89 @@

+
+#ifndef __VIR_RESCTRL_H__
+# define __VIR_RESCTRL_H__
+
+# include "virutil.h"
+# include "virbitmap.h"
+
+#define RESCTRL_DIR "/sys/fs/resctrl"
+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"
+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
+#define MAX_CPU_SOCKET_NUM 8
+#define MAX_CBM_BIT_LEN 32
+#define MAX_SCHEMATA_LEN 1024
+#define MAX_FILE_LEN ( 10 * 1024 * 1024)

Doesn't seem like any of this needs to be in the header file

okay, will move to c file.

+
+enum {
+ RDT_RESOURCE_L3,
+ RDT_RESOURCE_L3DATA,
+ RDT_RESOURCE_L3CODE,
+ RDT_RESOURCE_L2,
+ /* Must be the last */
+ RDT_NUM_RESOURCES,

Use a VIR_ prefix on these constants. Also the libvirt naming

convention is to use RDT_RESOURCE_LAST as the last element.

+};
+
+
+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.

+bool virResCtrlAvailable(void);
+int virResCtrlInit(void);
+virResCtrlPtr virResCtrlGet(int);

API docs for these would be nice, especially describing whether

virResCtrlPtr returned from this method is immutable or not.

Since libvirt is multi-threaded this is important to know.

Okay, I will fill all API docs for the public APIs