[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()


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 16 Jun 2020 16:58:01 +0100
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Paul Durrant <paul@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxx>
  • Delivery-date: Tue, 16 Jun 2020 15:58:39 +0000
  • Ironport-sdr: 9RTjJn1cu1zbGK1/wMEoAbg1cEEXppsSBe2Wu3i7UhI/yIW3aNhaqLAqTEaCYY/tQ/bTOy0xFC dOkiq4jbO59DYhDhnYZQF0JL2Sg6HJsPFMnrD0S2S3eQZHRW9mdL9xTfK90UOtJRqvjDCwR0M5 2QE0uMI4ZeLTQ0feSNAr+zcBU4xQi0LRw2sn9OsaUGjnAzEe17eGYIc+MP4eL2I7cRtl4XPrK1 8m6TFmc8tVwky8F9FVZs2NtPk97izsUgJnuJaZOo33dhczjXaHSA7f/Q3q8irRAq/iBsFeq7f3 jtw=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

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