Re: [PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
On 2015/3/5 8:18, Rafael J. Wysocki wrote: > On Thursday, March 05, 2015 07:50:26 AM Li, Aubrey wrote: >> On 2015/2/13 0:24, Rafael J. Wysocki wrote: >>> On Thursday, February 12, 2015 02:26:43 PM Peter Zijlstra wrote: Why bother with enter_freeze() for any but the deepest state (C6 in this case)? >>> >>> User space may disable the deepest one (and any of them in general) via >>> sysfs >>> and there's no good reason to ignore its choice in this particular case >>> while >>> we're honoring it otherwise. >>> >>> So the logic is basically "find the deepest one which isn't disabled" and >>> setting the pointers costs us nothing really. >>> >> >> If the user has chance to disable C6 via /sys, that means c6 works? >> Shouldn't we ignore user space setting during freeze? Otherwise, we will >> lost S0ix? > > We can't ignore it, because we don't know the reason why the state was > disabled. > > It may just not work reliably enough on the given platform. > okay, make sense to me. Thanks, -Aubrey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
On Thursday, March 05, 2015 07:50:26 AM Li, Aubrey wrote: > On 2015/2/13 0:24, Rafael J. Wysocki wrote: > > On Thursday, February 12, 2015 02:26:43 PM Peter Zijlstra wrote: > >> > >> Why bother with enter_freeze() for any but the deepest state (C6 in this > >> case)? > > > > User space may disable the deepest one (and any of them in general) via > > sysfs > > and there's no good reason to ignore its choice in this particular case > > while > > we're honoring it otherwise. > > > > So the logic is basically "find the deepest one which isn't disabled" and > > setting the pointers costs us nothing really. > > > > If the user has chance to disable C6 via /sys, that means c6 works? > Shouldn't we ignore user space setting during freeze? Otherwise, we will > lost S0ix? We can't ignore it, because we don't know the reason why the state was disabled. It may just not work reliably enough on the given platform. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
On 2015/2/13 0:24, Rafael J. Wysocki wrote: > On Thursday, February 12, 2015 02:26:43 PM Peter Zijlstra wrote: >> >> Why bother with enter_freeze() for any but the deepest state (C6 in this >> case)? > > User space may disable the deepest one (and any of them in general) via sysfs > and there's no good reason to ignore its choice in this particular case while > we're honoring it otherwise. > > So the logic is basically "find the deepest one which isn't disabled" and > setting the pointers costs us nothing really. > If the user has chance to disable C6 via /sys, that means c6 works? Shouldn't we ignore user space setting during freeze? Otherwise, we will lost S0ix? Thanks, -Aubrey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
On Thu, Feb 12, 2015 at 05:24:51PM +0100, Rafael J. Wysocki wrote: > On Thursday, February 12, 2015 02:26:43 PM Peter Zijlstra wrote: > > Why bother with enter_freeze() for any but the deepest state (C6 in this > > case)? > > User space may disable the deepest one (and any of them in general) via sysfs > and there's no good reason to ignore its choice in this particular case while > we're honoring it otherwise. > > So the logic is basically "find the deepest one which isn't disabled" and > setting the pointers costs us nothing really. > > > Also, should we ignore things like intel_idle.max_cstate for this > > selection? > > No, we shouldn't. The deeper ones may just not work then. OK, fair enough. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
On Thursday, February 12, 2015 02:26:43 PM Peter Zijlstra wrote: > On Wed, Feb 11, 2015 at 05:04:17AM +0100, Rafael J. Wysocki wrote: > > @@ -131,28 +133,32 @@ static struct cpuidle_state nehalem_csta > > .flags = MWAIT2flg(0x00), > > .exit_latency = 3, > > .target_residency = 6, > > - .enter = &intel_idle }, > > + .enter = &intel_idle, > > + .enter_freeze = intel_idle_freeze, }, > > { > > .name = "C1E-NHM", > > .desc = "MWAIT 0x01", > > .flags = MWAIT2flg(0x01), > > .exit_latency = 10, > > .target_residency = 20, > > - .enter = &intel_idle }, > > + .enter = &intel_idle, > > + .enter_freeze = intel_idle_freeze, }, > > { > > .name = "C3-NHM", > > .desc = "MWAIT 0x10", > > .flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED, > > .exit_latency = 20, > > .target_residency = 80, > > - .enter = &intel_idle }, > > + .enter = &intel_idle, > > + .enter_freeze = intel_idle_freeze, }, > > { > > .name = "C6-NHM", > > .desc = "MWAIT 0x20", > > .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED, > > .exit_latency = 200, > > .target_residency = 800, > > - .enter = &intel_idle }, > > + .enter = &intel_idle, > > + .enter_freeze = intel_idle_freeze, }, > > { > > .enter = NULL } > > }; > > Why bother with enter_freeze() for any but the deepest state (C6 in this > case)? User space may disable the deepest one (and any of them in general) via sysfs and there's no good reason to ignore its choice in this particular case while we're honoring it otherwise. So the logic is basically "find the deepest one which isn't disabled" and setting the pointers costs us nothing really. > Also, should we ignore things like intel_idle.max_cstate for this > selection? No, we shouldn't. The deeper ones may just not work then. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
On Wed, Feb 11, 2015 at 05:04:17AM +0100, Rafael J. Wysocki wrote: > @@ -131,28 +133,32 @@ static struct cpuidle_state nehalem_csta > .flags = MWAIT2flg(0x00), > .exit_latency = 3, > .target_residency = 6, > - .enter = &intel_idle }, > + .enter = &intel_idle, > + .enter_freeze = intel_idle_freeze, }, > { > .name = "C1E-NHM", > .desc = "MWAIT 0x01", > .flags = MWAIT2flg(0x01), > .exit_latency = 10, > .target_residency = 20, > - .enter = &intel_idle }, > + .enter = &intel_idle, > + .enter_freeze = intel_idle_freeze, }, > { > .name = "C3-NHM", > .desc = "MWAIT 0x10", > .flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 20, > .target_residency = 80, > - .enter = &intel_idle }, > + .enter = &intel_idle, > + .enter_freeze = intel_idle_freeze, }, > { > .name = "C6-NHM", > .desc = "MWAIT 0x20", > .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED, > .exit_latency = 200, > .target_residency = 800, > - .enter = &intel_idle }, > + .enter = &intel_idle, > + .enter_freeze = intel_idle_freeze, }, > { > .enter = NULL } > }; Why bother with enter_freeze() for any but the deepest state (C6 in this case)? Also, should we ignore things like intel_idle.max_cstate for this selection? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/6] intel_idle: Add ->enter_freeze callbacks
From: Rafael J. Wysocki Add an ->enter_freeze callback routine, intel_idle_freeze(), to the intel_idle driver and point the ->enter_freeze callback pointers of all of the driver's state objects to it. Signed-off-by: Rafael J. Wysocki --- drivers/idle/intel_idle.c | 179 -- 1 file changed, 125 insertions(+), 54 deletions(-) Index: linux-pm/drivers/idle/intel_idle.c === --- linux-pm.orig/drivers/idle/intel_idle.c +++ linux-pm/drivers/idle/intel_idle.c @@ -97,6 +97,8 @@ static const struct idle_cpu *icpu; static struct cpuidle_device __percpu *intel_idle_cpuidle_devices; static int intel_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); +static void intel_idle_freeze(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index); static int intel_idle_cpu_init(int cpu); static struct cpuidle_state *cpuidle_state_table; @@ -131,28 +133,32 @@ static struct cpuidle_state nehalem_csta .flags = MWAIT2flg(0x00), .exit_latency = 3, .target_residency = 6, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C1E-NHM", .desc = "MWAIT 0x01", .flags = MWAIT2flg(0x01), .exit_latency = 10, .target_residency = 20, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C3-NHM", .desc = "MWAIT 0x10", .flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 20, .target_residency = 80, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C6-NHM", .desc = "MWAIT 0x20", .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 200, .target_residency = 800, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .enter = NULL } }; @@ -164,35 +170,40 @@ static struct cpuidle_state snb_cstates[ .flags = MWAIT2flg(0x00), .exit_latency = 2, .target_residency = 2, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C1E-SNB", .desc = "MWAIT 0x01", .flags = MWAIT2flg(0x01), .exit_latency = 10, .target_residency = 20, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C3-SNB", .desc = "MWAIT 0x10", .flags = MWAIT2flg(0x10) | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 80, .target_residency = 211, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C6-SNB", .desc = "MWAIT 0x20", .flags = MWAIT2flg(0x20) | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 104, .target_residency = 345, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C7-SNB", .desc = "MWAIT 0x30", .flags = MWAIT2flg(0x30) | CPUIDLE_FLAG_TLB_FLUSHED, .exit_latency = 109, .target_residency = 345, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .enter = NULL } }; @@ -204,42 +215,48 @@ static struct cpuidle_state byt_cstates[ .flags = MWAIT2flg(0x00), .exit_latency = 1, .target_residency = 1, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C1E-BYT", .desc = "MWAIT 0x01", .flags = MWAIT2flg(0x01), .exit_latency = 15, .target_residency = 30, - .enter = &intel_idle }, + .enter = &intel_idle, + .enter_freeze = intel_idle_freeze, }, { .name = "C6N-BYT", .desc = "MWAIT 0x58", .flags = MWAIT2flg(0x58) | CPUIDLE_FLAG_TLB_