Received-SPF: pass (google.com: domain of linux-kernel+bounces-48357-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 1 Feb 2024 16:05:21 +0100
From: Frederic Weisbecker <frederic@kernel.org>
To: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
	John Stultz <jstultz@google.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Eric Dumazet <edumazet@google.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
	Arjan van de Ven <arjan@infradead.org>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Rik van Riel <riel@surriel.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Sebastian Siewior <bigeasy@linutronix.de>,
	Giovanni Gherdovich <ggherdovich@suse.cz>,
	Lukasz Luba <lukasz.luba@arm.com>,
	"Gautham R . Shenoy" <gautham.shenoy@amd.com>,
	Srinivas Pandruvada <srinivas.pandruvada@intel.com>,
	K Prateek Nayak <kprateek.nayak@amd.com>
Subject: Re: [PATCH v10 18/20] timers: Implement the hierarchical pull model
Message-ID: <ZbuzMeKlCgebhJZ-@localhost.localdomain>
References: <20240115143743.27827-1-anna-maria@linutronix.de>
 <20240115143743.27827-19-anna-maria@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240115143743.27827-19-anna-maria@linutronix.de>

Le Mon, Jan 15, 2024 at 03:37:41PM +0100, Anna-Maria Behnsen a ?crit :
> +static void tmigr_connect_child_parent(struct tmigr_group *child,
> +				       struct tmigr_group *parent)
> +{
> +	union tmigr_state childstate;
> +
> +	raw_spin_lock_irq(&child->lock);
> +	raw_spin_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
> +
> +	child->parent = parent;
> +	child->childmask = BIT(parent->num_children++);
> +
> +	raw_spin_unlock(&parent->lock);
> +	raw_spin_unlock_irq(&child->lock);
> +
> +	/*
> +	 * To prevent inconsistent states, active children need to be active in
> +	 * the new parent as well. Inactive children are already marked inactive
> +	 * in the parent group.
> +	 */
> +	childstate.state = atomic_read(&child->migr_state);
> +	if (childstate.migrator != TMIGR_NONE) {

Is it possible here to connect a running online child (not one that we just
created) to a new parent? If not, is it possible that a newly created child is
not TMIGR_NONE?


> +		struct tmigr_walk data;
> +
> +		data.childmask = child->childmask;
> +
> +		/*
> +		 * There is only one new level per time. When connecting the
> +		 * child and the parent and set the child active when the parent
> +		 * is inactive, the parent needs to be the uppermost
> +		 * level. Otherwise there went something wrong!
> +		 */
> +		WARN_ON(!tmigr_active_up(parent, child, &data) && parent->parent);
> +	}
> +}
[...]
> +static int tmigr_cpu_online(unsigned int cpu)
> +{
> +	struct tmigr_cpu *tmc = this_cpu_ptr(&tmigr_cpu);
> +	int ret;
> +
> +	/* First online attempt? Initialize CPU data */
> +	if (!tmc->tmgroup) {
> +		raw_spin_lock_init(&tmc->lock);
> +
> +		ret = tmigr_add_cpu(cpu);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (tmc->childmask == 0)
> +			return -EINVAL;
> +
> +		timerqueue_init(&tmc->cpuevt.nextevt);
> +		tmc->cpuevt.nextevt.expires = KTIME_MAX;
> +		tmc->cpuevt.ignore = true;
> +		tmc->cpuevt.cpu = cpu;
> +
> +		tmc->remote = false;
> +		WRITE_ONCE(tmc->wakeup, KTIME_MAX);
> +	}
> +	raw_spin_lock_irq(&tmc->lock);
> +	tmc->idle = timer_base_is_idle();
> +	if (!tmc->idle)
> +		__tmigr_cpu_activate(tmc);

Heh, I was about to say that it's impossible that timer_base_is_idle()
at this stage but actually if we run in nohz_full...

It happens so that nohz_full is deactivated until rcutree_online_cpu()
which calls tick_dep_clear() but it's a pure coincidence that might
disappear one day. So yes, let's keep it that way.

Thanks.

> +	tmc->online = true;
> +	raw_spin_unlock_irq(&tmc->lock);
> +	return 0;
> +}