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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/04/15 08:33, Chao Peng wrote:
> 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@xxxxxxxxxxxxxxx>
>>> ---
>>>  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()?

Yes - looks best.

>
>>> +{
>>> +    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.

I suppose that this is all working around the fact that we don't
currently maintain "socket" state like we maintain node and core
information when cpus go offline/come online.

It would probably be best to fix that up properly, rather than to try
and retrofit part of into the CAT infrastructure.  I think you can get
away with simply introducing socket variants of "cpu_to_node" and
"node_to_cpumask".  The underlying cpumasks themselves need not change.

~Andrew

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