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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.15 at 10:11, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> On Mon, Mar 16, 2015 at 05:10:32PM +0000, Jan Beulich wrote:
>> >>> On 13.03.15 at 11:13, <chao.p.peng@xxxxxxxxxxxxxxx> 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.

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

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

Jan


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