Re: [PATCH v2 12/44] cpuidle,dt: Push RCU-idle into driver
On Wed, 16 Nov 2022 at 16:29, Peter Zijlstra wrote:
>
>
> Sorry; things keep getting in the way of finishing this :/
>
> As such, I need a bit of time to get on-track again..
>
> On Tue, Oct 04, 2022 at 01:03:57PM +0200, Ulf Hansson wrote:
>
> > > --- a/drivers/acpi/processor_idle.c
> > > +++ b/drivers/acpi/processor_idle.c
> > > @@ -1200,6 +1200,8 @@ static int acpi_processor_setup_lpi_stat
> > > state->target_residency = lpi->min_residency;
> > > if (lpi->arch_flags)
> > > state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> > > + if (lpi->entry_method == ACPI_CSTATE_FFH)
> > > + state->flags |= CPUIDLE_FLAG_RCU_IDLE;
> >
> > I assume the state index here will never be 0?
> >
> > If not, it may lead to that acpi_processor_ffh_lpi_enter() may trigger
> > CPU_PM_CPU_IDLE_ENTER_PARAM() to call ct_cpuidle_enter|exit() for an
> > idle-state that doesn't have the CPUIDLE_FLAG_RCU_IDLE bit set.
>
> I'm not quite sure I see how. AFAICT this condition above implies
> acpi_processor_ffh_lpi_enter() gets called, no?
>
> Which in turn is an unconditional __CPU_PM_CPU_IDLE_ENTER() user, so
> even if idx==0, it ends up in ct_idle_{enter,exit}().
Seems like I was overlooking something here, you are right, this
shouldn't really be a problem.
>
> >
> > > state->enter = acpi_idle_lpi_enter;
> > > drv->safe_state_index = i;
> > > }
> > > --- a/drivers/cpuidle/cpuidle-arm.c
> > > +++ b/drivers/cpuidle/cpuidle-arm.c
> > > @@ -53,6 +53,7 @@ static struct cpuidle_driver arm_idle_dr
> > > * handler for idle state index 0.
> > > */
> > > .states[0] = {
> > > + .flags = CPUIDLE_FLAG_RCU_IDLE,
> >
> > Comparing arm64 and arm32 idle-states/idle-drivers, the $subject
> > series ends up setting the CPUIDLE_FLAG_RCU_IDLE for the ARM WFI idle
> > state (state zero), but only for the arm64 and psci cases (mostly
> > arm64). For arm32 we would need to update the ARM_CPUIDLE_WFI_STATE
> > too, as that is what most arm32 idle-drivers are using. My point is,
> > the code becomes a bit inconsistent.
>
> True.
>
> > Perhaps it's easier to avoid setting the CPUIDLE_FLAG_RCU_IDLE bit for
> > all of the ARM WFI idle states, for both arm64 and arm32?
>
> As per the below?
>
> >
> > > .enter = arm_enter_idle_state,
> > > .exit_latency = 1,
> > > .target_residency = 1,
>
> > > --- a/include/linux/cpuidle.h
> > > +++ b/include/linux/cpuidle.h
> > > @@ -282,14 +282,18 @@ extern s64 cpuidle_governor_latency_req(
> > > int __ret = 0; \
> > > \
> > > if (!idx) { \
> > > + ct_idle_enter();\
> >
> > According to my comment above, we should then drop these calls to
> > ct_idle_enter and ct_idle_exit() here. Right?
>
> Yes, if we ensure idx==0 never has RCU_IDLE set then these must be
> removed.
>
> > > cpu_do_idle(); \
> > > + ct_idle_exit(); \
> > > return idx; \
> > > } \
> > > \
> > > if (!is_retention) \
> > > __ret = cpu_pm_enter();\
> > > if (!__ret) { \
> > > + ct_idle_enter();\
> > > __ret = low_level_idle_enter(state);\
> > > + ct_idle_exit(); \
> > > if (!is_retention) \
> > > cpu_pm_exit(); \
> > > } \
> > >
>
> So the basic premise is that everything that ne
Re: [RFC PATCH 01/21] block: add and use init tagset helper
On Wed, 5 Oct 2022 at 07:11, Damien Le Moal
wrote:
>
> On 10/5/22 12:22, Chaitanya Kulkarni wrote:
> > Add and use the helper to initialize the common fields of the tag_set
> > such as blk_mq_ops, number of h/w queues, queue depth, command size,
> > numa_node, timeout, BLK_MQ_F_XXX flags, driver data. This initialization
> > is spread all over the block drivers. This avoids the code repetation of
> > the inialization code of the tag set in current block drivers and any
>
> s/inialization/initialization
> s/repetation/repetition
>
> > future ones.
> >
> > Signed-off-by: Chaitanya Kulkarni
> > ---
> > block/blk-mq.c| 20
> > drivers/block/null_blk/main.c | 10 +++---
> > include/linux/blk-mq.h| 5 +
> > 3 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 8070b6c10e8d..e3a8dd81bbe2 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -4341,6 +4341,26 @@ static int blk_mq_alloc_tag_set_tags(struct
> > blk_mq_tag_set *set,
> > return blk_mq_realloc_tag_set_tags(set, 0, new_nr_hw_queues);
> > }
> >
> > +void blk_mq_init_tag_set(struct blk_mq_tag_set *set,
> > + const struct blk_mq_ops *ops, unsigned int nr_hw_queues,
> > + unsigned int queue_depth, unsigned int cmd_size, int
> > numa_node,
> > + unsigned int timeout, unsigned int flags, void *driver_data)
>
> That is an awful lot of arguments... I would be tempted to say pack all
> these into a struct but then that would kind of negate this patchset goal.
> Using a function with that many arguments will be error prone, and hard to
> review... Not a fan.
I completely agree.
But there is also another problem going down this route. If/when we
realize that there is another parameter needed in the blk_mq_tag_set.
Today that's quite easy to add (assuming the parameter can be
optional), without changing the blk_mq_init_tag_set() interface.
>
> > +{
> > + if (!set)
> > + return;
> > +
> > + set->ops = ops;
> > + set->nr_hw_queues = nr_hw_queues;
> > + set->queue_depth = queue_depth;
> > + set->cmd_size = cmd_size;
> > + set->numa_node = numa_node;
> > + set->timeout = timeout;
> > + set->flags = flags;
> > + set->driver_data = driver_data;
> > +}
> > +
>
[...]
Kind regards
Uffe
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 00/44] cpuidle,rcu: Clean up the mess
/process_32.c| 1 - > arch/sparc/kernel/process_64.c| 3 +- > arch/sparc/kernel/vmlinux.lds.S | 1 - > arch/um/kernel/dyn.lds.S | 1 - > arch/um/kernel/process.c | 1 - > arch/um/kernel/uml.lds.S | 1 - > arch/x86/boot/compressed/vmlinux.lds.S| 1 + > arch/x86/coco/tdx/tdcall.S| 15 +-- > arch/x86/coco/tdx/tdx.c | 25 > arch/x86/events/amd/brs.c | 13 +++ > arch/x86/include/asm/fpu/xcr.h| 4 +- > arch/x86/include/asm/irqflags.h | 11 ++ > arch/x86/include/asm/mwait.h | 14 +++ > arch/x86/include/asm/nospec-branch.h | 2 +- > arch/x86/include/asm/paravirt.h | 6 ++- > arch/x86/include/asm/perf_event.h | 2 +- > arch/x86/include/asm/shared/io.h | 4 +- > arch/x86/include/asm/shared/tdx.h | 1 - > arch/x86/include/asm/special_insns.h | 8 ++-- > arch/x86/include/asm/xen/hypercall.h | 2 +- > arch/x86/kernel/cpu/bugs.c| 2 +- > arch/x86/kernel/fpu/core.c| 4 +- > arch/x86/kernel/paravirt.c| 14 ++- > arch/x86/kernel/process.c | 65 > +++ > arch/x86/kernel/vmlinux.lds.S | 1 - > arch/x86/lib/memcpy_64.S | 5 +-- > arch/x86/lib/memmove_64.S | 4 +- > arch/x86/lib/memset_64.S | 4 +- > arch/x86/xen/enlighten_pv.c | 2 +- > arch/x86/xen/irq.c| 2 +- > arch/xtensa/kernel/process.c | 1 + > arch/xtensa/kernel/vmlinux.lds.S | 1 - > drivers/acpi/processor_idle.c | 36 ++--- > drivers/base/power/runtime.c | 24 ++-- > drivers/clk/clk.c | 8 ++-- > drivers/cpuidle/cpuidle-arm.c | 1 + > drivers/cpuidle/cpuidle-big_little.c | 8 +++- > drivers/cpuidle/cpuidle-mvebu-v7.c| 7 > drivers/cpuidle/cpuidle-psci.c| 10 +++-- > drivers/cpuidle/cpuidle-qcom-spm.c| 1 + > drivers/cpuidle/cpuidle-riscv-sbi.c | 10 +++-- > drivers/cpuidle/cpuidle-tegra.c | 21 +++--- > drivers/cpuidle/cpuidle.c | 21 +- > drivers/cpuidle/dt_idle_states.c | 2 +- > drivers/cpuidle/poll_state.c | 10 - > drivers/idle/intel_idle.c | 19 + > drivers/perf/arm_pmu.c| 11 +- > drivers/perf/riscv_pmu_sbi.c | 8 +--- > include/asm-generic/vmlinux.lds.h | 9 ++--- > include/linux/compiler_types.h| 8 +++- > include/linux/cpu.h | 3 -- > include/linux/cpuidle.h | 34 > include/linux/cpumask.h | 4 +- > include/linux/percpu-defs.h | 2 +- > include/linux/sched/idle.h| 40 ++- > include/linux/thread_info.h | 18 - > include/linux/tracepoint.h| 13 ++- > kernel/cpu_pm.c | 9 - > kernel/printk/printk.c| 2 +- > kernel/sched/idle.c | 47 +++--- > kernel/time/tick-broadcast-hrtimer.c | 29 ++ > kernel/time/tick-broadcast.c | 6 ++- > kernel/trace/trace.c | 3 ++ > lib/ubsan.c | 5 ++- > mm/kasan/kasan.h | 4 ++ > mm/kasan/shadow.c | 38 ++ > tools/objtool/check.c | 17 > 121 files changed, 511 insertions(+), 420 deletions(-) Thanks for cleaning up the situation! I have applied this on a plain v6.0 (only one patch had a minor conflict) and tested this on an ARM64 Dragonboard 410c, which uses cpuidle-psci and the cpuidle-psci-domain. I didn't observe any problems, so feel free to add: Tested-by: Ulf Hansson Kind regards Uffe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 12/44] cpuidle,dt: Push RCU-idle into driver
On Tue, 4 Oct 2022 at 13:03, Ulf Hansson wrote:
>
> On Mon, 19 Sept 2022 at 12:18, Peter Zijlstra wrote:
> >
> > Doing RCU-idle outside the driver, only to then temporarily enable it
> > again before going idle is daft.
> >
> > Notably: this converts all dt_init_idle_driver() and
> > __CPU_PM_CPU_IDLE_ENTER() users for they are inextrably intertwined.
> >
> > Signed-off-by: Peter Zijlstra (Intel)
>
> Reviewed-by: Ulf Hansson
This was not (yet) my intention. Please have a look at the comments I
provided below.
Kind regards
Uffe
>
> > ---
> > arch/arm/mach-omap2/cpuidle34xx.c|4 ++--
> > drivers/acpi/processor_idle.c|2 ++
> > drivers/cpuidle/cpuidle-arm.c|1 +
> > drivers/cpuidle/cpuidle-big_little.c |8 ++--
> > drivers/cpuidle/cpuidle-psci.c |1 +
> > drivers/cpuidle/cpuidle-qcom-spm.c |1 +
> > drivers/cpuidle/cpuidle-riscv-sbi.c |1 +
> > drivers/cpuidle/dt_idle_states.c |2 +-
> > include/linux/cpuidle.h |4
> > 9 files changed, 19 insertions(+), 5 deletions(-)
> >
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -1200,6 +1200,8 @@ static int acpi_processor_setup_lpi_stat
> > state->target_residency = lpi->min_residency;
> > if (lpi->arch_flags)
> > state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> > + if (lpi->entry_method == ACPI_CSTATE_FFH)
> > + state->flags |= CPUIDLE_FLAG_RCU_IDLE;
>
> I assume the state index here will never be 0?
>
> If not, it may lead to that acpi_processor_ffh_lpi_enter() may trigger
> CPU_PM_CPU_IDLE_ENTER_PARAM() to call ct_cpuidle_enter|exit() for an
> idle-state that doesn't have the CPUIDLE_FLAG_RCU_IDLE bit set.
>
> > state->enter = acpi_idle_lpi_enter;
> > drv->safe_state_index = i;
> > }
> > --- a/drivers/cpuidle/cpuidle-arm.c
> > +++ b/drivers/cpuidle/cpuidle-arm.c
> > @@ -53,6 +53,7 @@ static struct cpuidle_driver arm_idle_dr
> > * handler for idle state index 0.
> > */
> > .states[0] = {
> > + .flags = CPUIDLE_FLAG_RCU_IDLE,
>
> Comparing arm64 and arm32 idle-states/idle-drivers, the $subject
> series ends up setting the CPUIDLE_FLAG_RCU_IDLE for the ARM WFI idle
> state (state zero), but only for the arm64 and psci cases (mostly
> arm64). For arm32 we would need to update the ARM_CPUIDLE_WFI_STATE
> too, as that is what most arm32 idle-drivers are using. My point is,
> the code becomes a bit inconsistent.
>
> Perhaps it's easier to avoid setting the CPUIDLE_FLAG_RCU_IDLE bit for
> all of the ARM WFI idle states, for both arm64 and arm32?
>
> > .enter = arm_enter_idle_state,
> > .exit_latency = 1,
> > .target_residency = 1,
> > --- a/drivers/cpuidle/cpuidle-big_little.c
> > +++ b/drivers/cpuidle/cpuidle-big_little.c
> > @@ -64,7 +64,8 @@ static struct cpuidle_driver bl_idle_lit
> > .enter = bl_enter_powerdown,
> > .exit_latency = 700,
> > .target_residency = 2500,
> > - .flags = CPUIDLE_FLAG_TIMER_STOP,
> > + .flags = CPUIDLE_FLAG_TIMER_STOP |
> > + CPUIDLE_FLAG_RCU_IDLE,
> > .name = "C1",
> > .desc = "ARM little-cluster power down",
> > },
> > @@ -85,7 +86,8 @@ static struct cpuidle_driver bl_idle_big
> > .enter = bl_enter_powerdown,
> > .exit_latency = 500,
> > .target_residency = 2000,
> > - .flags = CPUIDLE_FLAG_TIMER_STOP,
> > + .flags = CPUIDLE_FLAG_TIMER_STOP |
> > + CPUIDLE_FLAG_RCU_IDLE,
> > .name = "C1",
> > .desc = "ARM big-cluster power down",
> > },
> > @@ -124,11 +126,13 @@ static int bl_enter_powerdown(struct cpu
> > struct cpuidle_driver *drv, int idx)
> > {
> > cpu_pm_enter();
> > + ct_idle_enter();
> >
> > cpu_suspend(0, bl_powerdown
Re: [PATCH v2 38/44] cpuidle,powerdomain: Remove trace_.*_rcuidle()
On Mon, 19 Sept 2022 at 12:17, Peter Zijlstra wrote:
>
> OMAP was the one and only user.
>
> Signed-off-by: Peter Zijlstra (Intel)
There are changes to the runtime PM core as part of $subject patch.
Perhaps move those parts into a separate patch? In any case, the code
looks good to me.
Reviewed-by: Ulf Hansson
Kind regards
Uffe
> ---
> arch/arm/mach-omap2/powerdomain.c | 10 +-
> drivers/base/power/runtime.c | 24
> 2 files changed, 17 insertions(+), 17 deletions(-)
>
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -187,9 +187,9 @@ static int _pwrdm_state_switch(struct po
> trace_state = (PWRDM_TRACE_STATES_FLAG |
>((next & OMAP_POWERSTATE_MASK) << 8) |
>((prev & OMAP_POWERSTATE_MASK) << 0));
> - trace_power_domain_target_rcuidle(pwrdm->name,
> - trace_state,
> -
> raw_smp_processor_id());
> + trace_power_domain_target(pwrdm->name,
> + trace_state,
> + raw_smp_processor_id());
> }
> break;
> default:
> @@ -541,8 +541,8 @@ int pwrdm_set_next_pwrst(struct powerdom
>
> if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
> /* Trace the pwrdm desired target state */
> - trace_power_domain_target_rcuidle(pwrdm->name, pwrst,
> - raw_smp_processor_id());
> + trace_power_domain_target(pwrdm->name, pwrst,
> + raw_smp_processor_id());
> /* Program the pwrdm desired target state */
> ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
> }
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -442,7 +442,7 @@ static int rpm_idle(struct device *dev,
> int (*callback)(struct device *);
> int retval;
>
> - trace_rpm_idle_rcuidle(dev, rpmflags);
> + trace_rpm_idle(dev, rpmflags);
> retval = rpm_check_suspend_allowed(dev);
> if (retval < 0)
> ; /* Conditions are wrong. */
> @@ -481,7 +481,7 @@ static int rpm_idle(struct device *dev,
> dev->power.request_pending = true;
> queue_work(pm_wq, >power.work);
> }
> - trace_rpm_return_int_rcuidle(dev, _THIS_IP_, 0);
> + trace_rpm_return_int(dev, _THIS_IP_, 0);
> return 0;
> }
>
> @@ -493,7 +493,7 @@ static int rpm_idle(struct device *dev,
> wake_up_all(>power.wait_queue);
>
> out:
> - trace_rpm_return_int_rcuidle(dev, _THIS_IP_, retval);
> + trace_rpm_return_int(dev, _THIS_IP_, retval);
> return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO);
> }
>
> @@ -557,7 +557,7 @@ static int rpm_suspend(struct device *de
> struct device *parent = NULL;
> int retval;
>
> - trace_rpm_suspend_rcuidle(dev, rpmflags);
> + trace_rpm_suspend(dev, rpmflags);
>
> repeat:
> retval = rpm_check_suspend_allowed(dev);
> @@ -708,7 +708,7 @@ static int rpm_suspend(struct device *de
> }
>
> out:
> - trace_rpm_return_int_rcuidle(dev, _THIS_IP_, retval);
> + trace_rpm_return_int(dev, _THIS_IP_, retval);
>
> return retval;
>
> @@ -760,7 +760,7 @@ static int rpm_resume(struct device *dev
> struct device *parent = NULL;
> int retval = 0;
>
> - trace_rpm_resume_rcuidle(dev, rpmflags);
> + trace_rpm_resume(dev, rpmflags);
>
> repeat:
> if (dev->power.runtime_error) {
> @@ -925,7 +925,7 @@ static int rpm_resume(struct device *dev
> spin_lock_irq(>power.lock);
> }
>
> - trace_rpm_return_int_rcuidle(dev, _THIS_IP_, retval);
> + trace_rpm_return_int(dev, _THIS_IP_, retval);
>
> return retval;
> }
> @@ -1081,7 +1081,7 @@ int __pm_runtime_idle(struct device *dev
> if (retval < 0) {
> return retval;
> } else if (retval > 0) {
> - trace_rpm_usage_rcuidle(dev, rpmflags);
> + trace_rpm_usage(dev, rpmflags);
> return 0;
> }
> }
>
Re: [PATCH v2 39/44] cpuidle,clk: Remove trace_.*_rcuidle()
On Mon, 19 Sept 2022 at 12:17, Peter Zijlstra wrote:
>
> OMAP was the one and only user.
OMAP? :-)
>
> Signed-off-by: Peter Zijlstra (Intel)
Reviewed-by: Ulf Hansson
Kind regards
Uffe
> ---
> drivers/clk/clk.c |8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -978,12 +978,12 @@ static void clk_core_disable(struct clk_
> if (--core->enable_count > 0)
> return;
>
> - trace_clk_disable_rcuidle(core);
> + trace_clk_disable(core);
>
> if (core->ops->disable)
> core->ops->disable(core->hw);
>
> - trace_clk_disable_complete_rcuidle(core);
> + trace_clk_disable_complete(core);
>
> clk_core_disable(core->parent);
> }
> @@ -1037,12 +1037,12 @@ static int clk_core_enable(struct clk_co
> if (ret)
> return ret;
>
> - trace_clk_enable_rcuidle(core);
> + trace_clk_enable(core);
>
> if (core->ops->enable)
> ret = core->ops->enable(core->hw);
>
> - trace_clk_enable_complete_rcuidle(core);
> + trace_clk_enable_complete(core);
>
> if (ret) {
> clk_core_disable(core->parent);
>
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 23/44] arm,smp: Remove trace_.*_rcuidle() usage
On Mon, 19 Sept 2022 at 12:18, Peter Zijlstra wrote:
>
> None of these functions should ever be ran with RCU disabled anymore.
>
> Specifically, do_handle_IPI() is only called from handle_IPI() which
> explicitly does irq_enter()/irq_exit() which ensures RCU is watching.
>
> The problem with smp_cross_call() was, per commit 7c64cc0531fa ("arm: Use
> _rcuidle for smp_cross_call() tracepoints"), that
> cpuidle_enter_state_coupled() already had RCU disabled, but that's
> long been fixed by commit 1098582a0f6c ("sched,idle,rcu: Push rcu_idle
> deeper into the idle path").
>
> Signed-off-by: Peter Zijlstra (Intel)
FWIW:
Reviewed-by: Ulf Hansson
Kind regards
Uffe
> ---
> arch/arm/kernel/smp.c |6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -639,7 +639,7 @@ static void do_handle_IPI(int ipinr)
> unsigned int cpu = smp_processor_id();
>
> if ((unsigned)ipinr < NR_IPI)
> - trace_ipi_entry_rcuidle(ipi_types[ipinr]);
> + trace_ipi_entry(ipi_types[ipinr]);
>
> switch (ipinr) {
> case IPI_WAKEUP:
> @@ -686,7 +686,7 @@ static void do_handle_IPI(int ipinr)
> }
>
> if ((unsigned)ipinr < NR_IPI)
> - trace_ipi_exit_rcuidle(ipi_types[ipinr]);
> + trace_ipi_exit(ipi_types[ipinr]);
> }
>
> /* Legacy version, should go away once all irqchips have been converted */
> @@ -709,7 +709,7 @@ static irqreturn_t ipi_handler(int irq,
>
> static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> {
> - trace_ipi_raise_rcuidle(target, ipi_types[ipinr]);
> + trace_ipi_raise(target, ipi_types[ipinr]);
> __ipi_send_mask(ipi_desc[ipinr], target);
> }
>
>
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 14/44] cpuidle, cpu_pm: Remove RCU fiddling from cpu_pm_{enter, exit}()
On Mon, 19 Sept 2022 at 12:17, Peter Zijlstra wrote:
>
> All callers should still have RCU enabled.
>
> Signed-off-by: Peter Zijlstra (Intel)
> Acked-by: Mark Rutland
Reviewed-by: Ulf Hansson
Kind regards
Uffe
> ---
> kernel/cpu_pm.c |9 -
> 1 file changed, 9 deletions(-)
>
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -30,16 +30,9 @@ static int cpu_pm_notify(enum cpu_pm_eve
> {
> int ret;
>
> - /*
> -* This introduces a RCU read critical section, which could be
> -* disfunctional in cpu idle. Copy RCU_NONIDLE code to let RCU know
> -* this.
> -*/
> - ct_irq_enter_irqson();
> rcu_read_lock();
> ret = raw_notifier_call_chain(_pm_notifier.chain, event, NULL);
> rcu_read_unlock();
> - ct_irq_exit_irqson();
>
> return notifier_to_errno(ret);
> }
> @@ -49,11 +42,9 @@ static int cpu_pm_notify_robust(enum cpu
> unsigned long flags;
> int ret;
>
> - ct_irq_enter_irqson();
> raw_spin_lock_irqsave(_pm_notifier.lock, flags);
> ret = raw_notifier_call_chain_robust(_pm_notifier.chain,
> event_up, event_down, NULL);
> raw_spin_unlock_irqrestore(_pm_notifier.lock, flags);
> - ct_irq_exit_irqson();
>
> return notifier_to_errno(ret);
> }
>
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 12/44] cpuidle,dt: Push RCU-idle into driver
On Mon, 19 Sept 2022 at 12:18, Peter Zijlstra wrote:
>
> Doing RCU-idle outside the driver, only to then temporarily enable it
> again before going idle is daft.
>
> Notably: this converts all dt_init_idle_driver() and
> __CPU_PM_CPU_IDLE_ENTER() users for they are inextrably intertwined.
>
> Signed-off-by: Peter Zijlstra (Intel)
Reviewed-by: Ulf Hansson
Kind regards
Uffe
> ---
> arch/arm/mach-omap2/cpuidle34xx.c|4 ++--
> drivers/acpi/processor_idle.c|2 ++
> drivers/cpuidle/cpuidle-arm.c|1 +
> drivers/cpuidle/cpuidle-big_little.c |8 ++--
> drivers/cpuidle/cpuidle-psci.c |1 +
> drivers/cpuidle/cpuidle-qcom-spm.c |1 +
> drivers/cpuidle/cpuidle-riscv-sbi.c |1 +
> drivers/cpuidle/dt_idle_states.c |2 +-
> include/linux/cpuidle.h |4
> 9 files changed, 19 insertions(+), 5 deletions(-)
>
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1200,6 +1200,8 @@ static int acpi_processor_setup_lpi_stat
> state->target_residency = lpi->min_residency;
> if (lpi->arch_flags)
> state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> + if (lpi->entry_method == ACPI_CSTATE_FFH)
> + state->flags |= CPUIDLE_FLAG_RCU_IDLE;
I assume the state index here will never be 0?
If not, it may lead to that acpi_processor_ffh_lpi_enter() may trigger
CPU_PM_CPU_IDLE_ENTER_PARAM() to call ct_cpuidle_enter|exit() for an
idle-state that doesn't have the CPUIDLE_FLAG_RCU_IDLE bit set.
> state->enter = acpi_idle_lpi_enter;
> drv->safe_state_index = i;
> }
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -53,6 +53,7 @@ static struct cpuidle_driver arm_idle_dr
> * handler for idle state index 0.
> */
> .states[0] = {
> + .flags = CPUIDLE_FLAG_RCU_IDLE,
Comparing arm64 and arm32 idle-states/idle-drivers, the $subject
series ends up setting the CPUIDLE_FLAG_RCU_IDLE for the ARM WFI idle
state (state zero), but only for the arm64 and psci cases (mostly
arm64). For arm32 we would need to update the ARM_CPUIDLE_WFI_STATE
too, as that is what most arm32 idle-drivers are using. My point is,
the code becomes a bit inconsistent.
Perhaps it's easier to avoid setting the CPUIDLE_FLAG_RCU_IDLE bit for
all of the ARM WFI idle states, for both arm64 and arm32?
> .enter = arm_enter_idle_state,
> .exit_latency = 1,
> .target_residency = 1,
> --- a/drivers/cpuidle/cpuidle-big_little.c
> +++ b/drivers/cpuidle/cpuidle-big_little.c
> @@ -64,7 +64,8 @@ static struct cpuidle_driver bl_idle_lit
> .enter = bl_enter_powerdown,
> .exit_latency = 700,
> .target_residency = 2500,
> - .flags = CPUIDLE_FLAG_TIMER_STOP,
> + .flags = CPUIDLE_FLAG_TIMER_STOP |
> + CPUIDLE_FLAG_RCU_IDLE,
> .name = "C1",
> .desc = "ARM little-cluster power down",
> },
> @@ -85,7 +86,8 @@ static struct cpuidle_driver bl_idle_big
> .enter = bl_enter_powerdown,
> .exit_latency = 500,
> .target_residency = 2000,
> - .flags = CPUIDLE_FLAG_TIMER_STOP,
> + .flags = CPUIDLE_FLAG_TIMER_STOP |
> + CPUIDLE_FLAG_RCU_IDLE,
> .name = "C1",
> .desc = "ARM big-cluster power down",
> },
> @@ -124,11 +126,13 @@ static int bl_enter_powerdown(struct cpu
> struct cpuidle_driver *drv, int idx)
> {
> cpu_pm_enter();
> + ct_idle_enter();
>
> cpu_suspend(0, bl_powerdown_finisher);
>
> /* signals the MCPM core that CPU is out of low power state */
> mcpm_cpu_powered_up();
> + ct_idle_exit();
>
> cpu_pm_exit();
>
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -357,6 +357,7 @@ static int psci_idle_init_cpu(struct dev
> * PSCI idle states relies on architectural WFI to be represented as
> * state index 0.
> */
> + drv->states[0].flags = CPUIDLE_FLAG_RCU_IDLE;
> drv->states[0].enter = psci_enter_idle_
Re: [PATCH v2 4/4] bus: Make remove callback return void
On Tue, 6 Jul 2021 at 17:53, Uwe Kleine-König wrote: > > The driver core ignores the return value of this callback because there > is only little it can do when a device disappears. > > This is the final bit of a long lasting cleanup quest where several > buses were converted to also return void from their remove callback. > Additionally some resource leaks were fixed that were caused by drivers > returning an error code in the expectation that the driver won't go > away. > > With struct bus_type::remove returning void it's prevented that newly > implemented buses return an ignored error code and so don't anticipate > wrong expectations for driver authors. > > Acked-by: Russell King (Oracle) (For ARM, Amba > and related parts) > Acked-by: Mark Brown > Acked-by: Chen-Yu Tsai (for drivers/bus/sunxi-rsb.c) > Acked-by: Pali Rohár > Acked-by: Mauro Carvalho Chehab (for drivers/media) > Acked-by: Hans de Goede (For drivers/platform) > Acked-by: Alexandre Belloni > Acked-By: Vinod Koul > Acked-by: Juergen Gross (For Xen) > Acked-by: Lee Jones (For drivers/mfd) > Acked-by: Johannes Thumshirn (For drivers/mcb) > Acked-by: Johan Hovold > Acked-by: Srinivas Kandagatla (For > drivers/slimbus) > Acked-by: Kirti Wankhede (For drivers/vfio) > Acked-by: Maximilian Luz > Acked-by: Heikki Krogerus (For ulpi and > typec) > Acked-by: Samuel Iglesias Gonsálvez (For ipack) > Reviewed-by: Tom Rix (For fpga) > Acked-by: Geoff Levand (For ps3) > Signed-off-by: Uwe Kleine-König Acked-by: Ulf Hansson # For MMC [...] Kind regards Uffe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 08/30] mspro: use blk_mq_alloc_disk
On Wed, 2 Jun 2021 at 08:54, Christoph Hellwig wrote:
>
> Use the blk_mq_alloc_disk API to simplify the gendisk and request_queue
> allocation.
>
> Signed-off-by: Christoph Hellwig
Acked-by: Ulf Hansson
Kind regards
Uffe
> ---
> drivers/memstick/core/mspro_block.c | 26 +++---
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/memstick/core/mspro_block.c
> b/drivers/memstick/core/mspro_block.c
> index cf7fe0d58ee7..22778d0e24f5 100644
> --- a/drivers/memstick/core/mspro_block.c
> +++ b/drivers/memstick/core/mspro_block.c
> @@ -1205,21 +1205,17 @@ static int mspro_block_init_disk(struct memstick_dev
> *card)
> if (disk_id < 0)
> return disk_id;
>
> - msb->disk = alloc_disk(1 << MSPRO_BLOCK_PART_SHIFT);
> - if (!msb->disk) {
> - rc = -ENOMEM;
> + rc = blk_mq_alloc_sq_tag_set(>tag_set, _mq_ops, 2,
> +BLK_MQ_F_SHOULD_MERGE);
> + if (rc)
> goto out_release_id;
> - }
>
> - msb->queue = blk_mq_init_sq_queue(>tag_set, _mq_ops, 2,
> - BLK_MQ_F_SHOULD_MERGE);
> - if (IS_ERR(msb->queue)) {
> - rc = PTR_ERR(msb->queue);
> - msb->queue = NULL;
> - goto out_put_disk;
> + msb->disk = blk_mq_alloc_disk(>tag_set, card);
> + if (IS_ERR(msb->disk)) {
> + rc = PTR_ERR(msb->disk);
> + goto out_free_tag_set;
> }
> -
> - msb->queue->queuedata = card;
> + msb->queue = msb->disk->queue;
>
> blk_queue_max_hw_sectors(msb->queue, MSPRO_BLOCK_MAX_PAGES);
> blk_queue_max_segments(msb->queue, MSPRO_BLOCK_MAX_SEGS);
> @@ -1228,10 +1224,10 @@ static int mspro_block_init_disk(struct memstick_dev
> *card)
>
> msb->disk->major = major;
> msb->disk->first_minor = disk_id << MSPRO_BLOCK_PART_SHIFT;
> + msb->disk->minors = 1 << MSPRO_BLOCK_PART_SHIFT;
> msb->disk->fops = _block_bdops;
> msb->usage_count = 1;
> msb->disk->private_data = msb;
> - msb->disk->queue = msb->queue;
>
> sprintf(msb->disk->disk_name, "mspblk%d", disk_id);
>
> @@ -1247,8 +1243,8 @@ static int mspro_block_init_disk(struct memstick_dev
> *card)
> msb->active = 1;
> return 0;
>
> -out_put_disk:
> - put_disk(msb->disk);
> +out_free_tag_set:
> + blk_mq_free_tag_set(>tag_set);
> out_release_id:
> mutex_lock(_block_disk_lock);
> idr_remove(_block_disk_idr, disk_id);
> --
> 2.30.2
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 07/30] ms_block: use blk_mq_alloc_disk
On Wed, 2 Jun 2021 at 08:54, Christoph Hellwig wrote:
>
> Use the blk_mq_alloc_disk API to simplify the gendisk and request_queue
> allocation.
>
> Signed-off-by: Christoph Hellwig
Acked-by: Ulf Hansson
Kind regards
Uffe
> ---
> drivers/memstick/core/ms_block.c | 25 ++---
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/memstick/core/ms_block.c
> b/drivers/memstick/core/ms_block.c
> index 0bacf4268f83..dac258d12aca 100644
> --- a/drivers/memstick/core/ms_block.c
> +++ b/drivers/memstick/core/ms_block.c
> @@ -2110,21 +2110,17 @@ static int msb_init_disk(struct memstick_dev *card)
> if (msb->disk_id < 0)
> return msb->disk_id;
>
> - msb->disk = alloc_disk(0);
> - if (!msb->disk) {
> - rc = -ENOMEM;
> + rc = blk_mq_alloc_sq_tag_set(>tag_set, _mq_ops, 2,
> +BLK_MQ_F_SHOULD_MERGE);
> + if (rc)
> goto out_release_id;
> - }
>
> - msb->queue = blk_mq_init_sq_queue(>tag_set, _mq_ops, 2,
> - BLK_MQ_F_SHOULD_MERGE);
> - if (IS_ERR(msb->queue)) {
> - rc = PTR_ERR(msb->queue);
> - msb->queue = NULL;
> - goto out_put_disk;
> + msb->disk = blk_mq_alloc_disk(>tag_set, card);
> + if (IS_ERR(msb->disk)) {
> + rc = PTR_ERR(msb->disk);
> + goto out_free_tag_set;
> }
> -
> - msb->queue->queuedata = card;
> + msb->queue = msb->disk->queue;
>
> blk_queue_max_hw_sectors(msb->queue, MS_BLOCK_MAX_PAGES);
> blk_queue_max_segments(msb->queue, MS_BLOCK_MAX_SEGS);
> @@ -2135,7 +2131,6 @@ static int msb_init_disk(struct memstick_dev *card)
> sprintf(msb->disk->disk_name, "msblk%d", msb->disk_id);
> msb->disk->fops = _bdops;
> msb->disk->private_data = msb;
> - msb->disk->queue = msb->queue;
>
> capacity = msb->pages_in_block * msb->logical_block_count;
> capacity *= (msb->page_size / 512);
> @@ -2155,8 +2150,8 @@ static int msb_init_disk(struct memstick_dev *card)
> dbg("Disk added");
> return 0;
>
> -out_put_disk:
> - put_disk(msb->disk);
> +out_free_tag_set:
> + blk_mq_free_tag_set(>tag_set);
> out_release_id:
> mutex_lock(_disk_lock);
> idr_remove(_disk_idr, msb->disk_id);
> --
> 2.30.2
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] virtio: disable partitions scanning for no partitions block
On Mon, 24 May 2021 at 16:57, Christoph Hellwig wrote: > > On Mon, May 24, 2021 at 03:29:22PM +0100, Stefan Hajnoczi wrote: > > GENHD_FL_NO_PART_SCAN is not used much in other drivers. This makes me > > wonder if the same use case is addressed through other means with SCSI, > > NVMe, etc devices. Maybe Christoph or Jens can weigh in on whether > > adding a bit to disable partition scanning for a virtio-blk fits into > > the big picture? > > > > Is your goal to avoid accidentally detecting partitions because it's > > confusing when that happens? > > I'm really confused what the use case is here. GENHD_FL_NO_PART_SCAN > has four users: > > - the block core setting it for hidden devices, for which the concept >of paritions doesn't make sense. Looking back this should have never >used GENHD_FL_NO_PART_SCAN, and instead the partition scanning code >should just check GENHD_FL_HIDDEN as well. > - mmc uses it for boot partitions and rpmb. I'm not even sure how >these can be exposed as block devices as they don't require block >granularity access IIRC, but if the allow block layer access there >is no reason to ever set these flags. For RPMB, we have converted them into char devices, thus GENHD_FL_NO_PART_SCAN is never set for them. The code needs a cleanup to clarify this. When it comes to eMMC boot partitions, those can be read/written to as any other block device. Although, it's unlikely that they need partitions as they are usually very small, 512Kb or 2MB in that ballpark. At least, that was the thinking behind it when we added GENHD_FL_NO_PART_SCAN for them. If you want to drop GENHD_FL_NO_PART_SCAN for eMMC boot partitions, I don't think it will be an issue. > - loop is a bit of a mess. IIRC the story is that originally the >loop device did not support partitions, then in 2008 support for >partitions was added by partitioning the minor number space, and >then in 2011 support for partitions without that parameter was >added using a new flag in the loop device creation ioctl that uses >the extended dev_t space added since. But even that might be >something we can handled without that flag without breaking the >userspace ABI > - m64card sets it for no good reason at all > > In other words: in a perfect would GENHD_FL_NO_PART_SCAN would not > exist, and it certainly should not be added to a new driver, never > mind a protocol. > ___ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization Kind regards Uffe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 2/2] block: remove unnecessary argument from blk_execute_rq
On Fri, 22 Jan 2021 at 10:28, Guoqing Jiang wrote: > > We can remove 'q' from blk_execute_rq as well after the previous change > in blk_execute_rq_nowait. > > And more importantly it never really was needed to start with given > that we can trivial derive it from struct request. > > Cc: linux-s...@vger.kernel.org > Cc: virtualization@lists.linux-foundation.org > Cc: linux-...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: linux-n...@lists.infradead.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Guoqing Jiang [...] > drivers/mmc/core/block.c | 10 +- [...] >From mmc point of view, please add: Acked-by: Ulf Hansson At the moment I don't think this will conflict with any changes to mmc, but if that happens let's sort it then... Kind regards Uffe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] sd, mmc, virtio_blk, string_helpers: fix block size units
On 6 March 2015 at 03:47, James Bottomley
james.bottom...@hansenpartnership.com wrote:
From: James Bottomley jbottom...@parallels.com
The current string_get_size() overflows when the device size goes over
2^64 bytes because the string helper routine computes the suffix from
the size in bytes. However, the entirety of Linux thinks in terms of
blocks, not bytes, so this will artificially induce an overflow on very
large devices. Fix this by making the function string_get_size() take
blocks and the block size instead of bytes. This should allow us to
keep working until the current SCSI standard overflows.
Also fix virtio_blk and mmc (both of which were also artificially
multiplying by the block size to pass a byte side to string_get_size()).
The mathematics of this is pretty simple: we're taking a product of
size in blocks (S) and block size (B) and trying to re-express this in
exponential form: S*B = R*N^E (where N, the exponent is either 1000 or
1024) and R N. Mathematically, S = RS*N^ES and B=RB*N^EB, so if RS*RB
N it's easy to see that S*B = RS*RB*N^(ES+EB). However, if RS*BS N,
we can see that this can be re-expressed as RS*BS = R*N (where R =
RS*BS/N N) so the whole exponent becomes R*N^(ES+EB+1)
Signed-off-by: James Bottomley jbottom...@parallels.com
For the mmc parts;
Acked-by: Ulf Hansson ulf.hans...@linaro.org
Kind regards
Uffe
---
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cdfbd21..26d2440 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -344,7 +344,7 @@ static void virtblk_config_changed_work(struct
work_struct *work)
struct request_queue *q = vblk-disk-queue;
char cap_str_2[10], cap_str_10[10];
char *envp[] = { RESIZE=1, NULL };
- u64 capacity, size;
+ u64 capacity;
/* Host must always specify the capacity. */
virtio_cread(vdev, struct virtio_blk_config, capacity, capacity);
@@ -356,9 +356,10 @@ static void virtblk_config_changed_work(struct
work_struct *work)
capacity = (sector_t)-1;
}
- size = capacity * queue_logical_block_size(q);
- string_get_size(size, STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
- string_get_size(size, STRING_UNITS_10, cap_str_10,
sizeof(cap_str_10));
+ string_get_size(capacity, queue_logical_block_size(q),
+ STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+ string_get_size(capacity, queue_logical_block_size(q),
+ STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
dev_notice(vdev-dev,
new size: %llu %d-byte logical blocks (%s/%s)\n,
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c69afb5..2fc4269 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2230,7 +2230,7 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
part_md-part_type = part_type;
list_add(part_md-part, md-part);
- string_get_size((u64)get_capacity(part_md-disk) 9, STRING_UNITS_2,
+ string_get_size((u64)get_capacity(part_md-disk), 512, STRING_UNITS_2,
cap_str, sizeof(cap_str));
pr_info(%s: %s %s partition %u %s\n,
part_md-disk-disk_name, mmc_card_id(card),
@@ -2436,7 +2436,7 @@ static int mmc_blk_probe(struct device *dev)
if (IS_ERR(md))
return PTR_ERR(md);
- string_get_size((u64)get_capacity(md-disk) 9, STRING_UNITS_2,
+ string_get_size((u64)get_capacity(md-disk), 512, STRING_UNITS_2,
cap_str, sizeof(cap_str));
pr_info(%s: %s %s %s %s\n,
md-disk-disk_name, mmc_card_id(card), mmc_card_name(card),
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6b78476..3c7fe4e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2235,11 +2235,11 @@ got_data:
{
char cap_str_2[10], cap_str_10[10];
- u64 sz = (u64)sdkp-capacity ilog2(sector_size);
- string_get_size(sz, STRING_UNITS_2, cap_str_2,
- sizeof(cap_str_2));
- string_get_size(sz, STRING_UNITS_10, cap_str_10,
+ string_get_size(sdkp-capacity, sector_size,
+ STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
+ string_get_size(sdkp-capacity, sector_size,
+ STRING_UNITS_10, cap_str_10,
sizeof(cap_str_10));
if (sdkp-first_scan || old_capacity != sdkp-capacity) {
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 6575718..2633280 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,7 +10,7 @@ enum string_size_units {
STRING_UNITS_2, /* use binary powers of 2^10 */
};
-void string_get_size(u64 size
