The internal struct list domainall is a list which are resctral domain
status shared by all VMs, especiall the default domain, each VM should
access it concomitantly. Ues a mutex to control it.

Each bank's cache_left field is also a global shared resource we need
to be care, add a mutex for each bank.

We need also to add lock to access /sys/fs/resctrl, use flock.

Signed-off-by: Eli Qiao <liyong.qiao@intel.com>
---
 src/util/virresctrl.c | 89 ++++++++++++++++++++++++++++++++++++++++++++-------
 src/util/virresctrl.h |  3 ++
 2 files changed, 81 insertions(+), 11 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 17d95a8..8808017 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -24,6 +24,7 @@
 #if defined HAVE_SYS_SYSCALL_H
 # include <sys/syscall.h>
 #endif
+#include <sys/file.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
@@ -68,8 +69,8 @@ do { \
 
 #define VIR_RESCTRL_GET_SCHEMATA(count) ((1 << count) - 1)
 
-#define VIR_RESCTRL_SET_SCHEMATA(p, type, pos, val) \
-    p->schematas[type]->schemata_items[pos] = val
+#define VIR_RESCTRL_LOCK(fd, op) flock(fd, op)
+#define VIR_RESCTRL_UNLOCK(fd) flock(fd, LOCK_UN)
 
 /**
  * a virResSchemata represents a schemata object under a resource control
@@ -114,6 +115,8 @@ typedef virResCtrlDomain *virResCtrlDomainPtr;
 struct _virResCtrlDomain {
     unsigned int num_domains;
     virResDomainPtr domains;
+
+    virMutex lock;
 };
 
 
@@ -166,16 +169,34 @@ static int virResCtrlGetStr(const char *domain_name, const char *item_name, char
 {
     char *path;
     int rc = 0;
+    int readfd;
+    int len;
 
     CONSTRUCT_RESCTRL_PATH(domain_name, item_name);
 
-    if (virFileReadAll(path, MAX_FILE_LEN, ret) < 0) {
-        rc = -1;
+    if (!virFileExists(path))
+        goto cleanup;
+
+    if ((readfd = open(path, O_RDONLY)) < 0)
+        goto cleanup;
+
+    if (VIR_RESCTRL_LOCK(readfd, LOCK_SH) < 0) {
+        virReportSystemError(errno, _("Unable to lock '%s'"), path);
+        goto cleanup;
+    }
+
+    len = virFileReadLimFD(readfd, MAX_FILE_LEN, ret);
+
+    if (len < 0) {
+        virReportSystemError(errno, _("Failed to read file '%s'"), path);
         goto cleanup;
     }
+    rc = 0;
 
  cleanup:
     VIR_FREE(path);
+    VIR_RESCTRL_UNLOCK(readfd);
+    VIR_FORCE_CLOSE(readfd);
     return rc;
 }
 
@@ -616,6 +637,9 @@ virResCtrlRefresh(void)
     unsigned int origin_count = domainall.num_domains;
     virResDomainPtr p, pre, del;
     pre = domainall.domains;
+
+    virMutexLock(&domainall.lock);
+
     p = del = NULL;
     if (pre)
         p = pre->next;
@@ -640,10 +664,11 @@ virResCtrlRefresh(void)
             p = p->next;
         }
         VIR_FREE(tasks);
-
     }
 
-    return virResCtrlRefreshSchemata();
+    virResCtrlRefreshSchemata();
+    virMutexUnlock(&domainall.lock);
+    return 0;
 }
 
 /* Get a domain ptr by domain's name*/
@@ -694,6 +719,11 @@ virResCtrlWrite(const char *name, const char *item, const char *content)
     if ((writefd = open(path, O_WRONLY | O_APPEND, S_IRUSR | S_IWUSR)) < 0)
         goto cleanup;
 
+    if (VIR_RESCTRL_LOCK(writefd, LOCK_EX) < 0) {
+        virReportSystemError(errno, _("Unable to lock '%s'"), path);
+        goto cleanup;
+    }
+
     if (safewrite(writefd, content, strlen(content)) < 0)
         goto cleanup;
 
@@ -701,6 +731,7 @@ virResCtrlWrite(const char *name, const char *item, const char *content)
 
  cleanup:
     VIR_FREE(path);
+    VIR_RESCTRL_UNLOCK(writefd);
     VIR_FORCE_CLOSE(writefd);
     return rc;
 }
@@ -873,10 +904,15 @@ static int
 virResCtrlAppendDomain(virResDomainPtr dom)
 {
     virResDomainPtr p = domainall.domains;
+
+    virMutexLock(&domainall.lock);
+
     while (p->next != NULL) p = p->next;
     p->next = dom;
     dom->pre = p;
     domainall.num_domains += 1;
+
+    virMutexUnlock(&domainall.lock);
     return 0;
 }
 
@@ -899,18 +935,22 @@ virResCtrlCalculateSchemata(int type,
 {
     size_t i;
     int count;
+    int rc = -1;
     virResDomainPtr p;
     unsigned int tmp_schemata;
     unsigned int schemata_sum = 0;
     int pair_type = 0;
 
+    virMutexLock(&resctrlall[type].cache_banks[sid].lock);
+
     if (resctrlall[type].cache_banks[sid].cache_left < size) {
         VIR_ERROR(_("Not enough cache left on bank %u"), hostid);
-        return -1;
+        goto cleanup;
     }
+
     if ((count = size / resctrlall[type].cache_banks[sid].cache_min) <= 0) {
         VIR_ERROR(_("Error cache size %llu"), size);
-        return -1;
+        goto cleanup;
     }
 
     tmp_schemata = VIR_RESCTRL_GET_SCHEMATA(count);
@@ -925,7 +965,7 @@ virResCtrlCalculateSchemata(int type,
     if (type == VIR_RDT_RESOURCE_L3CODE)
         pair_type = VIR_RDT_RESOURCE_L3DATA;
 
-    for (i = 1; i < domainall.num_domains; i ++) {
+    for (i = 1; i < domainall.num_domains; i++) {
         schemata_sum |= p->schematas[type]->schemata_items[sid].schemata;
         if (pair_type > 0)
             schemata_sum |= p->schematas[pair_type]->schemata_items[sid].schemata;
@@ -936,7 +976,16 @@ virResCtrlCalculateSchemata(int type,
 
     while ((tmp_schemata & schemata_sum) != 0)
         tmp_schemata = tmp_schemata >> 1;
-    return tmp_schemata;
+
+    resctrlall[type].cache_banks[sid].cache_left -= size;
+    if (pair_type > 0)
+        resctrlall[pair_type].cache_banks[sid].cache_left = resctrlall[type].cache_banks[sid].cache_left;
+
+    rc = tmp_schemata;
+
+ cleanup:
+    virMutexUnlock(&resctrlall[type].cache_banks[sid].lock);
+    return rc;
 }
 
 int virResCtrlSetCacheBanks(virDomainCachetunePtr cachetune,
@@ -1048,7 +1097,7 @@ int virResCtrlUpdate(void)
 int
 virResCtrlInit(void)
 {
-    size_t i = 0;
+    size_t i, j;
     char *tmp;
     int rc = 0;
 
@@ -1068,6 +1117,24 @@ virResCtrlInit(void)
 
     domainall.domains = virResCtrlGetAllDomains(&(domainall.num_domains));
 
+    for (i = 0; i < VIR_RDT_RESOURCE_LAST; i++) {
+        if (VIR_RESCTRL_ENABLED(i)) {
+            for (j = 0; j < resctrlall[i].num_banks; j++) {
+                if (virMutexInit(&resctrlall[i].cache_banks[j].lock) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                            _("Unable to initialize mutex"));
+                    return -1;
+                }
+            }
+        }
+    }
+
+    if (virMutexInit(&domainall.lock) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                _("Unable to initialize mutex"));
+        return -1;
+    }
+
     if ((rc = virResCtrlRefresh()) < 0)
         VIR_ERROR(_("Failed to refresh resource control"));
     return rc;
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index ee7e115..07b298d 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -27,6 +27,7 @@
 # include "virutil.h"
 # include "virbitmap.h"
 # include "domain_conf.h"
+# include "virthread.h"
 
 #define MAX_CPU_SOCKET_NUM 8
 
@@ -49,6 +50,8 @@ struct _virResCacheBank {
     unsigned long long cache_left;
     unsigned long long cache_min;
     virBitmapPtr cpu_mask;
+
+    virMutex lock;
 };
 
 /**
-- 
1.9.1

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

On Wed, Feb 15, 2017 at 01:41:52PM +0800, Eli Qiao wrote:
> The internal struct list domainall is a list which are resctral domain
> status shared by all VMs, especiall the default domain, each VM should
> access it concomitantly. Ues a mutex to control it.
> 
> Each bank's cache_left field is also a global shared resource we need
> to be care, add a mutex for each bank.
> 
> We need also to add lock to access /sys/fs/resctrl, use flock.
> 
> Signed-off-by: Eli Qiao <liyong.qiao@intel.com>

Hi Eli Qiao,

I don't think this is correct: the lock must be held across entire
operations, for example:

+4) Locking between applications
+
+Certain operations on the resctrl filesystem, composed of
+read / writes to multiple files, must be atomic.
+
+As an example, the allocation of an exclusive reservation
+of L3 cache involves:
+
+        1. read list of cbmmasks for each directory
+        2. find a contiguous set of bits in the global CBM bitmask
+           that is clear in any of the directory cbmmasks
+        3. create a new directory
+        4. set the bits found in step 2 to the new directory "schemata"
+           file

The lock must be held across steps 1 through 4, and not taken
and released in between these steps (which is what this patchset
has now).

Two libvirt examples:

VM INITIALIZATION:

	1) Lock filesystem lock (LOCK_EX).
	2) Scan directories. 
	3) Find free space.
	4) Create new directory.
	5) Write schemata files in new directory.
	6) Write tasks file with vcpus pid.
	7) Write default schemata to exclude space used by new
directory.
	8) Unlock filesystem lock.

GET FREE SPACE API CALL:

	1) Lock filesystem lock (LOCK_SH).
	2) Scan directories. 
	3) Unlock filesystem lock.
	4) return data to libvirt user.

Can you please fix this? 

Other than this, and testing of v6 relative to usage of other apps
(which i plan to do tomorrow), patchset looks good to me.

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