Re: tick/sched: Make jiffies update quick check more robust

[Peter Zijlstra]

On Fri, Dec 04, 2020 at 11:55:19AM +0100, Thomas Gleixner wrote:
>  	/*
> +	 * 64bit can do a quick check without holding jiffies lock and
> +	 * without looking at the sequence count. The smp_load_acquire()
>  	 * pairs with the update done later in this function.
>  	 *
> +	 * 32bit cannot do that because the store of tick_next_period
> +	 * consists of two 32bit stores and the first store could move it
> +	 * to a random point in the future.
>  	 */
> +	if (IS_ENABLED(CONFIG_64BIT)) {
> +		if (ktime_before(now, smp_load_acquire(&tick_next_period)))
> +			return;

Explicit ACQUIRE

> +	} else {
> +		unsigned int seq;
> +
> +		/*
> +		 * Avoid contention on jiffies_lock and protect the quick
> +		 * check with the sequence count.
> +		 */
> +		do {
> +			seq = read_seqcount_begin(&jiffies_seq);
> +			nextp = tick_next_period;
> +		} while (read_seqcount_retry(&jiffies_seq, seq));
> +
> +		if (ktime_before(now, nextp))
> +			return;

Actually has an implicit ACQUIRE:

	read_seqcount_retry() implies smp_rmb(), which ensures
	LOAD->LOAD order, IOW any later load must happen after our
	@tick_next_period load.

	Then it has a control dependency on ktime_before(,nextp), which
	ensures LOAD->STORE order.

	Combined we have a LOAD->{LOAD,STORE} order on the
	@tick_next_period load, IOW ACQUIRE.

> +	}
>  
> +	/* Quick check failed, i.e. update is required. */
>  	raw_spin_lock(&jiffies_lock);

Another ACQUIRE, which means the above ACQUIRE only ensures we load the
lock value after?

Or are we trying to guarantee the caller is sure to observe the new
jiffies value if we return?

> +	/*
> +	 * Reevaluate with the lock held. Another CPU might have done the
> +	 * update already.
> +	 */
>  	if (ktime_before(now, tick_next_period)) {
>  		raw_spin_unlock(&jiffies_lock);
>  		return;
> @@ -112,11 +118,25 @@ static void tick_do_update_jiffies64(kti
>  	jiffies_64 += ticks;
>  
>  	/*
> +	 * Keep the tick_next_period variable up to date.
>  	 */
> +	nextp = ktime_add_ns(last_jiffies_update, TICK_NSEC);
> +
> +	if (IS_ENABLED(CONFIG_64BIT)) {
> +		/*
> +		 * Pairs with smp_load_acquire() in the lockless quick
> +		 * check above and ensures that the update to jiffies_64 is
> +		 * not reordered vs. the store to tick_next_period, neither
> +		 * by the compiler nor by the CPU.
> +		 */
> +		smp_store_release(&tick_next_period, nextp);
> +	} else {
> +		/*
> +		 * A plain store is good enough on 32bit as the quick check
> +		 * above is protected by the sequence count.
> +		 */
> +		tick_next_period = nextp;
> +	}
>  
>  	/*
>  	 * Release the sequence count. calc_global_load() below is not