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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-07-12 23:20:24, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 07/13/17 5:00 AM >>>
> >On 17-07-12 13:37:02, Jan Beulich wrote:
> >> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 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?
> 
> Oh, I see. But yes, I still think it would be nice for const-ness to be
> expressible irrespective of this helper function, so making it e.g. just 
> update
> "len" without passing in the array pointer at all (leaving that part to the 
> caller)
> would seem desirable. Or possibly not even pass "array_len" via indirection,
> instead making the function return a non-negative increment value for the
> caller to apply to both (keeping negative value to indicate errors). But if 
> you
> think it's better the way it is, I could also live with it.
> 
Thank you! I will try to implement a version out according to your comments.

> >> >+    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.
> 
> Well, as said - whether it's worth the extra effort depends on whether there 
> is
> a (reasonable) scenario where this function may be executed frequently.
> 
This function is executed when 'psr-cat-set' command is executed. I consult
the libvirt guy, this command may be executed frequently under some scenarios.
E.g. user may dynamically adjust the cache allocation for VMs according to CMT
result.

> >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;
> >};
> 
> As you can likely understand, I can't really judge on this without seeing what
> you need this for. So I'd suggest to keep things the way they are in this 
> series
> and discuss changes to it in the context of that other series of yours.
> 
Ok, I will keep the codes in current series. Will modify them in MBA patch set
for review.

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