Re: [RFC] x86: merge nmi_32-64 to nmi.c

["Tom Spink" <tspink@gmail.com>]

2008/5/17 Cyrill Gorcunov <gorcunov@gmail.com>:
> +/* a few helper functions */
> +#ifdef CONFIG_X86_64
> +
> +static inline unsigned int get_nmi_count(int cpu)
> +{
> +       return cpu_pda(cpu)->__nmi_count;
> +}
> +
> +static inline int mce_in_progress(void)
> +{
> +#ifdef CONFIG_X86_MCE
> +       return atomic_read(&mce_entry) > 0;
> +#endif
> +       return 0;
> +}
> +
> +static inline void __die_nmi(char *str, struct pt_regs *regs, int do_panic)
> +{
> +       die_nmi(str, regs, do_panic);
> +}
> +
> +#else /* !CONFIG_X86_64 */
> +
> +static inline unsigned int get_nmi_count(int cpu)
> +{
> +       return nmi_count(cpu);
> +}
> +
> +static inline int mce_in_progress(void)
> +{
> +       return 0;
> +}
> +
> +static inline void __die_nmi(char *str, struct pt_regs *regs, int do_panic)
> +{
> +       die_nmi(regs, str);
> +}
> +
> +#endif /* !CONFIG_X86_64 */

Hi,

I've always wondered if it's cleaner to define variants of functions
like this with the conditionals inside the function, as opposed to one
big conditional encapsulating all these functions.  IMO, it's cleaner
to define the function with conditionals to define it's particular
behaviour in the two different cases, because that way there is one
definition of the function with both different behaviours inside,
e.g.:

static inline unsigned int get_nmi_count(int cpu)
{
#ifdef CONFIG_X86_64
        return cpu_pda(cpu)->__nmi_count;
#else
        return nmi_count(cpu);
#endif
}

I know it introduces a lot of these conditionals, but at least there
is one place to look for the get_nmi_count function, instead of
searching for all variants of the function.

Just a thought!

-- 
Regards,
Tom Spink