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