|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: 22 September 2020 19:25
> To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxxxxx>; Ian
> Jackson <iwj@xxxxxxxxxxxxxx>; Jan Beulich <JBeulich@xxxxxxxx>; 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>
> Subject: [PATCH v2 02/11] 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.
>
> 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 <iwj@xxxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> 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>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
>
> v2:
> * Fix CentOS 6 build by initialising vaddrs to NULL
> * Add ASSERT_UNREACHABLE() and gt->status typecheck
> ---
> xen/common/grant_table.c | 102
> +++++++++++++++++++++++++++++++-----------
> xen/common/memory.c | 42 +----------------
> xen/include/xen/grant_table.h | 19 +++-----
> 3 files changed, 84 insertions(+), 79 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index a5d3ed8bda..912f07be47 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -4013,6 +4013,81 @@ 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;
> + mfn_t tmp;
> + void **vaddrs = NULL;
> + int rc;
> +
> + /* Input sanity. */
Nit: inconsistency with full stops on single line comments.
> + if ( !nr_frames )
> + return -EINVAL;
> +
> + /* Overflow checks */
> + if ( frame + nr_frames < frame )
> + return -EINVAL;
> +
> + tot_frames = frame + nr_frames;
> + if ( tot_frames != frame + nr_frames )
> + return -EINVAL;
> +
> + /* Grow table if necessary. */
> + grant_write_lock(gt);
> + switch ( id )
> + {
> + case XENMEM_resource_grant_table_id_shared:
> + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
> + break;
> +
> + case XENMEM_resource_grant_table_id_status:
> + if ( gt->gt_version != 2 )
> + {
> + default:
Personally I dislike this kind of thing. IMO it would be neater to initialise
rc to -EINVAL at the top, then this could just be a 'break' (so no braces) and
you could have a simple 'default: break;' case instead.
> + rc = -EINVAL;
> + break;
> + }
> + rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp);
> + break;
> + }
> +
I think you could drop the write lock here...
> + /* Any errors from growing the table? */
> + if ( rc )
> + goto out;
> +
...and acquire it read here, since we know the table cannot shrink. You'd need
to re-check the gt_version for safety though.
Paul
> + switch ( id )
> + {
> + case XENMEM_resource_grant_table_id_shared:
> + vaddrs = gt->shared_raw;
> + break;
> +
> + case XENMEM_resource_grant_table_id_status:
> + /* Check that void ** is a suitable representation for gt->status. */
> + BUILD_BUG_ON(!__builtin_types_compatible_p(
> + typeof(gt->status), grant_status_t **));
> + vaddrs = (void **)gt->status;
> + break;
> + }
> +
> + if ( !vaddrs )
> + {
> + ASSERT_UNREACHABLE();
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + for ( i = 0; i < nr_frames; ++i )
> + mfn_list[i] = virt_to_mfn(vaddrs[frame + i]);
> +
> + out:
> + grant_write_unlock(gt);
> +
> + return rc;
> +}
> +
> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t
> *mfn)
> {
> int rc = 0;
> @@ -4047,33 +4122,6 @@ int gnttab_map_frame(struct domain *d, unsigned long
> idx, gfn_t gfn, mfn_t
> *mfn)
> return rc;
> }
>
> -int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> - mfn_t *mfn)
> -{
> - struct grant_table *gt = d->grant_table;
> - int rc;
> -
> - grant_write_lock(gt);
> - rc = gnttab_get_shared_frame_mfn(d, idx, mfn);
> - grant_write_unlock(gt);
> -
> - return rc;
> -}
> -
> -int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> - mfn_t *mfn)
> -{
> - struct grant_table *gt = d->grant_table;
> - int rc;
> -
> - grant_write_lock(gt);
> - rc = (gt->gt_version == 2) ?
> - gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL;
> - grant_write_unlock(gt);
> -
> - return rc;
> -}
> -
> static void gnttab_usage_print(struct domain *rd)
> {
> int first = 1;
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 1bab0e80c2..177fc378d9 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,44 +1007,6 @@ static long xatp_permission_check(struct domain *d,
> unsigned int space)
> return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> }
>
> -static int acquire_grant_table(struct domain *d, unsigned int id,
> - unsigned long frame,
> - unsigned int nr_frames,
> - xen_pfn_t mfn_list[])
> -{
> - unsigned int i = nr_frames;
> -
> - /* Iterate backwards in case table needs to grow */
> - while ( i-- != 0 )
> - {
> - mfn_t mfn = INVALID_MFN;
> - int rc;
> -
> - switch ( id )
> - {
> - case XENMEM_resource_grant_table_id_shared:
> - rc = gnttab_get_shared_frame(d, frame + i, &mfn);
> - break;
> -
> - case XENMEM_resource_grant_table_id_status:
> - rc = gnttab_get_status_frame(d, frame + i, &mfn);
> - break;
> -
> - default:
> - rc = -EINVAL;
> - break;
> - }
> -
> - if ( rc )
> - return rc;
> -
> - ASSERT(!mfn_eq(mfn, INVALID_MFN));
> - mfn_list[i] = mfn_x(mfn);
> - }
> -
> - return 0;
> -}
> -
> static int acquire_resource(
> XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> {
> @@ -1099,8 +1061,8 @@ static int acquire_resource(
> switch ( xmar.type )
> {
> case XENMEM_resource_grant_table:
> - rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames,
> - mfn_list);
> + rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames,
> + mfn_list);
> break;
>
> default:
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 98603604b8..5a2c75b880 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -56,10 +56,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt,
> grant_ref_t ref,
>
> int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn,
> mfn_t *mfn);
> -int gnttab_get_shared_frame(struct domain *d, unsigned long idx,
> - mfn_t *mfn);
> -int gnttab_get_status_frame(struct domain *d, unsigned long idx,
> - mfn_t *mfn);
> +
> +int gnttab_acquire_resource(
> + struct domain *d, unsigned int id, unsigned long frame,
> + unsigned int nr_frames, xen_pfn_t mfn_list[]);
>
> #else
>
> @@ -93,14 +93,9 @@ static inline int gnttab_map_frame(struct domain *d,
> unsigned long idx,
> return -EINVAL;
> }
>
> -static inline int gnttab_get_shared_frame(struct domain *d, unsigned long
> idx,
> - mfn_t *mfn)
> -{
> - return -EINVAL;
> -}
> -
> -static inline int gnttab_get_status_frame(struct domain *d, unsigned long
> idx,
> - mfn_t *mfn)
> +static inline int gnttab_acquire_resource(
> + struct domain *d, unsigned int id, unsigned long frame,
> + unsigned int nr_frames, xen_pfn_t mfn_list[])
> {
> return -EINVAL;
> }
> --
> 2.11.0
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |