Re: [Xen-devel] [PATCH v10 09/25] x86: refactor psr: L3 CAT: set value: implement framework.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.04.17 at 04:14, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-04-19 03:00:29, Jan Beulich wrote:
>> >>> On 19.04.17 at 10:22, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-04-18 05:46:43, Jan Beulich wrote:
>> >> >>> On 18.04.17 at 12:55, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> > I made a test patch based on v10 and attached it in mail. Could you 
>> >> > please
>> >> > help to check it? Thanks!
>> >> 
>> >> This looks reasonable at the first glance, albeit I continue to be
>> >> unconvinced that this is the only (reasonable) way of solving the
> 
> Do you have any other suggestion on this? Thanks!

I'm sorry, but no. I'm already spending _much_ more time on this
series than I should be, considering its (afaic) relatively low
priority. I really have to ask you to properly think through both
the data layout and code structure, including considering
alternatives especially in places where you can _anticipate_
controversy.

>> >> problem. After all we don't have to go through similar hoops for
>> >> any other of the register state associated with a vCPU. There
>> > 
>> > Sorry, I do not understand your meaning clearly.
>> > 1. DYM the ASSOC register which is set in 'psr_ctxt_switch_to'? In this 
>> > patch,
>> >    we check 'dom_ids' array to know if the domain's cos id has not been 
>> > set but
>> >    its 'psr_cos_ids[socket]' already has a non zero value. This case means 
>> > the
>> >    socket offline has happened so that we have to restore the domain's
>> >    'psr_cos_ids[socket]' to default value 0 which points to default COS 
>> > MSR.
>> >    I think we have discussed this in previous mails and achieved agreement 
>> > on
>> >    such logic. 
>> > 2. DYM the COS MSRs? We have discussed this before. Per your comments, when
>> >    socket is online, the registers values may be modified by FW and others.
>> >    So, we have to restore them to default value which is being done in
>> >    'cat_init_feature'.
>> > 
>> > So, what is your exactly meaning here? Thanks!
>> 
>> Neither of the two. I'm comparing with COS/PSR-_unrelated_
>> handling of register state. After all there are other MSRs which
>> we need to put into the right state after a core comes online.
>> 
> For PSR feature, the 'cos_reg_val[]' of the feature is destroied when socket
> is offline. The values in it are all 0 when socket is online again. This 
> causes
> error when user shows the CBMs. So, we have two options when socket is 
> online:
> 1. Read COS MSRs values and save them into 'cos_reg_val[]'.
> 2. Restore COS MSRs values and 'cos_reg_val[]' values to default CBM.

This re-states what you want to do; it does not answer my question.
Along the lines of what you say, for example FS and GS base MSRs
come back as zero too after a socket has been (re-)onlined. We
don't need to go through any hoops there, nevertheless.

> Per our discussion on v9 patch 05, we decided to adopt option 2. Below are
> what we discussed. FYR.

Well, we decided option 2 is better than option 1. I'm still
unconvinced there's not a yet better alternative.

>> >> are a number of cosmetic issues, but commenting on an attached
>> >> (rather than inlined) patch is a little difficult.
>> >> 
>> > Sorry for that, I will directly send patch out next time.
>> > 
>> >> One remark regarding the locking though: Acquiring a lock in the
>> >> context switch path should be made as low risk of long stalls as
>> >> possible. Therefore you will want to consider using r/w locks
>> >> instead of spin locks here, which would allow parallelism on all
>> >> cores of a socket as long as COS IDs aren't being updated.
>> >> 
>> > In 'psr_ctxt_switch_to', I use the lock only to protect 'write' actions.
>> > So, I do not understand why read-write lock is better? Anything I don't
>> > consider? Please help to point out. Thanks!
>> 
>> Hmm, looking again I can see that r/w locks indeed may not help
>> here. However, what you say still doesn't look correct to me: You
>> acquire the lock depending on _only_ psra->cos_mask being non-
>> zero. This means that all cores on one socket are being serialized
>> here. Quite possibly all you need is for some of the checks done
>> inside the locked region to be replicated (but _not_ moved) to the
>> outer if(), to limit the number of times where the lock is to be
>> acquired.
>> 
> I think your suggestions is to check old_cos outer the lock region.

My suggestion is to check as much state as possible, to prevent
having to acquire the lock whenever possible.

> If it is not 0 which means the domain's cos id does not need restore
> to 0, we can directly set it into ASSOC register. That can avoid
> unnecessary lock. I will send out the test patch again to ask your
> help to provide review comments (you said there are also 'a number
> of cosmetics issues').

And I would hope you would try to eliminate some (if not all) yourself
first. After all you can easily go over your patch yourself, checking
for e.g. style violations.

Jan

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