Re: [PATCH v2 1/5] PM: domains: Add GENPD_FLAG_RT_SAFE for PREEMPT_RT

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Kevin Hilman <khilman@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Adrien Thierry <athierry@redhat.com>,
	Brian Masney <bmasney@redhat.com>,
	linux-rt-users@vger.kernel.org
Subject: Re: [PATCH v2 1/5] PM: domains: Add GENPD_FLAG_RT_SAFE for PREEMPT_RT
Date: Thu, 12 Jan 2023 11:32:58 +0100	[thread overview]
Message-ID: <Y7/h2n271HBeXDl6@linutronix.de> (raw)
In-Reply-To: <20221219151503.385816-2-krzysztof.kozlowski@linaro.org>

On 2022-12-19 16:14:59 [+0100], Krzysztof Kozlowski wrote:
> Realtime kernels with PREEMPT_RT must use raw_spinlock_t for domains
> which are invoked from CPU idle (thus from atomic section).  Example is
> cpuidle PSCI domain driver which itself is PREEMPT_RT safe, but is being
> called as part of cpuidle.

I think it needs to be clarified what PREEMPT_RT safe means. PSCI is an
external interface which does not inform us what it does and how long
the operation will take.
The ACPI table for instance populate several idle states and their
entry/exit time. Then you can decide if and when an entry/exit latency
of 500us is PREEMPT_RT safe.

> Add a flag allowing a power domain provider to indicate it is RT safe.
> The flag is supposed to be used with existing GENPD_FLAG_IRQ_SAFE.
> 
> Cc: Adrien Thierry <athierry@redhat.com>
> Cc: Brian Masney <bmasney@redhat.com>
> Cc: linux-rt-users@vger.kernel.org
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
…
> index 1cd41bdf73cf..0a1600244963 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -61,6 +61,14 @@
>   * GENPD_FLAG_MIN_RESIDENCY:	Enable the genpd governor to consider its
>   *				components' next wakeup when determining the
>   *				optimal idle state.
> + *
> + * GENPD_FLAG_RT_SAFE:		When used with GENPD_FLAG_IRQ_SAFE, this informs
> + *				genpd that its backend callbacks, ->power_on|off(),
> + *				do not use other spinlocks. They might use
> + *				raw_spinlocks or other pre-emption-disable
> + *				methods, all of which are PREEMPT_RT safe. Note

Please use spinlock_t and raw_spinlock_t. Wouldn't it be better to write
"preemption" instead "pre-emption"?
The important part is probably that once a raw_spinlock_t has been
acquired, it is not possible to invoke any function that acquries
sleeping locks (which includes memory allocations). While even without
that flag it is possible to invoke a function which disables and enables
preemption on its own.

> + *				that, a genpd having this flag set, requires its
> + *				masterdomains to also have it set.

This could be verified upon registration, no?
It might be worth noting that preemption-off section during PM
operations contribute to the system's max latency. Depending on how low
the operation is, it may or may not be a problem.
The ->power_on|off() refers to the sate of the component, right?

>   */
>  #define GENPD_FLAG_PM_CLK	 (1U << 0)
>  #define GENPD_FLAG_IRQ_SAFE	 (1U << 1)

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel