<div>
<br>
</div>
<div><div><br></div><div>-- </div><div>Eli Qiao</div><div>Sent with <a href="http://www.sparrowmailapp.com/?sig">Sparrow</a></div><div><br></div></div>
<p style="color: #A0A0A8;">On Tuesday, 7 February 2017 at 12:11 AM, Daniel P. Berrange wrote:</p><blockquote type="cite"><div>
<span><div><div><div>On Mon, Feb 06, 2017 at 10:23:36AM +0800, Eli Qiao wrote:</div><blockquote type="cite"><div><div>This patch adds some utils struct and functions to expose resctrl</div><div>information.</div><div><br></div><div>virResCtrlAvailable: If resctrl interface exist on host</div><div>virResCtrlGet: get specify type resource contral information</div><div>virResCtrlInit: initialize resctrl struct from the host's sys fs.</div><div>ResCtrlAll[]: an array to maintain resource control information.</div><div><br></div><div>Signed-off-by: Eli Qiao <<a href="mailto:liyong.qiao@intel.com">liyong.qiao@intel.com</a>></div><div>---</div><div> include/libvirt/virterror.h | 1 +</div><div> src/<a href="http://Makefile.am">Makefile.am</a> | 1 +</div><div> src/libvirt_private.syms | 4 +</div><div> src/util/virerror.c | 1 +</div><div> src/util/virresctrl.c | 330 ++++++++++++++++++++++++++++++++++++++++++++</div><div> src/util/virresctrl.h | 89 ++++++++++++</div><div> 6 files changed, 426 insertions(+)</div><div> create mode 100644 src/util/virresctrl.c</div><div> create mode 100644 src/util/virresctrl.h</div></div></blockquote><div><br></div><blockquote type="cite"><div><div>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c</div><div>new file mode 100644</div><div>index 0000000..63bc808</div><div>--- /dev/null</div><div>+++ b/src/util/virresctrl.c</div><div>@@ -0,0 +1,330 @@</div><div>+/*</div><div>+ * virresctrl.c: methods for managing resource contral</div><div>+ *</div><div>+ * This library is free software; you can redistribute it and/or</div><div>+ * modify it under the terms of the GNU Lesser General Public</div><div>+ * License as published by the Free Software Foundation; either</div><div>+ * version 2.1 of the License, or (at your option) any later version.</div><div>+ *</div><div>+ * This library is distributed in the hope that it will be useful,</div><div>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of</div><div>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU</div><div>+ * Lesser General Public License for more details.</div><div>+ *</div><div>+ * You should have received a copy of the GNU Lesser General Public</div><div>+ * License along with this library. If not, see</div><div>+ * <<a href="http://www.gnu.org/licenses/">http://www.gnu.org/licenses/</a>>.</div><div>+ *</div><div>+ * Authors:</div><div>+ * Eli Qiao <<a href="mailto:liyong.qiao@intel.com">liyong.qiao@intel.com</a>></div><div>+ */</div><div>+#include <config.h></div><div>+</div><div>+#include <sys/ioctl.h></div><div>+#if defined HAVE_SYS_SYSCALL_H</div><div>+# include <sys/syscall.h></div><div>+#endif</div><div>+#include <sys/types.h></div><div>+#include <sys/stat.h></div><div>+#include <fcntl.h></div><div>+</div><div>+#include "virresctrl.h"</div><div>+#include "viralloc.h"</div><div>+#include "virerror.h"</div><div>+#include "virfile.h"</div><div>+#include "virhostcpu.h"</div><div>+#include "virlog.h"</div><div>+#include "virstring.h"</div><div>+#include "nodeinfo.h"</div><div>+</div><div>+VIR_LOG_INIT("util.resctrl");</div><div>+</div><div>+#define VIR_FROM_THIS VIR_FROM_RESCTRL</div><div>+</div><div>+static unsigned int host_id = 0;</div><div>+</div><div>+static virResCtrl ResCtrlAll[] = {</div></div></blockquote><div><br></div><div>Lowercase for global variable names please.</div><div><br></div></div></div></span></div></blockquote><div> </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+ {</div><div>+ .name = "L3",</div><div>+ .cache_level = "l3",</div><div>+ },</div><div>+ {</div><div>+ .name = "L3DATA",</div><div>+ .cache_level = "l3",</div><div>+ },</div><div>+ {</div><div>+ .name = "L3CODE",</div><div>+ .cache_level = "l3",</div><div>+ },</div><div>+ {</div><div>+ .name = "L2",</div><div>+ .cache_level = "l2",</div><div>+ },</div><div>+};</div><div>+</div><div>+static int virResCtrlGetInfoStr(const int type, const char *item, char **str)</div><div>+{</div><div>+ int ret = 0;</div><div>+ char *tmp;</div><div>+ char *path;</div><div>+</div><div>+ if (asprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[type].name, item) < 0)</div><div>+ return -1;</div></div></blockquote><div><br></div><div>Use of asprintf is forbidden in libvirt - use virAsprintf.</div><div><br></div><div>Please make sure to run 'make check' and 'make syntax-check' on each</div><div>patch to catch this kind of policy error. There's quite a few other</div><div>style rules missed in these patches - i won't list them all since</div><div>'make syntax-check' can tell you.</div></div></div></span></div></blockquote><div><br></div><div>okay, thanks Daniel. </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><div><div>+ if (virFileReadAll(path, 10, str) < 0) {</div><div>+ ret = -1;</div><div>+ goto cleanup;</div><div>+ }</div><div>+</div><div>+ if ((tmp = strchr(*str, '\n'))) {</div><div>+ *tmp = '\0';</div><div>+ }</div><div>+</div><div>+cleanup:</div><div>+ VIR_FREE(path);</div><div>+ return ret;</div><div>+}</div><div>+</div><div>+</div><div>+static int virResCtrlGetCPUValue(const char* path, char** value)</div><div>+{</div><div>+ int ret = -1;</div><div>+ char* tmp;</div></div></blockquote><div><br></div><div>The '*' should be next to the variable name, not the type name.</div><div>Multiple other cases follow</div><div><br></div></div></div></span></div></blockquote><div>okay </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+</div><div>+ if(virFileReadAll(path, 10, value) < 0) {</div><div>+ goto cleanup;</div><div>+ }</div><div>+ if ((tmp = strchr(*value, '\n'))) {</div><div>+ *tmp = '\0';</div><div>+ }</div><div>+ ret = 0;</div><div>+cleanup:</div><div>+ return ret;</div><div>+}</div><div>+</div></div></blockquote><div><br></div><div><br></div><blockquote type="cite"><div><div>+int virResCtrlInit(void) {</div><div>+ int i = 0;</div><div>+ char *tmp;</div><div>+ int rc = 0;</div><div>+</div><div>+ for(i = 0; i < RDT_NUM_RESOURCES; i++) {</div><div>+ if ((rc = asprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, ResCtrlAll[i].name)) < 0) {</div><div>+ continue;</div></div></blockquote><div><br></div><div>Silently ignoring OOM is not good - please return a proper error - using</div><div>virAsprintf would do so.</div><div><br></div></div></div></span></div></blockquote><div>okay. </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+ }</div><div>+ if (virFileExists(tmp)) {</div><div>+ if ((rc = virResCtrlGetConfig(i)) < 0 )</div><div>+ VIR_WARN("Ignor error while get config for %d", i);</div></div></blockquote><div><br></div><div>Again don't ignore errors like this - this should be fatal.</div><div><br></div></div></div></span></div></blockquote><div>sure </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+ }</div><div>+</div><div>+ VIR_FREE(tmp);</div><div>+ }</div><div>+ return rc;</div><div>+}</div><div>+</div><div>+bool virResCtrlAvailable(void) {</div><div>+ if (!virFileExists(RESCTRL_INFO_DIR))</div><div>+ return false;</div><div>+ return true;</div><div>+}</div><div>+</div><div>+virResCtrlPtr virResCtrlGet(int type) {</div><div>+ return &ResCtrlAll[type];</div><div>+}</div></div></blockquote><div><br></div><div><br></div><blockquote type="cite"><div><div>diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h</div><div>new file mode 100644</div><div>index 0000000..f713e66</div><div>--- /dev/null</div><div>+++ b/src/util/virresctrl.h</div><div>@@ -0,0 +1,89 @@</div></div></blockquote><div><br></div><blockquote type="cite"><div><div>+</div><div>+#ifndef __VIR_RESCTRL_H__</div><div>+# define __VIR_RESCTRL_H__</div><div>+</div><div>+# include "virutil.h"</div><div>+# include "virbitmap.h"</div><div>+</div><div>+#define RESCTRL_DIR "/sys/fs/resctrl"</div><div>+#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"</div><div>+#define SYSFS_SYSTEM_PATH "/sys/devices/system"</div><div>+</div><div>+#define MAX_CPU_SOCKET_NUM 8</div><div>+#define MAX_CBM_BIT_LEN 32</div><div>+#define MAX_SCHEMATA_LEN 1024</div><div>+#define MAX_FILE_LEN ( 10 * 1024 * 1024)</div></div></blockquote><div><br></div><div>Doesn't seem like any of this needs to be in the header file</div></div></div></span></div></blockquote><div>okay, will move to c file. </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><div><div>+</div><div>+enum {</div><div>+ RDT_RESOURCE_L3,</div><div>+ RDT_RESOURCE_L3DATA,</div><div>+ RDT_RESOURCE_L3CODE,</div><div>+ RDT_RESOURCE_L2,</div><div>+ /* Must be the last */</div><div>+ RDT_NUM_RESOURCES,</div></div></blockquote><div><br></div><div>Use a VIR_ prefix on these constants. Also the libvirt naming</div><div>convention is to use RDT_RESOURCE_LAST as the last element.</div><div><br></div></div></div></span></div></blockquote><div>okay </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+};</div><div>+</div><div>+</div><div>+typedef struct _virResCacheBank virResCacheBank;</div><div>+typedef virResCacheBank *virResCacheBankPtr;</div><div>+struct _virResCacheBank {</div><div>+ unsigned int host_id;</div><div>+ unsigned long long cache_size;</div><div>+ unsigned long long cache_left;</div><div>+ unsigned long long cache_min;</div><div>+ virBitmapPtr cpu_mask;</div><div>+};</div></div></blockquote><div><br></div><blockquote type="cite"><div><div>+typedef struct _virResCtrl virResCtrl;</div><div>+typedef virResCtrl *virResCtrlPtr;</div><div>+struct _virResCtrl {</div><div>+ bool enabled;</div><div>+ const char *name;</div><div>+ int num_closid;</div><div>+ int cbm_len;</div><div>+ int min_cbm_bits;</div><div>+ const char* cache_level;</div><div>+ int num_banks;</div><div>+ virResCacheBankPtr cache_banks;</div><div>+};</div></div></blockquote><div><br></div><div>Can any of these fields change at runtime, or are they all</div><div>immutable once populated.</div></div></div></span></div></blockquote><div><br></div><div>Only cache_banks will be change at runtime, such as cache_left on each socket will be updated if libvirt allocates cache to domains.</div><blockquote type="cite"><div><span><div><div><div> </div><div><br></div><blockquote type="cite"><div><div>+bool virResCtrlAvailable(void);</div><div>+int virResCtrlInit(void);</div><div>+virResCtrlPtr virResCtrlGet(int);</div></div></blockquote><div><br></div><div>API docs for these would be nice, especially describing whether</div><div>virResCtrlPtr returned from this method is immutable or not.</div><div>Since libvirt is multi-threaded this is important to know.</div></div></div></span></div></blockquote><div><br></div><div>Okay, I will fill all API docs for the public APIs </div><blockquote type="cite"><div><span><div><div><div><br></div><div><br></div><div>Regards,</div><div>Daniel</div><div>-- </div><div>|: <a href="http://berrange.com">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/">http://www.flickr.com/photos/dberrange/</a> :|</div><div>|: <a href="http://libvirt.org">http://libvirt.org</a> -o- <a href="http://virt-manager.org">http://virt-manager.org</a> :|</div><div>|: <a href="http://entangle-photo.org">http://entangle-photo.org</a> -o- <a href="http://search.cpan.org/~danberr/">http://search.cpan.org/~danberr/</a> :|</div></div></div></span>
</div></blockquote><div>
<br>
</div>