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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> 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.

>> >+    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.

>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.

Jan


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