Re: [resend v2 4/7] Resctrl: Add private interface to set cachebanks

[Eli Qiao <qiaoliyong@xxxxxxxxx>]

-- 

Eli Qiao

Sent with Sparrow

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 will

create new resource domain under `/sys/fs/resctrl` and fill the

schemata according the cache banks configration.

virResCtrlUpdate: Update the schemata after libvirt domain destroy.

Signed-off-by: Eli Qiao <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.h

index 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're

all private structs used only by the .c file.

Yep. 

The biggest issue I see here is a complete lack of any kind of

mutex locking. Libvirt is multi-threaded, so you must expect to

have virResCtrlSetCacheBanks() and virResCtrlUpdate called

concurrently and thus use mutexes to ensure safety.

okay. 

With the data structure design of using linked list of virResDomain

though, doing good locking is going to be hard. It'll force you to

have a single global mutex across the whole set of data structures

which will really harm concurrency for VM startup.

Really each virResDomain needs to be completely standalone, with

its own dedicate mutex. A global mutex for virResCtrlDomain can

be acquired whle you lookup the virResDomain object to use. Thereafter

the global mutex should be released and only the mutex for the specific

domain 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 not

passing any info into the virResCtrlUpdate() method you've created the

need to do global rescans when updating.

yes, what if I use a global mutex variable in virresctrl.c?

IMHO virResCtrlSetCacheBanks needs to return an object that represents the

config for that particular VM. This can be passed back in, when the guest

is shutdown to release resources once again, avoiding any global scans.

Hmm.. what object should virResCtrlSetCacheBanks return? schemata setting?

I expect that when the VM reboot, we recalculate from cachetune(in xml setting) again, that because we are not sure if the host can offer the VM for enough cache when it restart again.

 

Regards,

Daniel

--

|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|

|: http://libvirt.org -o- http://virt-manager.org :|

|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

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