Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Yi Sun]

On 16-11-30 02:42:20, Jan Beulich wrote:
> >>> On 30.11.16 at 10:08,  wrote:
> > On 16-11-29 02:43:24, Jan Beulich wrote:
> >> >>> On 29.11.16 at 05:38,  wrote:
> >> > To make codes be better reviewable, I propose below three suggestions:
> >> > 1) I write a design document about refactor to make you more easily 
> >> > understand the ideas.
> >> 
> >> This is appreciated, but I don't think it's strictly necessary. Describing
> >> the new design (rather than the changes to the existing one) is what
> >> likely would be more useful (I'm sorry if I've misunderstood what you
> >> said, and you in fact had meant just this), which iirc you already have
> >> in patch 1.
> >> 
> >> > 2) Replace the psr.c with a new file which discards all old codes so
> >> > that you do not need to consider old implementations which may cause
> >> > confusion.
> >> > 3) Discard the refactor codes. Just implement L2 CAT based on current
> >> > codes. Because L2 CAT does not have much difference with L3.
> >> 
> >> I don't think introducing a new file is the ideal approach. I'd suggest
> >> to rip out the entire implementation in a first patch, leaving just
> >> empty functions to avoid breaking the build (i.e. perhaps mostly the
> >> ones used by domctl/sysctl, and maybe some init one). Then
> >> introduce new code, ideally of course not in one big patch, but
> >> broken up into logical pieces where possible (one such split would be
> >> that of course you don't need to re-implement domctl/sysctl handling
> >> in the same patch as everything else).
> >> 
> > Thanks for your suggestion! Just want to confirm if my understanding is
> > right. So, you mean I can remove all old codes but keep the interfaces as
> > empty functions to make sure the build can pass. Then, implement whole
> > functionality step by step. Right?
> 
> Yes.
> 
> Thanks, Jan

Got it. Thanks!

Sun Yi

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

Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Jan Beulich]

>>> On 30.11.16 at 10:08,  wrote:
> On 16-11-29 02:43:24, Jan Beulich wrote:
>> >>> On 29.11.16 at 05:38,  wrote:
>> > To make codes be better reviewable, I propose below three suggestions:
>> > 1) I write a design document about refactor to make you more easily 
>> > understand the ideas.
>> 
>> This is appreciated, but I don't think it's strictly necessary. Describing
>> the new design (rather than the changes to the existing one) is what
>> likely would be more useful (I'm sorry if I've misunderstood what you
>> said, and you in fact had meant just this), which iirc you already have
>> in patch 1.
>> 
>> > 2) Replace the psr.c with a new file which discards all old codes so
>> > that you do not need to consider old implementations which may cause
>> > confusion.
>> > 3) Discard the refactor codes. Just implement L2 CAT based on current
>> > codes. Because L2 CAT does not have much difference with L3.
>> 
>> I don't think introducing a new file is the ideal approach. I'd suggest
>> to rip out the entire implementation in a first patch, leaving just
>> empty functions to avoid breaking the build (i.e. perhaps mostly the
>> ones used by domctl/sysctl, and maybe some init one). Then
>> introduce new code, ideally of course not in one big patch, but
>> broken up into logical pieces where possible (one such split would be
>> that of course you don't need to re-implement domctl/sysctl handling
>> in the same patch as everything else).
>> 
> Thanks for your suggestion! Just want to confirm if my understanding is
> right. So, you mean I can remove all old codes but keep the interfaces as
> empty functions to make sure the build can pass. Then, implement whole
> functionality step by step. Right?

Yes.

Thanks, Jan


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

Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Yi Sun]

On 16-11-29 02:43:24, Jan Beulich wrote:
> >>> On 29.11.16 at 05:38,  wrote:
> > To make codes be better reviewable, I propose below three suggestions:
> > 1) I write a design document about refactor to make you more easily 
> > understand the ideas.
> 
> This is appreciated, but I don't think it's strictly necessary. Describing
> the new design (rather than the changes to the existing one) is what
> likely would be more useful (I'm sorry if I've misunderstood what you
> said, and you in fact had meant just this), which iirc you already have
> in patch 1.
> 
> > 2) Replace the psr.c with a new file which discards all old codes so
> > that you do not need to consider old implementations which may cause
> > confusion.
> > 3) Discard the refactor codes. Just implement L2 CAT based on current
> > codes. Because L2 CAT does not have much difference with L3.
> 
> I don't think introducing a new file is the ideal approach. I'd suggest
> to rip out the entire implementation in a first patch, leaving just
> empty functions to avoid breaking the build (i.e. perhaps mostly the
> ones used by domctl/sysctl, and maybe some init one). Then
> introduce new code, ideally of course not in one big patch, but
> broken up into logical pieces where possible (one such split would be
> that of course you don't need to re-implement domctl/sysctl handling
> in the same patch as everything else).
> 
> Jan
> 
Thanks for your suggestion! Just want to confirm if my understanding is
right. So, you mean I can remove all old codes but keep the interfaces as
empty functions to make sure the build can pass. Then, implement whole
functionality step by step. Right?

Thanks,
Sun Yi

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

Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Jan Beulich]

>>> On 29.11.16 at 05:38,  wrote:
> To make codes be better reviewable, I propose below three suggestions:
> 1) I write a design document about refactor to make you more easily 
> understand the ideas.

This is appreciated, but I don't think it's strictly necessary. Describing
the new design (rather than the changes to the existing one) is what
likely would be more useful (I'm sorry if I've misunderstood what you
said, and you in fact had meant just this), which iirc you already have
in patch 1.

> 2) Replace the psr.c with a new file which discards all old codes so
> that you do not need to consider old implementations which may cause
> confusion.
> 3) Discard the refactor codes. Just implement L2 CAT based on current
> codes. Because L2 CAT does not have much difference with L3.

I don't think introducing a new file is the ideal approach. I'd suggest
to rip out the entire implementation in a first patch, leaving just
empty functions to avoid breaking the build (i.e. perhaps mostly the
ones used by domctl/sysctl, and maybe some init one). Then
introduce new code, ideally of course not in one big patch, but
broken up into logical pieces where possible (one such split would be
that of course you don't need to re-implement domctl/sysctl handling
in the same patch as everything else).

Jan


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

Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Yi Sun]

On 16-11-25 09:57:31, Jan Beulich wrote:
> >>> On 25.11.16 at 17:27,  wrote:
>  On 25.10.16 at 05:40,  wrote:
> >> 'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info.
> >> So encapsulate them into 'struct psr_cat_hw_info'. If new
> >> feature is supported, we can define other structure to save
> >> its HW info.
> > 
> > Part of my problem following you here is that you talk about
> > cbm_max, but the code changes cos_max, which so far I had
> > understood would be a generic limit,
> 
> So I've gone and looked back at patch 1, where indeed you say
> the limits might differ. Which raises the question then what
> opt_cos_max is representing.
> 
> Having seen v1, v2, and v3 up to patch 5 I start wondering
> whether the whole current code wouldn't better be ripped out
> and then be replaced by something written from scratch. That's
> because the split, while having reduced individual patch size,
> doesn't really make the whole thing much better reviewable.
> And the original implementation apparently simply didn't have
> in mind how future additions on the hardware side could look
> like.
> 
> Thoughts?
> 
> Jan

Firstly, I want to say sorry that the V3 still does not make you
feel good. I planned to split the big patch based on data structure
changes to make you understand more easily.

The original implementation of psr.c does not consider much about
future features because there was only L3 CAT introduced and we do
not know there will be new features and how they work. That is the
reason I refactor the psr.c to make it be easy to add new features
by abstracting the common things.

To make codes be better reviewable, I propose below three suggestions:
1) I write a design document about refactor to make you more easily 
understand the ideas.
2) Replace the psr.c with a new file which discards all old codes so
that you do not need to consider old implementations which may cause
confusion.
3) Discard the refactor codes. Just implement L2 CAT based on current
codes. Because L2 CAT does not have much difference with L3.

How do you think? If you can offer your ideas to design and implement
the codes, that would be a great opportunity for me to learn! Thank you!

BRs,
Sun Yi

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

Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Jan Beulich]

>>> On 25.11.16 at 17:27,  wrote:
 On 25.10.16 at 05:40,  wrote:
>> 'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info.
>> So encapsulate them into 'struct psr_cat_hw_info'. If new
>> feature is supported, we can define other structure to save
>> its HW info.
> 
> Part of my problem following you here is that you talk about
> cbm_max, but the code changes cos_max, which so far I had
> understood would be a generic limit,

So I've gone and looked back at patch 1, where indeed you say
the limits might differ. Which raises the question then what
opt_cos_max is representing.

Having seen v1, v2, and v3 up to patch 5 I start wondering
whether the whole current code wouldn't better be ripped out
and then be replaced by something written from scratch. That's
because the split, while having reduced individual patch size,
doesn't really make the whole thing much better reviewable.
And the original implementation apparently simply didn't have
in mind how future additions on the hardware side could look
like.

Thoughts?

Jan


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

Re: [Xen-devel] [PATCH v3 04/15] x86: refactor psr: Encapsulate 'cbm_len' and 'cbm_max'

[Jan Beulich]

>>> On 25.10.16 at 05:40,  wrote:
> 'cbm_len' and 'cbm_max' are CAT/CDP specific feature HW info.
> So encapsulate them into 'struct psr_cat_hw_info'. If new
> feature is supported, we can define other structure to save
> its HW info.

Part of my problem following you here is that you talk about
cbm_max, but the code changes cos_max, which so far I had
understood would be a generic limit, which appears to be
supported by ...

> @@ -26,9 +26,14 @@
>  /* Per spec, the maximum COS register number is 128. */
>  #define MAX_COS_REG_NUM  128
>  
> -struct psr_cat_socket_info {
> +/* CAT/CDP HW info data structure. */
> +struct psr_cat_hw_info {
>  unsigned int cbm_len;
>  unsigned int cos_max;
> +};
> +
> +struct psr_cat_socket_info {
> +struct psr_cat_hw_info l3_info;
>  /*
>   * Store the values of COS registers:
>   * CAT uses 1 entry for one COS ID;

... you leaving this comment in place.

Btw., perhaps it would also help if psr.c had (near its beginning) a
glossary of the various acronyms. By having a way to quickly refresh
one's memory of what COS, CAT, CBM, and who know what else
stand for, it might be a little easier to reason about changes like this
one.

Jan


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