[PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Dongli Zhang]

This patch cleaned up the code by replacing complicated tlbflush check with
an inline function. We should use this inline function to avoid the long
and complicated to read tlbflush check when implementing TODOs left in
commit a902c12ee45fc9389eb8fe54eeddaf267a555c58.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
Changed since v3:
  * Wrap the complicated tlbflush condition check as inline function
    (suggested by Dario).

---
 xen/common/page_alloc.c |  7 +++----
 xen/include/xen/mm.h    | 11 +++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 18ff6cf..5b93a01 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -827,10 +827,9 @@ static struct page_info *alloc_heap_pages(
         BUG_ON(pg[i].count_info != PGC_state_free);
         pg[i].count_info = PGC_state_inuse;
 
-        if ( pg[i].u.free.need_tlbflush &&
-             (pg[i].tlbflush_timestamp <= tlbflush_current_time()) &&
-             (!need_tlbflush ||
-              (pg[i].tlbflush_timestamp > tlbflush_timestamp)) )
+        if ( page_needs_tlbflush(&pg[i], need_tlbflush,
+                                 tlbflush_timestamp,
+                                 tlbflush_current_time()) )
         {
             need_tlbflush = 1;
             tlbflush_timestamp = pg[i].tlbflush_timestamp;
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 58bc0b8..766559d 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -567,4 +567,15 @@ int prepare_ring_for_helper(struct domain *d, unsigned long gmfn,
                             struct page_info **_page, void **_va);
 void destroy_ring_for_helper(void **_va, struct page_info *page);
 
+static inline int page_needs_tlbflush(struct page_info *page,
+                                      bool_t need_tlbflush,
+                                      uint32_t tlbflush_timestamp,
+                                      uint32_t tlbflush_current_time)
+{
+    return page->u.free.need_tlbflush &&
+           page->tlbflush_timestamp <= tlbflush_current_time &&
+           (!need_tlbflush ||
+            page->tlbflush_timestamp > tlbflush_timestamp);
+}
+
 #endif /* __XEN_MM_H__ */
-- 
1.9.1


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

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

[Dongli Zhang]

This patch implemented parts of TODO left in commit id
a902c12ee45fc9389eb8fe54eeddaf267a555c58. It moved TLB-flush filtering out
into populate_physmap. Because of TLB-flush in alloc_heap_pages, it's very
slow to create a guest with memory size of more than 100GB on host with
100+ cpus.

This patch introduced a "MEMF_no_tlbflush" bit to memflags to indicate
whether TLB-flush should be done in alloc_heap_pages or its caller
populate_physmap. Once this bit is set in memflags, alloc_heap_pages will
ignore TLB-flush. To use this bit after vm is created might lead to
security issue, that is, this would make pages accessible to the guest B,
when guest A may still have a cached mapping to them.

Therefore, this patch also introduced a "is_ever_unpaused" field to struct
domain to indicate whether this domain has ever got unpaused by hypervisor.
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.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
Changed since v3:
  * Set the flag to true in domain_unpause_by_systemcontroller when
    unpausing the guest domain for the first time.
  * Use true/false for all boot_t variables.
  * Add unlikely to optimize "if statement".
  * Correct comment style.

Changed since v2:
  * Limit this optimization to domain creation time.

---
 xen/common/domain.c     | 11 +++++++++++
 xen/common/memory.c     | 34 ++++++++++++++++++++++++++++++++++
 xen/common/page_alloc.c |  3 ++-
 xen/include/xen/mm.h    |  2 ++
 xen/include/xen/sched.h |  3 +++
 5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index a8804e4..7be1bee 100644
--- 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;
+
     if ( domcr_flags & DOMCRF_hvm )
         d->guest_type = guest_type_hvm;
     else if ( domcr_flags & DOMCRF_pvh )
@@ -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;
+
     do
     {
         old = prev;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index cc0f69e..f3a733b 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -141,6 +141,8 @@ static void populate_physmap(struct memop_args *a)
     unsigned int i, j;
     xen_pfn_t gpfn, mfn;
     struct domain *d = a->domain, *curr_d = current->domain;
+    bool_t need_tlbflush = false;
+    uint32_t tlbflush_timestamp = 0;
 
     if ( !guest_handle_subrange_okay(a->extent_list, a->nr_done,
                                      a->nr_extents-1) )
@@ -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;
+
     for ( i = a->nr_done; i < a->nr_extents; i++ )
     {
         if ( i != a->nr_done && hypercall_preempt_check() )
@@ -214,6 +224,20 @@ static void populate_physmap(struct memop_args *a)
                     goto out;
                 }
 
+                if ( unlikely(!d->is_ever_unpaused) )
+                {
+                    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);
+        if ( !cpumask_empty(&mask) )
+        {
+            perfc_incr(need_flush_tlb_flush);
+            flush_tlb_mask(&mask);
+        }
+    }
     a->nr_done = i;
 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5b93a01..04ca26a 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -827,7 +827,8 @@ static struct page_info *alloc_heap_pages(
         BUG_ON(pg[i].count_info != PGC_state_free);
         pg[i].count_info = PGC_state_inuse;
 
-        if ( page_needs_tlbflush(&pg[i], need_tlbflush,
+        if ( !(memflags & MEMF_no_tlbflush) &&
+             page_needs_tlbflush(&pg[i], need_tlbflush,
                                  tlbflush_timestamp,
                                  tlbflush_current_time()) )
         {
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 766559d..04b10e9 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -221,6 +221,8 @@ struct npfec {
 #define  MEMF_exact_node  (1U<<_MEMF_exact_node)
 #define _MEMF_no_owner    5
 #define  MEMF_no_owner    (1U<<_MEMF_no_owner)
+#define _MEMF_no_tlbflush 6
+#define  MEMF_no_tlbflush (1U<<_MEMF_no_tlbflush)
 #define _MEMF_node        8
 #define  MEMF_node_mask   ((1U << (8 * sizeof(nodeid_t))) - 1)
 #define  MEMF_node(n)     ((((n) + 1) & MEMF_node_mask) << _MEMF_node)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2f9c15f..7fe8841 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -474,6 +474,9 @@ struct domain
         unsigned int guest_request_enabled       : 1;
         unsigned int guest_request_sync          : 1;
     } monitor;
+
+    /* set to true the first time this domain gets unpaused. */
+    bool_t is_ever_unpaused;
 };
 
 /* Protect updates/reads (resp.) of domain_list and domain_hash. */
-- 
1.9.1


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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 4444 bytes --]

On Mon, 2016-09-12 at 16:16 +0800, Dongli Zhang wrote:
> This patch implemented parts of TODO left in commit id
> a902c12ee45fc9389eb8fe54eeddaf267a555c58. 
>
We usually put both the (not necessarily full) hash and the subject
line of the commit in here.

> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index a8804e4..7be1bee 100644
> @@ -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;
> +
>
I'd go for something like "first_unpaused" or "creation_finished", but
if maintainers are happy with this one already, I'm fine too.

> @@ -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.
> +     */

"We record this information here for populate_physmap to figure out
 that the domain has already been unpaused, after finishing being
 created. That's because we're allowed to set MEMF_no_tlbflush only
 during VM creation."

Or, de-focusing the unpausing even more:

"We record this information here for populate_physmap to figure out
 tha
t the domain has finished being created. In fact, we're only
 allowed to
set the MEMF_no_tlbflush flag during VM creation."

I.e., the important thing is not really the unpausing (that's where we
found it handy to put the check), it's the fact that something should
only happen at creation time and why (see below).

> +    if ( unlikely(!d->is_ever_unpaused) )
> +        d->is_ever_unpaused = true;
> +
>      do
>      {
>          old = prev;

> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index cc0f69e..f3a733b 100644
> @@ -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.
> +     */
>
What about, 'citing' from the changelog:

"With MEMF_no_tlbflush set, alloc_heap_pages() will ignore TLB-
 flushes. After VM creation, this is a security issue (it can make
 pages accessible to guest B, when guest A may still have a cached
 mapping to them). So we only do this only during domain creation,
 when the domain itself has not yet been unpaused for the first
 time."

> +    if ( unlikely(!d->is_ever_unpaused) )
> +        a->memflags |= MEMF_no_tlbflush;
> +
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
>          if ( i != a->nr_done && hypercall_preempt_check() )

> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 2f9c15f..7fe8841 100644
> @@ -474,6 +474,9 @@ struct domain
>          unsigned int guest_request_enabled       : 1;
>          unsigned int guest_request_sync          : 1;
>      } monitor;
> +
> +    /* set to true the first time this domain gets unpaused. */
>
I think it's relevant to say _when_ that is. What about:

/*
 * Set to true at the very end of domain creation, when the domain is 
 * unpaused for the first time by the systemcontroller.
 */

(not 100% happy about the "by the systemcontroller" part... but that's
the idea.)

> +    bool_t is_ever_unpaused;
>
As said by Jan already --here and elsewhere-- new code should use
'bool'.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> On 12.09.16 at 10:16, <dongli.zhang@oracle.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Jan Beulich]

>>> On 12.09.16 at 10:16, <dongli.zhang@oracle.com> wrote:
> This patch cleaned up the code by replacing complicated tlbflush check with
> an inline function. We should use this inline function to avoid the long
> and complicated to read tlbflush check when implementing TODOs left in
> commit a902c12ee45fc9389eb8fe54eeddaf267a555c58.



> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -567,4 +567,15 @@ int prepare_ring_for_helper(struct domain *d, unsigned long gmfn,
>                              struct page_info **_page, void **_va);
>  void destroy_ring_for_helper(void **_va, struct page_info *page);
>  
> +static inline int page_needs_tlbflush(struct page_info *page,

bool and const please.

> +                                      bool_t need_tlbflush,

bool

But really passing this into a function with this name is kind of
awkward. Perhaps a better function name would be e.g.
accumulate_tlbflush(), and then this parameter would maybe
better be the first one.

> +                                      uint32_t tlbflush_timestamp,
> +                                      uint32_t tlbflush_current_time)

I don't think you should pass this into the function ...

> +{
> +    return page->u.free.need_tlbflush &&
> +           page->tlbflush_timestamp <= tlbflush_current_time &&

... and use tlbflush_current_time() here instead.

Jan



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

Re: [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Dongli Zhang]

> I don't think you should pass this into the function ...
> 
> > +{
> > +    return page->u.free.need_tlbflush &&
> > +           page->tlbflush_timestamp <= tlbflush_current_time &&
> 
> ... and use tlbflush_current_time() here instead.

I rewrite the inline function in xen/include/xen/mm.h to:

+#include <asm/flushtlb.h>
+
+static inline bool accumulate_tlbflush(bool need_tlbflush,
+                                       const struct page_info *page,
+                                       uint32_t tlbflush_timestamp)
+{
+    return page->u.free.need_tlbflush &&
+           page->tlbflush_timestamp <= tlbflush_current_time() &&
+           (!need_tlbflush ||
+            page->tlbflush_timestamp > tlbflush_timestamp);
+}

However, to use tlbflush_current_time and "asm/flushtlb.h" would lead
to the following compiling error:

>>>>>>>>>>>>>>>>>>>>>>>>>>>
In file included from /home/zhang/test/mainline-xen/xen/include/asm/flushtlb.h:14:0,
                 from suspend.c:13:
/home/zhang/test/mainline-xen/xen/include/xen/mm.h: In function ‘accumulate_tlbflush’:
/home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: implicit declaration of function ‘tlbflush_current_time’ [-Werror=implicit-function-declaration]
            page->tlbflush_timestamp <= tlbflush_current_time() &&
            ^
/home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: nested extern declaration of ‘tlbflush_current_time’ [-Werror=nested-externs]
cc1: all warnings being treated as errors
make[4]: *** [suspend.o] Error 1
>>>>>>>>>>>>>>>>>>>>>>>>>>>

I can workaround the issue by removing "#include <asm/flushtlb.h>" from
xen/arch/x86/acpi/suspend.c and then everything works fine.


Can I just rewrite the inline function to a #define macro? This minimizes the
changes to the code.

+#define accumulate_tlbflush(need_tlbflush, page, tlbflush_timestamp) \
+    (page)->u.free.need_tlbflush &&                                  \
+    (page)->tlbflush_timestamp <= tlbflush_current_time() &&         \
+    (!need_tlbflush ||                                               \
+     (page)->tlbflush_timestamp > tlbflush_timestamp)

Thank you very much!

Dongli Zhang

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

Re: [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Jan Beulich]

>>> On 15.09.16 at 00:36, <dongli.zhang@oracle.com> wrote:
>>  I don't think you should pass this into the function ...
>> 
>> > +{
>> > +    return page->u.free.need_tlbflush &&
>> > +           page->tlbflush_timestamp <= tlbflush_current_time &&
>> 
>> ... and use tlbflush_current_time() here instead.
> 
> I rewrite the inline function in xen/include/xen/mm.h to:
> 
> +#include <asm/flushtlb.h>
> +
> +static inline bool accumulate_tlbflush(bool need_tlbflush,
> +                                       const struct page_info *page,
> +                                       uint32_t tlbflush_timestamp)
> +{
> +    return page->u.free.need_tlbflush &&
> +           page->tlbflush_timestamp <= tlbflush_current_time() &&
> +           (!need_tlbflush ||
> +            page->tlbflush_timestamp > tlbflush_timestamp);
> +}
> 
> However, to use tlbflush_current_time and "asm/flushtlb.h" would lead
> to the following compiling error:
> 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> In file included from 
> /home/zhang/test/mainline-xen/xen/include/asm/flushtlb.h:14:0,
>                  from suspend.c:13:
> /home/zhang/test/mainline-xen/xen/include/xen/mm.h: In function 
> ‘accumulate_tlbflush’:
> /home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: implicit 
> declaration of function ‘tlbflush_current_time’ 
> [-Werror=implicit-function-declaration]
>             page->tlbflush_timestamp <= tlbflush_current_time() &&
>             ^
> /home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: nested 
> extern declaration of ‘tlbflush_current_time’ [-Werror=nested-externs]
> cc1: all warnings being treated as errors
> make[4]: *** [suspend.o] Error 1
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> 
> I can workaround the issue by removing "#include <asm/flushtlb.h>" from
> xen/arch/x86/acpi/suspend.c and then everything works fine.

Removing? You should _add_ asm/tlbflush.h inclusion to xen/mm.h.

> Can I just rewrite the inline function to a #define macro? This minimizes 
> the
> changes to the code.
> 
> +#define accumulate_tlbflush(need_tlbflush, page, tlbflush_timestamp) \
> +    (page)->u.free.need_tlbflush &&                                  \
> +    (page)->tlbflush_timestamp <= tlbflush_current_time() &&         \
> +    (!need_tlbflush ||                                               \
> +     (page)->tlbflush_timestamp > tlbflush_timestamp)

Generally we prefer inline functions whenever possible. And note
that if for some reason the macro variant would indeed be necessary
to us, you'd complete parenthesization of the uses of the various
macro parameters. And ideally you'd also avoid evaluating any of
the arguments other than exactly once. All of which are a non-issues
with inline functions.

Jan

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

Re: [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Dongli Zhang]

> > I rewrite the inline function in xen/include/xen/mm.h to:
> > 
> > +#include <asm/flushtlb.h>
> > +
> > +static inline bool accumulate_tlbflush(bool need_tlbflush,
> > +                                       const struct page_info *page,
> > +                                       uint32_t tlbflush_timestamp)
> > +{
> > +    return page->u.free.need_tlbflush &&
> > +           page->tlbflush_timestamp <= tlbflush_current_time() &&
> > +           (!need_tlbflush ||
> > +            page->tlbflush_timestamp > tlbflush_timestamp);
> > +}
> > 
> > However, to use tlbflush_current_time and "asm/flushtlb.h" would lead
> > to the following compiling error:
> > 
> >>>>>>>>>>>>>>>>>>>>>>>>>>>>
> > In file included from 
> > /home/zhang/test/mainline-xen/xen/include/asm/flushtlb.h:14:0,
> >                  from suspend.c:13:
> > /home/zhang/test/mainline-xen/xen/include/xen/mm.h: In function 
> > ‘accumulate_tlbflush’:
> > /home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: implicit 
> > declaration of function ‘tlbflush_current_time’ 
> > [-Werror=implicit-function-declaration]
> >             page->tlbflush_timestamp <= tlbflush_current_time() &&
> >             ^
> > /home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: nested 
> > extern declaration of ‘tlbflush_current_time’ [-Werror=nested-externs]
> > cc1: all warnings being treated as errors
> > make[4]: *** [suspend.o] Error 1
> >>>>>>>>>>>>>>>>>>>>>>>>>>>>
> > 
> > I can workaround the issue by removing "#include <asm/flushtlb.h>" from
> > xen/arch/x86/acpi/suspend.c and then everything works fine.

> Removing? You should _add_ asm/tlbflush.h inclusion to xen/mm.h.

As mentioned in previous email, there was a compiling error from suspend.c:13
even after asm/tlbflush.h is added to xen/mm.h

The compiling error is bypassed if I remove the include of asm/flushtlb.h from
suspend.c. 

The following patch can make the inline function work while avoiding compiling
error. Since asm/flushtlb.h depends on get_order_from_bytes in xen/mm.h, it is
included after get_order_from_bytes in xen/mm.h.

If removing asm/flushtlb.h in suspend.c is not preferred, the other two options
I have are (1) use #define macro (2) pass tlbflush_current_time() as an arg in
accumulate_tlbflush.

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index 1d8344c..d5c67ee 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -10,7 +10,6 @@
 #include <asm/processor.h>
 #include <asm/msr.h>
 #include <asm/debugreg.h>
-#include <asm/flushtlb.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
 #include <asm/i387.h>
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 58bc0b8..03bcbc3 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -567,4 +567,16 @@ int prepare_ring_for_helper(struct domain *d, unsigned long gmfn,
                             struct page_info **_page, void **_va);
 void destroy_ring_for_helper(void **_va, struct page_info *page);
 
+#include <asm/flushtlb.h>
+
+static inline bool accumulate_tlbflush(bool need_tlbflush,
+                                       const struct page_info *page,
+                                       uint32_t tlbflush_timestamp)
+{
+    return page->u.free.need_tlbflush &&
+           page->tlbflush_timestamp <= tlbflush_current_time() &&
+           (!need_tlbflush ||
+            page->tlbflush_timestamp > tlbflush_timestamp);
+}
+
 #endif /* __XEN_MM_H__ */


Thank you very much!

Dongli Zhang

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

Re: [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Jan Beulich]

>>> On 15.09.16 at 08:47, <dongli.zhang@oracle.com> wrote:
>> > I rewrite the inline function in xen/include/xen/mm.h to:
>> > 
>> > +#include <asm/flushtlb.h>
>> > +
>> > +static inline bool accumulate_tlbflush(bool need_tlbflush,
>> > +                                       const struct page_info *page,
>> > +                                       uint32_t tlbflush_timestamp)
>> > +{
>> > +    return page->u.free.need_tlbflush &&
>> > +           page->tlbflush_timestamp <= tlbflush_current_time() &&
>> > +           (!need_tlbflush ||
>> > +            page->tlbflush_timestamp > tlbflush_timestamp);
>> > +}
>> > 
>> > However, to use tlbflush_current_time and "asm/flushtlb.h" would lead
>> > to the following compiling error:
>> > 
>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>
>> > In file included from 
>> > /home/zhang/test/mainline-xen/xen/include/asm/flushtlb.h:14:0,
>> >                  from suspend.c:13:
>> > /home/zhang/test/mainline-xen/xen/include/xen/mm.h: In function 
>> > ‘accumulate_tlbflush’:
>> > /home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: implicit 
>> > declaration of function ‘tlbflush_current_time’ 
>> > [-Werror=implicit-function-declaration]
>> >             page->tlbflush_timestamp <= tlbflush_current_time() &&
>> >             ^
>> > /home/zhang/test/mainline-xen/xen/include/xen/mm.h:577:12: error: nested 
>> > extern declaration of ‘tlbflush_current_time’ [-Werror=nested-externs]
>> > cc1: all warnings being treated as errors
>> > make[4]: *** [suspend.o] Error 1
>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>
>> > 
>> > I can workaround the issue by removing "#include <asm/flushtlb.h>" from
>> > xen/arch/x86/acpi/suspend.c and then everything works fine.
> 
>> Removing? You should _add_ asm/tlbflush.h inclusion to xen/mm.h.
> 
> As mentioned in previous email, there was a compiling error from 
> suspend.c:13
> even after asm/tlbflush.h is added to xen/mm.h
> 
> The compiling error is bypassed if I remove the include of asm/flushtlb.h 
> from
> suspend.c. 
> 
> The following patch can make the inline function work while avoiding 
> compiling
> error. Since asm/flushtlb.h depends on get_order_from_bytes in xen/mm.h, it 
> is
> included after get_order_from_bytes in xen/mm.h.

Ah - that wasn't mentioned in your earlier mail.

> If removing asm/flushtlb.h in suspend.c is not preferred, the other two 
> options

I don't mind it being removed, it just needs to be clear why.

Jan

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