Re: [PATCH v4 03/24] x86: refactor psr: implement main data structures.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.01.17 at 09:49, <yi.y.sun@linux.intel.com> wrote:
> On 17-01-03 01:00:37, Jan Beulich wrote:
>> >>> On 26.12.16 at 07:56, <yi.y.sun@linux.intel.com> wrote:
>> > On 16-12-22 09:13:43, Jan Beulich wrote:
>> >> >>> On 14.12.16 at 05:07, <yi.y.sun@linux.intel.com> wrote:
>> >> > +struct feat_node;
>> >> > +
>> >> > +/*
>> >> > + * This structure defines feature operation callback functions. Every feature
>> >> > + * enabled MUST implement such callback functions and register them to ops.
>> >> > + *
>> >> > + * Feature specific behaviors will be encapsulated into these callback
>> >> > + * functions. Then, the main flows will not be changed when introducing a new
>> >> > + * feature.
>> >> > + */
>> >> > +struct feat_ops {
>> >> > +    /*
>> >> > +     * init_feature is used in cpu initialization process to do feature
>> >> > +     * specific initialization works.
>> >> > +     */
>> >> > +    void (*init_feature)(unsigned int eax, unsigned int ebx,
>> >> > +                         unsigned int ecx, unsigned int edx,
>> >> > +                         struct feat_node *feat,
>> >> > +                         struct psr_socket_info *info);
>> >> > +};
>> >> 
>> >> What is the reason to have a separate structure for this, when you
>> >> don't store a pointer in struct feat_node? If this was inlined there,
>> >> the odd forward declaration of struct feat_node wouldn't be needed
>> >> either. (The same question may apply to struct feat_hw_info.) 
>> >> 
>> > I just want to make codes be clear. If you prefer inline declaration, I 
>> > think I
>> > should change it as below, right?
>> > 
>> > struct feat_node {
>> > ......
>> >     struct feat_ops {
>> >         ......
>> >     } ops;
>> >     struct feat_hw_info {
>> >         ......
>> >     } info;
>> > ......
>> > };
>> 
>> Well, not exactly: The struct <tag> { ... } <name>; wrappers
>> are unnecessary then too. With them kept there indeed would be
>> no big difference between both variants.
>> 
> To facilitate the callback functions assignment for a feature, I defined
> feature specific callback function ops like below.
> 
> struct feat_ops l3_cat_ops = {
>     .init_feature = l3_cat_init_feature,
>     .get_max_cos_max = l3_cat_get_max_cos_max,
>     ......
> };
> 
> And directly assign it to global feature node in initialization process like
> below.
> 
> static void cpu_init_work(void)
> {
> ......
>             feat_tmp = feat_l3_cat;
>             feat_l3_cat = NULL;
>             feat_tmp->ops = l3_cat_ops;
> ......
> }
> 
> I think this can make codes be clear. How do you think? Is this way 
> acceptable?

Yes.

Jan


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