Re: [PATCH RESEND v5 04/24] x86: refactor psr: implement CPU init and free flow.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jan 31, 2017 at 03:14:25AM -0700, Jan Beulich wrote:
> >>> On 31.01.17 at 03:44, <konrad.wilk@oracle.com> wrote:
> > On Thu, Jan 19, 2017 at 02:01:06PM +0800, Yi Sun wrote:
> >> @@ -127,6 +130,13 @@ struct feat_node {
> >>      struct list_head list;
> >>  };
> >>  
> >> +struct cpuid_leaf_regs {
> >> +    unsigned int eax;
> >> +    unsigned int ebx;
> >> +    unsigned int ecx;
> >> +    unsigned int edx;
> >> +};
> > 
> > Could you use 'struct cpuid_leaf' in x86_emulate.h ?
> > 
> > It would only require "#include <asm/x86_emulate.h>" I believe.
> 
> Indeed - I recall specifically having asked for that.
> 
> >>  static int psr_cpu_prepare(unsigned int cpu)
> >>  {
> >> +    if ( !socket_info )
> >> +        return 0;
> >> +
> >> +    /* Malloc memory for the global feature head here. */
> >> +    if ( feat_l3_cat == NULL &&
> >> +         (feat_l3_cat = xzalloc(struct feat_node)) == NULL )
> > 
> > /me scratches his head.
> > 
> > Lets say we have a two socket machine. Each socket has
> > two CPUs (so total of 4 CPUs).
> > 
> > On CPU0  (in psr_presmp_init) it allocates feat_l3_cat. Then
> > later in (in psr_presmp_init) you call psr_cpu_init which
> > sets feat_l3_cat to NULL (and 'feat' is feat_l3_cat).
> > 
> > When CPU1 starts up then, psr_cpu_prepare is called
> > which means we allocate a new feat_l3_cat. 'cpu_init_work'
> > short-circuits (as info->feat_mask has a value) and does not
> > do anything.
> > 
> > When CPU2 (socket two), starts up then feat_l3_cat (as 'feat')
> > is used (in psr_cpu_init and again sets feat_l3_cat to NULL).
> > 
> > The last CPU3 (socket #2) starts up, and since feat_l3_cat is
> > NULL - it is allocated. But in 'cpu_init_work' it short-circuits
> > so we don't use third allocated 'feat_l3_cat' .
> > 
> > And we have an 'feat_l3_cat' that is not used for anything..
> > 
> > In other words - a memory leak. Granted a very small one.
> 
> I did recommend to do it that way, to simplify things a little.

Aha! In which case perhaps a little comment saying that it is
OK to have this one memory leak to make the code simpler.


> 
> Jan
> 

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