| Message ID | 20231215220612.173603-3-peter.hilber@opensynergy.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | treewide: Use clocksource id for get_device_system_crosststamp() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Dec 15, 2023 at 11:06:07PM +0100, Peter Hilber wrote: > Add a clocksource ID for TSC and a distinct one for the early TSC. > > Use distinct IDs for TSC and early TSC, since those also have distinct > clocksource structs. This should help to keep existing semantics when > comparing clocksources. > > Also, set the recently added struct system_counterval_t member cs_id to the > TSC ID in the cases where the clocksource member is being set to the TSC > clocksource. In the future, this will keep get_device_system_crosststamp() > working, when it will compare the clocksource id in struct > system_counterval_t, rather than the clocksource. > > For the x86 ART related code, system_counterval_t.cs == NULL corresponds to > system_counterval_t.cs_id == CSID_GENERIC (0). > > Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com> Hi Peter, some minor feedback from my side that you may consider for a future revision. > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c ... > @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc); > * that this flag is set before conversion to TSC is attempted. > * > * Return: > - * struct system_counterval_t - system counter value with the pointer to the > + * struct system_counterval_t - system counter value with the ID of the > * corresponding clocksource > * @cycles: System counter value > * @cs: Clocksource corresponding to system counter value. Used > * by timekeeping code to verify comparability of two cycle > * values. > + * @cs_id: Clocksource ID corresponding to system counter value. > + * Used by timekeeping code to verify comparability of two > + * cycle values. None of the documented parameters to convert_art_ns_to_tsc() above correspond to the parameters of convert_art_ns_to_tsc() below. I would suggest a separate patch to address this. And dropping this hunk from this patch. The same patch that corrects the kernel doc for convert_art_ns_to_tsc() could also correct the kernel doc for tsc_refine_calibration_work() by documenting it's work parameter. > */ > > struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns) > @@ -1347,8 +1357,11 @@ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns) > do_div(tmp, USEC_PER_SEC); > res += tmp; > > - return (struct system_counterval_t) { .cs = art_related_clocksource, > - .cycles = res}; > + return (struct system_counterval_t) { > + .cs = art_related_clocksource, > + .cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC, > + .cycles = res > + }; > } > EXPORT_SYMBOL(convert_art_ns_to_tsc); > > @@ -1454,8 +1467,10 @@ static void tsc_refine_calibration_work(struct work_struct *work) > if (tsc_unstable) > goto unreg; > > - if (boot_cpu_has(X86_FEATURE_ART)) > + if (boot_cpu_has(X86_FEATURE_ART)) { > art_related_clocksource = &clocksource_tsc; > + have_art = true; > + } > clocksource_register_khz(&clocksource_tsc, tsc_khz); > unreg: > clocksource_unregister(&clocksource_tsc_early); ...
On 24.12.23 17:27, Simon Horman wrote: > On Fri, Dec 15, 2023 at 11:06:07PM +0100, Peter Hilber wrote: >> Add a clocksource ID for TSC and a distinct one for the early TSC. >> >> Use distinct IDs for TSC and early TSC, since those also have distinct >> clocksource structs. This should help to keep existing semantics when >> comparing clocksources. >> >> Also, set the recently added struct system_counterval_t member cs_id to the >> TSC ID in the cases where the clocksource member is being set to the TSC >> clocksource. In the future, this will keep get_device_system_crosststamp() >> working, when it will compare the clocksource id in struct >> system_counterval_t, rather than the clocksource. >> >> For the x86 ART related code, system_counterval_t.cs == NULL corresponds to >> system_counterval_t.cs_id == CSID_GENERIC (0). >> >> Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com> > > Hi Peter, > > some minor feedback from my side that you may consider for > a future revision. > >> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c > > ... > >> @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc); >> * that this flag is set before conversion to TSC is attempted. >> * >> * Return: >> - * struct system_counterval_t - system counter value with the pointer to the >> + * struct system_counterval_t - system counter value with the ID of the >> * corresponding clocksource >> * @cycles: System counter value >> * @cs: Clocksource corresponding to system counter value. Used >> * by timekeeping code to verify comparability of two cycle >> * values. >> + * @cs_id: Clocksource ID corresponding to system counter value. >> + * Used by timekeeping code to verify comparability of two >> + * cycle values. > > None of the documented parameters to convert_art_ns_to_tsc() above > correspond to the parameters of convert_art_ns_to_tsc() below. > > I would suggest a separate patch to address this. > And dropping this hunk from this patch. > In the quoted documentation, @cycles, @cs and @cs_id document members of the return type struct system_counterval_t (not parameters). I will just drop the members documentation, since they are documented at the struct definition site anyway. > The same patch that corrects the kernel doc for convert_art_ns_to_tsc() > could also correct the kernel doc for tsc_refine_calibration_work() > by documenting it's work parameter. > OK. Thanks for the comments, Peter
On Sun, Dec 24 2023 at 16:27, Simon Horman wrote: > On Fri, Dec 15, 2023 at 11:06:07PM +0100, Peter Hilber wrote: >> @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc); >> * that this flag is set before conversion to TSC is attempted. >> * >> * Return: >> - * struct system_counterval_t - system counter value with the pointer to the >> + * struct system_counterval_t - system counter value with the ID of the >> * corresponding clocksource >> * @cycles: System counter value >> * @cs: Clocksource corresponding to system counter value. Used >> * by timekeeping code to verify comparability of two cycle >> * values. >> + * @cs_id: Clocksource ID corresponding to system counter value. >> + * Used by timekeeping code to verify comparability of two >> + * cycle values. > > None of the documented parameters to convert_art_ns_to_tsc() above > correspond to the parameters of convert_art_ns_to_tsc() below. Obviously not because they document the return value. The sole argument of the function @art_ns is documented correctly. > The same patch that corrects the kernel doc for convert_art_ns_to_tsc() > could also correct the kernel doc for tsc_refine_calibration_work() > by documenting it's work parameter. That's a separate cleanup. Feel free to send a patch for that. Thanks, tglx
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c index 15f97c0abc9d..9367174f7920 100644 --- a/arch/x86/kernel/tsc.c +++ b/arch/x86/kernel/tsc.c @@ -11,6 +11,7 @@ #include <linux/cpufreq.h> #include <linux/delay.h> #include <linux/clocksource.h> +#include <linux/clocksource_ids.h> #include <linux/percpu.h> #include <linux/timex.h> #include <linux/static_key.h> @@ -54,6 +55,7 @@ static u32 art_to_tsc_numerator; static u32 art_to_tsc_denominator; static u64 art_to_tsc_offset; static struct clocksource *art_related_clocksource; +static bool have_art; struct cyc2ns { struct cyc2ns_data data[2]; /* 0 + 2*16 = 32 */ @@ -1168,6 +1170,7 @@ static struct clocksource clocksource_tsc_early = { .mask = CLOCKSOURCE_MASK(64), .flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_MUST_VERIFY, + .id = CSID_X86_TSC_EARLY, .vdso_clock_mode = VDSO_CLOCKMODE_TSC, .enable = tsc_cs_enable, .resume = tsc_resume, @@ -1190,6 +1193,7 @@ static struct clocksource clocksource_tsc = { CLOCK_SOURCE_VALID_FOR_HRES | CLOCK_SOURCE_MUST_VERIFY | CLOCK_SOURCE_VERIFY_PERCPU, + .id = CSID_X86_TSC, .vdso_clock_mode = VDSO_CLOCKMODE_TSC, .enable = tsc_cs_enable, .resume = tsc_resume, @@ -1309,8 +1313,11 @@ struct system_counterval_t convert_art_to_tsc(u64 art) do_div(tmp, art_to_tsc_denominator); res += tmp + art_to_tsc_offset; - return (struct system_counterval_t) {.cs = art_related_clocksource, - .cycles = res}; + return (struct system_counterval_t) { + .cs = art_related_clocksource, + .cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC, + .cycles = res + }; } EXPORT_SYMBOL(convert_art_to_tsc); @@ -1327,12 +1334,15 @@ EXPORT_SYMBOL(convert_art_to_tsc); * that this flag is set before conversion to TSC is attempted. * * Return: - * struct system_counterval_t - system counter value with the pointer to the + * struct system_counterval_t - system counter value with the ID of the * corresponding clocksource * @cycles: System counter value * @cs: Clocksource corresponding to system counter value. Used * by timekeeping code to verify comparability of two cycle * values. + * @cs_id: Clocksource ID corresponding to system counter value. + * Used by timekeeping code to verify comparability of two + * cycle values. */ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns) @@ -1347,8 +1357,11 @@ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns) do_div(tmp, USEC_PER_SEC); res += tmp; - return (struct system_counterval_t) { .cs = art_related_clocksource, - .cycles = res}; + return (struct system_counterval_t) { + .cs = art_related_clocksource, + .cs_id = have_art ? CSID_X86_TSC : CSID_GENERIC, + .cycles = res + }; } EXPORT_SYMBOL(convert_art_ns_to_tsc); @@ -1454,8 +1467,10 @@ static void tsc_refine_calibration_work(struct work_struct *work) if (tsc_unstable) goto unreg; - if (boot_cpu_has(X86_FEATURE_ART)) + if (boot_cpu_has(X86_FEATURE_ART)) { art_related_clocksource = &clocksource_tsc; + have_art = true; + } clocksource_register_khz(&clocksource_tsc, tsc_khz); unreg: clocksource_unregister(&clocksource_tsc_early); @@ -1480,8 +1495,10 @@ static int __init init_tsc_clocksource(void) * the refined calibration and directly register it as a clocksource. */ if (boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)) { - if (boot_cpu_has(X86_FEATURE_ART)) + if (boot_cpu_has(X86_FEATURE_ART)) { art_related_clocksource = &clocksource_tsc; + have_art = true; + } clocksource_register_khz(&clocksource_tsc, tsc_khz); clocksource_unregister(&clocksource_tsc_early); diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h index 16775d7d8f8d..f8467946e9ee 100644 --- a/include/linux/clocksource_ids.h +++ b/include/linux/clocksource_ids.h @@ -6,6 +6,8 @@ enum clocksource_ids { CSID_GENERIC = 0, CSID_ARM_ARCH_COUNTER, + CSID_X86_TSC_EARLY, + CSID_X86_TSC, CSID_MAX, };
Add a clocksource ID for TSC and a distinct one for the early TSC. Use distinct IDs for TSC and early TSC, since those also have distinct clocksource structs. This should help to keep existing semantics when comparing clocksources. Also, set the recently added struct system_counterval_t member cs_id to the TSC ID in the cases where the clocksource member is being set to the TSC clocksource. In the future, this will keep get_device_system_crosststamp() working, when it will compare the clocksource id in struct system_counterval_t, rather than the clocksource. For the x86 ART related code, system_counterval_t.cs == NULL corresponds to system_counterval_t.cs_id == CSID_GENERIC (0). Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com> --- Notes: v2: - Name clock id according to Thomas Gleixner's mockup. - Refer to clocksource IDs as such in comments (Thomas Gleixner). - Update comments which were still referring to clocksource pointers. arch/x86/kernel/tsc.c | 31 ++++++++++++++++++++++++------- include/linux/clocksource_ids.h | 2 ++ 2 files changed, 26 insertions(+), 7 deletions(-)