Xen users - Apr 2012 - xl doesn't honour the parameter cpu_weight from my config file while xm does honour it

Hi,

I''ve installed xen-unstable 4.2 from actual git (last commit was
4dc7dbef5400f0608321d579aebb57f933e8f707).

When I start a domU with xm all is fine include the cpu_weight I
configured in my domU config.

When I start the domU with xl then all my domU have the default
cpu_weight of 256 instead of the configured one.

Was the name of cpu_weight being changed for xl command ?

My domU config looks like this:

--snip--
name="vdrserver"
description="vdrserver for my clients"
memory=768
maxmem=2048
vcpus=1
cpus="1"
cpu_weight = 128
on_poweroff="destroy"
on_reboot="restart"
on_crash="destroy"

localtime=0
keymap="de"

builder="linux"
bootloader="/usr/bin/pygrub"
bootargs=""
extra="console=hvc0 tmem cgroup_disable=memory independent_wallclock=1
iommu=soft"
nographic=1
keymap = ''de''

disk=[ 
  ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
  ''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
]
vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]

pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
--snip--


-- 
Gruß

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.

Hi Dieter, 

thanks for the report.

It seems that support for this config file variable is not present in xl
at the moment. We should try and add this for 4.2 IMHO.

If you know a little bit of C (or are interested in learning) then this
should be a pretty simple thing to implement -- please let me know if
you want more details.

Ian.

On Fri, 2012-04-20 at 16:00 +0100, Dieter Bloms wrote:
> Hi,
> 
> I''ve installed xen-unstable 4.2 from actual git (last commit was
> 4dc7dbef5400f0608321d579aebb57f933e8f707).
> 
> When I start a domU with xm all is fine include the cpu_weight I
> configured in my domU config.
> 
> When I start the domU with xl then all my domU have the default
> cpu_weight of 256 instead of the configured one.
> 
> Was the name of cpu_weight being changed for xl command ?
> 
> My domU config looks like this:
> 
> --snip--
> name="vdrserver"
> description="vdrserver for my clients"
> memory=768
> maxmem=2048
> vcpus=1
> cpus="1"
> cpu_weight = 128
> on_poweroff="destroy"
> on_reboot="restart"
> on_crash="destroy"
> 
> localtime=0
> keymap="de"
> 
> builder="linux"
> bootloader="/usr/bin/pygrub"
> bootargs=""
> extra="console=hvc0 tmem cgroup_disable=memory independent_wallclock=1
iommu=soft"
> nographic=1
> keymap = ''de''
> 
> disk=[ 
>   ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
>   ''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
> ]
> vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]
> 
> pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
> --snip--
> 
>

Hi Dieter, 

thanks for the report.

It seems that support for this config file variable is not present in xl
at the moment. We should try and add this for 4.2 IMHO.

If you know a little bit of C (or are interested in learning) then this
should be a pretty simple thing to implement -- please let me know if
you want more details.

Ian.

On Fri, 2012-04-20 at 16:00 +0100, Dieter Bloms wrote:
> Hi,
> 
> I''ve installed xen-unstable 4.2 from actual git (last commit was
> 4dc7dbef5400f0608321d579aebb57f933e8f707).
> 
> When I start a domU with xm all is fine include the cpu_weight I
> configured in my domU config.
> 
> When I start the domU with xl then all my domU have the default
> cpu_weight of 256 instead of the configured one.
> 
> Was the name of cpu_weight being changed for xl command ?
> 
> My domU config looks like this:
> 
> --snip--
> name="vdrserver"
> description="vdrserver for my clients"
> memory=768
> maxmem=2048
> vcpus=1
> cpus="1"
> cpu_weight = 128
> on_poweroff="destroy"
> on_reboot="restart"
> on_crash="destroy"
> 
> localtime=0
> keymap="de"
> 
> builder="linux"
> bootloader="/usr/bin/pygrub"
> bootargs=""
> extra="console=hvc0 tmem cgroup_disable=memory independent_wallclock=1
iommu=soft"
> nographic=1
> keymap = ''de''
> 
> disk=[ 
>   ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
>   ''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
> ]
> vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]
> 
> pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
> --snip--
> 
>

Hi,

I''am not a programmer, but will have a look at it the next days and let
you know if my skills are good enough.

On Fri, Apr 20, Ian Campbell wrote:
> Hi Dieter, 
> 
> thanks for the report.
> 
> It seems that support for this config file variable is not present in xl
> at the moment. We should try and add this for 4.2 IMHO.
> 
> If you know a little bit of C (or are interested in learning) then this
> should be a pretty simple thing to implement -- please let me know if
> you want more details.
> 
> Ian.
> 
> On Fri, 2012-04-20 at 16:00 +0100, Dieter Bloms wrote:
> > Hi,
> > 
> > I''ve installed xen-unstable 4.2 from actual git (last commit
was
> > 4dc7dbef5400f0608321d579aebb57f933e8f707).
> > 
> > When I start a domU with xm all is fine include the cpu_weight I
> > configured in my domU config.
> > 
> > When I start the domU with xl then all my domU have the default
> > cpu_weight of 256 instead of the configured one.
> > 
> > Was the name of cpu_weight being changed for xl command ?
> > 
> > My domU config looks like this:
> > 
> > --snip--
> > name="vdrserver"
> > description="vdrserver for my clients"
> > memory=768
> > maxmem=2048
> > vcpus=1
> > cpus="1"
> > cpu_weight = 128
> > on_poweroff="destroy"
> > on_reboot="restart"
> > on_crash="destroy"
> > 
> > localtime=0
> > keymap="de"
> > 
> > builder="linux"
> > bootloader="/usr/bin/pygrub"
> > bootargs=""
> > extra="console=hvc0 tmem cgroup_disable=memory
independent_wallclock=1 iommu=soft"
> > nographic=1
> > keymap = ''de''
> > 
> > disk=[ 
> >   ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
> >  
''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
> > ]
> > vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]
> > 
> > pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
> > --snip--
> > 
> > 
> 
> 
-- 
Gruß

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.

Hi,

I''am not a programmer, but will have a look at it the next days and let
you know if my skills are good enough.

On Fri, Apr 20, Ian Campbell wrote:
> Hi Dieter, 
> 
> thanks for the report.
> 
> It seems that support for this config file variable is not present in xl
> at the moment. We should try and add this for 4.2 IMHO.
> 
> If you know a little bit of C (or are interested in learning) then this
> should be a pretty simple thing to implement -- please let me know if
> you want more details.
> 
> Ian.
> 
> On Fri, 2012-04-20 at 16:00 +0100, Dieter Bloms wrote:
> > Hi,
> > 
> > I''ve installed xen-unstable 4.2 from actual git (last commit
was
> > 4dc7dbef5400f0608321d579aebb57f933e8f707).
> > 
> > When I start a domU with xm all is fine include the cpu_weight I
> > configured in my domU config.
> > 
> > When I start the domU with xl then all my domU have the default
> > cpu_weight of 256 instead of the configured one.
> > 
> > Was the name of cpu_weight being changed for xl command ?
> > 
> > My domU config looks like this:
> > 
> > --snip--
> > name="vdrserver"
> > description="vdrserver for my clients"
> > memory=768
> > maxmem=2048
> > vcpus=1
> > cpus="1"
> > cpu_weight = 128
> > on_poweroff="destroy"
> > on_reboot="restart"
> > on_crash="destroy"
> > 
> > localtime=0
> > keymap="de"
> > 
> > builder="linux"
> > bootloader="/usr/bin/pygrub"
> > bootargs=""
> > extra="console=hvc0 tmem cgroup_disable=memory
independent_wallclock=1 iommu=soft"
> > nographic=1
> > keymap = ''de''
> > 
> > disk=[ 
> >   ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
> >  
''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
> > ]
> > vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]
> > 
> > pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
> > --snip--
> > 
> > 
> 
> 
-- 
Gruß

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.

Hello Ian,

I''ve made a little patch to set the cpu_weight for credit, credit2 and
sedf scheduler from the config file.

Btw.: for the sedf scheduler there seems to be some type mismatch.  
The functions xc_sedf_domain_set and xc_sedf_domain_get expect the type
''uint16_t'' for variables ''extratime'' and
''weight'' while the structure
''xen_domctl_sched_sedf'' defines the type uint32_t for them.
I think they should be the same, or not ?

Anyway, I''ve tested this patch for the credit scheduler and it works so
far.


On Fri, Apr 20, Ian Campbell wrote:
> Hi Dieter, 
> 
> thanks for the report.
> 
> It seems that support for this config file variable is not present in xl
> at the moment. We should try and add this for 4.2 IMHO.
> 
> If you know a little bit of C (or are interested in learning) then this
> should be a pretty simple thing to implement -- please let me know if
> you want more details.
> 
> Ian.
> 
> On Fri, 2012-04-20 at 16:00 +0100, Dieter Bloms wrote:
> > Hi,
> > 
> > I''ve installed xen-unstable 4.2 from actual git (last commit
was
> > 4dc7dbef5400f0608321d579aebb57f933e8f707).
> > 
> > When I start a domU with xm all is fine include the cpu_weight I
> > configured in my domU config.
> > 
> > When I start the domU with xl then all my domU have the default
> > cpu_weight of 256 instead of the configured one.
> > 
> > Was the name of cpu_weight being changed for xl command ?
> > 
> > My domU config looks like this:
> > 
> > --snip--
> > name="vdrserver"
> > description="vdrserver for my clients"
> > memory=768
> > maxmem=2048
> > vcpus=1
> > cpus="1"
> > cpu_weight = 128
> > on_poweroff="destroy"
> > on_reboot="restart"
> > on_crash="destroy"
> > 
> > localtime=0
> > keymap="de"
> > 
> > builder="linux"
> > bootloader="/usr/bin/pygrub"
> > bootargs=""
> > extra="console=hvc0 tmem cgroup_disable=memory
independent_wallclock=1 iommu=soft"
> > nographic=1
> > keymap = ''de''
> > 
> > disk=[ 
> >   ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
> >  
''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
> > ]
> > vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]
> > 
> > pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
> > --snip--
> > 
> > 
> 
> 
> 
> _______________________________________________
> Xen-users mailing list
> Xen-users@lists.xen.org
> http://lists.xen.org/xen-users
-- 
Gruß

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.


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

Hello Ian,

I''ve made a little patch to set the cpu_weight for credit, credit2 and
sedf scheduler from the config file.

Btw.: for the sedf scheduler there seems to be some type mismatch.  
The functions xc_sedf_domain_set and xc_sedf_domain_get expect the type
''uint16_t'' for variables ''extratime'' and
''weight'' while the structure
''xen_domctl_sched_sedf'' defines the type uint32_t for them.
I think they should be the same, or not ?

Anyway, I''ve tested this patch for the credit scheduler and it works so
far.


On Fri, Apr 20, Ian Campbell wrote:
> Hi Dieter, 
> 
> thanks for the report.
> 
> It seems that support for this config file variable is not present in xl
> at the moment. We should try and add this for 4.2 IMHO.
> 
> If you know a little bit of C (or are interested in learning) then this
> should be a pretty simple thing to implement -- please let me know if
> you want more details.
> 
> Ian.
> 
> On Fri, 2012-04-20 at 16:00 +0100, Dieter Bloms wrote:
> > Hi,
> > 
> > I''ve installed xen-unstable 4.2 from actual git (last commit
was
> > 4dc7dbef5400f0608321d579aebb57f933e8f707).
> > 
> > When I start a domU with xm all is fine include the cpu_weight I
> > configured in my domU config.
> > 
> > When I start the domU with xl then all my domU have the default
> > cpu_weight of 256 instead of the configured one.
> > 
> > Was the name of cpu_weight being changed for xl command ?
> > 
> > My domU config looks like this:
> > 
> > --snip--
> > name="vdrserver"
> > description="vdrserver for my clients"
> > memory=768
> > maxmem=2048
> > vcpus=1
> > cpus="1"
> > cpu_weight = 128
> > on_poweroff="destroy"
> > on_reboot="restart"
> > on_crash="destroy"
> > 
> > localtime=0
> > keymap="de"
> > 
> > builder="linux"
> > bootloader="/usr/bin/pygrub"
> > bootargs=""
> > extra="console=hvc0 tmem cgroup_disable=memory
independent_wallclock=1 iommu=soft"
> > nographic=1
> > keymap = ''de''
> > 
> > disk=[ 
> >   ''phy:/dev/mapper/xenimages-vdrserver,xvda1,w'',
> >  
''phy:/dev/mapper/xenimages-swap_vdrserver,xvda2,w'',
> > ]
> > vif=[ ''mac=00:00:00:00:00:80,bridge=br0'', ]
> > 
> > pci = [ ''06:00.0'', ''06:01.0'',
''00:12.2'', ''00:13.2'']
> > --snip--
> > 
> > 
> 
> 
> 
> _______________________________________________
> Xen-users mailing list
> Xen-users@lists.xen.org
> http://lists.xen.org/xen-users
-- 
Gruß

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.


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

(xen-users to BCC, this seems like a devel thing for now)

On Mon, 2012-04-23 at 10:46 +0100, Dieter Bloms wrote:
> Hello Ian,
> 
> I''ve made a little patch to set the cpu_weight for credit, credit2
and
> sedf scheduler from the config file.
Awesome, thank you!
> Btw.: for the sedf scheduler there seems to be some type mismatch.  
> The functions xc_sedf_domain_set and xc_sedf_domain_get expect the type
> ''uint16_t'' for variables ''extratime''
and ''weight'' while the structure
> ''xen_domctl_sched_sedf'' defines the type uint32_t for
them.
> I think they should be the same, or not ?
I''m not clear enough on the scheduling stuff to be able to say one way
or another. I''d be inclined to suggest that if this needs to change for
some reason then we should leave it for post 4.2.
> Anyway, I''ve tested this patch for the credit scheduler and it
works so
> far.
Cool. One thing which we need in order to able to apply a patch is a
"Signed-off-by" per the DCO as described in
http://wiki.xen.org/wiki/SubmittingXenPatches. 

I''ve also a few minor comments on the patch itself below.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e63c7bd..706e282 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -110,6 +110,10 @@ int libxl__domain_build_info_setdefault(libxl__gc
> *gc,
>          return ERROR_INVAL;
>      }
>  
> +    if (!b_info->weight)
> +        b_info->weight = 256;
I suppose that weight=0 does not make sense and therefore 0 is available
as a discriminating value meaning "the default".
> +    if (!b_info->cap)
> +        b_info->cap = 0;
!b_info->cap means that b_info_cap is already 0, although I guess this
serves as a more explicit indication of the default.
>      if (!b_info->max_vcpus)
>          b_info->max_vcpus = 1;
>      if (!b_info->cur_vcpus)
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 0bdd654..f858a42 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -65,9 +65,38 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      int tsc_mode;
>      char *xs_domid, *con_domid;
> +    libxl_scheduler sched;
> +    struct xen_domctl_scheduler_op sched_op;
> +
>      xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
> &info->cpumap);
>      xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> LIBXL_MAXMEM_CONSTANT);
> +
> +    sched = libxl_get_scheduler (ctx);
> +    
> +    switch (sched) {
> +    case LIBXL_SCHEDULER_SEDF:
> +      xc_sedf_domain_get (ctx->xch, domid,
&(sched_op.u.sedf.period),
> &(sched_op.u.sedf.slice), &(sched_op.u.sedf.latency), (uint16_t *)
> &(sched_op.u.sedf.extratime), (uint16_t *)
&(sched_op.u.sedf.weight));
> +      sched_op.u.sedf.weight = info->weight;
> +      xc_sedf_domain_set (ctx->xch, domid, sched_op.u.sedf.period,
> sched_op.u.sedf.slice, sched_op.u.sedf.latency, (uint16_t)
> sched_op.u.sedf.extratime, (uint16_t) sched_op.u.sedf.weight);
Wow, that''s a pretty sucky interface at the libxc level! Anyway no need
for you to fix it here but please do wrap your lines to <80 columns for
readability.
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +//      struct xen_domctl_sched_credit sdom;
Please don''t leave commented out code in place.
> +      sched_op.u.credit.weight = info->weight;
> +      sched_op.u.credit.cap = info->cap;
> +      xc_sched_credit_domain_set(ctx->xch, domid,
> &(sched_op.u.credit));
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT2:
> +      sched_op.u.credit2.weight = info->weight;
> +      xc_sched_credit2_domain_set(ctx->xch, domid,
> &(sched_op.u.credit2));
> +      break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +      /* not implemented */
> +      break;
> +    default:
> +        abort();
> +    }
> +
>      if (info->type == LIBXL_DOMAIN_TYPE_PV)
>          xc_domain_set_memmap_limit(ctx->xch, domid,
>                  (info->max_memkb + info->u.pv.slack_memkb));
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 5cf9708..f185d4c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -232,6 +232,8 @@ MemKB = UInt(64, init_val =
"LIBXL_MEMKB_DEFAULT")
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("cur_vcpus",       integer),
> +    ("weight",          integer),
> +    ("cap",             integer),
Are these values common parameters to all schedulers? Do they always
have the same meaning/units/etc?

I wonder if perhaps including each of libxl_sched_*_params in build_info
might be a preferable interface? I would probably cleanup the above code
in build_pre too since you could just call the appropriate
libxl_sched_*_params_set.

Ideally libxl_sched_*_params would be in a union in
libxl_domain_build_info but that would require that xl could easily
determine which scheduler was going to be used for the domain, having a
non-union here would keep things somewhat simpler from that PoV.

I''ve CC''d George and Dario for their thoughts on this
interface. We
should try and keep it simple so that we can get this into 4.2. 
>      ("cpumap",          libxl_cpumap),
>      ("tsc_mode",        libxl_tsc_mode),
>      ("max_memkb",       MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5703512..d7dcb84 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -587,6 +587,11 @@ static void parse_config_data(const char
> *configfile_filename_report,
>      libxl_domain_build_info_init_type(b_info, c_info->type);
>  
>      /* the following is the actual config parsing with overriding
> values in the structures */
> +    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
> +        b_info->weight = l;
> +    if (!xlu_cfg_get_long (config, "cap", &l, 0))
> +        b_info->cap = l;
> +
>      if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
>          b_info->max_vcpus = l;
>          b_info->cur_vcpus = (1 << l) - 1;

(xen-users to BCC, this seems like a devel thing for now)

On Mon, 2012-04-23 at 10:46 +0100, Dieter Bloms wrote:
> Hello Ian,
> 
> I''ve made a little patch to set the cpu_weight for credit, credit2
and
> sedf scheduler from the config file.
Awesome, thank you!
> Btw.: for the sedf scheduler there seems to be some type mismatch.  
> The functions xc_sedf_domain_set and xc_sedf_domain_get expect the type
> ''uint16_t'' for variables ''extratime''
and ''weight'' while the structure
> ''xen_domctl_sched_sedf'' defines the type uint32_t for
them.
> I think they should be the same, or not ?
I''m not clear enough on the scheduling stuff to be able to say one way
or another. I''d be inclined to suggest that if this needs to change for
some reason then we should leave it for post 4.2.
> Anyway, I''ve tested this patch for the credit scheduler and it
works so
> far.
Cool. One thing which we need in order to able to apply a patch is a
"Signed-off-by" per the DCO as described in
http://wiki.xen.org/wiki/SubmittingXenPatches. 

I''ve also a few minor comments on the patch itself below.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e63c7bd..706e282 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -110,6 +110,10 @@ int libxl__domain_build_info_setdefault(libxl__gc
> *gc,
>          return ERROR_INVAL;
>      }
>  
> +    if (!b_info->weight)
> +        b_info->weight = 256;
I suppose that weight=0 does not make sense and therefore 0 is available
as a discriminating value meaning "the default".
> +    if (!b_info->cap)
> +        b_info->cap = 0;
!b_info->cap means that b_info_cap is already 0, although I guess this
serves as a more explicit indication of the default.
>      if (!b_info->max_vcpus)
>          b_info->max_vcpus = 1;
>      if (!b_info->cur_vcpus)
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 0bdd654..f858a42 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -65,9 +65,38 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      int tsc_mode;
>      char *xs_domid, *con_domid;
> +    libxl_scheduler sched;
> +    struct xen_domctl_scheduler_op sched_op;
> +
>      xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
> &info->cpumap);
>      xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> LIBXL_MAXMEM_CONSTANT);
> +
> +    sched = libxl_get_scheduler (ctx);
> +    
> +    switch (sched) {
> +    case LIBXL_SCHEDULER_SEDF:
> +      xc_sedf_domain_get (ctx->xch, domid,
&(sched_op.u.sedf.period),
> &(sched_op.u.sedf.slice), &(sched_op.u.sedf.latency), (uint16_t *)
> &(sched_op.u.sedf.extratime), (uint16_t *)
&(sched_op.u.sedf.weight));
> +      sched_op.u.sedf.weight = info->weight;
> +      xc_sedf_domain_set (ctx->xch, domid, sched_op.u.sedf.period,
> sched_op.u.sedf.slice, sched_op.u.sedf.latency, (uint16_t)
> sched_op.u.sedf.extratime, (uint16_t) sched_op.u.sedf.weight);
Wow, that''s a pretty sucky interface at the libxc level! Anyway no need
for you to fix it here but please do wrap your lines to <80 columns for
readability.
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +//      struct xen_domctl_sched_credit sdom;
Please don''t leave commented out code in place.
> +      sched_op.u.credit.weight = info->weight;
> +      sched_op.u.credit.cap = info->cap;
> +      xc_sched_credit_domain_set(ctx->xch, domid,
> &(sched_op.u.credit));
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT2:
> +      sched_op.u.credit2.weight = info->weight;
> +      xc_sched_credit2_domain_set(ctx->xch, domid,
> &(sched_op.u.credit2));
> +      break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +      /* not implemented */
> +      break;
> +    default:
> +        abort();
> +    }
> +
>      if (info->type == LIBXL_DOMAIN_TYPE_PV)
>          xc_domain_set_memmap_limit(ctx->xch, domid,
>                  (info->max_memkb + info->u.pv.slack_memkb));
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 5cf9708..f185d4c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -232,6 +232,8 @@ MemKB = UInt(64, init_val =
"LIBXL_MEMKB_DEFAULT")
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("cur_vcpus",       integer),
> +    ("weight",          integer),
> +    ("cap",             integer),
Are these values common parameters to all schedulers? Do they always
have the same meaning/units/etc?

I wonder if perhaps including each of libxl_sched_*_params in build_info
might be a preferable interface? I would probably cleanup the above code
in build_pre too since you could just call the appropriate
libxl_sched_*_params_set.

Ideally libxl_sched_*_params would be in a union in
libxl_domain_build_info but that would require that xl could easily
determine which scheduler was going to be used for the domain, having a
non-union here would keep things somewhat simpler from that PoV.

I''ve CC''d George and Dario for their thoughts on this
interface. We
should try and keep it simple so that we can get this into 4.2. 
>      ("cpumap",          libxl_cpumap),
>      ("tsc_mode",        libxl_tsc_mode),
>      ("max_memkb",       MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5703512..d7dcb84 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -587,6 +587,11 @@ static void parse_config_data(const char
> *configfile_filename_report,
>      libxl_domain_build_info_init_type(b_info, c_info->type);
>  
>      /* the following is the actual config parsing with overriding
> values in the structures */
> +    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
> +        b_info->weight = l;
> +    if (!xlu_cfg_get_long (config, "cap", &l, 0))
> +        b_info->cap = l;
> +
>      if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
>          b_info->max_vcpus = l;
>          b_info->cur_vcpus = (1 << l) - 1;

Hi,
> Maybe weight is, but still, I think having some mechanism for specifying
> the full set of parameter of a specific scheduler is to be preferred...
> 
> > I wonder if perhaps including each of libxl_sched_*_params in
build_info
> > might be a preferable interface? I would probably cleanup the above
code
> > in build_pre too since you could just call the appropriate
> > libxl_sched_*_params_set.
> > 
> ... Exactly, that''s much better looking to me.
that''s a good hint.
> > Ideally libxl_sched_*_params would be in a union in
> > libxl_domain_build_info but that would require that xl could easily
> > determine which scheduler was going to be used for the domain, having
a
> > non-union here would keep things somewhat simpler from that PoV.
> > 
> Yes, again, I agree that a union will be even better, but maybe not so
> much a big deal for now (we can turn it into an union later, right? Or
> you think there will be some API implications?)
ok, I see the point and will change it to a union.
I checked out the source with git not hg, may I produce a diff with git
and send it via mail ?


-- 
Best regards

  Dieter Bloms

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.

On Mon, 2012-04-23 at 16:41 +0100, Dieter Bloms wrote:
> ok, I see the point and will change it to a union.
>
Cool!
> I checked out the source with git not hg, may I produce a diff with git
> and send it via mail ?
> 
I think you should. Either use something automatic like git
`send-email''
or just produce the diff and then inline or attach, following also the
other instructions Ian pointed you to (on the wiki)... Was it this you
were asking?

Regads,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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

Hi,


On Mon, Apr 23, Dario Faggioli wrote:
> > I checked out the source with git not hg, may I produce a diff with
git
> > and send it via mail ?
> > 
> I think you should. Either use something automatic like git
`send-email''
> or just produce the diff and then inline or attach, following also the
> other instructions Ian pointed you to (on the wiki)... Was it this you
> were asking?
yes.

ok, second try ;)
I hope this fit more your needs.

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.
I named the union ''us'' for union scheduler, because the letter
''u'' was
already used for the type of domain.


-- 
Best regards

  Dieter Bloms

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.


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

Hi,


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.


-- 
best regards

  Dieter Bloms

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.


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

On Tue, 2012-04-24 at 13:14 +0100, Dieter Bloms wrote:
> libxl: set domain scheduling parameters while creating the dom
> 
> the domain specific scheduling parameters like cpu_weight, cap,
> slice, ...
> will be set during creating the domain, so this parameters can be
> defined
> in the domain config file
> 
> Signed-off-by: Dieter Bloms <dieter@bloms.de>
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index e2cd251..4060933 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -112,6 +112,44 @@ List of which cpus the guest is allowed to use.
> Default behavior is
>  (all vcpus will run on cpus 0,2,3,5), or `cpus=["2",
"3"]` (all vcpus
>  will run on cpus 2 and 3).
>  
> +=item B<cpu-weight="weight of cpu (default 256)">
I think you mean cpu_weight rather than cpu-weight.

The typography in this file isn''t exactly strict but it seems that we
normally put the default in the text rather than in the header and use a
SHOUTY name for the value. e.g.

        =item B<cpu_weight=WEIGHT>
        
        Specifies the weight of the domain. A domain with a weight of
        512 will get twice as much CPU as a domain with a weight of 256
        on a contended host. Legal weights range from 1 to 65535 and the
        default is 256. Can be set for credit, credit2 and sedf
        scheduler.

This also avoids the ambiguous use of " in the title, since depending on
the item the quotes may be a required part of the syntax but not in this
case.

[...]
> +=item B<latency=" (default 0)">
=item B<latency=N>

I think you missed the value 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,
>      char *dom_path, *vm_path;
>      xs_transaction_t t;
>      char **ents, **hvm_ents;
> +    libxl_scheduler sched;
>      int i;
>  
> +    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();
> +    }
> +
>      libxl_cpuid_apply_policy(ctx, domid);
>      if (info->cpuid != NULL)
>          libxl_cpuid_set(ctx, domid, info->cpuid);
> 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",[
>  
>  MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>  
> +libxl_sched_credit_domain = Struct("sched_credit_domain", [
> +    ("weight", integer),
> +    ("cap", integer),
> +    ])
> +
> +libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
> +    ("weight", integer),
> +    ])
> +
> +libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
> +    ("period", integer),
> +    ("slice", integer),
> +    ("latency", integer),
> +    ("extratime", integer),
> +    ("weight", integer),
> +    ])
> +
> +libxl_sched_arinc653_domain = Struct("sched_arinc653_domain", [
> +    ("weight", integer),
> +    ])
> +
>  # Instances of libxl_file_reference contained in this struct which
>  # have been mapped (with libxl_file_reference_map) will be unmapped
>  # by libxl_domain_build/restore. If either of these are never called
> @@ -256,6 +277,13 @@ libxl_domain_build_info >
Struct("domain_build_info",[
>      # extra parameters pass directly to qemu for HVM guest, NULL
> terminated
>      ("extra_hvm",        libxl_string_list),
>  
> +    ("us", KeyedUnion(None, libxl_scheduler, "sched",
> +                 [("credit", libxl_sched_credit_domain),
> +                 ("credit2", libxl_sched_credit2_domain),
> +                 ("sedf", libxl_sched_sedf_domain),
> +                 ("arinc653", libxl_sched_arinc653_domain),
> +                 ], keyvar_init_val = "-1")),
I don''t think a KeyedUnion is right here, since the user of libxl
doesn''t really have a choice about which scheduler is in user
(that''s
set at boot time or via cpupool interfaces).

You could use a Union, but below I''ll make an argument that perhaps a
Struct would be better.

[...]
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5703512..db51ca6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -587,6 +587,23 @@ static void parse_config_data(const char
> *configfile_filename_report,
>      libxl_domain_build_info_init_type(b_info, c_info->type);
>  
>      /* the following is the actual config parsing with overriding
> values in the structures */
> +    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
> +        b_info->us.credit.weight = l;
> +        b_info->us.credit2.weight = l;
> +        b_info->us.sedf.weight = l;
You need {} here otherwise only the credit setting is associated with
the if.

However, since b_info->us is a union this is actually setting the same
value into different places in overlapping memory (since all member of a
union occupy the same storage). One solution would be to use
libxl_get_scheduler to get the scheduler and fill in only the correct
option, but then that gets complex when you have to deal with cpupools
etc.

I think you should make "us" a Struct and call it sched_params or
something along those lines. It''s just simpler all around, libxl can
internally figure out which set of parms to use (as you do correctly in
this patch).

Does the above make sense?

Ian.

> +    if (!xlu_cfg_get_long (config, "cap", &l, 0))
> +        b_info->us.credit.cap = l;
> +
> +    if (!xlu_cfg_get_long (config, "period", &l, 0))
> +        b_info->us.sedf.period = l;
> +    if (!xlu_cfg_get_long (config, "slice", &l, 0))
> +        b_info->us.sedf.period = l;
> +    if (!xlu_cfg_get_long (config, "latency", &l, 0))
> +        b_info->us.sedf.period = l;
> +    if (!xlu_cfg_get_long (config, "extratime", &l, 0))
> +        b_info->us.sedf.period = l;
> +
>      if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
>          b_info->max_vcpus = l;
>          b_info->cur_vcpus = (1 << l) - 1;
>

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 ?

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 ?  That''s just because
the idl file doesn''t support forward declarations, right ?

Thanks,
Ian.

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.

Ian Campbell 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, 2012-04-24 at 14:24 +0100, Ian Jackson wrote:
> > 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...
Fair enough.
> 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.
That would be nice but I wouldn''t insist on it until it was necessary
to avoid a second copy.

Ian.

Hi,

On Tue, Apr 24, Ian Campbell wrote:
> I think you mean cpu_weight rather than cpu-weight.
yes of course, fixed.
> =item B<latency=N>
> 
> I think you missed the value here.
I don''t know the value for it, so I added N as you suggested.
> > +    ("us", KeyedUnion(None, libxl_scheduler,
"sched",
> > +                 [("credit", libxl_sched_credit_domain),
> > +                 ("credit2", libxl_sched_credit2_domain),
> > +                 ("sedf", libxl_sched_sedf_domain),
> > +                 ("arinc653", libxl_sched_arinc653_domain),
> > +                 ], keyvar_init_val = "-1")),
> 
> I don''t think a KeyedUnion is right here, since the user of libxl
> doesn''t really have a choice about which scheduler is in user
(that''s
> set at boot time or via cpupool interfaces).
> 
> You could use a Union, but below I''ll make an argument that
perhaps a
> Struct would be better.
Hm, but when I don''t use a union with the structure for each scheduler
I
have to create a structure depend of scheduler at runtime, because the
functions libxl_sched_XXXXXX_domain_set use there own structure for
their scheduler.

I''ve create a new function libxl__sched_set_params, which set the
params
depend on the scheduler as you suggested.

I''am not a software developer, so please be forbear with me.

Here my next try.

-- 
Best regards

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.


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

On Tue, 2012-04-24 at 15:33 +0100, Dieter Bloms wrote:
> Hi,
> 
> On Tue, Apr 24, Ian Campbell wrote:
> 
> > I think you mean cpu_weight rather than cpu-weight.
> 
> yes of course, fixed.
> 
> > =item B<latency=N>
> > 
> > I think you missed the value here.
> 
> I don''t know the value for it, so I added N as you suggested.
> 
> > > +    ("us", KeyedUnion(None, libxl_scheduler,
"sched",
> > > +                 [("credit",
libxl_sched_credit_domain),
> > > +                 ("credit2",
libxl_sched_credit2_domain),
> > > +                 ("sedf", libxl_sched_sedf_domain),
> > > +                 ("arinc653",
libxl_sched_arinc653_domain),
> > > +                 ], keyvar_init_val = "-1")),
> > 
> > I don''t think a KeyedUnion is right here, since the user of
libxl
> > doesn''t really have a choice about which scheduler is in user
(that''s
> > set at boot time or via cpupool interfaces).
> > 
> > You could use a Union, but below I''ll make an argument that
perhaps a
> > Struct would be better.
> 
> Hm, but when I don''t use a union with the structure for each
scheduler I
> have to create a structure depend of scheduler at runtime, because the
> functions libxl_sched_XXXXXX_domain_set use there own structure for
> their scheduler.
I mean create a single struct with all of the options in it, from which
libxl will select the appropriate set of parameters (much like the code
you have now does. You don''t need to decide the content of that struct
at runtime.

The IDL patch is below, other than changing "us" to
"sched_params" I
think most of the rest of your patch remains the same.
> I''ve create a new function libxl__sched_set_params, which set the
params
> depend on the scheduler as you suggested.
> 
> I''am not a software developer, so please be forbear with me.
That''s ok, thanks for doing this!

You add a struct for arinc635 but don''t actually do anything with it,
you may as well leave it out for now?

Ian.

Ian Campbell writes ("Re: [Xen-devel] [Xen-users] xl doesn''t
honour the parameter cpu_weight from my config file while xm does honour
it"):
> I mean create a single struct with all of the options in it, from which
> libxl will select the appropriate set of parameters (much like the code
> you have now does. You don''t need to decide the content of that
struct
> at runtime.
> 
> The IDL patch is below, other than changing "us" to
"sched_params" I
> think most of the rest of your patch remains the same.
Missing attachment ?

Ian.

On Tue, 2012-04-24 at 17:03 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [Xen-users] xl doesn''t
honour the parameter cpu_weight from my config file while xm does honour
it"):
> > I mean create a single struct with all of the options in it, from
which
> > libxl will select the appropriate set of parameters (much like the
code
> > you have now does. You don''t need to decide the content of
that struct
> > at runtime.
> > 
> > The IDL patch is below, other than changing "us" to
"sched_params" I
> > think most of the rest of your patch remains the same.
> 
> Missing attachment ?
Yes, sorry.

Here it is:

diff -r aef90d90eb3b tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl	Tue Apr 24 16:53:00 2012 +0100
+++ b/tools/libxl/libxl_types.idl	Tue Apr 24 17:15:12 2012 +0100
@@ -224,6 +224,32 @@ libxl_domain_create_info = Struct("domai
 
 MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
 
+libxl_sched_credit_domain = Struct("sched_credit_domain", [
+    ("weight", integer),
+    ("cap", integer),
+    ])
+
+libxl_sched_credit_params = Struct("sched_credit_params", [
+    ("tslice_ms", integer),
+    ("ratelimit_us", integer),
+    ], dispose_fn=None)
+
+libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
+    ("weight", integer),
+    ])
+
+libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
+    ("period", integer),
+    ("slice", integer),
+    ("latency", integer),
+    ("extratime", integer),
+    ("weight", integer),
+    ])
+
+libxl_sched_arinc653_domain = Struct("sched_arinc653_domain", [
+    ("weight", integer),
+    ])
+
 # Instances of libxl_file_reference contained in this struct which
 # have been mapped (with libxl_file_reference_map) will be unmapped
 # by libxl_domain_build/restore. If either of these are never called
@@ -256,6 +282,12 @@ libxl_domain_build_info = Struct("domain
     # extra parameters pass directly to qemu for HVM guest, NULL terminated
     ("extra_hvm",        libxl_string_list),
 
+    ("sched_params",     Struct(None, [("credit",
libxl_sched_credit_domain),
+                                       ("credit2",
libxl_sched_credit2_domain),
+                                       ("sedf",
libxl_sched_sedf_domain),
+                                       ("arinc653",
libxl_sched_arinc653_domain),
+                                       ]))
+    ,    
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",        
string),
                                        ("bios",            
libxl_bios_type),
@@ -417,28 +449,6 @@ libxl_cputopology = Struct("cputopology"
     ("node", uint32),
     ], dir=DIR_OUT)
 
-libxl_sched_credit_domain = Struct("sched_credit_domain", [
-    ("weight", integer),
-    ("cap", integer),
-    ])
-
-libxl_sched_credit_params = Struct("sched_credit_params", [
-    ("tslice_ms", integer),
-    ("ratelimit_us", integer),
-    ], dispose_fn=None)
-
-libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
-    ("weight", integer),
-    ])
-
-libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
-    ("period", integer),
-    ("slice", integer),
-    ("latency", integer),
-    ("extratime", integer),
-    ("weight", integer),
-    ])
-
 libxl_event_type = Enumeration("event_type", [
     (1, "DOMAIN_SHUTDOWN"),
     (2, "DOMAIN_DEATH"),

Ian Campbell writes ("Re: [Xen-devel] [Xen-users] xl doesn''t
honour the parameter cpu_weight from my config file while xm does honour
it"):
> diff -r aef90d90eb3b tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl	Tue Apr 24 16:53:00 2012 +0100
> +++ b/tools/libxl/libxl_types.idl	Tue Apr 24 17:15:12 2012 +0100
...
> +libxl_sched_credit_domain = Struct("sched_credit_domain", [
> +    ("weight", integer),
...
...
> +libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
> +    ("weight", integer),
...
> +libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
...
> +    ("weight", integer),
> +    ])
> +
> +libxl_sched_arinc653_domain = Struct("sched_arinc653_domain", [
> +    ("weight", integer),
> +    ])
...
> +    ("sched_params",     Struct(None, [("credit",
libxl_sched_credit_domain),
> +                                       ("credit2",
libxl_sched_credit2_domain),
> +                                       ("sedf",
libxl_sched_sedf_domain),
> +                                       ("arinc653",
libxl_sched_arinc653_domain),
> +                                       ]))
The resulting sched_params structure contains four subfields called
"weight", all of which mean (roughly, obviously) the same thing and
all of which are to be set from the same "weight" xl configuration
parameter.  Is this really the most sensible way to do things ?

Perhaps it would be better to have a single sched_params struct which
contained all the parameters needed for any scheduler, and simply have
them ignored by libxl for schedulers we''re not using.

Ian.

On Tue, 2012-04-24 at 17:20 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [Xen-users] xl doesn''t
honour the parameter cpu_weight from my config file while xm does honour
it"):
> > diff -r aef90d90eb3b tools/libxl/libxl_types.idl
> > --- a/tools/libxl/libxl_types.idl	Tue Apr 24 16:53:00 2012 +0100
> > +++ b/tools/libxl/libxl_types.idl	Tue Apr 24 17:15:12 2012 +0100
> ...
> > +libxl_sched_credit_domain = Struct("sched_credit_domain", [
> > +    ("weight", integer),
> ...
> ...
> > +libxl_sched_credit2_domain = Struct("sched_credit2_domain",
[
> > +    ("weight", integer),
> ...
> > +libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
> ...
> > +    ("weight", integer),
> > +    ])
> > +
> > +libxl_sched_arinc653_domain =
Struct("sched_arinc653_domain", [
> > +    ("weight", integer),
> > +    ])
> ...
> > +    ("sched_params",     Struct(None, [("credit",
libxl_sched_credit_domain),
> > +                                       ("credit2",
libxl_sched_credit2_domain),
> > +                                       ("sedf",
libxl_sched_sedf_domain),
> > +                                       ("arinc653",
libxl_sched_arinc653_domain),
> > +                                       ]))
> 
> The resulting sched_params structure contains four subfields called
> "weight", all of which mean (roughly, obviously) the same thing
and
> all of which are to be set from the same "weight" xl
configuration
> parameter.  Is this really the most sensible way to do things ?
Perhaps not, but it would imply major surgery to the other scheduler
interfaces to do it some other way (since these same structs are used
for libxl_sched_*_{set,get}.

I''m not 100% sure that "weight" has the same meaning for each
scheduler
either. It''s at least plausible that I might want one weight for a
doamin if the scheduler is credit and something else if it is sedf.
> Perhaps it would be better to have a single sched_params struct which
> contained all the parameters needed for any scheduler, and simply have
> them ignored by libxl for schedulers we''re not using.
You''d need to reconstitute each of the split structs in order to call
libxl_sched_FOO_set internally, I don''t think I want to ask Dieter to
take on that much bigger job.

Modulo this IDL bit Dieter''s patch looks quite nice.

Ian.

Hi,

On Tue, Apr 24, Ian Jackson wrote:
> Perhaps it would be better to have a single sched_params struct which
> contained all the parameters needed for any scheduler, and simply have
> them ignored by libxl for schedulers we''re not using.
my first version has this type of design and then someone said that this
was not a good design and I have to use union with libxl_sched_*_params.

Anyway I think it is a good design to have one struct with all parameters
and I''am willing to implement it.


-- 
Gruß

  Dieter

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.

Hi,

On Tue, Apr 24, Dieter Bloms wrote:
> Hi,
> 
> On Tue, Apr 24, Ian Jackson wrote:
> 
> > Perhaps it would be better to have a single sched_params struct which
> > contained all the parameters needed for any scheduler, and simply have
> > them ignored by libxl for schedulers we''re not using.
...
> Anyway I think it is a good design to have one struct with all parameters
> and I''am willing to implement it.
I''ve made a new patch.
Maybe it fit all your needs.


-- 
best regards

  Dieter Bloms

--
I do not get viruses because I do not use MS software.
If you use Outlook then please do not put my email address in your
address-book so that WHEN you get a virus it won''t use my address in
the
From field.


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

On Tue, 2012-04-24 at 20:35 +0100, Dieter Bloms wrote:
> Hi,
> 
> On Tue, Apr 24, Dieter Bloms wrote:
> 
> > Hi,
> > 
> > On Tue, Apr 24, Ian Jackson wrote:
> > 
> > > Perhaps it would be better to have a single sched_params struct
which
> > > contained all the parameters needed for any scheduler, and simply
have
> > > them ignored by libxl for schedulers we''re not using.
> 
> ...
> 
> > Anyway I think it is a good design to have one struct with all
parameters
> > and I''am willing to implement it.
> 
> I''ve made a new patch.
> Maybe it fit all your needs.
Thanks, this patch look good to me, I''ll leave it to IanJ to decide
which approach he prefers.

Ian.

Ian Campbell 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, 2012-04-24 at 20:35 +0100, Dieter Bloms wrote:
> > I''ve made a new patch.
> > Maybe it fit all your needs.
> 
> Thanks, this patch look good to me, I''ll leave it to IanJ to
decide
> which approach he prefers.
Thanks, I''ll take that as an ack.  I''ve applied
Dieter''s latest
version, which I like.

I edited the docs message to change "can be set for" to "honoured
by", because variables can be set which will then be ignored.  Eg:

 +=item B<cpu_weight=WEIGHT>
 +
 +A domain with a weight of 512 will get twice as much CPU as a domain
 +with a weight of 256 on a contended host.
 +Legal weights range from 1 to 65535 and the default is 256.
-+Can be set for credit, credit2 and sedf scheduler.
++Honoured by the credit, credit2 and sedf schedulers.

Ian.