Hi Christophe,
On Wed, Dec 18, 2024 at 08:20:51AM +0100, Christophe Leroy wrote:
> Le 16/12/2024 à 15:10, Thomas Weißschuh a écrit :
[..]
> > #ifdef CONFIG_TIME_NS
> > -static __always_inline
> > -const struct vdso_data *__arch_get_timens_vdso_data(const struct vdso_data *vd)
> > +static __always_inline const struct vdso_time_data *__ppc_get_vdso_u_timens_data(void)
> > {
> > - return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
> > + struct vdso_time_data *time_data;
> > +
> > + asm(
> > + " bcl 20, 31, .+4\n"
> > + "0: mflr %0\n"
> > + " addis %0, %0, (vdso_u_timens_data - 0b)@ha\n"
> > + " addi %0, %0, (vdso_u_timens_data - 0b)@l\n"
> > + : "=r" (time_data) :: "lr");
> > +
> > + return time_data;
>
> Please don't do that, it kills optimisation efforts done when implementing
> VDSO time. Commit ce7d8056e38b ("powerpc/vdso: Prepare for switching VDSO to
> generic C implementation.") explains why.
>
> For time data, the bcl/mflr dance is done by get_datapage macro called by
> cvdso_call macro in gettimeofday.S, and given to
> __cvdso_clock_gettime_data() by __c_kernel_clock_gettime() in
> vgettimeofday.c . Use that information and don't redo the bcl/mflr sequence.
So instead keeping the logic of this:
static __always_inline
const struct vdso_data *__arch_get_timens_vdso_data(const struct vdso_data *vd)
{
return (void *)vd + (1U << CONFIG_PAGE_SHIFT);
}
Makes sense.
Adding a constant value should be cheaper or just as cheap as a
PC-relative addressing for all architectures, so it can go into the
generic code, too.
[..]
> > }
> > +#define __arch_get_vdso_u_timens_data __ppc_get_vdso_u_timens_data
>
> There is not #ifdef __arch_get_vdso_u_timens_data anywhere, this #define is
> not needed, the function should be called __arch_get_vdso_u_timens_data()
> directly as before, unnecessary indirections reduce readability.
I'll see how this works out with the include order and conflicts with
symbols in include/vdso/datapage.h.
> > #endif
> > -static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
> > +static inline bool vdso_clocksource_ok(const struct vdso_time_data *vd)
> > {
> > return true;
> > }
Thanks!
Thomas
