<div> <br> </div> <div><div><br></div><div>-- </div><div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-variant-ligatures:normal;background-color:rgb(255,255,255)">Best regards </div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-variant-ligatures:normal;background-color:rgb(255,255,255)">Eli</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-variant-ligatures:normal;background-color:rgb(255,255,255)"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-variant-ligatures:normal;background-color:rgb(255,255,255)">天涯无处不重逢</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:14px;font-variant-ligatures:normal;background-color:rgb(255,255,255)"><span style="color:rgb(0,0,0);font-family:"Microsoft YaHei",Verdana,arial,sans-serif">a leaf duckweed belongs to the sea, where not to meet in life </span></div></div><div>Sent with <a href="http://www.sparrowmailapp.com/?sig" target="_blank">Sparrow</a></div><div><br></div></div> <p style="color:#a0a0a8">On Friday, 17 February 2017 at 1:03 AM, Martin Kletzander wrote:</p><blockquote type="cite"><div> <span><div><div><div>On Thu, Feb 16, 2017 at 05:35:12PM +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></blockquote><div><br></div><div>One tiny nitpick, but it might actually help you as well. You can use</div><div>-v7 parameter to git send-email or git format-patch to automatically add</div><div>'v7' to the subject-prefix keeping the "PATCH" in there. Not only could</div><div>this be easier for you, but people like me, who filter patches and other</div><div>mails on the list to different folders, would have these properly</div><div>categorized.</div><div><br></div><div>Anyway, for the review now.</div></div></div></span></div></blockquote><div><br></div><div>Thx </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><div><div>virResCtrlAvailable: If resctrl interface exist on host</div><div>virResCtrlGet: get specify type resource contral information</div></div></blockquote><div><br></div><div>"get specific resource control information" ???</div></div></div></span></div></blockquote><div><br></div><div>Yep </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><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" target="_blank">liyong.qiao@intel.com</a>></div><div>---</div><div>include/libvirt/virterror.h | 1 +</div><div>po/<a href="http://POTFILES.in" target="_blank">POTFILES.in</a> | 1 +</div><div>src/<a href="http://Makefile.am" target="_blank">Makefile.am</a> | 1 +</div><div>src/libvirt_private.syms | 4 +</div><div>src/util/virerror.c | 1 +</div><div>src/util/virresctrl.c | 343 ++++++++++++++++++++++++++++++<wbr>++++++++++++++</div><div>src/util/virresctrl.h | 80 +++++++++++</div><div>7 files changed, 431 insertions(+)</div><div>create mode 100644 src/util/virresctrl.c</div><div>create mode 100644 src/util/virresctrl.h</div><div><br></div><div>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c</div><div>new file mode 100644</div><div>index 0000000..80481bc</div><div>--- /dev/null</div><div>+++ b/src/util/virresctrl.c</div><div>@@ -0,0 +1,343 @@</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/" target="_blank">http://www.gnu.org/licenses/</a>><wbr>.</div><div>+ *</div><div>+ * Authors:</div><div>+ * Eli Qiao <<a href="mailto:liyong.qiao@intel.com" target="_blank">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></blockquote><div><br></div><div>What do you need syscall.h for?</div></div></div></span></div></blockquote><div>Removed</div><div> </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><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>+#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></blockquote><div><br></div><div>If you need SYSFS_..._PATH for anything, it probably could be split into</div><div>other src/util/ files. Example below.</div><div><br></div><blockquote type="cite"><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>+</div><div>+</div><div>+static unsigned int host_id;</div><div>+</div><div>+static virResCtrl resctrlall[] = {</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 (virAsprintf(&path, "%s/%s/%s", RESCTRL_INFO_DIR, resctrlall[type].name, item) < 0)</div><div>+ return -1;</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'))) *tmp = '\0';</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></blockquote><div><br></div><div>It would be more consistent if you reused parts of virHostCPUGetValue(),</div><div>put them in a function, and use that one in both this one an the</div><div>original one. It chould be also put in the virhostcpu.c since that's</div><div>about the host cpu.</div><div><br></div></div></div></span></div></blockquote><div>Done </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div>+static int virResctrlGetCPUSocketID(const size_t cpu, int *socket_id)</div></blockquote><div><br></div><div>No need for this function, just use virHostCPUParseSocket()</div><div><br></div></div></div></span></div></blockquote><div>virHostCPUParseSocket is a static function, I created a new one. </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div>+static int virResCtrlGetCPUCache(const size_t cpu, int type, int *cache)</div></blockquote><div><br></div><div>So we have some places in the code that get info from sysfs. I</div><div>understand that cache is controlled in the resctrl, but one doesn't have</div><div>to have resctrl to get some cache info, so I would move this function</div><div>into virhostcpu.c and keep here only the stuff strictly related to</div><div>resource control.</div></div></div></span></div></blockquote><div>Done </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><div><div>+{</div><div>+ int ret = -1;</div><div>+ char *cache_dir = NULL;</div><div>+ char *cache_str = NULL;</div><div>+ char *tmp;</div><div>+ int carry = -1;</div><div>+</div><div>+ if (virAsprintf(&cache_dir,</div><div>+ "%s/cpu/cpu%zu/cache/index%d/<wbr>size",</div><div>+ SYSFS_SYSTEM_PATH, cpu, type) < 0)</div><div>+ return -1;</div><div>+</div><div>+ if (virResCtrlGetCPUValue(cache_<wbr>dir, &cache_str) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ tmp = cache_str;</div><div>+</div><div>+ while (*tmp != '\0') tmp++;</div><div>+</div><div>+ if (*(tmp - 1) == 'K') {</div><div>+ *(tmp - 1) = '\0';</div><div>+ carry = 1;</div><div>+ } else if (*(tmp - 1) == 'M') {</div><div>+ *(tmp - 1) = '\0';</div><div>+ carry = 1024;</div><div>+ }</div><div>+</div><div>+ if (virStrToLong_i(cache_str, NULL, 0, cache) < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ *cache = (*cache) * carry;</div><div>+</div><div>+ if (*cache < 0)</div><div>+ goto cleanup;</div><div>+</div><div>+ ret = 0;</div><div>+ cleanup:</div><div>+ VIR_FREE(cache_dir);</div><div>+ VIR_FREE(cache_str);</div><div>+ return ret;</div><div>+}</div><div>+</div></div></blockquote><div><br></div><div>Why all this fuzz? You should instead pass pointer to virStrToLong_i to</div><div>get where the number ends and then use virScaleInteger() to multiply it</div><div>properly.</div><div><br></div></div></div></span></div></blockquote><div>Good to know, thx. done. </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+/* Fill all cache bank informations */</div><div>+static virResCacheBankPtr virResCtrlGetCacheBanks(int type, int *n_sockets)</div><div>+{</div></div></blockquote><div><br></div><div>Still could be in virhostcpu.c</div></div></div></span></div></blockquote><div><br></div><div>okay, will move it to there. </div><blockquote type="cite"><div><span><div><div><div><br></div><blockquote type="cite"><div><div>+ int npresent_cpus;</div><div>+ int idx = -1;</div><div>+ size_t i;</div><div>+ virResCacheBankPtr bank;</div><div>+</div><div>+ *n_sockets = 1;</div><div>+ if ((npresent_cpus = virHostCPUGetCount()) < 0)</div><div>+ return NULL;</div><div>+</div><div>+ if (type == VIR_RDT_RESOURCE_L3</div><div>+ || type == VIR_RDT_RESOURCE_L3DATA</div><div>+ || type == VIR_RDT_RESOURCE_L3CODE)</div><div>+ idx = 3;</div><div>+ else if (type == VIR_RDT_RESOURCE_L2)</div><div>+ idx = 2;</div><div>+</div><div>+ if (idx == -1)</div><div>+ return NULL;</div><div>+</div></div></blockquote><div><br></div><div>Indentation, "||" should be on the previous line but, most importantly,</div><div>why not switch? That'd make sure you won't miss any enum value and if</div><div>someone adds a new one, compilator will see it's missing here.</div><div><br></div></div></div></span></div></blockquote><div>Done </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+ if (VIR_ALLOC_N(bank, *n_sockets) < 0) {</div><div>+ *n_sockets = 0;</div></div></blockquote><div><br></div><div>set this before the first return so that this function guarantees</div><div>n_sockets to be 0 on fail. Moreover, n_sockets is always set to 1</div><div>here. Due to the way the rest of the function is designed, this doesn't</div><div>have to be here at all.</div><div><br></div></div></div></span></div></blockquote><div>Done. </div><blockquote type="cite"><div><span><div><div><blockquote type="cite"><div><div>+ return NULL;</div><div>+ }</div><div>+</div><div>+ for (i = 0; i < npresent_cpus; i ++) {</div><div>+ int s_id;</div><div>+ int cache_size;</div><div>+</div><div>+ if (virResctrlGetCPUSocketID(i, &s_id) < 0)</div><div>+ goto error;</div><div>+</div><div>+ if (s_id > (*n_sockets - 1)) {</div><div>+ size_t cur = *n_sockets;</div><div>+ size_t exp = s_id - (*n_sockets) + 1;</div><div>+ if (VIR_EXPAND_N(bank, cur, exp) < 0)</div><div>+ goto error;</div><div>+ *n_sockets = s_id + 1;</div><div>+ }</div><div>+</div><div>+ if (bank[s_id].cpu_mask == NULL) {</div><div>+ if (!(bank[s_id].cpu_mask = virBitmapNew(npresent_cpus)))</div><div>+ goto error;</div><div>+ }</div><div>+</div><div>+ ignore_value(virBitmapSetBit(<wbr>bank[s_id].cpu_mask, i));</div><div>+</div><div>+ if (bank[s_id].cache_size == 0) {</div><div>+ if (virResCtrlGetCPUCache(i, idx, &cache_size) < 0)</div><div>+ goto error;</div><div>+</div><div>+ bank[s_id].cache_size = cache_size;</div><div>+ bank[s_id].cache_min = cache_size / resctrlall[type].cbm_len;</div><div>+ }</div><div>+ }</div><div>+ return bank;</div><div>+</div></div></blockquote><div><br></div><div>Wouldn't it be easier to have list of virResCacheBankPtr *banks; and</div><div>size_t nbanks; and then just populate each pointer when that socket is</div><div>on the system, so that you that NULL means the socket info was not</div><div>filled yet. Or just a list that isn't sparse (like yours is now). The</div><div>logic here seems hard to read.</div><div><br></div></div></div></span></div></blockquote><div>:( sorry, I don’t get you </div><div> </div><div> Are you saying pre-allocate memory for virResCacheBankPtr? here nbanks are equal to sockets.</div><div><br></div><div>I can not know how many nbanks(sockets) on the host before virResCtrlGetCacheBanks.</div><div><br></div><div>can you show me some code examples?</div><div><br></div><blockquote type="cite"><div><span><div><div><div>I'll continue the review tomorrow.</div><div><br></div><div>Martin</div><div><br></div><blockquote type="cite"><div><div>+ error:</div><div>+ *n_sockets = 0;</div><div>+ VIR_FREE(bank);</div><div>+ return NULL;</div><div>+}</div><div>+</div><div>+static int virResCtrlGetConfig(int type)</div><div>+{</div><div>+ int ret;</div><div>+ size_t i;</div><div>+ char *str;</div><div>+</div><div>+ /* Read min_cbm_bits from resctrl.</div><div>+ eg: /sys/fs/resctrl/info/L3/num_<wbr>closids</div><div>+ */</div><div>+ if ((ret = virResCtrlGetInfoStr(type, "num_closids", &str)) < 0)</div><div>+ return ret;</div><div>+</div><div>+ if (virStrToLong_i(str, NULL, 10, &resctrlall[type].num_closid) < 0)</div><div>+ return -1;</div><div>+</div><div>+ VIR_FREE(str);</div><div>+</div><div>+ /* Read min_cbm_bits from resctrl.</div><div>+ eg: /sys/fs/resctrl/info/L3/cbm_<wbr>mask</div><div>+ */</div><div>+ if ((ret = virResCtrlGetInfoStr(type, "min_cbm_bits", &str)) < 0)</div><div>+ return ret;</div><div>+</div><div>+ if (virStrToLong_i(str, NULL, 10, &resctrlall[type].min_cbm_<wbr>bits) < 0)</div><div>+ return -1;</div><div>+</div><div>+ VIR_FREE(str);</div><div>+</div><div>+ /* Read cbm_mask string from resctrl.</div><div>+ eg: /sys/fs/resctrl/info/L3/cbm_<wbr>mask</div><div>+ */</div><div>+ if ((ret = virResCtrlGetInfoStr(type, "cbm_mask", &str)) < 0)</div><div>+ return ret;</div><div>+</div><div>+ /* Calculate cbm length from the default cbm_mask. */</div><div>+ resctrlall[type].cbm_len = strlen(str) * 4;</div><div>+ VIR_FREE(str);</div><div>+</div><div>+ /* Get all cache bank informations */</div><div>+ resctrlall[type].cache_banks = virResCtrlGetCacheBanks(type,</div><div>+ &(resctrlall[type].num_banks))<wbr>;</div><div>+</div><div>+ if (resctrlall[type].cache_banks == NULL)</div><div>+ return -1;</div><div>+</div><div>+ for (i = 0; i < resctrlall[type].num_banks; i++) {</div><div>+ /*L3CODE and L3DATA shares same L3 resource, so they should</div><div>+ * have same host_id. */</div><div>+ if (type == VIR_RDT_RESOURCE_L3CODE)</div><div>+ resctrlall[type].cache_banks[<wbr>i].host_id = resctrlall[VIR_RDT_RESOURCE_<wbr>L3DATA].cache_banks[i].host_<wbr>id;</div><div>+ else</div><div>+ resctrlall[type].cache_banks[<wbr>i].host_id = host_id++;</div><div>+ }</div><div>+</div><div>+ resctrlall[type].enabled = true;</div><div>+</div><div>+ return ret;</div><div>+}</div><div>+</div><div>+int</div><div>+virResCtrlInit(void)</div><div>+{</div><div>+ size_t i = 0;</div><div>+ char *tmp;</div><div>+ int rc = 0;</div><div>+</div><div>+ for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {</div><div>+ if ((rc = virAsprintf(&tmp, "%s/%s", RESCTRL_INFO_DIR, resctrlall[i].name)) < 0) {</div><div>+ VIR_ERROR(_("Failed to initialize resource control config"));</div><div>+ return -1;</div><div>+ }</div><div>+ if (virFileExists(tmp)) {</div><div>+ if ((rc = virResCtrlGetConfig(i)) < 0)</div><div>+ VIR_ERROR(_("Failed to get resource control config"));</div><div>+ return -1;</div><div>+ }</div><div>+</div><div>+ VIR_FREE(tmp);</div><div>+ }</div><div>+ return rc;</div><div>+}</div><div>+</div><div>+/*</div><div>+ * Test whether the host support resource control</div><div>+ */</div><div>+bool</div><div>+virResCtrlAvailable(void)</div><div>+{</div><div>+ if (!virFileExists(RESCTRL_INFO_<wbr>DIR))</div><div>+ return false;</div><div>+ return true;</div><div>+}</div><div>+</div><div>+/*</div><div>+ * Return an virResCtrlPtr point to virResCtrl object,</div><div>+ * We should not modify it out side of virresctrl.c</div><div>+ */</div><div>+virResCtrlPtr</div><div>+virResCtrlGet(int type)</div><div>+{</div><div>+ return &resctrlall[type];</div><div>+}</div><div>diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h</div><div>new file mode 100644</div><div>index 0000000..aa113f4</div><div>--- /dev/null</div><div>+++ b/src/util/virresctrl.h</div><div>@@ -0,0 +1,80 @@</div><div>+/*</div><div>+ * * virresctrl.h: methods for managing rscctrl</div><div>+ * *</div><div>+ * * Copyright (C) 2016 Intel, Inc.</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/" target="_blank">http://www.gnu.org/licenses/</a>><wbr>.</div><div>+ *</div><div>+ * Authors:</div><div>+ * Eli Qiao <<a href="mailto:liyong.qiao@intel.com" target="_blank">liyong.qiao@intel.com</a>></div><div>+ */</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>+enum {</div><div>+ VIR_RDT_RESOURCE_L3,</div><div>+ VIR_RDT_RESOURCE_L3DATA,</div><div>+ VIR_RDT_RESOURCE_L3CODE,</div><div>+ VIR_RDT_RESOURCE_L2,</div><div>+ /* Must be the last */</div><div>+ VIR_RDT_RESOURCE_LAST,</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>+</div><div>+/**</div><div>+ * struct rdt_resource - attributes of an RDT resource</div><div>+ * @enabled: Is this feature enabled on this machine</div><div>+ * @name: Name to use in "schemata" file</div><div>+ * @num_closid: Number of CLOSIDs available</div><div>+ * @max_cbm: Largest Cache Bit Mask allowed</div><div>+ * @min_cbm_bits: Minimum number of consecutive bits to be set</div><div>+ * in a cache bit mask</div><div>+ * @cache_level: Which cache level defines scope of this domain</div><div>+ * @num_banks: Number of cache bank on this machine.</div><div>+ * @cache_banks: Array of cache bank</div><div>+ */</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>+</div><div>+</div><div>+bool virResCtrlAvailable(void);</div><div>+int virResCtrlInit(void);</div><div>+virResCtrlPtr virResCtrlGet(int);</div><div>+</div><div>+#endif</div><div>--</div><div>1.9.1</div></div></blockquote></div></div></span> </div></blockquote><div> <br> </div>