| Message ID | 1459486786-3085-4-git-send-email-lichong659@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-03-31 at 23:59 -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > > Acked-by: Wei Liu <wei.liu2@citrix.com> > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> > You added a new function, so you probably should have dropped the tags. Anyway, I'll re-issue mine: Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> There are still things I'd like being done better. There's a certain amount of code duplication, and there still are two instances of this pattern (the 'else' is superfluous): > + if (scinfo->num_vcpus > 0) { > + rc = ERROR_INVAL; > + goto out; > + } else { > + num_vcpus = info.max_vcpu_id + 1; > + GCNEW_ARRAY(vcpus, num_vcpus); > + for (i = 0; i < num_vcpus; i++) > + vcpus[i].vcpuid = i; > + } > + But Wei said (a few series versions ago) he is kinda fine with this, and I also don't think the series should be blocked because of it. We can improve and refactor things at a later point (adding this to my TODO list). Thanks and Regards, Dario
On Thu, Mar 31, 2016 at 11:59:45PM -0500, Chong Li wrote: > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU settings. > > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > > Acked-by: Wei Liu <wei.liu2@citrix.com> This should have been dropped. > Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Changes on PATCH v8: > 1) Add libxl_vcpu_sched_params_get_all() and > sched_rtds_vcpu_get_all() to output all vcpus of a domain. > [...] > > +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + GC_INIT(ctx); > + int rc; > + > + scinfo->sched = libxl__domain_scheduler(gc, domid); > + > + switch (scinfo->sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "SEDF scheduler is no longer available"); > + rc = ERROR_FEATURE_REMOVED; > + break; > + case LIBXL_SCHEDULER_CREDIT: > + case LIBXL_SCHEDULER_CREDIT2: > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler"); > + rc = ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_RTDS: > + rc = sched_rtds_vcpu_get(gc, domid, scinfo); > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + rc = ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return rc; > +} > + > +int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid, > + libxl_vcpu_sched_params *scinfo) > +{ > + GC_INIT(ctx); > + int rc; > + > + scinfo->sched = libxl__domain_scheduler(gc, domid); > + > + switch (scinfo->sched) { > + case LIBXL_SCHEDULER_SEDF: > + LOG(ERROR, "SEDF scheduler is no longer available"); > + rc = ERROR_FEATURE_REMOVED; > + break; > + case LIBXL_SCHEDULER_CREDIT: > + case LIBXL_SCHEDULER_CREDIT2: > + case LIBXL_SCHEDULER_ARINC653: > + LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler"); > + rc = ERROR_INVAL; > + break; > + case LIBXL_SCHEDULER_RTDS: > + rc = sched_rtds_vcpu_get_all(gc, domid, scinfo); > + break; > + default: > + LOG(ERROR, "Unknown scheduler"); > + rc = ERROR_INVAL; > + break; > + } > + > + GC_FREE; > + return rc; > +} > + These two functions LGTM, so I shall ack this patch again: Acked-by: Wei Liu <wei.liu2@citrix.com> Wei.
Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> On Thu, Mar 31, 2016 at 11:59:45PM -0500, Chong Li wrote:
> > Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> > functions to support per-VCPU settings.
> >
> > Signed-off-by: Chong Li <chong.li@wustl.edu>
> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> > Signed-off-by: Sisu Xi <xisisu@gmail.com>
> >
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> This should have been dropped.
...
>
> These two functions LGTM, so I shall ack this patch again:
>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
I have queued this patch.
Ian.
Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> > These two functions LGTM, so I shall ack this patch again:
> >
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> I have queued this patch.
It broke the build, see below. Was this patch build-tested ?
I have dropped it.
Ian.
libxl.c: In function 'sched_rtds_vcpu_get':
libxl.c:5980:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_get' [-Werror=implicit-function-declaration]
libxl.c: In function 'sched_rtds_vcpu_set':
libxl.c:6088:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_set' [-Werror=implicit-function-declaration]
On Wed, Apr 06, 2016 at 03:55:25PM +0100, Ian Jackson wrote: > Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"): > > Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"): > > > These two functions LGTM, so I shall ack this patch again: > > > > > > Acked-by: Wei Liu <wei.liu2@citrix.com> > > > > I have queued this patch. > > It broke the build, see below. Was this patch build-tested ? > I have dropped it. > > Ian. > > libxl.c: In function 'sched_rtds_vcpu_get': > libxl.c:5980:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_get' [-Werror=implicit-function-declaration] > libxl.c: In function 'sched_rtds_vcpu_set': > libxl.c:6088:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_set' [-Werror=implicit-function-declaration] Hi Chong, I could have fixed this for you, but I think it would be useful to you if you address this problem. I suggest you: 1. Fold in Reviewed-by and Acked-by tags to your patches. 2. Rebase these three patches on top of staging branch. 3. Use the following rune to build test every single commit and fix the problem if it fails. $ git rebase -i HEAD~3 --exec "make -j4 clean && ./configure && \ make -j4 dist" It will stop rebasing if one commit doesn't build. This is what I normally do when developing. Note that the deadline for applying patches is Friday, so if you can refresh this by tomorrow that would be good. Wei.
Wei Liu writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> On Wed, Apr 06, 2016 at 03:55:25PM +0100, Ian Jackson wrote:
> > libxl.c: In function 'sched_rtds_vcpu_get':
> > libxl.c:5980:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_get' [-Werror=implicit-function-declaration]
> > libxl.c: In function 'sched_rtds_vcpu_set':
> > libxl.c:6088:5: error: implicit declaration of function 'xc_sched_rtds_vcpu_set' [-Werror=implicit-function-declaration]
>
...
> I could have fixed this for you, but I think it would be useful to you
> if you address this problem.
>
> I suggest you:
>
> 1. Fold in Reviewed-by and Acked-by tags to your patches.
> 2. Rebase these three patches on top of staging branch.
> 3. Use the following rune to build test every single commit and fix the
> problem if it fails.
>
> $ git rebase -i HEAD~3 --exec "make -j4 clean && ./configure && \
> make -j4 dist"
>
> It will stop rebasing if one commit doesn't build. This is what I
> normally do when developing.
>
> Note that the deadline for applying patches is Friday, so if you can
> refresh this by tomorrow that would be good.
Dario points out on irc that perhaps the problem is that I didn't
apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a
hypervisor patch (and the HV parts are already in tree).
I will check my view of the xen-devel list.
Ian.
Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable per-VCPU parameter for RTDS"):
> Dario points out on irc that perhaps the problem is that I didn't
> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a
> hypervisor patch (and the HV parts are already in tree).
>
> I will check my view of the xen-devel list.
Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a
copy from the list.
I have pushed all four.
Ian.
On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: > Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable > per-VCPU parameter for RTDS"): > > > > Dario points out on irc that perhaps the problem is that I didn't > > apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a > > hypervisor patch (and the HV parts are already in tree). > > > > I will check my view of the xen-devel list. > Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a > copy from the list. > > I have pushed all four. > Thanks Ian! So, Chong, clearly, the build failure was not your fault (as there is no actual build failure), but please, always double check (even when sending new versions of a series) that the appropriate maintainers are Cc-ed... This would help limiting problems like this one we've seen here. Regards, Dario
On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: >> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable >> per-VCPU parameter for RTDS"): >> > >> > Dario points out on irc that perhaps the problem is that I didn't >> > apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a >> > hypervisor patch (and the HV parts are already in tree). >> > >> > I will check my view of the xen-devel list. >> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a >> copy from the list. >> >> I have pushed all four. >> > Thanks Ian! > > So, Chong, clearly, the build failure was not your fault (as there is > no actual build failure), but please, always double check (even when > sending new versions of a series) that the appropriate maintainers are > Cc-ed... This would help limiting problems like this one we've seen > here. > Yes, I'll. Thanks for your help on this. Chong
On 06/04/16 17:41, Chong Li wrote: > On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: >> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: >>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable >>> per-VCPU parameter for RTDS"): >>>> Dario points out on irc that perhaps the problem is that I didn't >>>> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a >>>> hypervisor patch (and the HV parts are already in tree). >>>> >>>> I will check my view of the xen-devel list. >>> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a >>> copy from the list. >>> >>> I have pushed all four. >>> >> Thanks Ian! >> >> So, Chong, clearly, the build failure was not your fault (as there is >> no actual build failure), but please, always double check (even when >> sending new versions of a series) that the appropriate maintainers are >> Cc-ed... This would help limiting problems like this one we've seen >> here. >> > Yes, I'll. > > Thanks for your help on this. > Chong Yet another build failure on CentOS. xc_rt.c: In function 'xc_sched_rtds_vcpu_set': xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized] int rc; ^ xc_rt.c: In function 'xc_sched_rtds_vcpu_get': xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized] int rc; ^ cc1: all warnings being treated as errors In both cases, if your while loop doesn't execute (i.e. the user passes num_vcpus = 0), rc is genuinely uninialised when used at the end of the function. ~Andrew
On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 06/04/16 17:41, Chong Li wrote: >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli >> <dario.faggioli@citrix.com> wrote: >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable >>>> per-VCPU parameter for RTDS"): >>>>> Dario points out on irc that perhaps the problem is that I didn't >>>>> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a >>>>> hypervisor patch (and the HV parts are already in tree). >>>>> >>>>> I will check my view of the xen-devel list. >>>> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a >>>> copy from the list. >>>> >>>> I have pushed all four. >>>> >>> Thanks Ian! >>> >>> So, Chong, clearly, the build failure was not your fault (as there is >>> no actual build failure), but please, always double check (even when >>> sending new versions of a series) that the appropriate maintainers are >>> Cc-ed... This would help limiting problems like this one we've seen >>> here. >>> >> Yes, I'll. >> >> Thanks for your help on this. >> Chong > > Yet another build failure on CentOS. > > xc_rt.c: In function 'xc_sched_rtds_vcpu_set': > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function > [-Werror=maybe-uninitialized] > int rc; > ^ > xc_rt.c: In function 'xc_sched_rtds_vcpu_get': > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function > [-Werror=maybe-uninitialized] > int rc; > ^ > cc1: all warnings being treated as errors > > In both cases, if your while loop doesn't execute (i.e. the user passes > num_vcpus = 0), rc is genuinely uninialised when used at the end of the > function. > > ~Andrew I see. I can do a sanity check on num_vcpus before the while loop. Do I have to re-send the whole patch series? Or maybe just something like a bug fix patch? Chong
On 06/04/16 20:20, Chong Li wrote: > On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 06/04/16 17:41, Chong Li wrote: >>> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli >>> <dario.faggioli@citrix.com> wrote: >>>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: >>>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable >>>>> per-VCPU parameter for RTDS"): >>>>>> Dario points out on irc that perhaps the problem is that I didn't >>>>>> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a >>>>>> hypervisor patch (and the HV parts are already in tree). >>>>>> >>>>>> I will check my view of the xen-devel list. >>>>> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a >>>>> copy from the list. >>>>> >>>>> I have pushed all four. >>>>> >>>> Thanks Ian! >>>> >>>> So, Chong, clearly, the build failure was not your fault (as there is >>>> no actual build failure), but please, always double check (even when >>>> sending new versions of a series) that the appropriate maintainers are >>>> Cc-ed... This would help limiting problems like this one we've seen >>>> here. >>>> >>> Yes, I'll. >>> >>> Thanks for your help on this. >>> Chong >> Yet another build failure on CentOS. >> >> xc_rt.c: In function 'xc_sched_rtds_vcpu_set': >> xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function >> [-Werror=maybe-uninitialized] >> int rc; >> ^ >> xc_rt.c: In function 'xc_sched_rtds_vcpu_get': >> xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function >> [-Werror=maybe-uninitialized] >> int rc; >> ^ >> cc1: all warnings being treated as errors >> >> In both cases, if your while loop doesn't execute (i.e. the user passes >> num_vcpus = 0), rc is genuinely uninialised when used at the end of the >> function. >> >> ~Andrew > I see. I can do a sanity check on num_vcpus before the while loop. > > Do I have to re-send the whole patch series? Or maybe just something > like a bug fix patch? The series has been committed. Please send a new patch based on top of the existing `staging` branch. ~Andrew
On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote: > On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 06/04/16 17:41, Chong Li wrote: > >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli > >> <dario.faggioli@citrix.com> wrote: > >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: > >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable > >>>> per-VCPU parameter for RTDS"): > >>>>> Dario points out on irc that perhaps the problem is that I didn't > >>>>> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a > >>>>> hypervisor patch (and the HV parts are already in tree). > >>>>> > >>>>> I will check my view of the xen-devel list. > >>>> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a > >>>> copy from the list. > >>>> > >>>> I have pushed all four. > >>>> > >>> Thanks Ian! > >>> > >>> So, Chong, clearly, the build failure was not your fault (as there is > >>> no actual build failure), but please, always double check (even when > >>> sending new versions of a series) that the appropriate maintainers are > >>> Cc-ed... This would help limiting problems like this one we've seen > >>> here. > >>> > >> Yes, I'll. > >> > >> Thanks for your help on this. > >> Chong > > > > Yet another build failure on CentOS. > > > > xc_rt.c: In function 'xc_sched_rtds_vcpu_set': > > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > int rc; > > ^ > > xc_rt.c: In function 'xc_sched_rtds_vcpu_get': > > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > int rc; > > ^ > > cc1: all warnings being treated as errors > > > > In both cases, if your while loop doesn't execute (i.e. the user passes > > num_vcpus = 0), rc is genuinely uninialised when used at the end of the > > function. > > > > ~Andrew > > I see. I can do a sanity check on num_vcpus before the while loop. > Not sure what kind of sanity check you were thinking about. But you can just set rc = 0 at the beginning of each function. That semantics should be sensible enough. What do you think? > Do I have to re-send the whole patch series? Or maybe just something > like a bug fix patch? > Please send a patch on top of staging branch. This series has already been committed. Wei. > Chong > > -- > Chong Li > Department of Computer Science and Engineering > Washington University in St.louis
On Wed, Apr 6, 2016 at 2:30 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote: >> On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> > On 06/04/16 17:41, Chong Li wrote: >> >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli >> >> <dario.faggioli@citrix.com> wrote: >> >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: >> >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable >> >>>> per-VCPU parameter for RTDS"): >> >>>>> Dario points out on irc that perhaps the problem is that I didn't >> >>>>> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a >> >>>>> hypervisor patch (and the HV parts are already in tree). >> >>>>> >> >>>>> I will check my view of the xen-devel list. >> >>>> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a >> >>>> copy from the list. >> >>>> >> >>>> I have pushed all four. >> >>>> >> >>> Thanks Ian! >> >>> >> >>> So, Chong, clearly, the build failure was not your fault (as there is >> >>> no actual build failure), but please, always double check (even when >> >>> sending new versions of a series) that the appropriate maintainers are >> >>> Cc-ed... This would help limiting problems like this one we've seen >> >>> here. >> >>> >> >> Yes, I'll. >> >> >> >> Thanks for your help on this. >> >> Chong >> > >> > Yet another build failure on CentOS. >> > >> > xc_rt.c: In function 'xc_sched_rtds_vcpu_set': >> > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function >> > [-Werror=maybe-uninitialized] >> > int rc; >> > ^ >> > xc_rt.c: In function 'xc_sched_rtds_vcpu_get': >> > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function >> > [-Werror=maybe-uninitialized] >> > int rc; >> > ^ >> > cc1: all warnings being treated as errors >> > >> > In both cases, if your while loop doesn't execute (i.e. the user passes >> > num_vcpus = 0), rc is genuinely uninialised when used at the end of the >> > function. >> > >> > ~Andrew >> >> I see. I can do a sanity check on num_vcpus before the while loop. >> > > Not sure what kind of sanity check you were thinking about. But you can > just set rc = 0 at the beginning of each function. That semantics should > be sensible enough. What do you think? Yes, I can do this change. But didn't you or Dario say that rc should not be initialized at the beginning of a function? > >> Do I have to re-send the whole patch series? Or maybe just something >> like a bug fix patch? >> > > Please send a patch on top of staging branch. This series has already > been committed. > Sure.
On Wed, Apr 06, 2016 at 02:41:31PM -0500, Chong Li wrote: > On Wed, Apr 6, 2016 at 2:30 PM, Wei Liu <wei.liu2@citrix.com> wrote: > > On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote: > >> On Wed, Apr 6, 2016 at 1:54 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> > On 06/04/16 17:41, Chong Li wrote: > >> >> On Wed, Apr 6, 2016 at 11:36 AM, Dario Faggioli > >> >> <dario.faggioli@citrix.com> wrote: > >> >>> On Wed, 2016-04-06 at 16:38 +0100, Ian Jackson wrote: > >> >>>> Ian Jackson writes ("Re: [PATCH v9 for Xen 4.7 3/4] libxl: enable > >> >>>> per-VCPU parameter for RTDS"): > >> >>>>> Dario points out on irc that perhaps the problem is that I didn't > >> >>>>> apply 2/4. I wasn't CC'd on 2/4, so I foolishly assumed it was a > >> >>>>> hypervisor patch (and the HV parts are already in tree). > >> >>>>> > >> >>>>> I will check my view of the xen-devel list. > >> >>>> Indeed. With 2/4 it builds. 4/4 was also not CC'd to me. I used a > >> >>>> copy from the list. > >> >>>> > >> >>>> I have pushed all four. > >> >>>> > >> >>> Thanks Ian! > >> >>> > >> >>> So, Chong, clearly, the build failure was not your fault (as there is > >> >>> no actual build failure), but please, always double check (even when > >> >>> sending new versions of a series) that the appropriate maintainers are > >> >>> Cc-ed... This would help limiting problems like this one we've seen > >> >>> here. > >> >>> > >> >> Yes, I'll. > >> >> > >> >> Thanks for your help on this. > >> >> Chong > >> > > >> > Yet another build failure on CentOS. > >> > > >> > xc_rt.c: In function 'xc_sched_rtds_vcpu_set': > >> > xc_rt.c:71:9: error: 'rc' may be used uninitialized in this function > >> > [-Werror=maybe-uninitialized] > >> > int rc; > >> > ^ > >> > xc_rt.c: In function 'xc_sched_rtds_vcpu_get': > >> > xc_rt.c:105:9: error: 'rc' may be used uninitialized in this function > >> > [-Werror=maybe-uninitialized] > >> > int rc; > >> > ^ > >> > cc1: all warnings being treated as errors > >> > > >> > In both cases, if your while loop doesn't execute (i.e. the user passes > >> > num_vcpus = 0), rc is genuinely uninialised when used at the end of the > >> > function. > >> > > >> > ~Andrew > >> > >> I see. I can do a sanity check on num_vcpus before the while loop. > >> > > > > Not sure what kind of sanity check you were thinking about. But you can > > just set rc = 0 at the beginning of each function. That semantics should > > be sensible enough. What do you think? > Yes, I can do this change. But didn't you or Dario say that rc should not be > initialized at the beginning of a function? > I'm fine with bending the rules a bit to make life easier. For a simple function like this I won't argue one way or another. Besides there isn't really a CODING_STYLE file in libxc. But really what I care about is to not return an uninitialised rc. If you don't want to set rc to 0 at the beginning of the function, you can do it before the loop -- that should both fix the error and make Dario happy. :-) Wei. > > > >> Do I have to re-send the whole patch series? Or maybe just something > >> like a bug fix patch? > >> > > > > Please send a patch on top of staging branch. This series has already > > been committed. > > > Sure. > > > > -- > Chong Li > Department of Computer Science and Engineering > Washington University in St.louis
On Wed, 2016-04-06 at 20:49 +0100, Wei Liu wrote: > On Wed, Apr 06, 2016 at 02:41:31PM -0500, Chong Li wrote: > > On Wed, Apr 6, 2016 at 2:30 PM, Wei Liu <wei.liu2@citrix.com> > > wrote: > > > On Wed, Apr 06, 2016 at 02:20:55PM -0500, Chong Li wrote: > > > Not sure what kind of sanity check you were thinking about. But > > > you can > > > just set rc = 0 at the beginning of each function. That semantics > > > should > > > be sensible enough. What do you think? > > Yes, I can do this change. But didn't you or Dario say that rc > > should not be > > initialized at the beginning of a function? > > > I'm fine with bending the rules a bit to make life easier. For a > simple > function like this I won't argue one way or another. Besides there > isn't > really a CODING_STYLE file in libxc. > I did say it, but it wasn't because of coding style, especially considering, as Wei says, that we're in libxc. I did because it looked a pointless (and hence avoidable) initialization. Of course, I was wrong, as I did not see the issue in case we get passed 0. Also, weirdly enough, my compiler did not spot this (and I build on Debian unstable, so with a fairly recent gcc). In any case, I don't think it make much sense to deal with num_vcpus=0, and in fact, in libxl, we reject it. But as a matter of fact, the easiest and cleanest way of "not dealing" with it, here in libxc, is really to just initialize rc to zero, let the loop be skipped and carry on. More advanced sanity checking is better done at both higher and lower levels, and both are happening already, so we're fine. > But really what I care about is to not return an uninitialised rc. If > you don't want to set rc to 0 at the beginning of the function, you > can > do it before the loop -- that should both fix the error and make > Dario > happy. :-) > Thanks for this effort of making me happy. :-) Jokes apart, just initialize it and get done with this. Again, it's the cleanest and simplest way, and I _am_ happy with it. Regards, Dario
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index ac4d1f6..7d6ea91 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5779,6 +5779,236 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, return 0; } +static int sched_rtds_validate_params(libxl__gc *gc, int period, int budget) +{ + int rc; + + if (period < 1) { + LOG(ERROR, "Invalid VCPU period of %d (it should be >= 1)", period); + rc = ERROR_INVAL; + goto out; + } + + if (budget < 1) { + LOG(ERROR, "Invalid VCPU budget of %d (it should be >= 1)", budget); + rc = ERROR_INVAL; + goto out; + } + + if (budget > period) { + LOG(ERROR, "VCPU budget must be smaller than or equal to period, " + "but %d > %d", budget, period); + rc = ERROR_INVAL; + goto out; + } + rc = 0; +out: + return rc; +} + +/* Get the RTDS scheduling parameters of vcpu(s) */ +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + uint32_t num_vcpus; + int i, r, rc; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + + if (scinfo->num_vcpus <= 0) { + rc = ERROR_INVAL; + goto out; + } else { + num_vcpus = scinfo->num_vcpus; + GCNEW_ARRAY(vcpus, num_vcpus); + for (i = 0; i < num_vcpus; i++) { + if (scinfo->vcpus[i].vcpuid < 0 || + scinfo->vcpus[i].vcpuid > info.max_vcpu_id) { + LOG(ERROR, "VCPU index is out of range, " + "valid values are within range from 0 to %d", + info.max_vcpu_id); + rc = ERROR_INVAL; + goto out; + } + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + } + } + + r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "getting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + scinfo->sched = LIBXL_SCHEDULER_RTDS; + for (i = 0; i < num_vcpus; i++) { + scinfo->vcpus[i].period = vcpus[i].u.rtds.period; + scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget; + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; + } + rc = 0; +out: + return rc; +} + +/* Get the RTDS scheduling parameters of all vcpus of a domain */ +static int sched_rtds_vcpu_get_all(libxl__gc *gc, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + uint32_t num_vcpus; + int i, r, rc; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + + if (scinfo->num_vcpus > 0) { + rc = ERROR_INVAL; + goto out; + } else { + num_vcpus = info.max_vcpu_id + 1; + GCNEW_ARRAY(vcpus, num_vcpus); + for (i = 0; i < num_vcpus; i++) + vcpus[i].vcpuid = i; + } + + r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "getting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + scinfo->sched = LIBXL_SCHEDULER_RTDS; + scinfo->num_vcpus = num_vcpus; + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus, + sizeof(libxl_sched_params)); + + for (i = 0; i < num_vcpus; i++) { + scinfo->vcpus[i].period = vcpus[i].u.rtds.period; + scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget; + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; + } + rc = 0; +out: + return rc; +} + +/* Set the RTDS scheduling parameters of vcpu(s) */ +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + int r, rc; + int i; + uint16_t max_vcpuid; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + max_vcpuid = info.max_vcpu_id; + + if (scinfo->num_vcpus <= 0) { + rc = ERROR_INVAL; + goto out; + } + for (i = 0; i < scinfo->num_vcpus; i++) { + if (scinfo->vcpus[i].vcpuid < 0 || + scinfo->vcpus[i].vcpuid > max_vcpuid) { + LOG(ERROR, "Invalid VCPU %d: valid range is [0, %d]", + scinfo->vcpus[i].vcpuid, max_vcpuid); + rc = ERROR_INVAL; + goto out; + } + rc = sched_rtds_validate_params(gc, scinfo->vcpus[i].period, + scinfo->vcpus[i].budget); + if (rc) { + rc = ERROR_INVAL; + goto out; + } + } + GCNEW_ARRAY(vcpus, scinfo->num_vcpus); + for (i = 0; i < scinfo->num_vcpus; i++) { + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid; + vcpus[i].u.rtds.period = scinfo->vcpus[i].period; + vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget; + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, scinfo->num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + rc = 0; +out: + return rc; +} + +/* Set the RTDS scheduling parameters of all vcpus of a domain */ +static int sched_rtds_vcpu_set_all(libxl__gc *gc, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + int r, rc; + int i; + uint16_t max_vcpuid; + xc_dominfo_t info; + struct xen_domctl_schedparam_vcpu *vcpus; + uint32_t num_vcpus; + + r = xc_domain_getinfo(CTX->xch, domid, 1, &info); + if (r < 0) { + LOGE(ERROR, "getting domain info"); + rc = ERROR_FAIL; + goto out; + } + max_vcpuid = info.max_vcpu_id; + + if (scinfo->num_vcpus != 1) { + rc = ERROR_INVAL; + goto out; + } + if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period, + scinfo->vcpus[0].budget)) { + rc = ERROR_INVAL; + goto out; + } + num_vcpus = max_vcpuid + 1; + GCNEW_ARRAY(vcpus, num_vcpus); + for (i = 0; i < num_vcpus; i++) { + vcpus[i].vcpuid = i; + vcpus[i].u.rtds.period = scinfo->vcpus[0].period; + vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget; + } + + r = xc_sched_rtds_vcpu_set(CTX->xch, domid, + vcpus, num_vcpus); + if (r != 0) { + LOGE(ERROR, "setting vcpu sched rtds"); + rc = ERROR_FAIL; + goto out; + } + rc = 0; +out: + return rc; +} + static int sched_rtds_domain_get(libxl__gc *gc, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5811,30 +6041,12 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "getting domain sched rtds"); return ERROR_FAIL; } - - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) { - if (scinfo->period < 1) { - LOG(ERROR, "VCPU period is not set or out of range, " - "valid values are larger than 1"); - return ERROR_INVAL; - } + if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) sdom.period = scinfo->period; - } - - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) { - if (scinfo->budget < 1) { - LOG(ERROR, "VCPU budget is not set or out of range, " - "valid values are larger than 1"); - return ERROR_INVAL; - } + if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) sdom.budget = scinfo->budget; - } - - if (sdom.budget > sdom.period) { - LOG(ERROR, "VCPU budget is larger than VCPU period, " - "VCPU budget should be no larger than VCPU period"); + if (sched_rtds_validate_params(gc, sdom.period, sdom.budget)) return ERROR_INVAL; - } rc = xc_sched_rtds_domain_set(CTX->xch, domid, &sdom); if (rc < 0) { @@ -5882,6 +6094,74 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, return ret; } +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + libxl_scheduler sched = scinfo->sched; + int rc; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpu_set(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + libxl_scheduler sched = scinfo->sched; + int rc; + + if (sched == LIBXL_SCHEDULER_UNKNOWN) + sched = libxl__domain_scheduler(gc, domid); + + switch (sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter setting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpu_set_all(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *scinfo) { @@ -5916,6 +6196,70 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, return ret; } +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + int rc; + + scinfo->sched = libxl__domain_scheduler(gc, domid); + + switch (scinfo->sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler is no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpu_get(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + +int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *scinfo) +{ + GC_INIT(ctx); + int rc; + + scinfo->sched = libxl__domain_scheduler(gc, domid); + + switch (scinfo->sched) { + case LIBXL_SCHEDULER_SEDF: + LOG(ERROR, "SEDF scheduler is no longer available"); + rc = ERROR_FEATURE_REMOVED; + break; + case LIBXL_SCHEDULER_CREDIT: + case LIBXL_SCHEDULER_CREDIT2: + case LIBXL_SCHEDULER_ARINC653: + LOG(ERROR, "per-VCPU parameter getting not supported for this scheduler"); + rc = ERROR_INVAL; + break; + case LIBXL_SCHEDULER_RTDS: + rc = sched_rtds_vcpu_get_all(gc, domid, scinfo); + break; + default: + LOG(ERROR, "Unknown scheduler"); + rc = ERROR_INVAL; + break; + } + + GC_FREE; + return rc; +} + static int libxl__domain_s3_resume(libxl__gc *gc, int domid) { int rc = 0; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 83d7cd3..4c36a69 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -218,6 +218,17 @@ #define LIBXL_HAVE_DEVICE_MODEL_USER 1 /* + * libxl_vcpu_sched_params is used to store per-vcpu params. + */ +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1 + +/* + * LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS indicates RTDS scheduler + * now supports per-vcpu settings. + */ +#define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1 + +/* * libxl_domain_build_info has the arm.gic_version field. */ #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 @@ -1849,11 +1860,41 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid, #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1 #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT -1 +/* Per-VCPU parameters */ +#define LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT -1 + +/* Get the per-domain scheduling parameters. + * For schedulers that support per-vcpu settings (e.g., RTDS), + * calling *_domain_get functions will get default scheduling + * parameters. + */ int libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid, libxl_domain_sched_params *params); + +/* Set the per-domain scheduling parameters. + * For schedulers that support per-vcpu settings (e.g., RTDS), + * calling *_domain_set functions will set all vcpus with the same + * scheduling parameters. + */ int libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid, const libxl_domain_sched_params *params); +/* Get the per-vcpu scheduling parameters */ +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *params); + +/* Get the per-vcpu scheduling parameters of all vcpus of a domain */ +int libxl_vcpu_sched_params_get_all(libxl_ctx *ctx, uint32_t domid, + libxl_vcpu_sched_params *params); + +/* Set the per-vcpu scheduling parameters */ +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *params); + +/* Set the per-vcpu scheduling parameters of all vcpus of a domain */ +int libxl_vcpu_sched_params_set_all(libxl_ctx *ctx, uint32_t domid, + const libxl_vcpu_sched_params *params); + int libxl_send_trigger(libxl_ctx *ctx, uint32_t domid, libxl_trigger trigger, uint32_t vcpuid); int libxl_send_sysrq(libxl_ctx *ctx, uint32_t domid, char sysrq); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 59b183c..d33607e 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -387,6 +387,20 @@ libxl_domain_restore_params = Struct("domain_restore_params", [ ("stream_version", uint32, {'init_val': '1'}), ]) +libxl_sched_params = Struct("sched_params",[ + ("vcpuid", integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}), + ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}), + ("cap", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}), + ("period", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}), + ("extratime", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}), + ("budget", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}), + ]) + +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[ + ("sched", libxl_scheduler), + ("vcpus", Array(libxl_sched_params, "num_vcpus")), + ]) + libxl_domain_sched_params = Struct("domain_sched_params",[ ("sched", libxl_scheduler), ("weight", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),