Re: [libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Martin Kletzander]

On Fri, Mar 31, 2017 at 09:23:39PM +0800, Eli Qiao wrote:



>
> How about for l3:
> 
>


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:



Or do I remember it wrong?

oh yeah, right!

for scope, it’s okay to use 'unified' to instead of ‘both’
for CDP enabled case would it be ?

1)





I like this ^^.


or

2)



or
3)



?

A correction, that would be 

the unit B is kinds of small for l3 cache.



Well, in that case just use unit='KiB' in case it's divisible.


Thx Eli.


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Eli Qiao]

> >  
> > How about for l3:
> >  > 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:
>  
> 
>  
> Or do I remember it wrong?
oh yeah, right!

for scope, it’s okay to use 'unified' to instead of ‘both’
for CDP enabled case would it be ?

1)  
 
 

or  

2)  



or  
3)



?

A correction, that would be 

the unit B is kinds of small for l3 cache.

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

Re: [libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Martin Kletzander]

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:
>
> > 
> 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.




> > 
> > + 
> > + 
> >
>
>
> 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.

> > + 
> 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.

> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> > + 
> >
>
>
>
> 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  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:




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:

 

Or do I remember it wrong?


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Eli Qiao]

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:
> >  
> > > 
> > 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.

  
>  
> > > 
> > > + 
> > > +  > > 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.
>  
> > > + 
> > 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.
>  
> > > +  > > cpus='0-1'/>
> > > + 
> > > + 
> > > +  > > cpus='2-3'/>
> > > + 
> > > + 
> > > +  > > cpus='4-5'/>
> > > + 
> > > + 
> > > +  > > cpus='6-7'/>
> > > + 
> > > + 
> > >  
> >  
> >  
> >  
> > 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.

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.

Any way, no harm.


> > 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:  



> > > 
> > >  
> > > 
> > > diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> > > index ffbe9a783811..dda0757766a8 100644
> > > --- a/tests/vircaps2xmltest.c
> > > +++ b/tests/vircaps2xmltest.c
> > > @@ -58,7 +58,8 @@ test_virCapabilities(const void *opaque)
> > > if (!caps)
> > > goto cleanup;
> > >  
> > > - if (virCapabilitiesInitNUMA(caps) < 0)
> > > + if (virCapabilitiesInitNUMA(caps) < 0 ||
> > > + virCapabilitiesInitCaches(caps) < 0)
> > > goto cleanup;
> > >  
> > > virSysfsSetSystemPath(NULL);
> > > --
> > > 2.12.2
> > >  
> > > --
> > > libvir-list mailing list
> > > libvir-list@redhat.com (mailto:libvir-list@redhat.com)
> > > https://www.redhat.com/mailman/listinfo/libvir-list
> > >  
> >  
>  
>  
>  


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

Re: [libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Martin Kletzander]

On Fri, Mar 31, 2017 at 09:56:32AM +0800, Eli Qiao wrote:





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?



+ 
+ 




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.


+ 

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.


+ 
+ 
+ 
+ 
+ 
+ 
+ 
+ 
+ 
+ 
+ 
+ 





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.


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?


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?





diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
index ffbe9a783811..dda0757766a8 100644
--- a/tests/vircaps2xmltest.c
+++ b/tests/vircaps2xmltest.c
@@ -58,7 +58,8 @@ test_virCapabilities(const void *opaque)
if (!caps)
goto cleanup;

- if (virCapabilitiesInitNUMA(caps) < 0)
+ if (virCapabilitiesInitNUMA(caps) < 0 ||
+ virCapabilitiesInitCaches(caps) < 0)
goto cleanup;

virSysfsSetSystemPath(NULL);
--
2.12.2

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







signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Eli Qiao]

> 
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.
> 
> + 
> + 
>  
>  

eg: if enabled CDP feature on the host, what the type of level=3 cache should 
be like?

> + 
for the bank id, it’s per cache level unique right (data/instruction shares 
same id)?  
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
> + 
>  
>  


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?

but I am not sure if that’s be okay to expose all cache information which we 
can not
do the allocation yet.

How can a user/admin to know from capabilities?
> 
>  
> 
> diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
> index ffbe9a783811..dda0757766a8 100644
> --- a/tests/vircaps2xmltest.c
> +++ b/tests/vircaps2xmltest.c
> @@ -58,7 +58,8 @@ test_virCapabilities(const void *opaque)
> if (!caps)
> goto cleanup;
>  
> - if (virCapabilitiesInitNUMA(caps) < 0)
> + if (virCapabilitiesInitNUMA(caps) < 0 ||
> + virCapabilitiesInitCaches(caps) < 0)
> goto cleanup;
>  
> virSysfsSetSystemPath(NULL);
> --  
> 2.12.2
>  
> --
> libvir-list mailing list
> libvir-list@redhat.com (mailto:libvir-list@redhat.com)
> https://www.redhat.com/mailman/listinfo/libvir-list
>  
>  


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

[libvirt] [PATCH 15/16] tests: Add tests for caches into vircaps2xmltest

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 src/conf/capabilities.c |  2 +-
 tests/vircaps2xmldata/vircaps-x86_64-caches.xml | 15 +++
 tests/vircaps2xmltest.c |  3 ++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index c07c64e7d73c..73431ee14237 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -1558,7 +1558,7 @@ virCapabilitiesInitCaches(virCapsPtr caps)
 virSysfsGetCpuCacheValueBitmap(pos, ent->d_name, 
"shared_cpu_list", >cpus) < 0)
 goto cleanup;

-for (tmp_c = type; tmp_c != '\0'; tmp_c++)
+for (tmp_c = type; *tmp_c != '\0'; tmp_c++)
 *tmp_c = c_tolower(*tmp_c);

 tmp_i = virCacheTypeFromString(type);
diff --git a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml 
b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
index 88f2ec62277e..c3defd686418 100644
--- a/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
+++ b/tests/vircaps2xmldata/vircaps-x86_64-caches.xml
@@ -28,6 +28,21 @@
 
   
 
+
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+  
+
   

 
diff --git a/tests/vircaps2xmltest.c b/tests/vircaps2xmltest.c
index ffbe9a783811..dda0757766a8 100644
--- a/tests/vircaps2xmltest.c
+++ b/tests/vircaps2xmltest.c
@@ -58,7 +58,8 @@ test_virCapabilities(const void *opaque)
 if (!caps)
 goto cleanup;

-if (virCapabilitiesInitNUMA(caps) < 0)
+if (virCapabilitiesInitNUMA(caps) < 0 ||
+virCapabilitiesInitCaches(caps) < 0)
 goto cleanup;

 virSysfsSetSystemPath(NULL);
-- 
2.12.2

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