Re: [Xen-devel] [PATCH v12 08/23] x86: refactor psr: L3 CAT: set value: implement framework.

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/29/17 7:12 AM >>>
>On 17-06-28 05:43:58, Jan Beulich wrote:
>> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/28/17 11:10 AM >>>
>> >On 17-06-28 01:14:59, Jan Beulich wrote:
>> >> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:25 AM >>>
>> >> > @@ -537,7 +556,16 @@ int psr_get_val(struct domain *d, unsigned int 
>> >> > socket,
>> >> >          return -ENOENT;
>> >> >      }
>> >> >  
>> >> > +    domain_lock(d);
>> >> > +    if ( !test_bit(d->domain_id, socket_info[socket].dom_set) )
>> >> > +    {
>> >> > +        d->arch.psr_cos_ids[socket] = 0;
>> >> > +        set_bit(d->domain_id, socket_info[socket].dom_set);
>> >> > +    }
>> >> 
>> >> Any reason not to use test_and_set_bit() here? I.e. is this on any hot 
>> >> path?
>> >> Or wait - I think it's even wrong to split the test from the set, as the 
>> >> lock
>> >> doesn't protect dom_set[].
>> 
>> With the last sentence here (which I had added after having written all of 
>> the
>> rest of the reply, I'm afraid I've managed to confuse you:
>> 
>> >>> 
>> >>Will change it to test_and_set_bit.
>> >...
>> >> > +    /*
>> >> > +     * Step 6:
>> >> > +     * Save the COS ID into current domain's psr_cos_ids[] so that we 
>> >> > can know
>> >> > +     * which COS the domain is using on the socket. One domain can 
>> >> > only use
>> >> > +     * one COS ID at same time on each socket.
>> >> > +     */
>> >> > +    domain_lock(d);
>> >> > +    d->arch.psr_cos_ids[socket] = cos;
>> >> > +    domain_unlock(d);
>> >> > +
>> >> > +    /*
>> >> > +     * Step 7:
>> >> > +     * Then, set the dom_set bit which corresponds to domain_id to 
>> >> > mark this
>> >> > +     * domain has been set and the COS ID of the domain is valid.
>> >> > +     */
>> >> > +    set_bit(d->domain_id, info->dom_set);
>> >> 
>> >> With the way things are being done above, doesn't this belong in the
>> >> domain_lock()-ed region?
>> 
>> I should have deleted this, since - as said above - the lock doesn't guard
>> against anything dom_set[]-wise. So ...
>> 
>> >Yes, should be. Thanks!
>> 
>> ... I think you rather shouldn't do this. Instead you may want to consider 
>> whether
>> the other domain_lock()-ed regions couldn't be further shrunk.
>> 
>I want to confirm below two points with you:
>1. remove this 'set_bit' here if above 'test_bit' is replaced to
   >'test_and_set_bit'.

I don't think so, at least not for the ones still visible in context here. I've 
only
suggested to convert test/set pairs into test_and_set. The one at step 7 doesn't
have a test next to it, so either it was redundant with some other set (in which
case it should indeed be dropped), or it needs to stay as is.

>2. For the 'be further shrunk', I think the 'domain_lock' above 'set_bit' can 
>be
   >removed if 'test_and_set_bit' is used.

I don't think it can be removed altogether, but I think it could be moved into 
the
body of the if().

Jan


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