Messages in this thread | | | From | Nadav Amit <> | Subject | Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr() | Date | Wed, 18 Oct 2023 14:46:23 +0000 |
| |
> On Oct 18, 2023, at 4:15 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > >> >> Looks like another case of underspecified functionality where both >> compilers differ. Luckily, both DTRT when aliases are hidden in >> another TU. > > Attached is the prototype patch that works for me (together with > Linus' FPU switching patch).
In general looks good. See some minor issues below.
> --- a/arch/x86/include/asm/current.h > +++ b/arch/x86/include/asm/current.h > @@ -36,10 +36,23 @@ static_assert(sizeof(struct pcpu_hot) == 64); > > DECLARE_PER_CPU_ALIGNED(struct pcpu_hot, pcpu_hot); > > +/* > + * > + */
Obviously some further comments to clarify why struct pcpu_hot is defined in percpu-hot.c (the GCC manual says: "It is an error if the alias target is not defined in the same translation unit as the alias” which can be used as part of the explanation.)
> +DECLARE_PER_CPU_ALIGNED(const struct pcpu_hot __percpu_seg_override, > + const_pcpu_hot); > + > +#ifdef CONFIG_USE_X86_SEG_SUPPORT > +static __always_inline struct task_struct *get_current(void) > +{ > + return const_pcpu_hot.current_task; > +} > +#else > static __always_inline struct task_struct *get_current(void) > { > return this_cpu_read_stable(pcpu_hot.current_task); > } > +#endif
Please consider using IS_ENABLED() to avoid the ifdef’ry.
So this would turn to be:
static __always_inline struct task_struct *get_current(void) { if (IS_ENABLED(CONFIG_USE_X86_SEG_SUPPORT)) return const_pcpu_hot.current_task;
return this_cpu_read_stable(pcpu_hot.current_task); }
| |