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

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

On Tue, May 19, 2015 at 11:22:54AM +0100, Jan Beulich wrote:
> >>> 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).

Do you want to try the failure initialization(both because CAT is not
even supported on the machine or fails to allocate cos_to_cbm) on and
on?

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

Yes, CPU_DEAD.

> 
> 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).

I know this is the general rules, but for this case, I don't think it's
going to work to allocate memory in CPU_UP_PREPARE while do the
initialization later(in CPU_STARTING).

As said, the allocation has dependency on the result of the
initialization. E.g. before

info->cos_to_cbm = xzalloc_array(struct psr_cat_cbm,
                                 info->cos_max + 1UL);

info->cos_max must have been known.

Chao

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