|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 5/5] tools, docs: add total/local memory bandwith monitoring
Thanks Ian for your words. It makes things much clear.
On Wed, Jan 28, 2015 at 02:04:59PM +0000, Ian Campbell wrote:
> On Wed, 2015-01-28 at 16:04 +0800, Chao Peng wrote:
> > @@ -1476,6 +1483,8 @@ detach: Detach the platform shared resource
> > monitoring service from a domain.
> > Show monitoring data for a certain domain or all domains. Current supported
> > monitor types are:
> > - "cache-occupancy": showing the L3 cache occupancy.
> > + - "total-mem-bandwidth": showing the total memory bandwidth.
> > + - "local-mem-bandwidth": showing the local memory bandwidth.
>
> Some mention of the units might be useful here (and I suspect elsewhere
> in the interface too).
No problem.
>
> > @@ -156,11 +158,12 @@ int xc_psr_cmt_get_l3_cache_size(xc_interface *xch,
> > uint32_t cpu,
> > }
> >
> > int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
> > - xc_psr_cmt_type type, uint64_t *monitor_data)
> > + xc_psr_cmt_type type, uint64_t *monitor_data,
> > + uint64_t *tsc)
> > {
> > xc_resource_op_t op;
> > - xc_resource_entry_t entries[2];
> > - uint32_t evtid;
> > + xc_resource_entry_t entries[3];
> > + uint32_t evtid, nr_entries;
> > int rc;
> >
> > switch ( type )
> > @@ -168,6 +171,12 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t
> > rmid, uint32_t cpu,
> > case XC_PSR_CMT_L3_OCCUPANCY:
> > evtid = EVTID_L3_OCCUPANCY;
> > break;
> > + case XC_PSR_CMT_TOTAL_MEM_BANDWIDTH:
> > + evtid = EVTID_TOTAL_MEM_BANDWIDTH;
> > + break;
> > + case XC_PSR_CMT_LOCAL_MEM_BANDWIDTH:
> > + evtid = EVTID_LOCAL_MEM_BANDWIDTH;
> > + break;
> > default:
> > return -1;
> > }
> > @@ -182,19 +191,35 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t
> > rmid, uint32_t cpu,
> > entries[1].val = 0;
> > entries[1].rsvd = 0;
> >
> > + if ( type == XC_PSR_CMT_L3_OCCUPANCY )
>
> Please use a switch.
>
> Is there no reason to want the TSC for L3_OCCUPANCY? Perhaps a simpler
> interface would be to always request the TSC if the tsc argument is
> non-null?
Only the value of memory bandwidth but not the cache occupancy is
time-related at present. Agreed with you to change the condition to "tsc
== NULL".
>
> > + nr_entries = 2;
> > + else
> > + {
> > + entries[2].u.cmd = XEN_RESOURCE_OP_MSR_READ;
> > + entries[2].idx = MSR_IA32_TSC;
> > + entries[2].val = 0;
> > + entries[2].rsvd = 0;
> > +
> > + nr_entries = 3;
> > + }
> > +
> > op.cpu = cpu;
> > - op.nr_entries = 2;
> > + op.nr_entries = nr_entries;
> > op.entries = entries;
> >
> > rc = xc_resource_op(xch, 1, &op);
> > if ( rc < 0 )
> > return rc;
> >
> > - if ( op.result !=2 || entries[1].val & IA32_CMT_CTR_ERROR_MASK )
> > + if ( op.result != nr_entries || entries[1].val &
> > IA32_CMT_CTR_ERROR_MASK )
> > return -1;
> >
> > *monitor_data = entries[1].val;
> >
> > + if ( type == XC_PSR_CMT_TOTAL_MEM_BANDWIDTH ||
> > + type == XC_PSR_CMT_LOCAL_MEM_BANDWIDTH )
> > + *tsc = entries[2].val;
>
> Are there going to be many more of these hardcoded array indexes as new
> features are added?
>
> Perhaps it should be
> entries[nr].foo = whatever;
> entries[nr].bar = another;
> nr++
>
> And for the TSC one, tsc_entry = &entries[nr], with tsc_entry being NULL
> otherwise so you can check it.
>
Sure, I will follow this pattern.
>
>
> > +
> > return 0;
> > }
> >
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 596d2a0..347ef52 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -1462,6 +1462,14 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
> > uint32_t domid,
> > uint32_t socketid,
> > uint32_t *l3_cache_occupancy);
> > +int libxl_psr_cmt_get_total_mem_bandwidth(libxl_ctx *ctx,
> > + uint32_t domid,
> > + uint32_t socketid,
> > + uint32_t *bandwidth);
> > +int libxl_psr_cmt_get_local_mem_bandwidth(libxl_ctx *ctx,
> > + uint32_t domid,
> > + uint32_t socketid,
> > + uint32_t *bandwidth);
>
> Needs #define LIBXL_HAVE_FOO (one for both sets of bandwidth should be
> OK).
OK.
>
> What are the units of bandwidth? Is a 32-bit number sufficient to cover
> future improvements to the hardware?
The unit is KB/s. Can change it to 64 bit. Thanks.
> >
> > +static int libxl__psr_cmt_get_mem_bandwidth(libxl__gc *gc,
> > + uint32_t domid,
> > + xc_psr_cmt_type type,
> > + uint32_t socketid,
> > + uint32_t *bandwidth)
> > +{
> > + uint64_t sample1, sample2;
> > + uint64_t tsc1, tsc2;
> > + uint32_t upscaling_factor;
> > + int retry_attempts = 0;
> > + int rc;
> > +
> > + while (1) {
> > + rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type,
> > socketid,
> > + &sample1, &tsc1);
> > + if (rc < 0) {
> > + rc = ERROR_FAIL;
> > + goto out;
> > + }
> > +
> > + usleep(10000);
>
> I'm afraid that sleeping in libxl in this way isn't acceptable,
> especially for such a long duration.
>
> Any function which does this certainly meets the definition of "slow" in
> libxl_internal.h and therefore really ought to be using the ao
> machinery.
>
> If that isn't possible/desirable for some reason then it needs to be
> made very clear to the caller, through at a minimum some very extensive
> comments in the interface header what the constraints and implications
> of using this function are.
>
> Another alternative would be to expose the instantaneous values to the
> libxl user and let it sleep as it wishes and calculate the bandwidth
> itself.
I'd like to adopt this suggestion. Move sleep to xl and return sampling
value here. This is acutally good for future enhancement (As Ian Jackson
said in another reply, It will allow us to record avarage bandwidth over
a longer time once hardware supports that).
>
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 1214d2e..46d160e 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -694,4 +694,6 @@ libxl_event = Struct("event",[
> >
> > libxl_psr_cmt_type = Enumeration("psr_cmt_type", [
> > (1, "CACHE_OCCUPANCY"),
> > + (2, "TOTAL_MEM_BANDWIDTH"),
> > + (3, "LOCAL_MEM_BANDWIDTH"),
>
> I'm really starting to wonder if wedging all of these logically
> unrelated statistics into an umbrella psr interface at the libxl level
> makes sense.
>
> But now I notice that this type isn't even used by libxl, it seems to be
> xl internal. If so then please can you arrange to move it to a new
> xl_types.idl and add these new ones there.
As will explain below: I'm likely to use it for
libxl_psr_cmt_type_supported(libxl_psr_cmt_type type), I'd keep it here.
>
> I think we can get away with deprecating the libxl variant pretty
> aggressively since it's not currently all that useful.
>
> > ])
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 1827c63..6d5c7af 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -7829,6 +7829,16 @@ static void
> > psr_cmt_print_domain_l3_info(libxl_dominfo *dominfo,
>
> I wondered this during the refactoring in an earlier patch, but why is
> "l3 info" a suitable name for this function, seems that only one of the
> three things is related to l3 caches.
>
> Does it really make sense to wedge these things (cache use and
> bandwidth) into a common function?
Generally speaking, the platform shared resource monitoring are designed
to allow monitoring of occupancy/bandwidth from one level of the cache
hierarchy to the next. What we called CACHE_OCCUPANY here is actually
for L3 occupancy. And MEM_BANDWIDTH is actually the bandwidth from L3 to
its next, which is typically the system memory. In that case, it
represents the memory bandwidth.
>
> What are the upcoming future PSR events and how will they fit in? (this
> is less critical at the xl level, but it feeds into my thinking about
> libxl interfaces too)
I can't see any upcoming event in the near future, but in the long
time, we may have L2 occupancy/bandwidth similarly.
>
> > socketid, &data))
> > printf("%13u KB", data);
> > break;
> > + case LIBXL_PSR_CMT_TYPE_TOTAL_MEM_BANDWIDTH:
> > + if (!libxl_psr_cmt_get_total_mem_bandwidth(ctx, dominfo->domid,
> > + socketid, &data))
> > + printf("%11u KB/s", data);
> > + break;
> > + case LIBXL_PSR_CMT_TYPE_LOCAL_MEM_BANDWIDTH:
> > + if (!libxl_psr_cmt_get_local_mem_bandwidth(ctx, dominfo->domid,
> > + socketid, &data))
> > + printf("%11u KB/s", data);
> > + break;
> > default:
> > return;
> > }
> > @@ -7840,7 +7850,7 @@ static void
> > psr_cmt_print_domain_l3_info(libxl_dominfo *dominfo,
> > static int psr_cmt_show_l3_info(libxl_psr_cmt_type type, uint32_t domid)
> > {
> > uint32_t i, socketid, nr_sockets, total_rmid;
> > - uint32_t l3_cache_size;
> > + uint32_t l3_cache_size, l3_event_mask;
> > libxl_physinfo info;
> > int rc, nr_domains;
> >
> > @@ -7849,6 +7859,13 @@ static int psr_cmt_show_l3_info(libxl_psr_cmt_type
> > type, uint32_t domid)
> > return -1;
> > }
> >
> > + rc = libxl_psr_cmt_get_l3_event_mask(ctx, &l3_event_mask);
> > + if (rc < 0 || !(l3_event_mask & (1 << (type - 1)))) {
>
> I'm not mad keen on this. I'd prefer either 3 predicate functions, one
> for each mask type or for a single one which takes a LIBXL_PSR_CMT_TYPE
> option.
In the v2, the following definition is used actually:
int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type)
But I changed it to what it's now, for performance consideration:
Basically because xl may call this function several times if it wants to
display all the available events to user. This will costs several
hypercalls once we are not allowed to cache the result with static in
libxc.
As now the xl code does not really support available events display, so I
feel comfortable to use it again.
>
> Is this concept of the PSR l3 even mask already exposed to users of
> libxl anywhere before this series?
This is the first time we expose it.
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |