linux-kernel - Re: [PATCH v5 16/18] timer: Implement the hierarchical pull model

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2023 16:05:27 +0200 (CEST)
From:   Anna-Maria Behnsen <anna-maria@...utronix.de>
To:     Frederic Weisbecker <frederic@...nel.org>
cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Eric Dumazet <edumazet@...gle.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Arjan van de Ven <arjan@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <fweisbec@...il.com>,
        Rik van Riel <riel@...riel.com>
Subject: Re: [PATCH v5 16/18] timer: Implement the hierarchical pull model

On Tue, 21 Mar 2023, Frederic Weisbecker wrote:

> On Wed, Mar 01, 2023 at 03:17:42PM +0100, Anna-Maria Behnsen wrote:
> > +static u64 tmigr_handle_remote_cpu(unsigned int cpu, u64 now,
> > +				   unsigned long jif)
> > +{
> > +	struct timer_events tevt;
> > +	struct tmigr_walk data;
> > +	struct tmigr_cpu *tmc;
> > +	u64 next = KTIME_MAX;
> > +	unsigned long flags;
> > +
> > +	tmc = per_cpu_ptr(&tmigr_cpu, cpu);
> > +
> > +	raw_spin_lock_irqsave(&tmc->lock, flags);
> > +	/*
> > +	 * Remote CPU is offline or no longer idle or other cpu handles cpu
> > +	 * timers already or next event was already expired - return!
> > +	 */
> > +	if (!tmc->online || tmc->remote || tmc->cpuevt.ignore ||
> > +	    now < tmc->cpuevt.nextevt.expires) {
> > +		raw_spin_unlock_irqrestore(&tmc->lock, flags);
> > +		return next;
> > +	}
> > +
> > +	tmc->remote = 1;
> > +
> > +	/* Drop the lock to allow the remote CPU to exit idle */
> > +	raw_spin_unlock_irqrestore(&tmc->lock, flags);
> > +
> > +	if (cpu != smp_processor_id())
> > +		timer_expire_remote(cpu);
> > +
> > +	/* next event of cpu */
> > +	fetch_next_timer_interrupt_remote(jif, now, &tevt, cpu);
> 
> If the target CPU gets an idle interrupt right after the above call and enqueues
> a new timer (which becomes the new earliest), tmigr_cpu_deactivate() ->
> tmigr_new_timer() is going to ignore it due to tmc->remote = 1, right?

It's worse. The newly enqueued timer is updated in the timer migration
hierarchy when CPU goes back idle and afterwards it will be overwritten by
the group walk propagating the old first timer in
tmigr_handle_remote_cpu()...

I will change the code after remote timer expiry:

1. take the remote timer bases locks
2. take the tmc->lock
3. get the next timer interrupt remote
4. drop the remote timer bases locks
5. propagate new timer changes
6. drop the tmc->lock

Thanks,

	Anna-Maria