Re: [patch 15/15] lib/vdso: Move VCLOCK_TIMENS to vdso_clock_modes

[Christophe Leroy]

Le 14/01/2020 Ã 19:52, Thomas Gleixner a ÃcritÂ:
> Move the time namespace indicator clock mode to the other ones for
> consistency sake.
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>   include/linux/clocksource.h |    3 +++
>   include/vdso/datapage.h     |    2 --
>   kernel/time/namespace.c     |    7 ++++---
>   lib/vdso/gettimeofday.c     |   18 ++++++++++--------
>   4 files changed, 17 insertions(+), 13 deletions(-)
>
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -34,6 +34,9 @@ enum vdso_clock_mode {
>   	VDSO_ARCH_CLOCKMODES,
>   #endif
>   	VDSO_CLOCKMODE_MAX,
> +
> +	/* Indicator for time namespace VDSO */ > +	VDSO_CLOCKMODE_TIMENS = INT_MAX

Can we shorten the names a bit in order to avoid code line splits ? 
WOuldn't something like VDSO_CMODE_XXX be explicit enough ?

Christophe

>   };
>   
>   /**
> --- a/include/vdso/datapage.h
> +++ b/include/vdso/datapage.h
> @@ -21,8 +21,6 @@
>   #define CS_RAW		1
>   #define CS_BASES	(CS_RAW + 1)
>   
> -#define VCLOCK_TIMENS	UINT_MAX
> -
>   /**
>    * struct vdso_timestamp - basetime per clock_id
>    * @sec:	seconds
> --- a/kernel/time/namespace.c
> +++ b/kernel/time/namespace.c
> @@ -8,6 +8,7 @@
>   #include <linux/user_namespace.h>
>   #include <linux/sched/signal.h>
>   #include <linux/sched/task.h>
> +#include <linux/clocksource.h>
>   #include <linux/seq_file.h>
>   #include <linux/proc_ns.h>
>   #include <linux/export.h>
> @@ -172,8 +173,8 @@ static struct timens_offset offset_from_
>    * for vdso_data->clock_mode is a non-issue. The task is spin waiting for the
>    * update to finish and for 'seq' to become even anyway.
>    *
> - * Timens page has vdso_data->clock_mode set to VCLOCK_TIMENS which enforces
> - * the time namespace handling path.
> + * Timens page has vdso_data->clock_mode set to VDSO_CLOCKMODE_TIMENS which
> + * enforces the time namespace handling path.
>    */
>   static void timens_setup_vdso_data(struct vdso_data *vdata,
>   				   struct time_namespace *ns)
> @@ -183,7 +184,7 @@ static void timens_setup_vdso_data(struc
>   	struct timens_offset boottime = offset_from_ts(ns->offsets.boottime);
>   
>   	vdata->seq			= 1;
> -	vdata->clock_mode		= VCLOCK_TIMENS;
> +	vdata->clock_mode		= VDSO_CLOCKMODE_TIMENS;
>   	offset[CLOCK_MONOTONIC]		= monotonic;
>   	offset[CLOCK_MONOTONIC_RAW]	= monotonic;
>   	offset[CLOCK_MONOTONIC_COARSE]	= monotonic;
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -116,10 +116,10 @@ static __always_inline int do_hres(const
>   
>   	do {
>   		/*
> -		 * Open coded to handle VCLOCK_TIMENS. Time namespace
> +		 * Open coded to handle VDSO_CLOCKMODE_TIMENS. Time namespace
>   		 * enabled tasks have a special VVAR page installed which
>   		 * has vd->seq set to 1 and vd->clock_mode set to
> -		 * VCLOCK_TIMENS. For non time namespace affected tasks
> +		 * VDSO_CLOCKMODE_TIMENS. For non time namespace affected tasks
>   		 * this does not affect performance because if vd->seq is
>   		 * odd, i.e. a concurrent update is in progress the extra
>   		 * check for vd->clock_mode is just a few extra
> @@ -128,7 +128,7 @@ static __always_inline int do_hres(const
>   		 */
>   		while (unlikely((seq = READ_ONCE(vd->seq)) & 1)) {
>   			if (IS_ENABLED(CONFIG_TIME_NS) &&
> -			    vd->clock_mode == VCLOCK_TIMENS)
> +			    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>   				return do_hres_timens(vd, clk, ts);
>   			cpu_relax();
>   		}
> @@ -200,12 +200,12 @@ static __always_inline int do_coarse(con
>   
>   	do {
>   		/*
> -		 * Open coded to handle VCLOCK_TIMENS. See comment in
> +		 * Open coded to handle VDSO_CLOCK_TIMENS. See comment in
>   		 * do_hres().
>   		 */
>   		while ((seq = READ_ONCE(vd->seq)) & 1) {
>   			if (IS_ENABLED(CONFIG_TIME_NS) &&
> -			    vd->clock_mode == VCLOCK_TIMENS)
> +			    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>   				return do_coarse_timens(vd, clk, ts);
>   			cpu_relax();
>   		}
> @@ -292,7 +292,7 @@ static __maybe_unused int
>   
>   	if (unlikely(tz != NULL)) {
>   		if (IS_ENABLED(CONFIG_TIME_NS) &&
> -		    vd->clock_mode == VCLOCK_TIMENS)
> +		    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>   			vd = __arch_get_timens_vdso_data();
>   
>   		tz->tz_minuteswest = vd[CS_HRES_COARSE].tz_minuteswest;
> @@ -308,7 +308,8 @@ static __maybe_unused __kernel_old_time_
>   	const struct vdso_data *vd = __arch_get_vdso_data();
>   	__kernel_old_time_t t;
>   
> -	if (IS_ENABLED(CONFIG_TIME_NS) && vd->clock_mode == VCLOCK_TIMENS)
> +	if (IS_ENABLED(CONFIG_TIME_NS) &&
> +	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>   		vd = __arch_get_timens_vdso_data();
>   
>   	t = READ_ONCE(vd[CS_HRES_COARSE].basetime[CLOCK_REALTIME].sec);
> @@ -333,7 +334,8 @@ int __cvdso_clock_getres_common(clockid_
>   	if (unlikely((u32) clock >= MAX_CLOCKS))
>   		return -1;
>   
> -	if (IS_ENABLED(CONFIG_TIME_NS) && vd->clock_mode == VCLOCK_TIMENS)
> +	if (IS_ENABLED(CONFIG_TIME_NS) &&
> +	    vd->clock_mode == VDSO_CLOCKMODE_TIMENS)
>   		vd = __arch_get_timens_vdso_data();
>   
>   	hrtimer_res = READ_ONCE(vd[CS_HRES_COARSE].hrtimer_res);