On Fri, Mar 31, 2017 at 08:33:12PM +0800, Eli Qiao wrote:
On Friday, 31 March 2017 at 7:19 PM, Martin Kletzander wrote:On Fri, Mar 31, 2017 at 09:56:32AM +0800, Eli Qiao wrote: > > > </cells> > Okay, cool, this comes better than my patches and have some differences. > I am open with this as long as that it can meet cache allocation requires and > everyone will be happy. > > I am ++ for this. > > But I am not sure expose all of cache information in the capabilities XML. ??? Are you not sure we "should expose" all of cache information? Or are you afraid we're not exposing enough information?Well, I was saying not to expose all of the information, but after your reply, it's okay for me.> > </topology> > > + <cache> > > + <bank id='0' level='3' type='unified' size='8192' unit='KiB' cpus='0-7'/> > > > > > eg: if enabled CDP feature on the host, what the type of level=3 cache should be like? This has nothing to do with resctrl yet. I'm just exposing the caches that exist on the host. > > + <bank id='0' level='2' type='unified' size='256' unit='KiB' cpus='0-1'/> > for the bank id, it’s per cache level unique right (data/instruction shares same id)? > It looks like it's per cache level/type unique. But it's precisely just what the kernel exposes to us. I'm not doing anything on top of that. > > + <bank id='0' level='1' type='instruction' size='32' unit='KiB' cpus='0-1'/> > > + <bank id='0' level='1' type='data' size='32' unit='KiB' cpus='0-1'/> > > + <bank id='1' level='2' type='unified' size='256' unit='KiB' cpus='2-3'/> > > + <bank id='1' level='1' type='instruction' size='32' unit='KiB' cpus='2-3'/> > > + <bank id='1' level='1' type='data' size='32' unit='KiB' cpus='2-3'/> > > + <bank id='2' level='2' type='unified' size='256' unit='KiB' cpus='4-5'/> > > + <bank id='2' level='1' type='instruction' size='32' unit='KiB' cpus='4-5'/> > > + <bank id='2' level='1' type='data' size='32' unit='KiB' cpus='4-5'/> > > + <bank id='3' level='2' type='unified' size='256' unit='KiB' cpus='6-7'/> > > + <bank id='3' level='1' type='instruction' size='32' unit='KiB' cpus='6-7'/> > > + <bank id='3' level='1' type='data' size='32' unit='KiB' cpus='6-7'/> > > + </cache> > > > > > > This’s really good that you have work this out by expose all these out to capabilities, > and it will be much easy to let resctrl keep focus on cache allocation. > > So if util/virresctrl.c would like to access some cache abilities, it will first get virCapsPtr.host.caches, > right? > Well yeah, it'll probably extend the CacheBank struct.+1> but I am not sure if that’s be okay to expose all cache information which we can not > do the allocation yet. > What's the harm?think about I have a host has 2 Socket 22 core and 2 thread per core, I will have 88 cpus so the cache bank will be a large list 2 for l3 , 44 for l2, 44 for l1d and 44 for l1c, not all of them are useful to users, and the capabilities XML are boring.
You will have as many <bank/> elements as you have different caches. So probably 1 for l3, 44 for l2 and 44 for each l1 (88). Anyway, you are right, that's still 133 lines of stuff not everyone cares about. And the capability XML is already pretty long.
Even though I am Okay to expose all of them. I remember that Daniel has some comments for this in the RFC to not expose all caches of the host if no cache allocation support yet.
I remember him saying that it's not necessary to expose all of it, but for it to be able to be extendable, and in the future, able to expose more caches. So now we see how the XML looks like, I'll change it in next version to just report l3 in a way that we can switch it later on by changing no more than one line of code.
Any way, no harm.
Yeah, there's no *real* harm, but you have a very good point that on bigger systems the capability XML will be ridiculously gross to read =)
> How can a user/admin to know from capabilities? Easily. The XML you see above just says what cache is on the host. If any of the banks are allocatable, then it will have a sub-element. Is there any problem with that?No problem, +1 to extend a sub-element . Any suggestions for what’s it should be? How about for l3: <control min="2816" avail=“56320” cbm_len=“20” scope=‘both’ reserved=“2816"/>
Well, yes, kind of what you had in your patches. Wasn't it without the 'cbm_len' and 'avail'? The 'cbm_len' is avail/min, so it's redundant and avail is the same as the size of the whole cache, right? Also 'reserved' should not be there as that would have to be refreshed every time the info is gathered and that's not what capabilities are for. Also, if we say 'unified' instead of 'both', it sounds little more consistent. So basically, I'm thinking we were somewhere along the lines of: <control min='2816' unit='B' scope=‘unified’/> Or do I remember it wrong?
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
