On 30/07/2020 09:14, Paul Durrant wrote: >> -----Original Message----- >> From: Andrew Cooper <andrew.coop...@citrix.com> >> Sent: 28 July 2020 12:37 >> To: Xen-devel <xen-devel@lists.xenproject.org> >> Cc: Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap >> <george.dun...@eu.citrix.com>; Ian >> Jackson <ian.jack...@citrix.com>; Jan Beulich <jbeul...@suse.com>; Konrad >> Rzeszutek Wilk >> <konrad.w...@oracle.com>; Stefano Stabellini <sstabell...@kernel.org>; Wei >> Liu <w...@xen.org>; Julien >> Grall <jul...@xen.org>; Paul Durrant <p...@xen.org>; Michał Leszczyński >> <michal.leszczyn...@cert.pl>; >> Hubert Jasudowicz <hubert.jasudow...@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.coop...@citrix.com> >> --- >> CC: George Dunlap <george.dun...@eu.citrix.com> >> CC: Ian Jackson <ian.jack...@citrix.com> >> CC: Jan Beulich <jbeul...@suse.com> >> CC: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> >> CC: Stefano Stabellini <sstabell...@kernel.org> >> CC: Wei Liu <w...@xen.org> >> CC: Julien Grall <jul...@xen.org> >> CC: Paul Durrant <p...@xen.org> >> CC: Michał Leszczyński <michal.leszczyn...@cert.pl> >> CC: Hubert Jasudowicz <hubert.jasudow...@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