From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH 3/3] credit2: xen related changes to add support for runqueue per cpupool. Date: Thu, 14 Sep 2017 16:08:42 +0200 Message-ID: <1505398122.13935.23.camel@citrix.com> References: <1505177142-14864-1-git-send-email-anshulmakkar@gmail.com> <1505177142-14864-4-git-send-email-anshulmakkar@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2226937132943645556==" Return-path: In-Reply-To: <1505177142-14864-4-git-send-email-anshulmakkar@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: anshulmakkar , xen-devel@lists.xen.org Cc: jgross@suse.com, sstabellini@kernel.org, wei.liu2@citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, marmarek@invisiblethingslab.com, robert.vanvossen@dornerworks.com, tim@xen.org, josh.whitehead@dornerworks.com, mengxu@cis.upenn.edu, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org --===============2226937132943645556== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-GttjJN0XQXhO4rR4Pkh8" --=-GttjJN0XQXhO4rR4Pkh8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2017-09-12 at 01:45 +0100, anshulmakkar wrote: > --- a/xen/common/cpupool.c > +++ b/xen/common/cpupool.c > @@ -129,12 +129,13 @@ void cpupool_put(struct cpupool *pool) > =C2=A0 * - unknown scheduler > =C2=A0 */ > =C2=A0static struct cpupool *cpupool_create( > -=C2=A0=C2=A0=C2=A0=C2=A0int poolid, unsigned int sched_id, int *perr) > +=C2=A0=C2=A0=C2=A0=C2=A0int poolid, unsigned int sched_id, > +=C2=A0=C2=A0=C2=A0=C2=A0xen_sysctl_sched_param_t param, > +=C2=A0=C2=A0=C2=A0=C2=A0int *perr) > I second Juergen's opinion about as much as possible of these xen_sysctl_sched_param to move around functions as (const?) pointers. > =C2=A0{ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct cpupool *c; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct cpupool **q; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int last =3D 0; > - Spurious blank line deletion. > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*perr =3D -ENOMEM; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if ( (c =3D alloc_cpupool_struct()) =3D=3D = NULL ) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return NULL; > @@ -600,10 +601,11 @@ int cpupool_do_sysctl(struct > xen_sysctl_cpupool_op *op) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case XEN_SYSCTL_CPUPOOL_OP_CREATE: > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0{ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int poolid; > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0xen_sysctl_sched_param_t= param =3D op->sched_param; > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0poolid =3D (op->cpu= pool_id =3D=3D XEN_SYSCTL_CPUPOOL_PAR_ANY) ? > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0CPUPOOLID_NONE: op->cpupool_id; > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0c =3D cpupool_create(poo= lid, op->sched_id, &ret); > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0c =3D cpupool_create(poo= lid, op->sched_id, param, &ret); > Why you need the 'param' temporary variable? > @@ -798,7 +800,8 @@ static int __init cpupool_presmp_init(void) > =C2=A0{ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int err; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0void *cpu =3D (void *)(long)smp_processor_i= d(); > -=C2=A0=C2=A0=C2=A0=C2=A0cpupool0 =3D cpupool_create(0, 0, &err); > +=C2=A0=C2=A0=C2=A0=C2=A0xen_sysctl_sched_param_t param; > +=C2=A0=C2=A0=C2=A0=C2=A0cpupool0 =3D cpupool_create(0, 0, param, &err); > And in fact, if you use pointers, here you can pass NULL (to mean "just use default parameters"). > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0BUG_ON(cpupool0 =3D=3D NULL); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpupool_put(cpupool0); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cpu_callback(&cpu_nfb, CPU_ONLINE, cpu); > --- a/xen/common/sched_arinc653.c > +++ b/xen/common/sched_arinc653.c > @@ -343,7 +343,7 @@ arinc653_sched_get( > =C2=A0 *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > =C2=A0 */ > =C2=A0static int > -a653sched_init(struct scheduler *ops) > +a653sched_init(struct scheduler *ops, xen_sysctl_sched_param_t > sched_param) > =C2=A0{ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0a653sched_priv_t *prv; > =C2=A0 And here, and in other schedulers that doesn't take parameters, still if you use pointers, you can check that things are being done properly, by putting an =C2=A0ASSERT(sched_param =3D=3D NULL); > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -3410,6 +3411,11 @@ csched2_init(struct scheduler *ops) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* initialize ratelimit */ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0prv->ratelimit_us =3D sched_ratelimit_us; > =C2=A0 > +=C2=A0=C2=A0=C2=A0=C2=A0/* not need of type checking here if sched_para.= type =3D credit2. > Code > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0* block is here means we have type as cred= it2. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > +=C2=A0=C2=A0=C2=A0=C2=A0prv->runqueue =3D sched_param.u.sched_credit2.ru= nq; > + I don't understand what the comment is trying to say (and its style is wrong: missing the opening 'wing'). > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -555,6 +582,8 @@ struct xen_sysctl_cpupool_op { > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint32_t cpu;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0/* IN: AR=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*/ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint32_t n_dom;=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0/*=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0OUT: I=C2=A0=C2=A0*/ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct xenctl_bitmap cpumap; /*=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0OUT: IF */ > +=C2=A0=C2=A0=C2=A0=C2=A0/* IN: scheduler param relevant for cpupool */ > +=C2=A0=C2=A0=C2=A0=C2=A0xen_sysctl_sched_param_t sched_param; > =C2=A0}; > For the comment, follow the same convention used for other fields (i.e., for now, 'IN: C'). We will certainly want to be able to also retrieve the scheduler parameter set for a certain pool, at which point this will have to become 'IN: C=C2=A0=C2=A0 OUT: I'... but that's for another patch series, I guess. > =C2=A0typedef struct xen_sysctl_cpupool_op xen_sysctl_cpupool_op_t; > =C2=A0DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpupool_op_t); > @@ -630,22 +659,6 @@ > DEFINE_XEN_GUEST_HANDLE(xen_sysctl_arinc653_schedule_t); > =C2=A0#define XEN_SYSCTL_SCHED_RATELIMIT_MAX 500000 > =C2=A0#define XEN_SYSCTL_SCHED_RATELIMIT_MIN 100 > =C2=A0 > -struct xen_sysctl_credit_schedule { > -=C2=A0=C2=A0=C2=A0=C2=A0/* Length of timeslice in milliseconds */ > -#define XEN_SYSCTL_CSCHED_TSLICE_MAX 1000 > -#define XEN_SYSCTL_CSCHED_TSLICE_MIN 1 > -=C2=A0=C2=A0=C2=A0=C2=A0unsigned tslice_ms; > -=C2=A0=C2=A0=C2=A0=C2=A0unsigned ratelimit_us; > -}; > -typedef struct xen_sysctl_credit_schedule > xen_sysctl_credit_schedule_t; > -DEFINE_XEN_GUEST_HANDLE(xen_sysctl_credit_schedule_t); > - > -struct xen_sysctl_credit2_schedule { > -=C2=A0=C2=A0=C2=A0=C2=A0unsigned ratelimit_us; > -}; > -typedef struct xen_sysctl_credit2_schedule > xen_sysctl_credit2_schedule_t; > -DEFINE_XEN_GUEST_HANDLE(xen_sysctl_credit2_schedule_t); > - > You're mixing moving and changing code. This is something we prefer to avoid. Please, so the moving in a pre-patch. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-GttjJN0XQXhO4rR4Pkh8 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJZuo1rAAoJEBZCeImluHPuqfkP+QFMDpZqQw7A+TENBXFALR10 Co8ZRW5fzpVVR2WRR3gslPwO8HJULnBHTQQDEZkhcj59QXzSqww9TTFmHo3lz6Ey O3FCwKJk1h7BA3yOcle7lyxBY4MK5FksGXgZ8/tplPA7Wqgtom6j9WiTZiUx7gpw cZ5uYVahD59krkGRqX3yYqHgerNRPXxkNeTJw6COCn2tKyym/mHBNrHOtAKJtRy6 kNY4muH4rTgNHnoESbU49QnaWcq/CxnUUQX775/u2MQcmMRLahUNakSxm7EBypZT /g+Eu5691KbC3dq57tzUWA33ZAVMC3J113n6gq031r4B7xwN11kcBZGBpjTOW6RB ZfJ3uVVVPkUKfAEFYKiy4dIrsP6smpNcoukxqzJH2iRP/6+65nC7je/xQ8qQcFTE vnCKoV17nkoNFlTf+UYeVl0xwDncxkPUiatWbsYO4RdRbkDfosh/CNZa+6ZUlutZ drzEiHguoPys+8eGbEFLpsA0Mpn1oIOvfZ5zC3uedbrS32f7IJtZ75Fw/0Ndmwx9 i5KRDP+ZOZ85X6HPvAP7cllQTR87DQBMj0gG6xx58YDZdY7uccpOLDDS+8lus0Uc LerJkaj4vyQANP2t/ll9dV8rtv8ydRINoN6VegaCV8gnNBOIY0MHZKcPHX7yJI4t s4pz+LosrQ/3As5CF5uO =x9s2 -----END PGP SIGNATURE----- --=-GttjJN0XQXhO4rR4Pkh8-- --===============2226937132943645556== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============2226937132943645556==--