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

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-06-29 00:24:50, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/29/17 7:12 AM >>>
> >On 17-06-28 05:43:58, Jan Beulich wrote:
> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 06/28/17 11:10 AM >>>
> >> >On 17-06-28 01:14:59, Jan Beulich wrote:
> >> >> >>> Yi Sun <yi.y.sun@linux.intel.com> 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.
> 
For the 'set_bit' at Step 7, it is redundant because the bit has been set anyway
when entering 'psr_set_val' if we use 'test_and_set_bit' there. So, I think we
can drop Step 7.

> >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().
> 
I think we still need to keep the lock protection range in current codes. We
need the lock to protect the action to get 'cos id' too.

There is a scenario to explain this: if the domain's bit in dom_set has been
cleared, 'psr_get_val' and 'psr_set_val' are called almost at same time, but
'psr_get_val' is a little bit eariler. It sets dom_set bit to 1 firstly. At
that time, 'psr_set_val' checks the bit and finds it has been set, it goes to
next instruction to get old_cos. But the 'psr_get_val' may not restore the
cos id to 0 yet. So, the old_cos got in 'psr_set_val' is wrong. To avoid
this, I think should use current codes to protect the whole range.

psr_get_val()                                psr_set_val()
    //old bit is 0, enter statement.
    if (!test_and_set_bit())
    {
                                                 //old bit is 1, skip statement.
                                                 if (!test_and_set_bit())
                                                 old_cos = d->arch.psr_cos_ids[socket];
        domain_lock();
        d->arch.psr_cos_ids[socket] = 0;
        domain_unlock();
    }

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