[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 3/4] tools: add tools support for Intel CDP
On Fri, 2015-09-25 at 16:43 +0800, He Chen wrote: > On Thu, Sep 24, 2015 at 12:07:27PM +0100, Ian Campbell wrote: > > On Thu, 2015-09-17 at 17:35 +0800, He Chen wrote: > > > @@ -8410,20 +8415,29 @@ static void > > > psr_cat_print_one_domain_cbm(uint32_t > > > domid, uint32_t socketid) > > > printf("%5d%25s", domid, domain_name); > > > free(domain_name); > > > > > > - if (!libxl_psr_cat_get_cbm(ctx, domid, > > > LIBXL_PSR_CBM_TYPE_L3_CBM, > > > - socketid, &cbm)) > > > - printf("%#16"PRIx64, cbm); > > > - > > > + if (!cdp_enabled) { > > > + if (!libxl_psr_cat_get_cbm(ctx, domid, > > > LIBXL_PSR_CBM_TYPE_L3_CBM, > > > + socketid, &cbm)) > > > + printf("%#16"PRIx64, cbm); > > > + } else { > > > + if (!libxl_psr_cat_get_cbm(ctx, domid, > > > LIBXL_PSR_CBM_TYPE_L3_CODE, > > > + socketid, &cbm)) > > > + printf("%10s%#8"PRIx64, "code:", cbm); > > > + if (!libxl_psr_cat_get_cbm(ctx, domid, > > > LIBXL_PSR_CBM_TYPE_L3_DATA, > > > + socketid, &cbm)) > > > + printf("%10s%#8"PRIx64, "data:", cbm); > > > + } > > > > Does cdp being enabled mean that the original L3_CBM functionality is > > no > > longer available then? > > > > Please could you give an example of the new output format for this > > command > > in the commit message. > > > > For the get side, the answer is Yes. But for the set side, L3_CBM means > that > setting the same code CBM and data CBM when CDP is enabled. > > > > static int psr_cat_show(uint32_t domid) > > > @@ -8489,6 +8503,8 @@ int main_psr_cat_cbm_set(int argc, char **argv) > > > libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM; > > > uint64_t cbm; > > > int ret, opt = 0; > > > + int opt_data = 0; > > > + int opt_code = 0; > > > libxl_bitmap target_map; > > > char *value; > > > libxl_string_list socket_list; > > > > > > [...] > > > > > @@ -8517,8 +8535,19 @@ int main_psr_cat_cbm_set(int argc, char > > > **argv) > > > libxl_string_list_dispose(&socket_list); > > > free(value); > > > break; > > > + case 'd': > > > + type = LIBXL_PSR_CBM_TYPE_L3_DATA; > > > + opt_data = 1; > > > + break; > > > + case 'c': > > > + type = LIBXL_PSR_CBM_TYPE_L3_CODE; > > > + opt_code = 1; > > > + break; > > > } > > > > > > + if (opt_data && opt_code) > > > > Do you not mean !opt_data && !opt_code? > > > > But also, isn't this assignment unnecessary since type is initialised > > to > > the same value when it is declared? > > > > In fact, because of that initialisation, aren't opt_data and opt_code > > unnecessary, since you set type appropriately elsewhere. > > > > Are -d and -c mutually exclusive, or is it expected that both can be > > given? > > > > -d and -c can be both given. > > The initialisation of type is L3_CBM, which corresponds to the situation > that neither -d nor -c is given. > > I add `if (opt_data && opt_code)` to address the situation that -d and -c > are both given. > If user gives both -d and -c options, image that without if() statement, > -d will be overwritten by the latter -c in switch, and type will be > L3_CODE instead of L3_CBM (means set both and what user wants). > > I hope I had made myself clear, or is there something wrong with my > understanding? Quoting the relevant bits of code for clarity: libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM; ... case 'd': type = LIBXL_PSR_CBM_TYPE_L3_DATA; opt_data = 1; break; case 'c': type = LIBXL_PSR_CBM_TYPE_L3_CODE; opt_code = 1; break; } if (opt_data && opt_code) type = LIBXL_PSR_CBM_TYPE_L3_CBM; So the behaviour if -d and -c are given is exactly the same as if neither of them were given, i.e. type = LIBXL_PSR_CBM_TYPE_L3_CBM? Is that correct and intended? If so then I think it would be clearer to only set opt_* during option parsing and then to figure out the correct LIBXL_PSR_CBM_TYPE_* explicitly afterwards, rather than have the code cycle through data->code->cbm. Or just outlaw passing both -d and -c together since it is confusing and equivalent to passing neither anyway. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |