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 resctrlinformation.virResCtrlAvailable: If resctrl interface exist on hostvirResCtrlGet: get specify type resource contral informationvirResCtrlInit: initialize resctrl struct from the host's sys fs.ResCtrlAll[]: an array to maintain resource control information.Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx>---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.ccreate mode 100644 src/util/virresctrl.hdiff --git a/src/util/virresctrl.c b/src/util/virresctrl.cnew file mode 100644index 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@xxxxxxxxx>+ */+#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 eachpatch to catch this kind of policy error. There's quite a few otherstyle 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(&tmp, "%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[i].name)) < 0) {+ continue;Silently ignoring OOM is not good - please return a proper error - usingvirAsprintf 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
+ }++ 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.hnew file mode 100644index 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 namingconvention is to use RDT_RESOURCE_LAST as the last element.
okay
+};+++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 allimmutable 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 whethervirResCtrlPtr 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
