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