|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.12 v2 15/17] xen/arm: p2m: Add support for preemption in p2m_cache_flush_range
On Tue, 4 Dec 2018, Julien Grall wrote:
> p2m_cache_flush_range does not yet support preemption, this may be an
> issue as cleaning the cache can take a long time. While the current
> caller (XEN_DOMCTL_cacheflush) does not stricly require preemption, this
> will be necessary for new caller in a follow-up patch.
>
> The preemption implemented is quite simple, a counter is incremented by:
> - 1 on region skipped
> - 10 for each page requiring a flush
>
> When the counter reach 512 or above, we will check if preemption is
> needed. If not, the counter will be reset to 0. If yes, the function
> will stop, update start (to allow resuming later on) and return
> -ERESTART. This allows the caller to decide how the preemption will be
> done.
>
> For now, XEN_DOMCTL_cacheflush will continue to ignore the preemption.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
>
> ---
> Changes in v2:
> - Patch added
> ---
> xen/arch/arm/domctl.c | 8 +++++++-
> xen/arch/arm/p2m.c | 35 ++++++++++++++++++++++++++++++++---
> xen/include/asm-arm/p2m.h | 4 +++-
> 3 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 20691528a6..9da88b8c64 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -54,6 +54,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
> domain *d,
> {
> gfn_t s = _gfn(domctl->u.cacheflush.start_pfn);
> gfn_t e = gfn_add(s, domctl->u.cacheflush.nr_pfns);
> + int rc;
This is unnecessary...
> if ( domctl->u.cacheflush.nr_pfns > (1U<<MAX_ORDER) )
> return -EINVAL;
> @@ -61,7 +62,12 @@ long arch_do_domctl(struct xen_domctl *domctl, struct
> domain *d,
> if ( gfn_x(e) < gfn_x(s) )
> return -EINVAL;
>
> - return p2m_cache_flush_range(d, s, e);
> + /* XXX: Handle preemption */
> + do
> + rc = p2m_cache_flush_range(d, &s, e);
> + while ( rc == -ERESTART );
... you can just do:
while ( -ERESTART == p2m_cache_flush_range(d, &s, e) )
But given that it is just style, I'll leave it up to you.
> + return rc;
> }
> case XEN_DOMCTL_bind_pt_irq:
> {
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index db22b53bfd..ca9f0d9ebe 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1524,13 +1524,17 @@ int relinquish_p2m_mapping(struct domain *d)
> return rc;
> }
>
> -int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end)
> +int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end)
> {
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> gfn_t next_block_gfn;
> + gfn_t start = *pstart;
> mfn_t mfn = INVALID_MFN;
> p2m_type_t t;
> unsigned int order;
> + int rc = 0;
> + /* Counter for preemption */
> + unsigned long count = 0;
>
> /*
> * The operation cache flush will invalidate the RAM assigned to the
> @@ -1547,6 +1551,25 @@ int p2m_cache_flush_range(struct domain *d, gfn_t
> start, gfn_t end)
>
> while ( gfn_x(start) < gfn_x(end) )
> {
> + /*
> + * Cleaning the cache for the P2M may take a long time. So we
> + * need to be able to preempt. We will arbitrarily preempt every
> + * time count reach 512 or above.
> +
> + *
> + * The count will be incremented by:
> + * - 1 on region skipped
> + * - 10 for each page requiring a flush
Why this choice? A page flush should cost much more than 10x a region
skipped, more like 100x or 1000x. In fact, doing the full loop without
calling flush_page_to_ram should be cheap and fast, right?. I would:
- not increase count on region skipped at all
- increase it by 1 on each page requiring a flush
- set the limit lower, if we go with your proposal it would be about 50,
I am not sure what the limit should be though
> + */
> + if ( count >= 512 )
> + {
> + if ( softirq_pending(smp_processor_id()) )
> + {
> + rc = -ERESTART;
> + break;
> + }
> + count = 0;
No need to set count to 0 here
> + }
> +
> /*
> * We want to flush page by page as:
> * - it may not be possible to map the full block (can be up to 1GB)
> @@ -1573,22 +1596,28 @@ int p2m_cache_flush_range(struct domain *d, gfn_t
> start, gfn_t end)
> */
> if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_any_ram(t) )
> {
> + count++;
This is just an iteration doing nothing, I would not increament count.
> start = next_block_gfn;
> continue;
> }
> }
>
> + count += 10;
This makes sense, but if we skip the count++ above, we might as well
just count++ here and have a lower limit.
> flush_page_to_ram(mfn_x(mfn), false);
>
> start = gfn_add(start, 1);
> mfn = mfn_add(mfn, 1);
> }
>
> - invalidate_icache();
> + if ( rc != -ERESTART )
> + invalidate_icache();
>
> p2m_read_unlock(p2m);
>
> - return 0;
> + *pstart = start;
> +
> + return rc;
> }
>
> mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 7c1d930b1d..a633e27cc9 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -232,8 +232,10 @@ bool p2m_resolve_translation_fault(struct domain *d,
> gfn_t gfn);
> /*
> * Clean & invalidate caches corresponding to a region [start,end) of guest
> * address space.
> + *
> + * start will get updated if the function is preempted.
> */
> -int p2m_cache_flush_range(struct domain *d, gfn_t start, gfn_t end);
> +int p2m_cache_flush_range(struct domain *d, gfn_t *pstart, gfn_t end);
>
> /*
> * Map a region in the guest p2m with a specific p2m type.
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |