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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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