diff mbox series

[v5,01/23] kernel: Standardize vdso_datapage

Message ID 20190222122430.21180-2-vincenzo.frascino@arm.com (mailing list archive)
State New, archived
Headers show
Series Unify vDSOs across more architectures | expand

Commit Message

Vincenzo Frascino Feb. 22, 2019, 12:24 p.m. UTC 
In an effort to unify the common code for managing the vdso library in
between all the architectures that support it, this patch tries to
provide a common format for the vdso datapage.

As a result of this, this patch generalized the data structures in vgtod.h
from x86 private includes to general includes (include/vdso).

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 include/vdso/datapage.h | 74 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 include/vdso/datapage.h

Comments

Mark Rutland Feb. 22, 2019, 12:58 p.m. UTC | #1 
Hi Vincenzo,

On Fri, Feb 22, 2019 at 12:24:08PM +0000, Vincenzo Frascino wrote:
> +#ifdef __KERNEL__

I believe that __KERNEL__ guards haven't been necessary (in non-UAPI
headers) since the UAPI split, and can be removed.

[...]

> +#include <linux/bits.h>
> +#include <linux/types.h>
> +#include <linux/time.h>

Nit: could we please keep these in alphabetical order?

> +/*
> + * There is one vdso_timestamp object in vvar for each vDSO-accelerated
> + * clock_id. For high-resolution clocks, this encodes the time
> + * corresponding to vdso_data.cycle_last. For coarse clocks this encodes
> + * the actual time.
> + *
> + * To be noticed that for highres clocks nsec is left-shifted by
> + * vdso_data.cs[x].shift.

Nit: this would read better as:

	Note that for highres clocks nsec is left-shifted by
	vdso_data.cs[x].shift.

[...]

> +/*
> + * vdso_data will be accessed by 32 and 64 bit code at the same time
> + * so we should be careful before modifying this structure.
> + */

Perhaps:

/*
 * This structure will be accessed by native and compat vDSOs, so we
 * need to ensure the layout is identical for native and compat code.
 */

... which I think is what you're trrying to say?

Thanks,
Mark.
Thomas Gleixner Feb. 23, 2019, 4:51 p.m. UTC | #2 
On Fri, 22 Feb 2019, Vincenzo Frascino wrote:
> +/*
> + * There is one vdso_clocksource object in vvar for each vDSO clocksource
> + * (mono, raw). This struct is designed to keep vdso_data "cache-line friendly"
> + * and optimal in terms of access pattern.
> + *
> + * Note that mask and shift are the same for mono and raw.
> + */
> +struct vdso_clocksource {
> +	u64 mask;		/* Clocksource mask */
> +	u32 mult;		/* Clocksource multiplier */
> +	u32 shift;		/* Clocksource shift */

Can you please get rid of the tail comments and use proper kerneldoc
format?

> +/*
> + * vdso_data will be accessed by 32 and 64 bit code at the same time
> + * so we should be careful before modifying this structure.
> + */
> +struct vdso_data {
> +	u32 seq;		/* Timebase sequence counter */
> +
> +	s32 clock_mode;
> +	u64 cycle_last;		/* Timebase at clocksource init */
> +
> +	struct vdso_clocksource cs[CLOCKSOURCE_BASES];

Why would you need different clocksource parameters? That really bloats the
data structure and makes the cache access pattern worse. Also the vdso
update needs to copy the same data over and over for no value.

The only clock ID which needs a different mult/shift would be
MONOTONIC_RAW, but if we expose that through the VDSO then we really can be
smarter than this. See incomplete and uncompilable patch below for
reference. You get the idea.

> +	struct vdso_timestamp basetime[VDSO_BASES];
> +
> +	s32 tz_minuteswest;	/* Timezone definitions */
> +	s32 tz_dsttime;

Also please keep the tabular alignment of the members

	u32		foo;
	struct bar	bar;

that's way better to read than

	u32 foo;
	struct bar bar;

Thanks

	tglx

8<-----------------

--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -139,9 +139,10 @@ notrace static inline u64 vgetcyc(int mo
 	return U64_MAX;
 }
 
-notrace static int do_hres(clockid_t clk, struct timespec *ts)
+notrace static int do_hres(clockid_t clk, struct timespec *ts,
+			   struct vsyscall_gtod_data *vdata)
 {
-	struct vgtod_ts *base = &gtod->basetime[clk];
+	struct vgtod_ts *base = &vdata->basetime[clk];
 	u64 cycles, last, sec, ns;
 	unsigned int seq;
 
@@ -168,9 +169,10 @@ notrace static int do_hres(clockid_t clk
 	return 0;
 }
 
-notrace static void do_coarse(clockid_t clk, struct timespec *ts)
+notrace static void do_coarse(clockid_t clk, struct timespec *ts,
+			      struct vsyscall_gtod_data *vdata)
 {
-	struct vgtod_ts *base = &gtod->basetime[clk];
+	struct vgtod_ts *base = &vdata->basetime[clk];
 	unsigned int seq;
 
 	do {
@@ -194,10 +196,12 @@ notrace int __vdso_clock_gettime(clockid
 	 */
 	msk = 1U << clock;
 	if (likely(msk & VGTOD_HRES)) {
-		return do_hres(clock, ts);
+		return do_hres(clock, ts, &gtod[0]);
 	} else if (msk & VGTOD_COARSE) {
-		do_coarse(clock, ts);
+		do_coarse(clock, ts, &gtod[0]);
 		return 0;
+	} else if (msk & VGTOD_RAW) {
+		return do_hres(clock, ts, &gtod[1]);
 	}
 	return vdso_fallback_gettime(clock, ts);
 }
@@ -210,7 +214,7 @@ notrace int __vdso_gettimeofday(struct t
 	if (likely(tv != NULL)) {
 		struct timespec *ts = (struct timespec *) tv;
 
-		do_hres(CLOCK_REALTIME, ts);
+		do_hres(CLOCK_REALTIME, ts, &gtod[0]);
 		tv->tv_usec /= 1000;
 	}
 	if (unlikely(tz != NULL)) {
--- a/arch/x86/include/asm/vgtod.h
+++ b/arch/x86/include/asm/vgtod.h
@@ -30,6 +30,7 @@ struct vgtod_ts {
 #define VGTOD_BASES	(CLOCK_TAI + 1)
 #define VGTOD_HRES	(BIT(CLOCK_REALTIME) | BIT(CLOCK_MONOTONIC) | BIT(CLOCK_TAI))
 #define VGTOD_COARSE	(BIT(CLOCK_REALTIME_COARSE) | BIT(CLOCK_MONOTONIC_COARSE))
+#define VGTOD_RAW	BIT(CLOCK_MOTONONIC_RAW)
 
 /*
  * vsyscall_gtod_data will be accessed by 32 and 64 bit code at the same time
@@ -49,7 +50,8 @@ struct vsyscall_gtod_data {
 	int		tz_minuteswest;
 	int		tz_dsttime;
 };
-extern struct vsyscall_gtod_data vsyscall_gtod_data;
+
+extern struct vsyscall_gtod_data vsyscall_gtod_data[2];
 
 extern int vclocks_used;
 static inline bool vclock_was_used(int vclock)
Vincenzo Frascino Feb. 27, 2019, 2:23 p.m. UTC | #3 
On 23/02/2019 16:51, Thomas Gleixner wrote:
> On Fri, 22 Feb 2019, Vincenzo Frascino wrote:
>> +/*
>> + * There is one vdso_clocksource object in vvar for each vDSO clocksource
>> + * (mono, raw). This struct is designed to keep vdso_data "cache-line friendly"
>> + * and optimal in terms of access pattern.
>> + *
>> + * Note that mask and shift are the same for mono and raw.
>> + */
>> +struct vdso_clocksource {
>> +	u64 mask;		/* Clocksource mask */
>> +	u32 mult;		/* Clocksource multiplier */
>> +	u32 shift;		/* Clocksource shift */
> 
> Can you please get rid of the tail comments and use proper kerneldoc
> format?
>

I will fix it in v6, thanks.

>> +/*
>> + * vdso_data will be accessed by 32 and 64 bit code at the same time
>> + * so we should be careful before modifying this structure.
>> + */
>> +struct vdso_data {
>> +	u32 seq;		/* Timebase sequence counter */
>> +
>> +	s32 clock_mode;
>> +	u64 cycle_last;		/* Timebase at clocksource init */
>> +
>> +	struct vdso_clocksource cs[CLOCKSOURCE_BASES];
> 
> Why would you need different clocksource parameters? That really bloats the
> data structure and makes the cache access pattern worse. Also the vdso
> update needs to copy the same data over and over for no value.
> 
> The only clock ID which needs a different mult/shift would be
> MONOTONIC_RAW, but if we expose that through the VDSO then we really can be
> smarter than this. See incomplete and uncompilable patch below for
> reference. You get the idea.
> 

Ok, thank you for providing the reference code. I will update my patches in v6
accordingly.

...
diff mbox series

Patch

diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
new file mode 100644
index 000000000000..da346ad02b03
--- /dev/null
+++ b/include/vdso/datapage.h
@@ -0,0 +1,74 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __VDSO_DATAPAGE_H
+#define __VDSO_DATAPAGE_H
+
+#ifdef __KERNEL__
+
+#ifndef __ASSEMBLY__
+
+#include <linux/bits.h>
+#include <linux/types.h>
+#include <linux/time.h>
+
+#define VDSO_BASES	(CLOCK_TAI + 1)
+#define VDSO_HRES	(BIT(CLOCK_REALTIME)		| \
+			 BIT(CLOCK_MONOTONIC)		| \
+			 BIT(CLOCK_MONOTONIC_RAW)	| \
+			 BIT(CLOCK_BOOTTIME)		| \
+			 BIT(CLOCK_TAI))
+#define VDSO_COARSE	(BIT(CLOCK_REALTIME_COARSE)	| \
+			 BIT(CLOCK_MONOTONIC_COARSE))
+
+/*
+ * There is one vdso_timestamp object in vvar for each vDSO-accelerated
+ * clock_id. For high-resolution clocks, this encodes the time
+ * corresponding to vdso_data.cycle_last. For coarse clocks this encodes
+ * the actual time.
+ *
+ * To be noticed that for highres clocks nsec is left-shifted by
+ * vdso_data.cs[x].shift.
+ */
+struct vdso_timestamp {
+	u64 sec;
+	u64 nsec;
+};
+
+#define CLOCKSOURCE_RAW		0
+#define CLOCKSOURCE_MONO	1
+#define CLOCKSOURCE_BASES	(CLOCKSOURCE_MONO + 1)
+
+/*
+ * There is one vdso_clocksource object in vvar for each vDSO clocksource
+ * (mono, raw). This struct is designed to keep vdso_data "cache-line friendly"
+ * and optimal in terms of access pattern.
+ *
+ * Note that mask and shift are the same for mono and raw.
+ */
+struct vdso_clocksource {
+	u64 mask;		/* Clocksource mask */
+	u32 mult;		/* Clocksource multiplier */
+	u32 shift;		/* Clocksource shift */
+};
+
+/*
+ * vdso_data will be accessed by 32 and 64 bit code at the same time
+ * so we should be careful before modifying this structure.
+ */
+struct vdso_data {
+	u32 seq;		/* Timebase sequence counter */
+
+	s32 clock_mode;
+	u64 cycle_last;		/* Timebase at clocksource init */
+
+	struct vdso_clocksource cs[CLOCKSOURCE_BASES];
+	struct vdso_timestamp basetime[VDSO_BASES];
+
+	s32 tz_minuteswest;	/* Timezone definitions */
+	s32 tz_dsttime;
+};
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __KERNEL__ */
+
+#endif /* __VDSO_DATAPAGE_H */