| Message ID | 1503537289-56036-9-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 24, 2017 at 09:14:42AM +0800, Yi Sun wrote: > This patch creates general interfaces in libxl to support all psr > allocation features. > > Add 'LIBXL_HAVE_PSR_MBA' to indicate interface change. I'm not sure this is enough to cover the changes you are doing here: you are introducing some MBA stuff, plus a kind of generic interface for PSR. I think this should be split into two patches, the first one adding the generic interface, and the second one adding the MBA stuff. > Please note, the functionality cannot work until later patches > are applied. > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> > --- > v2: > - remove '_INFO' in 'libxl_psr_feat_type' and make corresponding > changes in 'libxl_psr_hw_info'. > (suggested by Chao Peng) > --- > tools/libxl/libxl.h | 35 ++++++++++++++++++++++++++++++++++- > tools/libxl/libxl_psr.c | 25 +++++++++++++++++++++++++ > tools/libxl/libxl_types.idl | 22 ++++++++++++++++++++++ > 3 files changed, 81 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 229e289..c1d804c 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -931,6 +931,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src); > #define LIBXL_HAVE_PSR_L2_CAT 1 > > /* > + * LIBXL_HAVE_PSR_MBA > + * > + * If this is defined, the Memory Bandwidth Allocation feature is supported. > + */ > +#define LIBXL_HAVE_PSR_MBA 1 > + > +/* > * LIBXL_HAVE_MCA_CAPS > * > * If this is defined, setting MCA capabilities for HVM domain is supported. > @@ -2219,7 +2226,33 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, libxl_psr_cat_info **info, > int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info, > int *nr); > void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr); > -#endif > + > +#ifdef LIBXL_HAVE_PSR_MBA You don't need this, this is only for consumers of libxl. It is perfectly fine to have the prototypes of the functions, even if consumers don't use them. > +/* > + * Function to set a domain's value. It operates on a single or multiple > + * target(s) defined in 'target_map'. 'target_map' specifies all the sockets > + * to be operated on. > + */ > +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, > + libxl_psr_cbm_type type, libxl_bitmap *target_map, > + uint64_t val); > +/* > + * Function to get a domain's cbm. It operates on a single 'target'. > + * 'target' specifies which socket to be operated on. > + */ > +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, > + libxl_psr_cbm_type type, unsigned int target, > + uint64_t *val); > +/* > + * On success, the function returns an array of elements in 'info', > + * and the length in 'nr'. > + */ > +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info, > + unsigned int *nr, libxl_psr_feat_type type, > + unsigned int lvl); > +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr); > +#endif /* LIBXL_HAVE_PSR_MBA */ > +#endif /* LIBXL_HAVE_PSR_CAT */ Please send a patch to remove the already existing ifdef PSR pollution. > > /* misc */ > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c > index f55ba1e..cf368ba 100644 > --- a/tools/libxl/libxl_psr.c > +++ b/tools/libxl/libxl_psr.c > @@ -425,6 +425,31 @@ void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr) > free(list); > } > > +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, > + libxl_psr_cbm_type type, libxl_bitmap *target_map, > + uint64_t val) > +{ > + return ERROR_FAIL; > +} > + > +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, > + libxl_psr_cbm_type type, unsigned int target, > + uint64_t *val) > +{ > + return ERROR_FAIL; > +} > + > +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info, > + unsigned int *nr, libxl_psr_feat_type type, > + unsigned int lvl) > +{ > + return ERROR_FAIL; > +} > + > +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr) > +{ > +} > + > /* > * Local variables: > * mode: C > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 6e80d36..ab847f8 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [ > (2, "L3_CBM_CODE"), > (3, "L3_CBM_DATA"), > (4, "L2_CBM"), > + (5, "MBA_THRTL"), Is this really a CBM type? Roger.
On 17-08-30 09:42:56, Roger Pau Monn� wrote: > On Thu, Aug 24, 2017 at 09:14:42AM +0800, Yi Sun wrote: > > This patch creates general interfaces in libxl to support all psr > > allocation features. > > > > Add 'LIBXL_HAVE_PSR_MBA' to indicate interface change. > > I'm not sure this is enough to cover the changes you are doing here: > you are introducing some MBA stuff, plus a kind of generic interface > for PSR. > > I think this should be split into two patches, the first one adding > the generic interface, and the second one adding the MBA stuff. > This patch only introduces the generic interfaces without any MBA stuff. The 'LIBXL_HAVE_PSR_MBA' is used to indicate the interfaces change here. Per my understand, we should add a macro to indicate libxl interfaces change, right? > > --- a/tools/libxl/libxl.h > > +++ b/tools/libxl/libxl.h > > @@ -931,6 +931,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src); > > #define LIBXL_HAVE_PSR_L2_CAT 1 > > > > /* > > + * LIBXL_HAVE_PSR_MBA > > + * > > + * If this is defined, the Memory Bandwidth Allocation feature is supported. > > + */ > > +#define LIBXL_HAVE_PSR_MBA 1 > > + > > +/* > > * LIBXL_HAVE_MCA_CAPS > > * > > * If this is defined, setting MCA capabilities for HVM domain is supported. > > @@ -2219,7 +2226,33 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, libxl_psr_cat_info **info, > > int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info, > > int *nr); > > void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr); > > -#endif > > + > > +#ifdef LIBXL_HAVE_PSR_MBA > > You don't need this, this is only for consumers of libxl. It is > perfectly fine to have the prototypes of the functions, even if > consumers don't use them. > Oh, ok. So the interfaces declaration does not need be included by macro. I see some other interfaces are done so. So, I follow the convention. > > +/* > > + * Function to set a domain's value. It operates on a single or multiple > > + * target(s) defined in 'target_map'. 'target_map' specifies all the sockets > > + * to be operated on. > > + */ > > +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, > > + libxl_psr_cbm_type type, libxl_bitmap *target_map, > > + uint64_t val); > > +/* > > + * Function to get a domain's cbm. It operates on a single 'target'. > > + * 'target' specifies which socket to be operated on. > > + */ > > +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, > > + libxl_psr_cbm_type type, unsigned int target, > > + uint64_t *val); > > +/* > > + * On success, the function returns an array of elements in 'info', > > + * and the length in 'nr'. > > + */ > > +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info, > > + unsigned int *nr, libxl_psr_feat_type type, > > + unsigned int lvl); > > +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr); > > +#endif /* LIBXL_HAVE_PSR_MBA */ > > +#endif /* LIBXL_HAVE_PSR_CAT */ > > Please send a patch to remove the already existing ifdef PSR > pollution. > Ok, I will send a patch to remoev this '#ifdef LIBXL_HAVE_PSR_CAT' in this file. [...] > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > index 6e80d36..ab847f8 100644 > > --- a/tools/libxl/libxl_types.idl > > +++ b/tools/libxl/libxl_types.idl > > @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [ > > (2, "L3_CBM_CODE"), > > (3, "L3_CBM_DATA"), > > (4, "L2_CBM"), > > + (5, "MBA_THRTL"), > > Is this really a CBM type? > This is not CBM type. The 'libxl_psr_cbm_type' name is not good enough. But I have to introduce a new generic interface here if we want to make the name be generic. I think it is not so valuable. So, I reuse the 'libxl_psr_cbm_type' to cover MBA. How do you think? > Roger.
On Thu, Aug 31, 2017 at 10:38:46AM +0800, Yi Sun wrote: > On 17-08-30 09:42:56, Roger Pau Monn� wrote: > > On Thu, Aug 24, 2017 at 09:14:42AM +0800, Yi Sun wrote: > > > This patch creates general interfaces in libxl to support all psr > > > allocation features. > > > > > > Add 'LIBXL_HAVE_PSR_MBA' to indicate interface change. > > > > I'm not sure this is enough to cover the changes you are doing here: > > you are introducing some MBA stuff, plus a kind of generic interface > > for PSR. > > > > I think this should be split into two patches, the first one adding > > the generic interface, and the second one adding the MBA stuff. > > > This patch only introduces the generic interfaces without any MBA stuff. > > The 'LIBXL_HAVE_PSR_MBA' is used to indicate the interfaces change here. > Per my understand, we should add a macro to indicate libxl interfaces > change, right? So if there's no MBA specific interfaces introduced, why name the define PSR_MBA? Maybe PSR_GENERIC or whatever you find more suitable. > > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > > index 6e80d36..ab847f8 100644 > > > --- a/tools/libxl/libxl_types.idl > > > +++ b/tools/libxl/libxl_types.idl > > > @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [ > > > (2, "L3_CBM_CODE"), > > > (3, "L3_CBM_DATA"), > > > (4, "L2_CBM"), > > > + (5, "MBA_THRTL"), > > > > Is this really a CBM type? > > > This is not CBM type. The 'libxl_psr_cbm_type' name is not good enough. But I > have to introduce a new generic interface here if we want to make the name be > generic. I think it is not so valuable. So, I reuse the 'libxl_psr_cbm_type' > to cover MBA. How do you think? Maybe you could introduce a new typedef, so that old code call still work, ie: typedef enum libxl_psr_cbm_type libxl_psr_type; (Or whatever name you find suitable). Thanks, Roger.
On 17-08-31 09:37:45, Roger Pau Monn� wrote: > On Thu, Aug 31, 2017 at 10:38:46AM +0800, Yi Sun wrote: > > On 17-08-30 09:42:56, Roger Pau Monn� wrote: > > > On Thu, Aug 24, 2017 at 09:14:42AM +0800, Yi Sun wrote: > > > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > > > index 6e80d36..ab847f8 100644 > > > > --- a/tools/libxl/libxl_types.idl > > > > +++ b/tools/libxl/libxl_types.idl > > > > @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [ > > > > (2, "L3_CBM_CODE"), > > > > (3, "L3_CBM_DATA"), > > > > (4, "L2_CBM"), > > > > + (5, "MBA_THRTL"), > > > > > > Is this really a CBM type? > > > > > This is not CBM type. The 'libxl_psr_cbm_type' name is not good enough. But I > > have to introduce a new generic interface here if we want to make the name be > > generic. I think it is not so valuable. So, I reuse the 'libxl_psr_cbm_type' > > to cover MBA. How do you think? > > Maybe you could introduce a new typedef, so that old code call still > work, ie: > > typedef enum libxl_psr_cbm_type libxl_psr_type; > Is there a way in '.idl' to make such 'typedef'? Thanks! > (Or whatever name you find suitable). > > Thanks, Roger.
On Mon, Sep 04, 2017 at 10:09:48AM +0800, Yi Sun wrote: > On 17-08-31 09:37:45, Roger Pau Monn� wrote: > > On Thu, Aug 31, 2017 at 10:38:46AM +0800, Yi Sun wrote: > > > On 17-08-30 09:42:56, Roger Pau Monn� wrote: > > > > On Thu, Aug 24, 2017 at 09:14:42AM +0800, Yi Sun wrote: > > > > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > > > > index 6e80d36..ab847f8 100644 > > > > > --- a/tools/libxl/libxl_types.idl > > > > > +++ b/tools/libxl/libxl_types.idl > > > > > @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [ > > > > > (2, "L3_CBM_CODE"), > > > > > (3, "L3_CBM_DATA"), > > > > > (4, "L2_CBM"), > > > > > + (5, "MBA_THRTL"), > > > > > > > > Is this really a CBM type? > > > > > > > This is not CBM type. The 'libxl_psr_cbm_type' name is not good enough. But I > > > have to introduce a new generic interface here if we want to make the name be > > > generic. I think it is not so valuable. So, I reuse the 'libxl_psr_cbm_type' > > > to cover MBA. How do you think? > > > > Maybe you could introduce a new typedef, so that old code call still > > work, ie: > > > > typedef enum libxl_psr_cbm_type libxl_psr_type; > > > Is there a way in '.idl' to make such 'typedef'? Thanks! > No (or not yet?). Please do it in libxl.h.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 229e289..c1d804c 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -931,6 +931,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src); #define LIBXL_HAVE_PSR_L2_CAT 1 /* + * LIBXL_HAVE_PSR_MBA + * + * If this is defined, the Memory Bandwidth Allocation feature is supported. + */ +#define LIBXL_HAVE_PSR_MBA 1 + +/* * LIBXL_HAVE_MCA_CAPS * * If this is defined, setting MCA capabilities for HVM domain is supported. @@ -2219,7 +2226,33 @@ int libxl_psr_cat_get_info(libxl_ctx *ctx, libxl_psr_cat_info **info, int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info, int *nr); void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr); -#endif + +#ifdef LIBXL_HAVE_PSR_MBA +/* + * Function to set a domain's value. It operates on a single or multiple + * target(s) defined in 'target_map'. 'target_map' specifies all the sockets + * to be operated on. + */ +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, + libxl_psr_cbm_type type, libxl_bitmap *target_map, + uint64_t val); +/* + * Function to get a domain's cbm. It operates on a single 'target'. + * 'target' specifies which socket to be operated on. + */ +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, + libxl_psr_cbm_type type, unsigned int target, + uint64_t *val); +/* + * On success, the function returns an array of elements in 'info', + * and the length in 'nr'. + */ +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info, + unsigned int *nr, libxl_psr_feat_type type, + unsigned int lvl); +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr); +#endif /* LIBXL_HAVE_PSR_MBA */ +#endif /* LIBXL_HAVE_PSR_CAT */ /* misc */ diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c index f55ba1e..cf368ba 100644 --- a/tools/libxl/libxl_psr.c +++ b/tools/libxl/libxl_psr.c @@ -425,6 +425,31 @@ void libxl_psr_cat_info_list_free(libxl_psr_cat_info *list, int nr) free(list); } +int libxl_psr_set_val(libxl_ctx *ctx, uint32_t domid, + libxl_psr_cbm_type type, libxl_bitmap *target_map, + uint64_t val) +{ + return ERROR_FAIL; +} + +int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid, + libxl_psr_cbm_type type, unsigned int target, + uint64_t *val) +{ + return ERROR_FAIL; +} + +int libxl_psr_get_hw_info(libxl_ctx *ctx, libxl_psr_hw_info **info, + unsigned int *nr, libxl_psr_feat_type type, + unsigned int lvl) +{ + return ERROR_FAIL; +} + +void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr) +{ +} + /* * Local variables: * mode: C diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 6e80d36..ab847f8 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -977,6 +977,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [ (2, "L3_CBM_CODE"), (3, "L3_CBM_DATA"), (4, "L2_CBM"), + (5, "MBA_THRTL"), ]) libxl_psr_cat_info = Struct("psr_cat_info", [ @@ -985,3 +986,24 @@ libxl_psr_cat_info = Struct("psr_cat_info", [ ("cbm_len", uint32), ("cdp_enabled", bool), ]) + +libxl_psr_feat_type = Enumeration("psr_feat_type", [ + (1, "CAT"), + (2, "MBA"), + ]) + +libxl_psr_hw_info = Struct("psr_hw_info", [ + ("id", uint32), + ("u", KeyedUnion(None, libxl_psr_feat_type, "type", + [("cat", Struct(None, [ + ("cos_max", uint32), + ("cbm_len", uint32), + ("cdp_enabled", bool), + ])), + ("mba", Struct(None, [ + ("cos_max", uint32), + ("thrtl_max", uint32), + ("linear", bool), + ])), + ])) + ], dir=DIR_OUT)
This patch creates general interfaces in libxl to support all psr allocation features. Add 'LIBXL_HAVE_PSR_MBA' to indicate interface change. Please note, the functionality cannot work until later patches are applied. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v2: - remove '_INFO' in 'libxl_psr_feat_type' and make corresponding changes in 'libxl_psr_hw_info'. (suggested by Chao Peng) --- tools/libxl/libxl.h | 35 ++++++++++++++++++++++++++++++++++- tools/libxl/libxl_psr.c | 25 +++++++++++++++++++++++++ tools/libxl/libxl_types.idl | 22 ++++++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-)