| Message ID | 148977619315.29510.17519562424807312146.stgit@Palanthas.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 17 Mar 2017, Dario Faggioli wrote: > It being very very basic, also means this scheduler does > not need much support at the tools level (for now). > > Basically, just the definition of the symbol of the > scheduler itself and a couple of stubs. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Stefano Stabellini <stefano@aporeto.com> > Cc: Julien Grall <julien.grall@arm.com> > --- > tools/libxl/libxl.h | 6 ++++++ > tools/libxl/libxl_sched.c | 24 ++++++++++++++++++++++++ > tools/libxl/libxl_types.idl | 1 + > 3 files changed, 31 insertions(+) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 4c60e8f..5adac2d 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -210,6 +210,12 @@ > #define LIBXL_HAVE_SCHED_RTDS 1 > > /* > + * LIBXL_HAVE_SCHED_NULL indicates that the 'null' static scheduler > + * is available. > + */ > +#define LIBXL_HAVE_SCHED_NULL 1 > + > +/* > * libxl_domain_build_info has u.hvm.viridian_enable and _disable bitmaps > * of the specified width. > */ > diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c > index 84d3837..d44fbe1 100644 > --- a/tools/libxl/libxl_sched.c > +++ b/tools/libxl/libxl_sched.c > @@ -178,6 +178,20 @@ static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid, > return 0; > } > > +static int sched_null_domain_set(libxl__gc *gc, uint32_t domid, > + const libxl_domain_sched_params *scinfo) > +{ > + /* The null scheduler doesn't take any domain-specific parameters. */ > + return 0; > +} > + > +static int sched_null_domain_get(libxl__gc *gc, uint32_t domid, > + libxl_domain_sched_params *scinfo) > +{ > + /* The null scheduler doesn't have any domain-specific parameters. */ > + return ERROR_INVAL; > +} > + > static int sched_credit_domain_get(libxl__gc *gc, uint32_t domid, > libxl_domain_sched_params *scinfo) > { > @@ -730,6 +744,9 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_RTDS: > ret=sched_rtds_domain_set(gc, domid, scinfo); > break; > + case LIBXL_SCHEDULER_NULL: > + ret=sched_null_domain_set(gc, domid, scinfo); > + break; > default: > LOGD(ERROR, domid, "Unknown scheduler"); > ret=ERROR_INVAL; > @@ -758,6 +775,7 @@ int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_CREDIT: > case LIBXL_SCHEDULER_CREDIT2: > case LIBXL_SCHEDULER_ARINC653: > + case LIBXL_SCHEDULER_NULL: > LOGD(ERROR, domid, "per-VCPU parameter setting not supported for this scheduler"); > rc = ERROR_INVAL; > break; > @@ -792,6 +810,7 @@ int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_CREDIT: > case LIBXL_SCHEDULER_CREDIT2: > case LIBXL_SCHEDULER_ARINC653: > + case LIBXL_SCHEDULER_NULL: > LOGD(ERROR, domid, "per-VCPU parameter setting not supported for this scheduler"); > rc = ERROR_INVAL; > break; > @@ -832,6 +851,9 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_RTDS: > ret=sched_rtds_domain_get(gc, domid, scinfo); > break; > + case LIBXL_SCHEDULER_NULL: > + ret=sched_null_domain_get(gc, domid, scinfo); > + break; > default: > LOGD(ERROR, domid, "Unknown scheduler"); > ret=ERROR_INVAL; > @@ -858,6 +880,7 @@ int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_CREDIT: > case LIBXL_SCHEDULER_CREDIT2: > case LIBXL_SCHEDULER_ARINC653: > + case LIBXL_SCHEDULER_NULL: > LOGD(ERROR, domid, "per-VCPU parameter getting not supported for this scheduler"); > rc = ERROR_INVAL; > break; > @@ -890,6 +913,7 @@ int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid, > case LIBXL_SCHEDULER_CREDIT: > case LIBXL_SCHEDULER_CREDIT2: > case LIBXL_SCHEDULER_ARINC653: > + case LIBXL_SCHEDULER_NULL: > LOGD(ERROR, domid, "per-VCPU parameter getting not supported for this scheduler"); > rc = ERROR_INVAL; > break; > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 6d28dea..ce733c4 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -191,6 +191,7 @@ libxl_scheduler = Enumeration("scheduler", [ > (6, "credit2"), > (7, "arinc653"), > (8, "rtds"), > + (9, "null"), > ]) > > # Consistent with SHUTDOWN_* in sched.h (apart from UNKNOWN) >
On Fri, Mar 17, 2017 at 07:43:13PM +0100, Dario Faggioli wrote: > It being very very basic, also means this scheduler does > not need much support at the tools level (for now). > > Basically, just the definition of the symbol of the > scheduler itself and a couple of stubs. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com>
On 17/03/17 18:43, Dario Faggioli wrote: > It being very very basic, also means this scheduler does > not need much support at the tools level (for now). > > Basically, just the definition of the symbol of the > scheduler itself and a couple of stubs. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Stefano Stabellini <stefano@aporeto.com> > Cc: Julien Grall <julien.grall@arm.com> > --- > tools/libxl/libxl.h | 6 ++++++ > tools/libxl/libxl_sched.c | 24 ++++++++++++++++++++++++ > tools/libxl/libxl_types.idl | 1 + > 3 files changed, 31 insertions(+) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 4c60e8f..5adac2d 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -210,6 +210,12 @@ > #define LIBXL_HAVE_SCHED_RTDS 1 > > /* > + * LIBXL_HAVE_SCHED_NULL indicates that the 'null' static scheduler > + * is available. > + */ > +#define LIBXL_HAVE_SCHED_NULL 1 > + > +/* > * libxl_domain_build_info has u.hvm.viridian_enable and _disable bitmaps > * of the specified width. > */ > diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c > index 84d3837..d44fbe1 100644 > --- a/tools/libxl/libxl_sched.c > +++ b/tools/libxl/libxl_sched.c > @@ -178,6 +178,20 @@ static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid, > return 0; > } > > +static int sched_null_domain_set(libxl__gc *gc, uint32_t domid, > + const libxl_domain_sched_params *scinfo) > +{ > + /* The null scheduler doesn't take any domain-specific parameters. */ > + return 0; > +} > + > +static int sched_null_domain_get(libxl__gc *gc, uint32_t domid, > + libxl_domain_sched_params *scinfo) > +{ > + /* The null scheduler doesn't have any domain-specific parameters. */ > + return ERROR_INVAL; > +} Why the different return value? Why not return either INVAL or SUCCESS for both? -George
On Mon, 2017-03-27 at 11:50 +0100, George Dunlap wrote: > On 17/03/17 18:43, Dario Faggioli wrote: > > --- a/tools/libxl/libxl_sched.c > > +++ b/tools/libxl/libxl_sched.c > > @@ -178,6 +178,20 @@ static int sched_arinc653_domain_set(libxl__gc > > *gc, uint32_t domid, > > return 0; > > } > > > > +static int sched_null_domain_set(libxl__gc *gc, uint32_t domid, > > + const libxl_domain_sched_params > > *scinfo) > > +{ > > + /* The null scheduler doesn't take any domain-specific > > parameters. */ > > + return 0; > > +} > > + > > +static int sched_null_domain_get(libxl__gc *gc, uint32_t domid, > > + libxl_domain_sched_params *scinfo) > > +{ > > + /* The null scheduler doesn't have any domain-specific > > parameters. */ > > + return ERROR_INVAL; > > +} > > Why the different return value? Why not return either INVAL or > SUCCESS > for both? > Because domain_set() is called by libxl_domain_sched_params_set(), which is in turn called unconditionally within libxl__build_post(), with the purpose of setting the scheduling parameters chosen by the user during domain creation. If that fails (I've tried that), domain creation fails too. So either it returns success, or we'd have to modify (at least) liblx__build_post(), teaching it about acceptable failures. OTOH, we indeed could return success for domain_get() too, for the sake of having the two above functions return the same. But I really think that call should fail, as an indication to the callers that they won't get the value of any parameter for this scheduler. The behavior achieved by this patch is the same one already in place for ARINC653, with the difference that sched_arinc653_sched_get() is not implemented, which means it is libxl_domain_sched_params_get() that fails, if LIBXL_SCHED_ARINC653 is used. That's an option too, but then libxl will also print "Unknown scheduler", which is not really correct (not even in the ARINC case, actually!). So, having evaluated all the choices, the asymmetric behavior above seems the best one to me. And I'll add a comment to explain the situation. Thanks and regards, Dario
On 06/04/17 11:49, Dario Faggioli wrote: > On Mon, 2017-03-27 at 11:50 +0100, George Dunlap wrote: >> On 17/03/17 18:43, Dario Faggioli wrote: >>> --- a/tools/libxl/libxl_sched.c >>> +++ b/tools/libxl/libxl_sched.c >>> @@ -178,6 +178,20 @@ static int sched_arinc653_domain_set(libxl__gc >>> *gc, uint32_t domid, >>> return 0; >>> } >>> >>> +static int sched_null_domain_set(libxl__gc *gc, uint32_t domid, >>> + const libxl_domain_sched_params >>> *scinfo) >>> +{ >>> + /* The null scheduler doesn't take any domain-specific >>> parameters. */ >>> + return 0; >>> +} >>> + >>> +static int sched_null_domain_get(libxl__gc *gc, uint32_t domid, >>> + libxl_domain_sched_params *scinfo) >>> +{ >>> + /* The null scheduler doesn't have any domain-specific >>> parameters. */ >>> + return ERROR_INVAL; >>> +} >> >> Why the different return value? Why not return either INVAL or >> SUCCESS >> for both? >> > Because domain_set() is called by libxl_domain_sched_params_set(), > which is in turn called unconditionally within libxl__build_post(), > with the purpose of setting the scheduling parameters chosen by the > user during domain creation. > > If that fails (I've tried that), domain creation fails too. So either > it returns success, or we'd have to modify (at least) > liblx__build_post(), teaching it about acceptable failures. > > OTOH, we indeed could return success for domain_get() too, for the sake > of having the two above functions return the same. But I really think > that call should fail, as an indication to the callers that they won't > get the value of any parameter for this scheduler. I see. So if *our* code doesn't know that there aren't any parameters to set, that's OK; but if *other people's code doesn't know that there aren't any parameters to get, it needs to be changed to know that. Got it. ;-) There is a sort of mathematical logic to the idea that setting a null set of parameters should always succeed; and it's certainly convenient for tools to be able to always just call libxl_domain_sched_params_set() without having to check what scheduler is there. But the same logic I think applies to get(), so I would say to return 0 for both. But Wei and Ian have the final say. -George
On Thu, 2017-04-06 at 14:59 +0100, George Dunlap wrote: > On 06/04/17 11:49, Dario Faggioli wrote: > > > > If that fails (I've tried that), domain creation fails too. So > > either > > it returns success, or we'd have to modify (at least) > > liblx__build_post(), teaching it about acceptable failures. > > > > OTOH, we indeed could return success for domain_get() too, for the > > sake > > of having the two above functions return the same. But I really > > think > > that call should fail, as an indication to the callers that they > > won't > > get the value of any parameter for this scheduler. > > I see. So if *our* code doesn't know that there aren't any > parameters > to set, that's OK; but if *other people's code doesn't know that > there > aren't any parameters to get, it needs to be changed to know > that. Got > it. ;-) > Of course! I mean, there must be some advantages and benefits in being *us*! :-D Actually, jokes apart, this is indeed asymmetric, but looks fair to me. As in, _any_ caller (either xl/libxl or external) will see libxl_domain_sched_params_set() succeeding, as well as _any_ caller (either xl/libxl or external) will see libxl_domain_sched_params_get() failing. :-) > There is a sort of mathematical logic to the idea that setting a null > set of parameters should always succeed; and it's certainly > convenient > for tools to be able to always just call > libxl_domain_sched_params_set() > without having to check what scheduler is there. But the same logic > I > think applies to get(), so I would say to return 0 for both. > I understand your point, and I'm happy to do that. > But Wei and Ian have the final say. > Wei acked this patch in v1 (20170321170902.ndk6h5ylyfkk4coo@citrix.com) but that was before you raised this, so I'm happy to resend with this changed, and doing whatever he prefers with his ack. Wei? Regards, Dario
On Thu, Apr 06, 2017 at 05:18:33PM +0200, Dario Faggioli wrote: > On Thu, 2017-04-06 at 14:59 +0100, George Dunlap wrote: > > On 06/04/17 11:49, Dario Faggioli wrote: > > > > > > If that fails (I've tried that), domain creation fails too. So > > > either > > > it returns success, or we'd have to modify (at least) > > > liblx__build_post(), teaching it about acceptable failures. > > > > > > OTOH, we indeed could return success for domain_get() too, for the > > > sake > > > of having the two above functions return the same. But I really > > > think > > > that call should fail, as an indication to the callers that they > > > won't > > > get the value of any parameter for this scheduler. > > > > I see. So if *our* code doesn't know that there aren't any > > parameters > > to set, that's OK; but if *other people's code doesn't know that > > there > > aren't any parameters to get, it needs to be changed to know > > that. Got > > it. ;-) > > > Of course! I mean, there must be some advantages and benefits in being > *us*! :-D > > Actually, jokes apart, this is indeed asymmetric, but looks fair to me. > As in, _any_ caller (either xl/libxl or external) will see > libxl_domain_sched_params_set() succeeding, as well as _any_ caller > (either xl/libxl or external) will see libxl_domain_sched_params_get() > failing. :-) > > > There is a sort of mathematical logic to the idea that setting a null > > set of parameters should always succeed; and it's certainly > > convenient > > for tools to be able to always just call > > libxl_domain_sched_params_set() > > without having to check what scheduler is there. But the same logic > > I > > think applies to get(), so I would say to return 0 for both. > > > I understand your point, and I'm happy to do that. > > > But Wei and Ian have the final say. > > > Wei acked this patch in v1 (20170321170902.ndk6h5ylyfkk4coo@citrix.com) > but that was before you raised this, so I'm happy to resend with this > changed, and doing whatever he prefers with his ack. > > Wei? I don't feel strongly about this. But if you want to return success in the get function, please make sure you initialise output to a known state, instead of returning random garbage. Wei. > > Regards, > Dario > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 4c60e8f..5adac2d 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -210,6 +210,12 @@ #define LIBXL_HAVE_SCHED_RTDS 1 /* + * LIBXL_HAVE_SCHED_NULL indicates that the 'null' static scheduler + * is available. + */ +#define LIBXL_HAVE_SCHED_NULL 1 + +/* * libxl_domain_build_info has u.hvm.viridian_enable and _disable bitmaps * of the specified width. */ diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c index 84d3837..d44fbe1 100644 --- a/tools/libxl/libxl_sched.c +++ b/tools/libxl/libxl_sched.c @@ -178,6 +178,20 @@ static int sched_arinc653_domain_set(libxl__gc *gc, uint32_t domid, return 0; } +static int sched_null_domain_set(libxl__gc *gc, uint32_t domid, + const libxl_domain_sched_params *scinfo) +{ + /* The null scheduler doesn't take any domain-specific parameters. */ + return 0; +} + +static int sched_null_domain_get(libxl__gc *gc, uint32_t domid, + libxl_domain_sched_params *scinfo) +{ + /* The null scheduler doesn't have any domain-specific parameters. */ + return ERROR_INVAL; +} + static int sched_credit_domain_get(libxl__gc *gc, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -730,6 +744,9 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, case LIBXL_SCHEDULER_RTDS: ret=sched_rtds_domain_set(gc, domid, scinfo); break; + case LIBXL_SCHEDULER_NULL: + ret=sched_null_domain_set(gc, domid, scinfo); + break; default: LOGD(ERROR, domid, "Unknown scheduler"); ret=ERROR_INVAL; @@ -758,6 +775,7 @@ int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, case LIBXL_SCHEDULER_CREDIT: case LIBXL_SCHEDULER_CREDIT2: case LIBXL_SCHEDULER_ARINC653: + case LIBXL_SCHEDULER_NULL: LOGD(ERROR, domid, "per-VCPU parameter setting not supported for this scheduler"); rc = ERROR_INVAL; break; @@ -792,6 +810,7 @@ int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, case LIBXL_SCHEDULER_CREDIT: case LIBXL_SCHEDULER_CREDIT2: case LIBXL_SCHEDULER_ARINC653: + case LIBXL_SCHEDULER_NULL: LOGD(ERROR, domid, "per-VCPU parameter setting not supported for this scheduler"); rc = ERROR_INVAL; break; @@ -832,6 +851,9 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, case LIBXL_SCHEDULER_RTDS: ret=sched_rtds_domain_get(gc, domid, scinfo); break; + case LIBXL_SCHEDULER_NULL: + ret=sched_null_domain_get(gc, domid, scinfo); + break; default: LOGD(ERROR, domid, "Unknown scheduler"); ret=ERROR_INVAL; @@ -858,6 +880,7 @@ int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, case LIBXL_SCHEDULER_CREDIT: case LIBXL_SCHEDULER_CREDIT2: case LIBXL_SCHEDULER_ARINC653: + case LIBXL_SCHEDULER_NULL: LOGD(ERROR, domid, "per-VCPU parameter getting not supported for this scheduler"); rc = ERROR_INVAL; break; @@ -890,6 +913,7 @@ int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid, case LIBXL_SCHEDULER_CREDIT: case LIBXL_SCHEDULER_CREDIT2: case LIBXL_SCHEDULER_ARINC653: + case LIBXL_SCHEDULER_NULL: LOGD(ERROR, domid, "per-VCPU parameter getting not supported for this scheduler"); rc = ERROR_INVAL; break; diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 6d28dea..ce733c4 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -191,6 +191,7 @@ libxl_scheduler = Enumeration("scheduler", [ (6, "credit2"), (7, "arinc653"), (8, "rtds"), + (9, "null"), ]) # Consistent with SHUTDOWN_* in sched.h (apart from UNKNOWN)
It being very very basic, also means this scheduler does not need much support at the tools level (for now). Basically, just the definition of the symbol of the scheduler itself and a couple of stubs. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: Wei Liu <wei.liu2@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: George Dunlap <george.dunlap@citrix.com> Cc: Stefano Stabellini <stefano@aporeto.com> Cc: Julien Grall <julien.grall@arm.com> --- tools/libxl/libxl.h | 6 ++++++ tools/libxl/libxl_sched.c | 24 ++++++++++++++++++++++++ tools/libxl/libxl_types.idl | 1 + 3 files changed, 31 insertions(+)