Re: [Xen-devel] [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring
On Wed, Mar 04, 2015 at 10:07:56AM +, Ian Campbell wrote:
> On Wed, 2015-03-04 at 09:04 +0800, Chao Peng wrote:
> > On Tue, Mar 03, 2015 at 10:09:39AM +, Ian Campbell wrote:
> > > On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > > > On Mon, Mar 02, 2015 at 01:48:43PM +, Ian Campbell wrote:
> > > > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of
> > > > > > monitoring
> > > > > > are supported: total and local memory bandwidth monitoring. To use
> > > > > > it,
> > > > > > CMT should be enabled in hypervisor.
> > > > > >
> > > > > > Signed-off-by: Chao Peng
> > > > >
> > > > > This looks good. I have one question and one small comment/idea:
> > > > >
> > > > > [...]
> > > > > > diff --git a/tools/libxc/include/xenctrl.h
> > > > > > b/tools/libxc/include/xenctrl.h
> > > > > > index 09d819f..54043ee 100644
> > > > > > --- a/tools/libxc/include/xenctrl.h
> > > > > > +++ b/tools/libxc/include/xenctrl.h
> > > > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch,
> > > > > > uint32_t nr_ops, xc_resource_op_t *ops);
> > > > > > #if defined(__i386__) || defined(__x86_64__)
> > > > > > enum xc_psr_cmt_type {
> > > > > > XC_PSR_CMT_L3_OCCUPANCY,
> > > > > > +XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > > > +XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > > > >
> > > > > Is "bandwidth" still the correct term here (and more importantly in
> > > > > the
> > > > > libxl interface e.g. enum), given that we now do the sampling at the
> > > > > application level and just expose the current count from Xen via
> > > > > libxl?
> > > >
> > > > I feel comfortable either changing it or not. The reason to change it is
> > > > what you said here that we do return the counter value to the caller, so
> > > > a consistent name would be nice. While the reason to keep it is: the
> > > > names are listed as the "monitoring event type" from spec, so the caller
> > > > perhaps knows that the returned data is the sample value from event
> > > > counter register related to that type.
> > > >
> > > > Anyway, if you feel it's better to change, then I will do.
> > >
> > > What names does Intel's documentation use for these registers?
> >
> > The register is IA32_QM_CTR(Monitoring Counter Register), to read the
> > count in this register,
>
> I'm thinking then that COUNT is the right name, .e.g.
> XC_PSR_CMT_TOTAL_MEM_COUNT (and for libxl names too). Does that sound
> alright?
Yes. See my changes sent in another thread.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring
On Wed, 2015-03-04 at 09:04 +0800, Chao Peng wrote:
> On Tue, Mar 03, 2015 at 10:09:39AM +, Ian Campbell wrote:
> > On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > > On Mon, Mar 02, 2015 at 01:48:43PM +, Ian Campbell wrote:
> > > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > > > are supported: total and local memory bandwidth monitoring. To use it,
> > > > > CMT should be enabled in hypervisor.
> > > > >
> > > > > Signed-off-by: Chao Peng
> > > >
> > > > This looks good. I have one question and one small comment/idea:
> > > >
> > > > [...]
> > > > > diff --git a/tools/libxc/include/xenctrl.h
> > > > > b/tools/libxc/include/xenctrl.h
> > > > > index 09d819f..54043ee 100644
> > > > > --- a/tools/libxc/include/xenctrl.h
> > > > > +++ b/tools/libxc/include/xenctrl.h
> > > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t
> > > > > nr_ops, xc_resource_op_t *ops);
> > > > > #if defined(__i386__) || defined(__x86_64__)
> > > > > enum xc_psr_cmt_type {
> > > > > XC_PSR_CMT_L3_OCCUPANCY,
> > > > > +XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > > +XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > > >
> > > > Is "bandwidth" still the correct term here (and more importantly in the
> > > > libxl interface e.g. enum), given that we now do the sampling at the
> > > > application level and just expose the current count from Xen via libxl?
> > >
> > > I feel comfortable either changing it or not. The reason to change it is
> > > what you said here that we do return the counter value to the caller, so
> > > a consistent name would be nice. While the reason to keep it is: the
> > > names are listed as the "monitoring event type" from spec, so the caller
> > > perhaps knows that the returned data is the sample value from event
> > > counter register related to that type.
> > >
> > > Anyway, if you feel it's better to change, then I will do.
> >
> > What names does Intel's documentation use for these registers?
>
> The register is IA32_QM_CTR(Monitoring Counter Register), to read the
> count in this register,
I'm thinking then that COUNT is the right name, .e.g.
XC_PSR_CMT_TOTAL_MEM_COUNT (and for libxl names too). Does that sound
alright?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring
On Tue, Mar 03, 2015 at 10:09:39AM +, Ian Campbell wrote:
> On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> > On Mon, Mar 02, 2015 at 01:48:43PM +, Ian Campbell wrote:
> > > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > > are supported: total and local memory bandwidth monitoring. To use it,
> > > > CMT should be enabled in hypervisor.
> > > >
> > > > Signed-off-by: Chao Peng
> > >
> > > This looks good. I have one question and one small comment/idea:
> > >
> > > [...]
> > > > diff --git a/tools/libxc/include/xenctrl.h
> > > > b/tools/libxc/include/xenctrl.h
> > > > index 09d819f..54043ee 100644
> > > > --- a/tools/libxc/include/xenctrl.h
> > > > +++ b/tools/libxc/include/xenctrl.h
> > > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t
> > > > nr_ops, xc_resource_op_t *ops);
> > > > #if defined(__i386__) || defined(__x86_64__)
> > > > enum xc_psr_cmt_type {
> > > > XC_PSR_CMT_L3_OCCUPANCY,
> > > > +XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > > +XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> > >
> > > Is "bandwidth" still the correct term here (and more importantly in the
> > > libxl interface e.g. enum), given that we now do the sampling at the
> > > application level and just expose the current count from Xen via libxl?
> >
> > I feel comfortable either changing it or not. The reason to change it is
> > what you said here that we do return the counter value to the caller, so
> > a consistent name would be nice. While the reason to keep it is: the
> > names are listed as the "monitoring event type" from spec, so the caller
> > perhaps knows that the returned data is the sample value from event
> > counter register related to that type.
> >
> > Anyway, if you feel it's better to change, then I will do.
>
> What names does Intel's documentation use for these registers?
The register is IA32_QM_CTR(Monitoring Counter Register), to read the
count in this register, IA32_QM_EVTSEL(Monitoring Event Select Register)
must be set first. This enum corresponds to the event type in IA32_QM_EVTSEL.
>
> > > I'm not sure what I better term would be though. "count"?
> >
> > The returned value is actually read from monitor event counter, so
> > "count" you suggested or "sample" both sound OK to me, e.g.:
> > XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
> > XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT
>
> I was meaning XC_PSR_CMT_TOTAL_MEM_COUNT etc. not BANDWIDTH_COUNT (which
> I think doesn't make sense).
OK, I have no problem. Thanks for clarification.
Chao
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring
On Tue, 2015-03-03 at 16:00 +0800, Chao Peng wrote:
> On Mon, Mar 02, 2015 at 01:48:43PM +, Ian Campbell wrote:
> > On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > > are supported: total and local memory bandwidth monitoring. To use it,
> > > CMT should be enabled in hypervisor.
> > >
> > > Signed-off-by: Chao Peng
> >
> > This looks good. I have one question and one small comment/idea:
> >
> > [...]
> > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > > index 09d819f..54043ee 100644
> > > --- a/tools/libxc/include/xenctrl.h
> > > +++ b/tools/libxc/include/xenctrl.h
> > > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t
> > > nr_ops, xc_resource_op_t *ops);
> > > #if defined(__i386__) || defined(__x86_64__)
> > > enum xc_psr_cmt_type {
> > > XC_PSR_CMT_L3_OCCUPANCY,
> > > +XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > > +XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
> >
> > Is "bandwidth" still the correct term here (and more importantly in the
> > libxl interface e.g. enum), given that we now do the sampling at the
> > application level and just expose the current count from Xen via libxl?
>
> I feel comfortable either changing it or not. The reason to change it is
> what you said here that we do return the counter value to the caller, so
> a consistent name would be nice. While the reason to keep it is: the
> names are listed as the "monitoring event type" from spec, so the caller
> perhaps knows that the returned data is the sample value from event
> counter register related to that type.
>
> Anyway, if you feel it's better to change, then I will do.
What names does Intel's documentation use for these registers?
> > I'm not sure what I better term would be though. "count"?
>
> The returned value is actually read from monitor event counter, so
> "count" you suggested or "sample" both sound OK to me, e.g.:
> XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
> XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT
I was meaning XC_PSR_CMT_TOTAL_MEM_COUNT etc. not BANDWIDTH_COUNT (which
I think doesn't make sense).
> > > @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t
> > > rmid, uint32_t cpu,
> > > entries[nr].rsvd = 0;
> > > nr++;
> > >
> > > +if ( tsc != NULL )
> > > +{
> > > +tsc_entry = &entries[nr];
> > > +entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> > > +entries[nr].idx = MSR_IA32_TSC;
> > > +entries[nr].val = 0;
> > > +entries[nr].rsvd = 0;
> > > +nr++;
> > > +}
> >
> > Perhaps consider an assertion here that nr <= the number of elements in
> > the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
> > which switched to using nr++)
>
> Agreed. Given that patch (switched to using nr++) is aleady in, so will
> add it here.
Thanks.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring
On Mon, Mar 02, 2015 at 01:48:43PM +, Ian Campbell wrote:
> On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> > Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> > are supported: total and local memory bandwidth monitoring. To use it,
> > CMT should be enabled in hypervisor.
> >
> > Signed-off-by: Chao Peng
>
> This looks good. I have one question and one small comment/idea:
>
> [...]
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index 09d819f..54043ee 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t
> > nr_ops, xc_resource_op_t *ops);
> > #if defined(__i386__) || defined(__x86_64__)
> > enum xc_psr_cmt_type {
> > XC_PSR_CMT_L3_OCCUPANCY,
> > +XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> > +XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
>
> Is "bandwidth" still the correct term here (and more importantly in the
> libxl interface e.g. enum), given that we now do the sampling at the
> application level and just expose the current count from Xen via libxl?
I feel comfortable either changing it or not. The reason to change it is
what you said here that we do return the counter value to the caller, so
a consistent name would be nice. While the reason to keep it is: the
names are listed as the "monitoring event type" from spec, so the caller
perhaps knows that the returned data is the sample value from event
counter register related to that type.
Anyway, if you feel it's better to change, then I will do.
>
> I'm not sure what I better term would be though. "count"?
The returned value is actually read from monitor event counter, so
"count" you suggested or "sample" both sound OK to me, e.g.:
XC_PSR_CMT_TOTAL_MEM_BANDWIDTH => XC_PSR_CMT_TOTAL_MEM_BANDWIDTH_COUNT
XC_PSR_CMT_LOCAL_MEM_BANDWIDTH => XC_PSR_CMT_LOCAL_MEM_BANDWIDTH_COUNT
>
> > @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t
> > rmid, uint32_t cpu,
> > entries[nr].rsvd = 0;
> > nr++;
> >
> > +if ( tsc != NULL )
> > +{
> > +tsc_entry = &entries[nr];
> > +entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> > +entries[nr].idx = MSR_IA32_TSC;
> > +entries[nr].val = 0;
> > +entries[nr].rsvd = 0;
> > +nr++;
> > +}
>
> Perhaps consider an assertion here that nr <= the number of elements in
> the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
> which switched to using nr++)
Agreed. Given that patch (switched to using nr++) is aleady in, so will
add it here.
Chao
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v10 4/4] tools, docs: add total/local memory bandwith monitoring
On Thu, 2015-02-26 at 16:45 +0800, Chao Peng wrote:
> Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
> are supported: total and local memory bandwidth monitoring. To use it,
> CMT should be enabled in hypervisor.
>
> Signed-off-by: Chao Peng
This looks good. I have one question and one small comment/idea:
[...]
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 09d819f..54043ee 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops,
> xc_resource_op_t *ops);
> #if defined(__i386__) || defined(__x86_64__)
> enum xc_psr_cmt_type {
> XC_PSR_CMT_L3_OCCUPANCY,
> +XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
> +XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
Is "bandwidth" still the correct term here (and more importantly in the
libxl interface e.g. enum), given that we now do the sampling at the
application level and just expose the current count from Xen via libxl?
I'm not sure what I better term would be though. "count"?
> @@ -167,6 +194,16 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t
> rmid, uint32_t cpu,
> entries[nr].rsvd = 0;
> nr++;
>
> +if ( tsc != NULL )
> +{
> +tsc_entry = &entries[nr];
> +entries[nr].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> +entries[nr].idx = MSR_IA32_TSC;
> +entries[nr].val = 0;
> +entries[nr].rsvd = 0;
> +nr++;
> +}
Perhaps consider an assertion here that nr <= the number of elements in
the array (i.e. ARRAY_SIZE(entries))? (Either added here or in the patch
which switched to using nr++)
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
