[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v13 12/23] x86: refactor psr: L3 CAT: set value: implement write msr flow.
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |