|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |