Re: [Xen-devel] [PATCH v4 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 10:16, <dongli.zhang@xxxxxxxxxx> wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -303,6 +303,8 @@ struct domain *domain_create(domid_t domid, unsigned int 
> domcr_flags,
>      if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
>          goto fail;
>  
> +    d->is_ever_unpaused = false;

This it not needed - struct domain starts out as all zeros anyway.

> @@ -1004,6 +1006,15 @@ int domain_unpause_by_systemcontroller(struct domain 
> *d)
>  {
>      int old, new, prev = d->controller_pause_count;
>  
> +    /*
> +     * Set is_ever_unpaused to true when this domain gets unpaused for the
> +     * first time. We record this information here to help populate_physmap
> +     * verify whether the domain has ever been unpaused. MEMF_no_tlbflush
> +     * is allowed to be set by populate_physmap only during vm creation.
> +     */
> +    if ( unlikely(!d->is_ever_unpaused) )
> +        d->is_ever_unpaused = true;

As mentioned before, the conditional is pointless. And just like Dario,
I dislike the name of the field. How about "has_run", "was_unpaused",
or "is_alive"? Or even better, how about combining this with the
is_shutting_down and is_shut_down into an enum? For that latter
variant, that would presumably better be a patch on its own then.

> @@ -150,6 +152,14 @@ static void populate_physmap(struct memop_args *a)
>                              max_order(curr_d)) )
>          return;
>  
> +    /*
> +     * MEMF_no_tlbflush can be set only during vm creation phase when
> +     * is_ever_unpaused is still false before this domain gets unpaused for
> +     * the first time.
> +     */
> +    if ( unlikely(!d->is_ever_unpaused) )
> +        a->memflags |= MEMF_no_tlbflush;

So you no longer mean to expose this to the caller?

> @@ -214,6 +224,20 @@ static void populate_physmap(struct memop_args *a)
>                      goto out;
>                  }
>  
> +                if ( unlikely(!d->is_ever_unpaused) )

Please check MEMF_no_tlbflush here instead.

> +                {
> +                    for ( j = 0; j < (1U << a->extent_order); j++ )
> +                    {
> +                        if ( page_needs_tlbflush(&page[j], need_tlbflush,
> +                                                 tlbflush_timestamp,
> +                                                 tlbflush_current_time()) )
> +                        {
> +                            need_tlbflush = true;
> +                            tlbflush_timestamp = page[j].tlbflush_timestamp;
> +                        }
> +                    }
> +                }
> +
>                  mfn = page_to_mfn(page);
>              }
>  
> @@ -232,6 +256,16 @@ static void populate_physmap(struct memop_args *a)
>      }
>  
>  out:
> +    if ( need_tlbflush )
> +    {
> +        cpumask_t mask = cpu_online_map;
> +        tlbflush_filter(mask, tlbflush_timestamp);

Blank line between declarations and statements please. Also,
considering this repeats what gets done in page_alloc.c, I think
it should also be factored out into a function. And along those
lines I think the other abstraction should then also go further
and take care of the updating of need_tlbflush and
tlbflush_timestamp.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel