Re: [Xen-devel] [PATCH v11 6/9] x86: collect global QoS monitoring information

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, June 20, 2014 11:16 PM
> To: Xu, Dongxiao
> Cc: andrew.cooper3@xxxxxxxxxx; Ian.Campbell@xxxxxxxxxx;
> George.Dunlap@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx;
> stefano.stabellini@xxxxxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; dgdegra@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [PATCH v11 6/9] x86: collect global QoS monitoring information
> 
> >>> On 20.06.14 at 16:31, <dongxiao.xu@xxxxxxxxx> wrote:
> > This implementation tries to put all policies into user space, thus some
> > global QoS monitoring information needs to be exposed, such as the total
> > RMID count, L3 upscaling factor, etc.
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@xxxxxxxxx>
> > ---
> >  xen/arch/x86/sysctl.c       | 52
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/sysctl.h | 14 ++++++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> > index 49f95e4..ea9b2e4 100644
> > --- a/xen/arch/x86/sysctl.c
> > +++ b/xen/arch/x86/sysctl.c
> > @@ -28,6 +28,7 @@
> >  #include <xen/nodemask.h>
> >  #include <xen/cpu.h>
> >  #include <xsm/xsm.h>
> > +#include <asm/pqos.h>
> >
> >  #define get_xen_guest_handle(val, hnd)  do { val = (hnd).p; } while (0)
> >
> > @@ -152,6 +153,57 @@ long arch_do_sysctl(
> >      }
> >      break;
> >
> > +    case XEN_SYSCTL_pqos_monitor_op:
> > +        if ( !pqos_monitor_enabled() )
> > +            return -ENODEV;
> > +
> > +        switch ( sysctl->u.pqos_monitor_op.cmd )
> > +        {
> > +        case XEN_SYSCTL_PQOS_MONITOR_cqm_enabled:
> > +            sysctl->u.pqos_monitor_op.data =
> > +                (pqosm->qm_features & QOS_MONITOR_TYPE_L3) &&
> > +                (pqosm->l3m.l3_features & L3_FEATURE_OCCUPANCY);
> > +            break;
> > +        case XEN_SYSCTL_PQOS_MONITOR_get_total_rmid:
> > +            sysctl->u.pqos_monitor_op.data =
> > +                pqosm->rmid_max - pqosm->rmid_min + 1;
> > +            break;
> > +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor:
> > +            sysctl->u.pqos_monitor_op.data =
> pqosm->l3m.upscaling_factor;
> > +            break;
> > +        case XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size:
> > +            sysctl->u.pqos_monitor_op.data =
> boot_cpu_data.x86_cache_size;
> > +            break;
> > +        case XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu:
> > +        {
> > +            unsigned int i, cpu;
> > +            int socket = sysctl->u.pqos_monitor_op.data;
> 
> unsigned int

Okay.

> 
> > +
> > +            for ( i = 0; i < NR_CPUS; i++ )
> 
> Why NR_CPUS when a few lines down you show that you know of
> nr_cpu_ids?

Okay.

> 
> > +            {
> > +                if ( cpu_to_socket(i) < 0 || cpu_to_socket(i) != socket )
> > +                    continue;
> > +                cpu = cpumask_any(per_cpu(cpu_core_mask, i));
> > +                if ( cpu < nr_cpu_ids )
> > +                {
> > +                    sysctl->u.pqos_monitor_op.data = cpu;
> > +                    break;
> > +                }
> > +            }
> > +
> > +            if ( i == NR_CPUS )
> > +                ret = -EFAULT;
> 
> -EFAULT?

Here the intension is to tell we failed to select a CPU in this socket.
What errno do you prefer more?

> 
> > +        }
> > +        break;
> > +
> > +        default:
> > +            sysctl->u.pqos_monitor_op.data = 0;
> > +            ret = -ENOSYS;
> > +            break;
> > +        }
> > +        copyback = 1;
> > +    break;
> 
> Indentation.

Okay.

> 
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -659,6 +659,18 @@ struct msr_access_info {
> >      xen_sysctl_msr_data_t *msr_data;
> >  };
> >
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_total_rmid            0
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_upscaling_factor   1
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_l3_cache_size         2
> > +#define XEN_SYSCTL_PQOS_MONITOR_get_socket_cpu            3
> > +#define XEN_SYSCTL_PQOS_MONITOR_cqm_enabled               4
> > +struct xen_sysctl_pqos_monitor_op {
> > +    uint32_t cmd;
> > +    uint64_t data;
> 
> Missing explicit padding between the two fields, or need to use
> uint64_aligned_t.

Okay.

Thanks,
Dongxiao

> 
> Jan


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