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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/07/2020 09:14, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Sent: 28 July 2020 12:37
>> To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
>> <George.Dunlap@xxxxxxxxxxxxx>; Ian
>> Jackson <ian.jackson@xxxxxxxxxx>; Jan Beulich <JBeulich@xxxxxxxx>; Konrad 
>> Rzeszutek Wilk
>> <konrad.wilk@xxxxxxxxxx>; 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>
>> 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@xxxxxxxxxx>
>> ---
>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Paul Durrant <paul@xxxxxxx>
>> CC: Michał Leszczyński <michal.leszczynski@xxxxxxx>
>> CC: Hubert Jasudowicz <hubert.jasudowicz@xxxxxxx>
>> ---
>>  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