Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/07/2020 09:14, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper <andrew.cooper3@citrix.com>
>> Sent: 28 July 2020 12:37
>> To: Xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
>> Jackson <ian.jackson@citrix.com>; Jan Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien
>> Grall <julien@xen.org>; Paul Durrant <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>;
>> Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
>> Subject: [PATCH 2/5] xen/gnttab: Rework resource acquisition
>>
>> The existing logic doesn't function in the general case for mapping a guests
>> grant table, due to arbitrary 32 frame limit, and the default grant table
>> limit being 64.
>>
>> In order to start addressing this, rework the existing grant table logic by
>> implementing a single gnttab_acquire_resource().  This is far more efficient
>> than the previous acquire_grant_table() in memory.c because it doesn't take
>> the grant table write lock, and attempt to grow the table, for every single
>> frame.
>>
> But that should not have happened before because the code deliberately iterates backwards, thereby starting with the last frame, thereby growing the table at most once. (I agree that dropping and re-acquiring the lock every time was sub-optimal).

It still attempts to grow on every iteration.  Its just growing to a
smaller size than already succeeded.

>
>> The new gnttab_acquire_resource() function subsumes the previous two
>> gnttab_get_{shared,status}_frame() helpers.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Paul Durrant <paul@xen.org>
>> CC: Michał Leszczyński <michal.leszczynski@cert.pl>
>> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl>
>> ---
>>  xen/common/grant_table.c      | 93 ++++++++++++++++++++++++++++++-------------
>>  xen/common/memory.c           | 42 +------------------
>>  xen/include/xen/grant_table.h | 19 ++++-----
>>  3 files changed, 75 insertions(+), 79 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index 9f0cae52c0..122d1e7596 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>      return 0;
>>  }
>>
>> +int gnttab_acquire_resource(
>> +    struct domain *d, unsigned int id, unsigned long frame,
>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    unsigned int i = nr_frames, tot_frames;
>> +    void **vaddrs;
>> +    int rc = 0;
>> +
>> +    /* Input sanity. */
>> +    if ( !nr_frames )
>> +        return -EINVAL;
> This was not an error before. Does mapping 0 frames really need to be a failure?

Yes.

You have spotted the -1 which depends on nr_frames being nonzero to
function correctly.

>> +
>> +    /* Overflow checks */
>> +    if ( frame + nr_frames < frame )
>> +        return -EINVAL;
>> +
>> +    tot_frames = frame + nr_frames;
> That name is confusing. 'last_frame' perhaps (and then make the -1 implicit)?

How is that naming any less confusing?

>> +        break;
>> +    }
>> +
>> +    for ( i = 0; i < nr_frames; ++i )
>> +        mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
>> +
>> + out:
>> +    grant_write_unlock(gt);
> Since you deliberately grew the table first, could you not drop the write lock and acquire it a read lock before looping over the frames?

I tried originally.  That's not an operation supported by percpu
read/write locks, and this isn't a fastpath.

~Andrew