|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()
On 16/06/2020 10:16, Jan Beulich wrote:
> On 15.06.2020 16:15, Andrew Cooper wrote:
>> Currently, libxl__cpuid_legacy() passes each element of the policy list to
>> xc_cpuid_set() individually. This is wasteful both in terms of the number of
>> hypercalls made, and the quantity of repeated merging/auditing work performed
>> by Xen.
>>
>> Move the loop processing down into xc_cpuid_set(), which allows us to do one
>> set of hypercalls, rather than one per list entry.
>>
>> In xc_cpuid_set(), obtain the full host, guest max and current policies to
>> begin with, and loop over the xend array, processing one leaf at a time.
>> Replace the linear search with a binary search, seeing as the serialised
>> leaves are sorted.
>>
>> No change in behaviour from the guests point of view.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with a few remarks:
>
>> @@ -286,99 +311,101 @@ int xc_cpuid_set(
>> }
>>
>> rc = -ENOMEM;
>> - if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL )
>> + if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
>> + (max = calloc(nr_leaves, sizeof(*max))) == NULL ||
>> + (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
>> {
>> ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
>> goto fail;
>> }
>>
>> + /* Get the domain's current policy. */
>> + nr_msrs = 0;
>> + nr_cur = nr_leaves;
>> + rc = xc_get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
>> + if ( rc )
>> + {
>> + PERROR("Failed to obtain d%d current policy", domid);
>> + rc = -errno;
>> + goto fail;
>> + }
>> +
>> /* Get the domain's max policy. */
>> nr_msrs = 0;
>> - policy_leaves = nr_leaves;
>> + nr_max = nr_leaves;
>> rc = xc_get_system_cpu_policy(xch, di.hvm ?
>> XEN_SYSCTL_cpu_policy_hvm_max
>> :
>> XEN_SYSCTL_cpu_policy_pv_max,
>> - &policy_leaves, leaves, &nr_msrs, NULL);
>> + &nr_max, max, &nr_msrs, NULL);
>> if ( rc )
>> {
>> PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
>> rc = -errno;
>> goto fail;
>> }
>> - for ( i = 0; i < policy_leaves; ++i )
>> - if ( leaves[i].leaf == xend->leaf &&
>> - leaves[i].subleaf == xend->subleaf )
>> - {
>> - polregs[0] = leaves[i].a;
>> - polregs[1] = leaves[i].b;
>> - polregs[2] = leaves[i].c;
>> - polregs[3] = leaves[i].d;
>> - break;
>> - }
>>
>> /* Get the host policy. */
>> nr_msrs = 0;
>> - policy_leaves = nr_leaves;
>> + nr_host = nr_leaves;
>> rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
>> - &policy_leaves, leaves, &nr_msrs, NULL);
>> + &nr_host, host, &nr_msrs, NULL);
>> if ( rc )
>> {
>> PERROR("Failed to obtain host policy");
>> rc = -errno;
>> goto fail;
>> }
>> - for ( i = 0; i < policy_leaves; ++i )
>> - if ( leaves[i].leaf == xend->leaf &&
>> - leaves[i].subleaf == xend->subleaf )
>> - {
>> - regs[0] = leaves[i].a;
>> - regs[1] = leaves[i].b;
>> - regs[2] = leaves[i].c;
>> - regs[3] = leaves[i].d;
>> - break;
>> - }
>>
>> - for ( i = 0; i < 4; i++ )
>> + rc = -EINVAL;
>> + for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
>> {
>> - if ( xend->policy[i] == NULL )
>> + xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
>> + const xen_cpuid_leaf_t *max_leaf = find_leaf(max, nr_max, xend);
>> + const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
>> +
>> + if ( cur_leaf == NULL || max_leaf == NULL || host_leaf == NULL )
>> {
>> - regs[i] = polregs[i];
>> - continue;
>> + ERROR("Missing leaf %#x, subleaf %#x", xend->leaf,
>> xend->subleaf);
>> + goto fail;
>> }
>>
>> - /*
>> - * Notes for following this algorithm:
>> - *
>> - * While it will accept any leaf data, it only makes sense to use on
>> - * feature leaves. regs[] initially contains the host values.
>> This,
>> - * with the fall-through chain, is how the 's' and 'k' options work.
>> - */
>> - for ( j = 0; j < 32; j++ )
>> + for ( int i = 0; i < 4; i++ )
> As you move the declaration here, perhaps switch to unsigned int
> as well? And express 4 as ARRAY_SIZE()?
>
>> {
>> - unsigned char val = !!((regs[i] & (1U << (31 - j))));
>> - unsigned char polval = !!((polregs[i] & (1U << (31 - j))));
>> -
>> - rc = -EINVAL;
>> - if ( !strchr("10xks", xend->policy[i][j]) )
>> - goto fail;
>> -
>> - if ( xend->policy[i][j] == '1' )
>> - val = 1;
>> - else if ( xend->policy[i][j] == '0' )
>> - val = 0;
>> - else if ( xend->policy[i][j] == 'x' )
>> - val = polval;
>> -
>> - if ( val )
>> - set_feature(31 - j, regs[i]);
>> - else
>> - clear_feature(31 - j, regs[i]);
>> + uint32_t *cur_reg = &cur_leaf->a + i;
>> + const uint32_t *max_reg = &max_leaf->a + i;
>> + const uint32_t *host_reg = &host_leaf->a + i;
>> +
>> + if ( xend->policy[i] == NULL )
>> + continue;
>> +
>> + for ( int j = 0; j < 32; j++ )
> unsigned int again? I don't think there's a suitable array available
> to also use ARRAY_SIZE() here.
All fixed.
>
>> + {
>> + bool val;
>> +
>> + if ( xend->policy[i][j] == '1' )
>> + val = true;
>> + else if ( xend->policy[i][j] == '0' )
>> + val = false;
>> + else if ( xend->policy[i][j] == 'x' )
>> + val = test_bit(31 - j, max_reg);
> Still seeing "max" used here is somewhat confusing given the purpose
> of the series, and merely judging from the titles I can't yet spot
> where later on it'll change. But I assume it will ...
No - it won't change. The legacy xend adjustments have always been
based on the max featureset, and changing it will break VM migration.
The soon-to-exist (4.15 at this point) brand new "what do I do for a
fresh boot" path will do things differently even for the legacy Xend
leaves, but the logic here must not semantically change.
~Andrew
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |