Re: [Xen-devel] [PATCH v9 05/25] x86: refactor psr: L3 CAT: implement CPU init and free flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.17 at 10:16, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-27 00:34:29, Jan Beulich wrote:
>> >>> On 27.03.17 at 06:41, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-03-24 10:52:34, Jan Beulich wrote:
>> >> >>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> > @@ -46,6 +50,9 @@
>> >> >   */
>> >> >  #define MAX_COS_REG_CNT  128
>> >> >  
>> >> > +/* CAT features use 1 COS register in one access. */
>> >> > +#define CAT_COS_NUM      1
>> >> 
>> >> With it being stored into the feature node now I don't see why you
>> >> need this constant anymore. And indeed it's being used exactly
>> >> once.
>> >> 
>> > I remember somebody suggested me not to use constant but should define a
>> > macro. As it is only used once, I will remove this and 'CDP_COS_NUM' in
>> > later patch.
>> 
>> It may well have been me, back when this was used in multiple places.
>> 
> Ok, I got it. Will remove such macros.
> 
>> >> > +/*
>> >> > + * Use this function to check if any allocation feature has been 
>> >> > enabled
>> >> > + * in cmdline.
>> >> > + */
>> >> > +static bool psr_alloc_feat_enabled(void)
>> >> > +{
>> >> > +    return ((!socket_info) ? false : true );
>> >> 
>> >> Stray parentheses (all of them actually) and blank. Even more, why
>> >> not simply
>> >> 
>> >>     return socket_info;
>> >> 
>> >> ?
>> >> 
>> > How about 'return !!socket_info'?
>> 
>> And what would the !! be good for? Back when we were still using
>> bool_t that would have been a requirement (the code wouldn't
>> even have built without afaict), but now that we use bool I don't
>> see the point (other that cluttering code). In fact I consider the
>> presence of the function questionable as a whole, unless later
>> patches add to it.
>> 
> Per Wei's suggestion, I added this function to make readers clearly 
> understand
> the meaning of the code. In previous codes, we just check 'if ( !socket_info 
> )'.
> 
> Per test, 'return socket_info' causes warning if function type is 'bool'.

Oh, that is unfortunate (and then indeed requires to use !!).
I would have expected that conversion here works just like in
if(), where no !! would be needed.

>> >> > +                             struct feat_node *feat,
>> >> > +                             struct psr_socket_info *info,
>> >> > +                             enum psr_feat_type type)
>> >> > +{
>> >> > +    unsigned int socket, i;
>> >> > +    struct psr_cat_hw_info cat = { };
>> >> > +    uint64_t val;
>> >> > +
>> >> > +    /* No valid value so do not enable feature. */
>> >> > +    if ( !regs.a || !regs.d )
>> >> > +        return;
>> >> > +
>> >> > +    cat.cbm_len = (regs.a & CAT_CBM_LEN_MASK) + 1;
>> >> > +    cat.cos_max = min(opt_cos_max, regs.d & CAT_COS_MAX_MASK);
>> >> > +
>> >> > +    /* cos=0 is reserved as default cbm(all bits within cbm_len are 
>> >> > 1). */
>> >> > +    feat->cos_reg_val[0] = cat_default_val(cat.cbm_len);
>> >> > +    /*
>> >> > +     * To handle cpu offline and then online case, we need read MSRs 
>> >> > back to
>> >> > +     * save values into cos_reg_val array.
>> >> > +     */
>> >> > +    for ( i = 1; i <= cat.cos_max; i++ )
>> >> > +    {
>> >> > +        rdmsrl(MSR_IA32_PSR_L3_MASK(i), val);
>> >> > +        feat->cos_reg_val[i] = (uint32_t)val;
>> >> > +    }
>> >> 
>> >> You mention this in the changes done, but I don't understand why
>> >> you do this. What meaning to these values have to you? If you
>> >> want hardware and cached values to match up, the much more
>> >> conventional way of enforcing this would be to write the values
>> >> you actually want (normally all zero).
>> >> 
>> > When all cpus on a socket are offline, the free_feature() is called to free
>> > features resources so that the values saved in cos_reg_val[] are lost. 
>> > When the
>> > socket is online again, features are allocated again so that cos_reg_val[]
>> > members are all initialized to 0. Only is cos_reg_val[0] initialized to 
>> > default
>> > value in this function in old codes.
>> > 
>> > But domain is still alive so that its cos id on the socket is kept. The
>> > corresponding MSR value is kept too per test. To make cos_reg_val[] values 
>> > be
>> > same as HW to not to mislead user, we should read back the valid values on 
>> > HW
>> > into cos_reg_val[].
>> 
>> Okay, I understand the background, but I don't view this solution
>> as viable: Once the last core on a socket goes offline, all
>> references to it should be cleaned up. After all what will be
>> brought back online may be a different physical CPU altogether;
>> you can't assume MSR values to have survived even if it is the
>> same CPU which comes back online, as it may have undergone
>> a reset cycle, or BIOS/SMM may have played with the MSRs.
>> That's even a possibility for a single core coming back online, so
>> you have to reload MSRs explicitly anyway if implicit reloading
>> (i.e. once vCPU-s get scheduled onto it) doesn't suffice.
>> 
> So, you think the MSRs values may not be valid after such process and
> reloading (write MSRs to default value) is needed. If so, I would like
> to do more operations in 'free_feature()':
> 1. Iterate all domains working on the offline socket to change
>    'd->arch.psr_cos_ids[socket]' to COS 0, i.e restore it back to init
>    status.
> 2. Restore 'socket_info[socket].cos_ref[]' to all 0.
> 
> These can make the socket's info be totally restored back to init status.

Yes, that's what I think is needed.

>> >> > +/* L3 CAT ops */
>> >> > +static const struct feat_ops l3_cat_ops = {
>> >> > +};
>> >> 
>> >> Leaving an already declared function pointer as NULL? Please don't.
>> >> 
>> > Ok, will consider to move it and below code into later patch.
>> >     feat->ops = l3_cat_ops;
>> 
>> I don't mind the empty structure instance above, as long as the
>> structure doesn't have any function pointer members yet (data
>> members are almost always fine).
>> 
> To explain how the data structures are, I declared '(*get_cos_max)' in
> 'struct feat_ops' in patch 3. So, do you mind I remove this declaration
> and just keep an empty 'struct feat_ops' in patch 3 so that we can keep
> current codes in this patch?

As said, I have no problem with the structure remaining empty
until subsequent patches start filling it. No need to re-structure
several patches.

Jan

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