Re: [Xen-devel] [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Punit Agrawal <punit.agrawal@xxxxxxx>]

Hi Wei,

Thanks for taking a look at this RFC. Responses/questions below...

Wei Liu <wei.liu2@xxxxxxxxxx> writes:

> On Fri, Mar 31, 2017 at 11:24:24AM +0100, Punit Agrawal wrote:
>> populate_physmap() calls alloc_heap_pages() per requested extent. As
>> alloc_heap_pages() performs icache maintenance operations affecting the
>> entire instruction cache, this leads to redundant cache flushes when
>> allocating multiple extents in populate_physmap().
>> 
>> To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
>> which can be used prevent alloc_heap_pages() to perform unnecessary
>> icache maintenance operations. Use the flag in populate_physmap() and
>> perform the required icache maintenance function at the end of the
>> operation.
>> 
>> Signed-off-by: Punit Agrawal <punit.agrawal@xxxxxxx>
>> ---
>>  xen/common/memory.c        | 6 ++++++
>>  xen/common/page_alloc.c    | 2 +-
>>  xen/include/asm-x86/page.h | 4 ++++
>>  xen/include/xen/mm.h       | 2 ++
>>  4 files changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index ad0b33ceb6..507f363924 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
>>      if ( unlikely(!d->creation_finished) )
>>          a->memflags |= MEMF_no_tlbflush;
>>  
>> +    a->memflags |= MEMF_no_icache_flush;
>> +
>
> Not a good idea.
>
> I think what you need is probably to group this under
> !d->creation_finished.

Can you explain why you think the above is not a good idea?

Without additional synchronisation there is a race between
d->creation_finished being set and used (Consider the case where
populate_physmap() being called on a different cpu to where
d->creation_finished is set to true).

>
>>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>>      {
>>          if ( i != a->nr_done && hypercall_preempt_check() )
>> @@ -253,6 +255,10 @@ static void populate_physmap(struct memop_args *a)
>>  out:
>>      if ( need_tlbflush )
>>          filtered_flush_tlb_mask(tlbflush_timestamp);
>> +
>> +    if ( a->memflags & MEMF_no_icache_flush )
>
> This should be inverted, right?

The MEMF_no_icache_flush flag is used to indicate to alloc_heap_pages()
(called via alloc_domheap_pages() here) that the caller is going to take
care of synchronising the icache with the dcache. As such, when the flag
is set, we want to perform icache maintenance operations here.

>
> The modification to this function is definitely wrong as-is. You end up
> always setting the no flush flag and later always flushing the cache.

Correct!

invalidate_icache() flushes the entire instruction cache which ends up
being called each time flush_page_to_ram() is invoked from
alloc_heap_pages(). The patch prevents repeated calls to
invalidate_icache() and moves the responsibility to populate_physmap()
which was the intention.

It would help if you can explain why you think the changes are
wrong.

Thanks,
Punit

>
>
> Wei.

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