[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 17-06-28 01:14:59, Jan Beulich wrote:
> >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 06/14/17 3:25 AM >>>
> > @@ -179,6 +182,10 @@ static void free_socket_resources(unsigned int socket)
> >      }
> >  
> >      info->feat_init = false;
> > +
> > +    memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int));
> > +
> > +    bitmap_zero(info->dom_set, DOMID_IDLE + 1);
> >  }
> 
> I can see the point of the latter (as you add the new structure member), but
> if the former is necessary, shouldn't it have been done in an earlier patch?
> Or should be field only be introduced here?
> 
You are right. I will move the cos_ref clearing into earlier patch.

> > @@ -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[].
> 
Will change it to test_and_set_bit.

> > +int psr_set_val(struct domain *d, unsigned int socket,
> > +                uint64_t new_val, enum cbm_type type)
> > +{
> > +    unsigned int old_cos, array_len;
> > +    int cos, ret;
> > +    unsigned int *ref;
> > +    uint32_t *val_array, val;
> > +    struct psr_socket_info *info = get_socket_info(socket);
> > +    enum psr_feat_type feat_type;
> > +
> > +    if ( IS_ERR(info) )
> > +        return PTR_ERR(info);
> > +
> > +    val = new_val;
> > +    if ( new_val != val )
> > +        return -EINVAL;
> > +
> > +    feat_type = psr_cbm_type_to_feat_type(type);
> > +    if ( feat_type >= ARRAY_SIZE(info->features) ||
> > +         !info->features[feat_type] )
> > +        return -ENOENT;
> > +
> > +    /*
> > +     * Step 0:
> > +     * old_cos means the COS ID current domain is using. By default, it is 
> > 0.
> > +     *
> > +     * For every COS ID, there is a reference count to record how many 
> > domains
> > +     * are using the COS register corresponding to this COS ID.
> > +     * - If ref[old_cos] is 0, that means this COS is not used by any 
> > domain.
> > +     * - If ref[old_cos] is 1, that means this COS is only used by current
> > +     *   domain.
> > +     * - If ref[old_cos] is more than 1, that mean multiple domains are 
> > using
> > +     *   this COS.
> > +     */
> > +    domain_lock(d);
> > +    if ( !test_bit(d->domain_id, info->dom_set) )
> > +    {
> > +        d->arch.psr_cos_ids[socket] = 0;
> > +        set_bit(d->domain_id, info->dom_set);
> > +    }
> 
> Same here.
> 
> > +    old_cos = d->arch.psr_cos_ids[socket];
> > +    domain_unlock(d);
> > +
> > +    ASSERT(old_cos < MAX_COS_REG_CNT);
> > +
> > +    ref = info->cos_ref;
> > +
> > +    /*
> > +     * Step 1:
> > +     * Gather a value array to store all features cos_reg_val[old_cos].
> > +     * And, set the input new val into array according to the feature's
> > +     * position in array.
> > +     */
> > +    array_len = get_cos_num(info);
> > +    val_array = xzalloc_array(uint32_t, array_len);
> > +    if ( !val_array )
> > +        return -ENOMEM;
> > +
> > +    if ( (ret = gather_val_array(val_array, array_len, info, old_cos)) != 
> > 0 )
> > +        goto free_array;
> > +
> > +    if ( (ret = insert_val_into_array(val_array, array_len, info,
> > +                                      feat_type, type, val)) != 0 )
> > +        goto free_array;
> > +
> > +    spin_lock(&info->ref_lock);
> > +
> > +    /*
> > +     * Step 2:
> > +     * Try to find if there is already a COS ID on which all features' 
> > values
> > +     * are same as the array. Then, we can reuse this COS ID.
> > +     */
> > +    cos = find_cos(val_array, array_len, feat_type, info);
> > +    if ( cos == old_cos )
> > +    {
> > +        ret = 0;
> > +        goto unlock_free_array;
> > +    }
> > +
> > +    /*
> > +     * Step 3:
> > +     * If fail to find, we need pick an available COS ID.
> > +     * In fact, only COS ID which ref is 1 or 0 can be picked for current
> > +     * domain. If old_cos is not 0 and its ref==1, that means only current
> > +     * domain is using this old_cos ID. So, this old_cos ID certainly can
> > +     * be reused by current domain. Ref==0 means there is no any domain
> > +     * using this COS ID. So it can be used for current domain too.
> > +     */
> > +    if ( cos < 0 )
> > +    {
> > +        cos = pick_avail_cos(info, val_array, array_len, old_cos, 
> > feat_type);
> > +        if ( cos < 0 )
> > +        {
> > +            ret = cos;
> > +            goto unlock_free_array;
> > +        }
> > +
> > +        /*
> > +         * Step 4:
> > +         * Write the feature's MSRs according to the COS ID.
> > +         */
> > +        ret = write_psr_msrs(socket, cos, val_array, array_len, feat_type);
> > +        if ( ret )
> > +            goto unlock_free_array;
> > +    }
> > +
> > +    /*
> > +     * Step 5:
> > +     * Find the COS ID (find_cos result is '>= 0' or an available COS ID is
> > +     * picked, then update ref according to COS ID.
> > +     */
> > +    ref[cos]++;
> > +    ASSERT(!cos || ref[cos]);
> > +    ASSERT(!old_cos || ref[old_cos]);
> > +    ref[old_cos]--;
> > +    spin_unlock(&info->ref_lock);
> > +
> > +    /*
> > +     * 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?
> 
Yes, should be. Thanks!

> Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.