Re: [PATCH v3 4/8] x86: add support for COS/CBM manangement

[Chao Peng <chao.p.peng@linux.intel.com>]

On Fri, Mar 27, 2015 at 08:16:10PM +0000, Andrew Cooper wrote:
> On 26/03/15 12:38, Chao Peng wrote:
> > @@ -238,6 +450,8 @@ static void cat_cpu_init(unsigned int cpu)
> >      ASSERT(socket < nr_sockets);
> >  
> >      info = cat_socket_info + socket;
> > +    if ( info->socket_cpu_mask == NULL )
> > +        info->socket_cpu_mask = per_cpu(cpu_core_mask, cpu);
> 
> Surely this wants to be skipped if info is already initialised?

The mask will be set to NULL in psr_cpu_fini() when the last CPU of the
socket is hot un-plugged. If any CPU of the socket is plugged in again
then the mask needs to be initialized but not skip.
> 
> >  
> >      /* Avoid initializing more than one times for the same socket. */
> >      if ( test_and_set_bool(info->initialized) )
> > @@ -254,6 +468,14 @@ static void cat_cpu_init(unsigned int cpu)
> >          info->cbm_len = (eax & 0x1f) + 1;
> >          info->cos_max = (edx & 0xffff);
> >  
> > +        info->cos_cbm_map = xzalloc_array(struct psr_cat_cbm,
> > +                                          info->cos_max + 1UL);
> > +        if ( !info->cos_cbm_map )
> > +            return;
> 
> This indicates that cat_cpu_init() needs to be able to signal ENOMEM.

The current behavior for ENOMEM case is: CAT is not enabled on the
socket. Both cat_cpu_init() and the caller will not actually make use
of this error code. 
> 
> > +
> > +        /* cos=0 is reserved as default cbm(all ones). */
> > +        info->cos_cbm_map[0].cbm = (1ull << info->cbm_len) - 1;
> > +
> >          info->enabled = 1;
> >          printk(XENLOG_DEBUG "CAT: enabled on socket %u, cos_max:%u, cbm_len:%u\n",
> >                 socket, info->cos_max, info->cbm_len);
> > @@ -274,6 +496,24 @@ static void psr_cpu_init(unsigned int cpu)
> >      psr_assoc_init();
> >  }
> >  
> > +static void psr_cpu_fini(unsigned int cpu)
> > +{
> > +    unsigned int socket, next;
> > +    cpumask_t *cpu_mask;
> > +
> > +    if ( cat_socket_info )
> > +    {
> > +        socket = cpu_to_socket(cpu);
> > +        cpu_mask = cat_socket_info[socket].socket_cpu_mask;
> > +
> > +        if ( (next = cpumask_cycle(cpu, cpu_mask)) == cpu )
> > +            cat_socket_info[socket].socket_cpu_mask = NULL;
> > +        else
> > +            cat_socket_info[socket].socket_cpu_mask =
> > +                                    per_cpu(cpu_core_mask, next);
> > +    }
> > +}
> > +
> Overall, this patch has a lot of moving parts in it.  I have not spotted
> any major problems, but I also don't feel confident that I understand
> all of what is going on.
> 
> It would certainly be easier to review if you split the patch into at
> least 3; the core infrastructure, the domctl and the sysctl bits.  Even
> then, there appear to be several different bits of core changes going
> on, with some per-socket infrastructure and per-domain infrastructure.

OK, I will split it.

> 
> The code itself appears to attempt to deal with sockets having a
> different quantity of COS entries, but how does it resolve having a
> different number of entries in the COS bitmaps?  This would appear to
> mean that a domain given a certain COS would exhibit different behaviour
> depending on which socket it happened to be scheduled on.

Hypervisor maintains per-socket COS bitmask for each domain as well. So
a domain actually has COS bitmask for each socket but not only one COS
bitmask. xen_domctl_psr_cat_op interface allows toolstack to set/get
each COS bitmask on socket basis.

Chao