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

Commit Message

Dongli Zhang Sept. 15, 2016, 6:47 a.m. UTC

> > 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.



Thank you very much!

Dongli Zhang

Comments

Jan Beulich Sept. 15, 2016, 6:59 a.m. UTC | #1

>>> On 15.09.16 at 08:47, <[e-mail]> 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

Patch

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__ */