Re: [Xen-devel] [PATCH v7 03/14] x86: detect and initialize Intel CAT feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.15 at 11:33, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> On Tue, May 19, 2015 at 09:42:07AM +0100, Jan Beulich wrote:
>> >>> On 19.05.15 at 09:40, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
>> > On Mon, May 18, 2015 at 02:33:51PM +0100, Jan Beulich wrote:
>> >> >>> On 08.05.15 at 10:56, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
>> >> > +static unsigned long *__read_mostly cat_socket_init_bitmap;
>> >> > +static unsigned long *__read_mostly cat_socket_enable_bitmap;
>> >> 
>> >> Didn't we agree to fold these two into one? Apart from that the
>> >> _bitmap name tag doesn't seem very useful...
>> > 
>> > Looks like this?
>> > 
>> > static unsigned long *__read_mostly cat_socket_init,
>> >                      *__read_mostly cat_socket_enable;
>> 
>> Yes, except that you will want to drop one (or make clear why you
>> need both).
> 
> As said in the previous version, cat_socket_enable is the actual CAT
> enabled status.
> 
> And cat_socket_init is used for performance optimization purpose only.
> It indicate if the socket has been initialized, so that initialization
> happens only once for each socket, but not __cpus_in_socket__ times.
> 
> There are three possibilities:
> 1)  Not initialized;
> 2)  Initialized, CAT disabled;
> 3)  Initialized, CAT enabled;
> 
> So it's not possible to use only one bit to represent three values.

Why? The two bits get set together, and get cleared together. Even
if they have formally different meaning, there is - afaict - no case
where their values will differ (except for the brief period between
setting one and then the other).

>> >> >  static int cpu_callback(
>> >> >      struct notifier_block *nfb, unsigned long action, void *hcpu)
>> >> >  {
>> >> >      if ( action == CPU_STARTING )
>> >> >          psr_cpu_init();
>> >> > +    else if ( action == CPU_DYING )
>> >> > +        psr_cpu_fini();
>> >> 
>> >> Are these the right notifiers for doing things involving memory
>> >> allocation / freeing?
>> > 
>> > psr_cpu_fini() does not really have memory allocation/freeing but
>> > psr_cpu_init() does have.
>> 
>> Hmm, wait - where did I see this allocation? Looking again, I don't see
>> it now. But if there was one, then surely it would be wrong for _fini
>> to not free it.
>> 
>> > While one thing to clarify is: psr_cpu_init() will not propagate
>> > the error even when the memory allocation fails(instead it disables
>> > the CAT on the socket).
>> > 
>> > If there is still problem then perhaps I need change CPU_STARTING back
>> > to CPU_ONLINE and make on_selected_cpus() call on target cpu.
>> 
>> No, if there was any allocation needed, you should do the allocation
>> in CPU_UP_PREPARE and the initialization in CPU_STARTING. But it
>> looks like this is moot now anyway.
> 
> As I will add allocation in patch 4, so I will move psr_cpu_init() to
> CPU_UP_PREPARE(As the allocation length is based on the cpuid result, so
> the initialization will also be done here), but still need to move
> psr_cpu_fini(which will add the freeing code) to CPU_DOWN_PREPARE?

CPU_DEAD you mean.

But again - only the allocation/freeing should be done in the
alternative named notifications, not the actual data setup (as that
iirc needs to run on the CPU).

Jan


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