Re: [Xen-devel] [PATCH v8 07/24] x86: refactor psr: implement get value flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.17 at 02:50, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-08 08:35:53, Jan Beulich wrote:
>> >>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > @@ -498,6 +516,15 @@ int psr_get_info(unsigned int socket, enum cbm_type 
>> > type,
>> >          if ( feat->feature != feat_type )
>> >              continue;
>> >  
>> > +        if ( d )
>> > +        {
>> > +            cos = d->arch.psr_cos_ids[socket];
>> > +            if ( feat->ops.get_val(feat, cos, type, val) )
>> > +                return 0;
>> > +            else
>> > +                break;
>> > +        }
>> > +
>> >          if ( feat->ops.get_feat_info(feat, data, array_len) )
>> >              return 0;
>> >          else
>> 
>> Looking at the context here - is it really a good idea to overload the
>> function in this way, rather than creating a second one? Your
>> only complicating the live of the callers, as can be seen e.g. ...
>> 
> These codes were separated into two functions before, 'psr_get_info' and
> 'psr_get_val'. But there are some common codes. So, Konrad suggested me
> to create a helper function to reduce redundant codes.

I think you went too far - breaking out common code is nice, but if
the two callers now pass NULL/0 each as two of the last four
arguments, this is a clear indication that you've folded more code
than was actually common.

>> > @@ -507,10 +534,16 @@ int psr_get_info(unsigned int socket, enum cbm_type 
> type,
>> >      return -ENOENT;
>> >  }
>> >  
>> > -int psr_get_l3_cbm(struct domain *d, unsigned int socket,
>> > -                   uint64_t *cbm, enum cbm_type type)
>> > +int psr_get_info(unsigned int socket, enum cbm_type type,
>> > +                 uint32_t data[], unsigned int array_len)
>> > +{
>> > +    return psr_get(socket, type, data, array_len, NULL, NULL);
>> 
>> ... here and ...
>> 
>> > +}
>> > +
>> > +int psr_get_val(struct domain *d, unsigned int socket,
>> > +                uint64_t *val, enum cbm_type type)
>> >  {
>> > -    return 0;
>> > +    return psr_get(socket, type, NULL, 0, d, val);
>> >  }
>> 
>> ... here (it is a bad sign that both pass NULL on either side).
>> 
> Yes, these things look not good. But to keep a common helper I have to pass
> all necessary parameters into it. What is your suggestion?

If there's really that much code to be shared (which I continue not
to be convinced of), use of a function pointer may make this look a
little better. But I think when having tried to carry out the earlier
suggestion, you should have responded back right away indicating
this would result in other ugliness.

Jan


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