Re: [Xen-devel] [Xen-users] xl doesn't honour the parameter cpu_weight from my config file while xm does honour it

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-04-24 at 14:24 +0100, Ian Jackson wrote:
> Dieter Bloms writes ("Re: [Xen-devel] [Xen-users] xl doesn't honour the 
> parameter cpu_weight from my config file while xm does honour it"):
> > On Tue, Apr 24, Dario Faggioli wrote:
> > > What might be missing is some documentation in docs/man/xl.cfg.pod.5,
> > > explaining about the new options... :-)
> > 
> > I've added a little documentation, so now I hope it is ok.
> 
> This is better, thanks.  I have some comments.
> 
> > +=item B<cpu-weight="weight of cpu (default 256)">
> 
> I'm not qualified to review the documented semantics here.
> 
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 0bdd654..38acff4 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -124,8 +124,27 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
> > +    sched = libxl_get_scheduler (ctx);
> > +    switch (sched) {
> > +    case LIBXL_SCHEDULER_SEDF:
> > +      libxl_sched_sedf_domain_set(ctx, domid, &(info->us.sedf));
> > +      break;
> > +    case LIBXL_SCHEDULER_CREDIT:
> > +      libxl_sched_credit_domain_set(ctx, domid, &(info->us.credit));
> > +      break;
> > +    case LIBXL_SCHEDULER_CREDIT2:
> > +      libxl_sched_credit2_domain_set(ctx, domid, &(info->us.credit2));
> > +      break;
> > +    case LIBXL_SCHEDULER_ARINC653:
> > +      /* not implemented */
> > +      break;
> > +    default:
> > +        abort();
> > +    }
> 
> This is a very repetitive piece of code.  Can't it be autogenerated
> somehow by the idl compiler ?  Ian C ?

Not without a bunch more plumbing in the infrastructure, which would
probably heavily outweigh the code here...

A compromise might be to make this a libxl__sched_set_params(gc, domid,
&info->us), so at least it wouldn't be repeated again somewhere.

> Also, we use 4-character indents and you have used 2.  (If this were
> the only thing that needed changing I would fix it when I committed.)
> 
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 5cf9708..c1cdc3c 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -224,6 +224,27 @@ libxl_domain_create_info = 
> > Struct("domain_create_info",[
> > +libxl_sched_credit_domain = Struct("sched_credit_domain", [
> > +    ("weight", integer),
> > +    ("cap", integer),
> > +    ])
> 
> You seem to have just moved many of these about ?

I think he said as much in his commit message? Oh, it was actually in
the comment of hte previous posting:
> I've defined a new union in the libxl_domain_build_info structure.
> For this I had to move some structure definition like
> libxl_sched_credit_domain more to the top.


>   That's just because
> the idl file doesn't support forward declarations, right ?
> 
> Thanks,
> Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel