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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2016-09-08 at 13:30 +0800, Dongli Zhang wrote:
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index f34dd56..3641469 100644
> @@ -150,6 +152,12 @@ 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
> +     * already_scheduled is still 0 before this domain gets
> scheduled for
> +     * the first time. */
>
/*
 * Comment style for multi line comments in Xen
 * includes the 'wings'. :-)
 */

Yes, I know there's some inconsistency in this file (and in many others
:-/), but still.

> +    if ( d->already_scheduled == 0 )
>
unlikely() maybe?

> +        a->memflags |= MEMF_no_tlbflush;
> +
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
>          if ( i != a->nr_done && hypercall_preempt_check() )
> @@ -214,6 +222,21 @@ static void populate_physmap(struct memop_args
> *a)
>                      goto out;
>                  }
>  
> +                if ( d->already_scheduled == 0 )
> +                {
> +                    for ( j = 0; j < (1U << a->extent_order); j++ )
> +                    {
> +                        if ( page[j].u.free.need_tlbflush &&
> +                             (page[j].tlbflush_timestamp <=
> tlbflush_current_time()) &&
> +                             (!need_tlbflush ||
> +                             (page[j].tlbflush_timestamp >
> tlbflush_timestamp)) )
>
This check is long, complicated to read (at least to a non TLBflush
guru), and also appear twice.. can it be put in an inline function with
a talking name?

Oh, and I think you don't need the parenthesis around these twos:

 (page[j].tlbflush_timestamp <= tlbflush_current_time())
 (page[j].tlbflush_timestamp > tlbflush_timestamp)

> +                        {
> +                            need_tlbflush = 1;
> +                            tlbflush_timestamp =
> page[j].tlbflush_timestamp;
> +                        }
> +                    }
> +                }
> +
>                  mfn = page_to_mfn(page);
>              }

> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 32a300f..593541a 100644
> @@ -1376,6 +1376,11 @@ static void schedule(void)
>  
>      next = next_slice.task;
>  
> +    /* Set already_scheduled to 1 when this domain gets scheduled
> for the
> +     * first time */
>
Wings again.

And, about the content, it's already clear from the code that this gets
set when a vcpu of a domain is scheduled. What we want here is a
_quick_ explanation of why we need the scheduler to record this
information.

> +    if ( next->domain->already_scheduled == 0 )
>
unlikely() (and here I'm sure :-)).

> +        next->domain->already_scheduled = 1;
> +
>
And, finally, I'd move this toward the bottom of the function, outside
of the pcpu_schedule_lock() critical section, e.g., around the call to
vcpu_periodic_timer_work(next);

>      sd->curr = next;
>  
>      if ( next_slice.time >= 0 ) /* -ve means no limit */

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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