Re: [PATCH v4 05/12] x86: maintain socket CPU mask for CAT

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

On Thu, Apr 09, 2015 at 10:45:41PM +0100, Andrew Cooper wrote:
> On 09/04/2015 10:18, Chao Peng wrote:
> > Some CAT resource/registers exist in socket level and they must be
> > accessed from the CPU of the corresponding socket. It's common to pick
> > an arbitrary CPU from the socket. To make the picking easy, it's useful
> > to maintain a reference to the cpu_core_mask which contains all the
> > siblings of a CPU in the same socket. The reference needs to be
> > synchronized with the CPU up/down.
> >
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > ---
> >  xen/arch/x86/psr.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
> > index 4aff5f6..7de2504 100644
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -32,6 +32,7 @@ struct psr_cat_socket_info {
> >      unsigned int cbm_len;
> >      unsigned int cos_max;
> >      struct psr_cat_cbm *cos_cbm_map;
> > +    cpumask_t *socket_cpu_mask;
> 
> At a pinch, you could get away with just "cpus" as a variable name, as
> it is part of a structure, and instanced in an array, with "socket" in
> the name.

I like 'cpus' either.

> 
> >  };
> >  
> >  struct psr_assoc {
> > @@ -234,6 +235,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 after the test_and_set_bool() ?

OK.

> 
> >  
> >      /* Avoid initializing more than one times for the same socket. */
> >      if ( test_and_set_bool(info->initialized) )
> > @@ -274,6 +277,24 @@ static void psr_cpu_init(unsigned int cpu)
> >      psr_assoc_init(cpu);
> >  }
> >  
> > +static void psr_cpu_fini(unsigned int cpu)
> 
> cat_cpu_fini() to mirror cat_cpu_init() or perhaps both?

How about: Change this one to cat_cpu_fini() and add helper
psr_cpu_fini() to call cat_cpu_fini()?

> 
> > +{
> > +    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);
> 
> Might it be easier to copy cpu_core_mask rather than playing these games
> to avoid pointing into a stale per_cpu() area?

I didn't quite catch up with you on this. Even with copied
cpu_core_mask, we still need to sync it with cpu online/offline.
Otherwise its value may not be correct. The key point here, I think,
is: We should always trust per_cpu(cpu_core_mask, cpu) in a cpu's life
cycle, Otherwise copy or point to it should both be wrong.

Chao