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/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?

> @@ -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[].

> +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?

Jan

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