| Message ID | 20170104095559.3523-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 04, 2017 at 09:55:59AM +0000, Roger Pau Monne wrote: > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reported-by: Fatih Acar <fatih@gandi.net> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > This should be backported to all supported stable branches (4.6, 4.7, 4.8). > --- > tools/libxl/libxl.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 6fd4fe1..7aa6d41 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -6925,6 +6925,16 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, > } > } > > + /* Scheduler params */ > + { Please add the following: libxl_sched_params_dispose(&d_config->b_info.sched_params); libxl_sched_params_init(&d_config->b_info.sched_params); Also I would like to move this block before device handling. > + rc = libxl_domain_sched_params_get(ctx, domid, > + &d_config->b_info.sched_params); > + if (rc) { > + LOGD(ERROR, domid, "Fail to get scheduler parameters"); > + goto out; > + } > + } > + > out: > if (lock) libxl__unlock_domain_userdata(lock); > CTX_UNLOCK; > -- > 2.10.1 (Apple Git-78) >
On Wed, Jan 04, 2017 at 11:08:51AM +0000, Wei Liu wrote: > On Wed, Jan 04, 2017 at 09:55:59AM +0000, Roger Pau Monne wrote: > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reported-by: Fatih Acar <fatih@gandi.net> > > --- > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Wei Liu <wei.liu2@citrix.com> > > --- > > This should be backported to all supported stable branches (4.6, 4.7, 4.8). > > --- > > tools/libxl/libxl.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > index 6fd4fe1..7aa6d41 100644 > > --- a/tools/libxl/libxl.c > > +++ b/tools/libxl/libxl.c > > @@ -6925,6 +6925,16 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, > > } > > } > > > > + /* Scheduler params */ > > + { > > Please add the following: > > libxl_sched_params_dispose(&d_config->b_info.sched_params); > libxl_sched_params_init(&d_config->b_info.sched_params); libxl_domain_sched_params_get already calls libxl_sched_params_init which performs a memset plus sets the default values, so I don't see the need to call _dispose and _init from libxl_retrieve_domain_configuration. > Also I would like to move this block before device handling. I can certainly do that. Roger.
On Wed, Jan 04, 2017 at 11:20:41AM +0000, Roger Pau Monne wrote: > On Wed, Jan 04, 2017 at 11:08:51AM +0000, Wei Liu wrote: > > On Wed, Jan 04, 2017 at 09:55:59AM +0000, Roger Pau Monne wrote: > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > Reported-by: Fatih Acar <fatih@gandi.net> > > > --- > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > --- > > > This should be backported to all supported stable branches (4.6, 4.7, 4.8). > > > --- > > > tools/libxl/libxl.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > > index 6fd4fe1..7aa6d41 100644 > > > --- a/tools/libxl/libxl.c > > > +++ b/tools/libxl/libxl.c > > > @@ -6925,6 +6925,16 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, > > > } > > > } > > > > > > + /* Scheduler params */ > > > + { > > > > Please add the following: > > > > libxl_sched_params_dispose(&d_config->b_info.sched_params); > > libxl_sched_params_init(&d_config->b_info.sched_params); > > libxl_domain_sched_params_get already calls libxl_sched_params_init which > performs a memset plus sets the default values, so I don't see the need to call > _dispose and _init from libxl_retrieve_domain_configuration. > At the very least please call _dispose. The _init call can be dropped. > > Also I would like to move this block before device handling. > > I can certainly do that. > > Roger. >
On Wed, Jan 04, 2017 at 11:35:54AM +0000, Wei Liu wrote: > On Wed, Jan 04, 2017 at 11:20:41AM +0000, Roger Pau Monne wrote: > > On Wed, Jan 04, 2017 at 11:08:51AM +0000, Wei Liu wrote: > > > On Wed, Jan 04, 2017 at 09:55:59AM +0000, Roger Pau Monne wrote: > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > Reported-by: Fatih Acar <fatih@gandi.net> > > > > --- > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > --- > > > > This should be backported to all supported stable branches (4.6, 4.7, 4.8). > > > > --- > > > > tools/libxl/libxl.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > > > index 6fd4fe1..7aa6d41 100644 > > > > --- a/tools/libxl/libxl.c > > > > +++ b/tools/libxl/libxl.c > > > > @@ -6925,6 +6925,16 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, > > > > } > > > > } > > > > > > > > + /* Scheduler params */ > > > > + { > > > > > > Please add the following: > > > > > > libxl_sched_params_dispose(&d_config->b_info.sched_params); > > > libxl_sched_params_init(&d_config->b_info.sched_params); > > > > libxl_domain_sched_params_get already calls libxl_sched_params_init which > > performs a memset plus sets the default values, so I don't see the need to call > > _dispose and _init from libxl_retrieve_domain_configuration. > > > > At the very least please call _dispose. The _init call can be dropped. If the _dispose call is really needed, shouldn't it be added to libxl_domain_sched_params_get? It seems asymmetric IMHO to do the _init call inside of libxl_domain_sched_params_get but not the _dispose one (which I still think it's unneeded). Roger.
On Wed, Jan 04, 2017 at 11:45:15AM +0000, Roger Pau Monne wrote: > On Wed, Jan 04, 2017 at 11:35:54AM +0000, Wei Liu wrote: > > On Wed, Jan 04, 2017 at 11:20:41AM +0000, Roger Pau Monne wrote: > > > On Wed, Jan 04, 2017 at 11:08:51AM +0000, Wei Liu wrote: > > > > On Wed, Jan 04, 2017 at 09:55:59AM +0000, Roger Pau Monne wrote: > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > Reported-by: Fatih Acar <fatih@gandi.net> > > > > > --- > > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > > --- > > > > > This should be backported to all supported stable branches (4.6, 4.7, 4.8). > > > > > --- > > > > > tools/libxl/libxl.c | 10 ++++++++++ > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > > > > index 6fd4fe1..7aa6d41 100644 > > > > > --- a/tools/libxl/libxl.c > > > > > +++ b/tools/libxl/libxl.c > > > > > @@ -6925,6 +6925,16 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, > > > > > } > > > > > } > > > > > > > > > > + /* Scheduler params */ > > > > > + { > > > > > > > > Please add the following: > > > > > > > > libxl_sched_params_dispose(&d_config->b_info.sched_params); > > > > libxl_sched_params_init(&d_config->b_info.sched_params); > > > > > > libxl_domain_sched_params_get already calls libxl_sched_params_init which > > > performs a memset plus sets the default values, so I don't see the need to call > > > _dispose and _init from libxl_retrieve_domain_configuration. > > > > > > > At the very least please call _dispose. The _init call can be dropped. > > If the _dispose call is really needed, shouldn't it be added to > libxl_domain_sched_params_get? It seems asymmetric IMHO to do the _init call > inside of libxl_domain_sched_params_get but not the _dispose one (which I still > think it's unneeded). > Missed this email yesterday, sorry. libxl_domain_sched_params_get was written before we codified how libxl types should be used, so it is not exactly in line with newer code. It is too late to change that now. So yes, _dispose is needed, and it shouldn't be added to _sched_params_get. Wei. > Roger.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 6fd4fe1..7aa6d41 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -6925,6 +6925,16 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, } } + /* Scheduler params */ + { + rc = libxl_domain_sched_params_get(ctx, domid, + &d_config->b_info.sched_params); + if (rc) { + LOGD(ERROR, domid, "Fail to get scheduler parameters"); + goto out; + } + } + out: if (lock) libxl__unlock_domain_userdata(lock); CTX_UNLOCK;
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reported-by: Fatih Acar <fatih@gandi.net> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- This should be backported to all supported stable branches (4.6, 4.7, 4.8). --- tools/libxl/libxl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)