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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

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'.
2. For the 'be further shrunk', I think the 'domain_lock' above 'set_bit' can be
   removed if 'test_and_set_bit' is used.

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