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

[Jan Beulich]

>>> On 15.09.16 at 08:47,  wrote:
>> > I rewrite the inline function in xen/include/xen/mm.h to:
>> > 
>> > +#include 
>> > +
>> > +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 " 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

Re: [Xen-devel] [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 
> > +
> > +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 " 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 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
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 
+
+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: [Xen-devel] [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Jan Beulich]

>>> On 15.09.16 at 00:36,  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 
> +
> +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 " 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: [Xen-devel] [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 
+
+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 " 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: [Xen-devel] [PATCH v4 1/2] xen: replace complicated tlbflush check with an inline function

[Jan Beulich]

>>> On 12.09.16 at 10:16,  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

[Xen-devel] [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 
---
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([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