Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Feng Tang <feng.tang@intel.com>, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>, x86@kernel.org,
        linux-kernel@vger.kernel.org
Cc:     rui.zhang@intel.com, andi.kleen@intel.com, dave.hansen@intel.com,
        len.brown@intel.com, Feng Tang <feng.tang@intel.com>
Subject: Re: [RFC 1/2] x86/tsc: add a timer to make sure tsc_adjust is always checked
In-Reply-To: <1617092747-15769-1-git-send-email-feng.tang@intel.com>
References: <1617092747-15769-1-git-send-email-feng.tang@intel.com>
Date:   Sat, 10 Apr 2021 11:27:11 +0200
Message-ID: <87y2dq32xc.ffs@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Tue, Mar 30 2021 at 16:25, Feng Tang wrote:
> Normally the tsc_sync will be checked every time system enters idle state,
> but there is still caveat that a system won't enter idle, either because
> it's too busy or configured purposely to not enter idle. Setup a periodic
> timer to make sure the check is always on.

Bah. I really hate the fact that we don't have a knob to disable writes
to the TSC/TSC_ADJUST msrs. That would spare this business alltogether.

> +/*
> + * Normally the tsc_sync will be checked every time system enters idle state,
> + * but there is still caveat that a system won't enter idle, either because
> + * it's too busy or configured purposely to not enter idle.
> + *
> + * So setup a periodic timer to make sure the check is always on.
> + */
> +
> +#define SYNC_CHECK_INTERVAL		(HZ * 600)
> +static void tsc_sync_check_timer_fn(struct timer_list *unused)

I've surely mentioned this before that glueing a define without an empty
newline to a function definition is horrible to read.

> +{
> +	int next_cpu;
> +
> +	tsc_verify_tsc_adjust(false);
> +
> +	/* Loop to do the check for all onlined CPUs */

I don't see a loop here.

> +	next_cpu = cpumask_next(raw_smp_processor_id(), cpu_online_mask);

Why raw_smp_processor_id()? What's wrong with smp_processor_id()?

> +	if (next_cpu >= nr_cpu_ids)
> +		next_cpu = cpumask_first(cpu_online_mask);
> +
> +	tsc_sync_check_timer.expires += SYNC_CHECK_INTERVAL;
> +	add_timer_on(&tsc_sync_check_timer, next_cpu);
> +}
> +
> +static int __init start_sync_check_timer(void)
> +{
> +	if (!boot_cpu_has(X86_FEATURE_TSC_ADJUST))
> +		return 0;
> +
> +	timer_setup(&tsc_sync_check_timer, tsc_sync_check_timer_fn, 0);
> +	tsc_sync_check_timer.expires = jiffies + SYNC_CHECK_INTERVAL;
> +	add_timer(&tsc_sync_check_timer);
> +
> +	return 0;
> +}
> +late_initcall(start_sync_check_timer);

So right now, if someone adds 'tsc=reliable' on the kernel command line
then all of the watchdog checking, except for the idle enter TSC_ADJUST
check is disabled. The NOHZ full people are probably going to be pretty
unhappy about yet another unconditional timer they have to chase down.

So this needs some more thought.

Thanks,

        tglx