Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

From: Chong Li <lichong659@gmail.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: Chong Li <chong.li@wustl.edu>, Sisu Xi <xisisu@gmail.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	"dario.faggioli" <dario.faggioli@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	xen-devel <xen-devel@lists.xen.org>,
	Ian Campbell <ian.campbell@eu.citrix.com>,
	Meng Xu <mengxu@cis.upenn.edu>,
	Dagaen Golomb <dgolomb@seas.upenn.edu>
Subject: Re: [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
Date: Mon, 8 Feb 2016 16:59:46 -0600	[thread overview]
Message-ID: <CAGHO-iqPirGwh5xXu+WZo7QcdOhYw35PLFhJbN0=9sN4=Nz8=w@mail.gmail.com> (raw)
In-Reply-To: <20160208110717.GM23178@citrix.com>

On Mon, Feb 8, 2016 at 5:07 AM, Wei Liu <wei.liu2@citrix.com> wrote:
>
>
> [...]
> > >> +        num_vcpus = max_vcpuid + 1;
> > >> +        GCNEW_ARRAY(vcpus, num_vcpus);
> > >> +        if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> > >> +                                 scinfo->vcpus[0].budget,
> > >
> > > This doesn't make sense. You take this path because scinfo->num_vcpus is
> > > 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
> > For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000"
> > (which sets all vcpus with
> > the same scheduling parameters), we pass the budget and period via
> > scinfo->vcpus[0].
> >
> > I'll add more explanation here.
>
> No, adding more explanation won't help.
>
> Let me explain a bit. Libxl is the library that can be used by multiple
> applications. Xl is just one of the applications. The other application
> that I know of is libvirt.
>
> So, the incarnation of a particular xl command is of no concern how we
> define the semantics of a libxl API. That is, you can come up with an
> unambiguous API but still support the same xl command.
>
> Currently the semantics of this (new?) libxl API seems to be broken,
> because you're (ab)using num_vcpus to represent a special case. In
> effect you can't really whether the array is empty. When num_vcpus is 0,
> you shouldn't dereference vcpus array, at all, because the semantics of
> num_vcpus == 0 is that the array is empty.
>
> Wei.

I see. I'll think about re-designing the data structure of
libxl_vcpu_sched_params.

Chong
>
> > >
> > >> +                                 &vcpus[0].s.rtds.period,
> > >> +                                 &vcpus[0].s.rtds.budget))
> > >
> >
> > --
> > Chong Li
> > Department of Computer Science and Engineering
> > Washington University in St.louis

-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis