Re: [PATCH v13 12/23] x86: refactor psr: L3 CAT: set value: implement write msr flow.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-07-12 13:37:02, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@linux.intel.com> 07/06/17 4:07 AM >>>
> >v13:
>     >- use 'skip_prior_features'.
> >- add 'const' for some variables.
> 
> You didn't go quite far enough with this:
> 
> >+struct cos_write_info
> >+{
> >+    unsigned int cos;
> >+    struct feat_node *feature;
> >+    const uint32_t *val;
> 
> With this, ...
> 
> >static int write_psr_msrs(unsigned int socket, unsigned int cos,
>                            >uint32_t val[], unsigned int array_len,
>                            
> ... I can't see why this can't be const too. Of course that would then affect an
> earlier patch.
> 
The 'val' is input into 'skip_prior_features'. In 'skip_prior_features', there
is '*val += props->cos_num;' to change the value. So, I do not add 'const' here.
Of course, I can change the way to skip value array, e.g. using a variable as
index. Which one do you like?

> >enum psr_feat_type feat_type)
> >{
> >-    return -ENOENT;
> >+    int ret;
> >+    struct psr_socket_info *info = get_socket_info(socket);
> >+    struct cos_write_info data =
> >+    {
> >+        .cos = cos,
> >+        .feature = info->features[feat_type],
> >+        .props = feat_props[feat_type],
> >+    };
> >+
> >+    if ( cos > info->features[feat_type]->cos_max )
> >+        return -EINVAL;
> >+
> >+    /* Skip to the feature's value head. */
> >+    ret = skip_prior_features(&val, &array_len, feat_type);
> >+    if ( ret )
> >+        return ret;
> >+
> >+    if ( array_len < feat_props[feat_type]->cos_num )
> >+        return -ENOSPC;
> >+
> >+    data.val = val;
> >+
> >+    if ( socket == cpu_to_socket(smp_processor_id()) )
> >+        do_write_psr_msrs(&data);
> >+    else
> >+    {
> >+        unsigned int cpu = get_socket_cpu(socket);
> >+
> >+        if ( cpu >= nr_cpu_ids )
> >+            return -ENOTSOCK;
> >+        on_selected_cpus(cpumask_of(cpu), do_write_psr_msrs, &data, 1);
> 
> How frequent an operation can this be? Considering that the actual MSR write(s)
> in the handler is (are) conditional I wonder whether it wouldn't be worthwhile
> trying to avoid the IPI altogether, by pre-checking whether any write actually
> needs doing.
> 
Yes, I think I can check if the value to set is same as 'feat->cos_reg_val[cos]'
before calling IPI.

There is one more thing. During implementing MBA, I find there is an issue here.
The current codes in 'struct cos_write_info' and 'write_psr_msrs' only consider
one feature's value setting. In fact, we should consider to set all values in
'val' array to the MSRs with new cos id for all features.

So, the 'cos_write_info' should be something like below to input feature array
and props array to handle all features. Of course, we do not need skip value
array anymore.

struct cos_write_info
{
    unsigned int cos;
    struct feat_node **features;
    uint32_t *val;
    unsigned int array_len;
    const struct feat_props **props;
};

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

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