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

Re: [PATCH v3 5/7] xen/memory: Improve compat XENMEM_acquire_resource handling


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Thu, 28 Jan 2021 23:32:57 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=iLVwV46W+YBJSO5OZt3Zxa12crLy8t/a08LlrxrYFVE=; b=JZ5n3rHso3IY9GjQo7fQfGKPSpQ8Z8ZjWpJ4kTvyEPvScgH5Msk+Sa38AvrVutOXv5L2T/qDyLZN7vsuj4CPaACOASS2ZX+RxwmLKTJzHrA0liyOSjfKM8pzLRhWa8Ig7EyQNry8Q7vmR9l2K5ES+c4tXJPT3OpOC1B0xtAsGnzJG3jpXXCYa7nczcPx1wUZBiVjpM8rdo/qUr2CWq6eEKneuTOEnW7vyK0XAotUpCxTAtLk1xihUHemF4bpxVcMDBnSKissEG7bulWnTIwA9m21DnAJMaOLHdCQJ3zFqqY/y1W7s3XLXXrZaDKABn6kn5AwwyipmajZeHBAza+kcA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=d9Ez63ns23aqAaSZ9t36bw3JfUwtxect2hZizg2sKGmaYatw3deqpUpXjVxECKle4BPdHVL1uLS9q4w5JpG2siH+SMB+pVOqvskt/yCFpg2R8uo9jfH/PCoYkXsBkIhH3SAH747lmweXE0EY0uuqAZPeNvc8vbyNLlmteynrtaxY8tii16NOamzHiHJt4kwJjt6yFPCJshMrZo9ZNaiA6mwSbWukrB9+nqkKMWGKB1f7bhs9jjwfzxpE4RlmSdevUH2tw4F9e2/cgG7N/pM7zIrNrRwxK6+gwbzyzdlcH4A6IMxA2QFtUBlLyM/ol/l4Kpg28KNHMHvtXaL5Yw3ELQ==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, Michał Leszczyński <michal.leszczynski@xxxxxxx>, Hubert Jasudowicz <hubert.jasudowicz@xxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 28 Jan 2021 23:33:29 +0000
  • Ironport-sdr: 95o08M4YoNjUSW/qy/MXzhCGD1myZreowWnrzKO4EwQF+sSOvHBoosHl+6XzkrZNn/QXSqvEml aEjkzltz568p9fuBUrOrwuM1+8shRr6YdNsZGbIMZ2ZPLww557eVovj3tg1wRGQeg+V6c1EKXt gMLOyB7NHNHhGqo91oNATfcdHaT7EJntxwlPfh+7ZaM7LOsmq5ArWR45yo4JQOT9M1AztmbyW0 H7amJh/JhZi9QPBkpHEir2N6cK3EczdbzL5d8/PY+Hf3qoS8KwbpbnmjznxK4x4cCuUhcfPiWl 7m4=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 15/01/2021 15:37, Jan Beulich wrote:
> On 12.01.2021 20:48, Andrew Cooper wrote:
>> @@ -446,6 +430,31 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>  
>>  #undef XLAT_mem_acquire_resource_HNDL_frame_list
>>  
>> +            if ( xen_frame_list && cmp.mar.nr_frames )
>> +            {
>> +                /*
>> +                 * frame_list is an input for translated guests, and an 
>> output
>> +                 * for untranslated guests.  Only copy in for translated 
>> guests.
>> +                 */
>> +                if ( paging_mode_translate(currd) )
>> +                {
>> +                    compat_pfn_t *compat_frame_list = (void 
>> *)xen_frame_list;
>> +
>> +                    if ( !compat_handle_okay(cmp.mar.frame_list,
>> +                                             cmp.mar.nr_frames) ||
>> +                         __copy_from_compat_offset(
>> +                             compat_frame_list, cmp.mar.frame_list,
>> +                             0, cmp.mar.nr_frames) )
>> +                        return -EFAULT;
>> +
>> +                    /*
>> +                     * Iterate backwards over compat_frame_list[] expanding
>> +                     * compat_pfn_t to xen_pfn_t in place.
>> +                     */
>> +                    for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
>> +                        xen_frame_list[x] = compat_frame_list[x];
> Just as a nit, without requiring you to adjust (but with the
> request to consider adjusting) - x getting used as array index
> would generally suggest it wants to be an unsigned type (despite
> me guessing the compiler ought to be able to avoid an explicit
> sign-extension for the actual memory accesses):
>
>                     for ( unsigned int x = cmp.mar.nr_frames; x--; )
>                         xen_frame_list[x] = compat_frame_list[x];

Signed numbers are not inherently evil.  The range of x is between 0 and
1020 so there is no issue with failing to enter the loop.

It is the compilers job to make this optimisation.  It is a very poor
use of a developers time to write logic which takes extra effort to
figure out whether it is correct or not.

You know what my attitude will be towards a compiler which is incapable
of making the optimisation, and you've got to go back a decade to find a
processor old enough to not have identical performance between the
unoptimised signed and unsigned forms.

Both signs of numbers have their uses, and a rigid policy of using
unsigned numbers does more harm than good (in this case, concerning the
simplicity of the code).

~Andrew



 


Rackspace

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