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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.09.16 at 00:36, <dongli.zhang@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel