On Tuesday, 7 February 2017 at 6:15 PM, Daniel P. Berrange wrote:
On Tue, Feb 07, 2017 at 02:43:04PM +0800, Eli Qiao wrote:On Tuesday, 7 February 2017 at 12:17 AM, Daniel P. Berrange wrote:On Mon, Feb 06, 2017 at 10:23:39AM +0800, Eli Qiao wrote:virResCtrlSetCacheBanks: Set cache banks of a libvirt domain. It willcreate new resource domain under `/sys/fs/resctrl` and fill theschemata according the cache banks configration.virResCtrlUpdate: Update the schemata after libvirt domain destroy.Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx (mailto:liyong.qiao@xxxxxxxxx)>---src/libvirt_private.syms | 2 +src/util/virresctrl.c | 644 ++++++++++++++++++++++++++++++++++++++++++++++-src/util/virresctrl.h | 47 +++-3 files changed, 691 insertions(+), 2 deletions(-)diff --git a/src/util/virresctrl.h b/src/util/virresctrl.hindex 3cc41da..11f43d8 100644--- a/src/util/virresctrl.h+++ b/src/util/virresctrl.h@@ -26,6 +26,7 @@# include "virutil.h"# include "virbitmap.h"+# include "domain_conf.h"#define RESCTRL_DIR "/sys/fs/resctrl"#define RESCTRL_INFO_DIR "/sys/fs/resctrl/info"@@ -82,9 +83,53 @@ struct _virResCtrl {virResCacheBankPtr cache_banks;};+/**+ * a virResSchemata represents a schemata object under a resource control+ * domain.+ */+typedef struct _virResSchemataItem virResSchemataItem;+typedef virResSchemataItem *virResSchemataItemPtr;+struct _virResSchemataItem {+ unsigned int socket_no;+ unsigned schemata;+};++typedef struct _virResSchemata virResSchemata;+typedef virResSchemata *virResSchemataPtr;+struct _virResSchemata {+ unsigned int n_schemata_items;+ virResSchemataItemPtr schemata_items;+};++/**+ * a virResDomain represents a resource control domain. It's a double linked+ * list.+ */++typedef struct _virResDomain virResDomain;+typedef virResDomain *virResDomainPtr;++struct _virResDomain {+ char* name;+ virResSchemataPtr schematas[RDT_NUM_RESOURCES];+ char* tasks;+ int n_sockets;+ virResDomainPtr pre;+ virResDomainPtr next;+};++/* All resource control domains on this host*/++typedef struct _virResCtrlDomain virResCtrlDomain;+typedef virResCtrlDomain *virResCtrlDomainPtr;+struct _virResCtrlDomain {+ unsigned int num_domains;+ virResDomainPtr domains;+};I don't think any of these need to be in the header file - they'reall private structs used only by the .c file.Yep.The biggest issue I see here is a complete lack of any kind ofmutex locking. Libvirt is multi-threaded, so you must expect tohave virResCtrlSetCacheBanks() and virResCtrlUpdate calledconcurrently and thus use mutexes to ensure safety.okay.With the data structure design of using linked list of virResDomainthough, doing good locking is going to be hard. It'll force you tohave a single global mutex across the whole set of data structureswhich will really harm concurrency for VM startup.Really each virResDomain needs to be completely standalone, withits own dedicate mutex. A global mutex for virResCtrlDomain canbe acquired whle you lookup the virResDomain object to use. Thereafterthe global mutex should be released and only the mutex for the specificdomain used.oh, yes, but lock is really a hard thing, I need to be careful to avoid dead lock.bool virResCtrlAvailable(void);int virResCtrlInit(void);virResCtrlPtr virResCtrlGet(int);-+int virResCtrlSetCacheBanks(virDomainCachetunePtr, unsigned char*, pid_t);+int virResCtrlUpdate(void);This API design doesn't really make locking very easy - since you are notpassing any info into the virResCtrlUpdate() method you've created theneed to do global rescans when updating.yes, what if I use a global mutex variable in virresctrl.c?I'd like to see finer grained locking if possible. We try really hard tomake guest startup be highly parallizeable, so any global locks that willbe required by all VMs starting hurts that.
hi Daniel, thanks for your reply,
hmm.. I know what’s your mean, but just have no idea how to add a finer mutex/locking
when you boot a VM and require cache allocation, we need to get the cache information on host, which is a global value,
every VM should share it and modify it after the allocation, then flush changes to /sys/fs/resctrl. which is to say there should be one operation on cache allocation at one time.
the process may be like:
1) start a VM1
2) grain locking/mutex, get cache left information on host ——— 1) start VM2
3) calculate the schemata of this VM and flush it to /sys/fs/resctrl ——— 2) wait for locking
4) update global cache left information 3) wait for locking
5) release lock/mutex, 4) grain locking/mutex, get cache left information on host
6) VM1 started 5) calculate the schemata of this VM and flush it to /sys/fs/resctrl
6) update ..
7) release locking..
VM2 should wait after VM1 flush schemata to /sys/fs/resctrl, or it will cause racing...
IMHO virResCtrlSetCacheBanks needs to return an object that represents theconfig for that particular VM. This can be passed back in, when the guestis shutdown to release resources once again, avoiding any global scans.Hmm.. what object should virResCtrlSetCacheBanks return? schemata setting?Well it might not need to return an object neccessarily. Perhaps you canjust pass the VM PID into the method when your shutdown instead, and havea hash table keyed off PID to lookup the data structure needed to cleanup.
not only cleanup the struct, but still need to calculate the default schemata of host (release resources to host)
I expect that when the VM reboot, we recalculate from cachetune(in xmlsetting) again, that because we are not sure if the host can offer theVM for enough cache when it restart again.You shouldn't need to care about reboot as a concept in these APIs. Fromthe QEMU driver POV, a cold reboot will just be done as a stop followedby start. So these low level cache APIs just need to cope with start+stop.Regards,Daniel--|: http://libvirt.org -o- http://virt-manager.org :|
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
