Re: [PATCH 2/6] x86: add support for COS/CBM manangement

[Chao Peng <chao.p.peng@linux.intel.com>]

On Tue, Mar 17, 2015 at 09:25:48AM +0000, Jan Beulich wrote:
> >>> On 17.03.15 at 10:11, <chao.p.peng@linux.intel.com> wrote:
> > On Mon, Mar 16, 2015 at 05:10:32PM +0000, Jan Beulich wrote:
> >> >>> On 13.03.15 at 11:13, <chao.p.peng@linux.intel.com> wrote:
> >> > +    else
> >> > +    {
> >> > +        unsigned int cpu = cpumask_check(get_socket_cpu(socket));
> >> 
> >> Isn't this going to trigger an assertion when the socket count got
> >> specified on the command line?
> > 
> > Yes, the assertion is needed in tools side anyway. Here the check seems
> > unnecessary.
> 
> No - if the get_socket_cpu() may return nr_cpu_ids, you need a
> check. Just not in the form of an assertion.

Right, error code can be returned.

> 
> >> > +    if( !d->arch.psr_cos_ids )
> >> > +        return;
> >> 
> >> Considering this check ...
> >> 
> >> > +    for ( socket = 0; socket < opt_socket_num; socket++)
> >> > +    {
> >> > +        cos = d->arch.psr_cos_ids[socket];
> >> > +        if ( cos == 0 )
> >> > +            continue;
> >> > +
> >> > +        map = cat_socket_info[socket].cos_cbm_map;
> >> > +        if ( map )
> >> > +            map[cos].ref--;
> >> > +    }
> >> > +
> >> > +    xfree(d->arch.psr_cos_ids);
> >> 
> >> ... I think you want to clear the pointer here.
> > 
> > This function is called by arch_domain_destroy() or the failure path of
> > arch_domain_create(). In both cases the domain structure will be freed
> > later so setting NULL is pointless.
> 
> But this is not visible from this function. The alternative to not
> clearing the pointer here would be to do the checks of the
> pointer in the callers, which then would make clear whether this
> is really only on error and domain destruction paths (and that
> hence the pointer can't be double freed).

Then I'd like to clear the pointer here, so that even in the future the
code will not surprise somebody.

> 
> >> > @@ -222,6 +422,17 @@ static void do_cat_cpu_init(void* data)
> >> >          info->cbm_len = (eax & 0x1f) + 1;
> >> 
> >> This means cbm_len <= 32. Why is cos_cbm_map[].cbm then a
> >> uint64_t?
> > 
> > Currently the cbm_len is EAX[4:0], 64 bits cbm here is for future
> > possible enhancement.
> 
> Unless the specification explicitly says that the field width in EAX
> may be extended in the future, please omit such preparations
> for possible future extensions - it is simply going to confuse
> readers (as it did for me already) trying to understand why the
> type of the field is what it is.

I will try to understand this internally, if 32 bit is enough, then it
will be used.

Thanks,
Chao