Re: [Xen-devel] [PATCH 1/6] x86: detect and initialize Intel CAT feature

[Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>]

On Mon, Mar 16, 2015 at 01:47:06PM +0000, Jan Beulich wrote:
> >>> On 13.03.15 at 11:13, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> > @@ -1112,6 +1117,12 @@ The following resources are available:
> >    total/local memory bandwidth. Follow the same options with Cache 
> > Monitoring
> >    Technology.
> >  
> > +* Cache Alllocation Technology (Broadwell and later).  Information 
> > regarding
> > +  the cache allocation.
> > +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> > +  * `socket_num` indicates socket number. Detecte automatically at boot 
> > time
> > +    if not specified(0). Useful for CPU hot-plug case.
> 
> While saying something, at least for me what is being said doesn't at
> all make clear what "socket number" here is: The number of a specific
> socket? The number of sockets? Yet something else? Without knowing
> what it means by _just_ reading this description, people won't know
> what to use the command line option for.

I agree, the name is a little confusing. Or perhaps: max_socket? E.g.

`max_socket` indicates the maximum number of available sockets for CAT
feature detection. All the sockets up to max_socket will be checked for
CAT feature. The value is normally detected at boot time automatically
if not specified(0). While the value may need to be specified manually
for CPU hot-plug scenario in which case the maximum socket number detected
at boot time may be not correct.

> 
> > -static void __init init_psr_cmt(unsigned int rmid_max)
> > +static void __init psr_cmt_init(unsigned int rmid_max)
> 
> Is this renaming really an integral part of this patch?

OK, I will move it to separate one or just keep it unchanged.

> 
> > +        on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, info, 0);
> 
> Hmm, using an IPI here seems odd. Is there a reason to can't hook
> this onto CPU_STARTING instead of CPU_ONLINE?

Looks like CPU_STARTING is what I need, IPI then is unnecessary.

> 
> > +static unsigned int get_max_socket(void)
> > +{
> > +    unsigned int cpu, max_apicid = boot_cpu_physical_apicid;
> > +
> > +    for_each_present_cpu(cpu)
> > +        if (max_apicid < x86_cpu_to_apicid[cpu])
> 
> Coding style.
> 
> > +            max_apicid = x86_cpu_to_apicid[cpu];
> > +
> > +    return apicid_to_socket(max_apicid);
> 
> Since when is the socket with the highest numbered APIC ID
> the highest numbered socket? I think the whole function needs
> to act on socket numbers only.

Then perhaps looping cpus and checking against cpu_to_socket(cpu)
directly make sense.

Thanks Jan.
Chao

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