| Message ID | 20200221174331.1480468-1-mail@maciej.szmigiero.name (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint | expand |
(CC'ing also Marcelo as the cpuidle-haltpoll driver author and the KVM ML). On 21.02.2020 18:43, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Before commit 1328edca4a14 ("cpuidle-haltpoll: Enable kvm guest polling > when dedicated physical CPUs are available") the cpuidle-haltpoll driver > could also be used in scenarios when the host does not advertise the > KVM_HINTS_REALTIME hint. > > While the behavior introduced by the aforementioned commit makes sense as > the default there are cases where the old behavior is desired, for example, > when other kernel changes triggered by presence by this hint are unwanted, > for some workloads where the latency benefit from polling overweights the > loss from idle CPU capacity that otherwise would be available, or just when > running under older Qemu versions that lack this hint. > > Let's provide a typical "force" module parameter that allows restoring the > old behavior. > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > --- > drivers/cpuidle/cpuidle-haltpoll.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c > index b0ce9bc78113..07e5b36076bb 100644 > --- a/drivers/cpuidle/cpuidle-haltpoll.c > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > @@ -18,6 +18,11 @@ > #include <linux/kvm_para.h> > #include <linux/cpuidle_haltpoll.h> > > +static bool force __read_mostly; > +module_param(force, bool, 0444); > +MODULE_PARM_DESC(force, > + "Load even if the host does not provide the REALTIME hint"); > + > static struct cpuidle_device __percpu *haltpoll_cpuidle_devices; > static enum cpuhp_state haltpoll_hp_state; > > @@ -90,6 +95,11 @@ static void haltpoll_uninit(void) > haltpoll_cpuidle_devices = NULL; > } > > +static bool haltpool_want(void) > +{ > + return kvm_para_has_hint(KVM_HINTS_REALTIME) || force; > +} > + > static int __init haltpoll_init(void) > { > int ret; > @@ -102,7 +112,7 @@ static int __init haltpoll_init(void) > cpuidle_poll_state_init(drv); > > if (!kvm_para_available() || > - !kvm_para_has_hint(KVM_HINTS_REALTIME)) > + !haltpool_want()) > return -ENODEV; > > ret = cpuidle_register_driver(drv); >
A friendly ping here. Maciej On 21.02.2020 18:43, Maciej S. Szmigiero wrote: > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > Before commit 1328edca4a14 ("cpuidle-haltpoll: Enable kvm guest polling > when dedicated physical CPUs are available") the cpuidle-haltpoll driver > could also be used in scenarios when the host does not advertise the > KVM_HINTS_REALTIME hint. > > While the behavior introduced by the aforementioned commit makes sense as > the default there are cases where the old behavior is desired, for example, > when other kernel changes triggered by presence by this hint are unwanted, > for some workloads where the latency benefit from polling overweights the > loss from idle CPU capacity that otherwise would be available, or just when > running under older Qemu versions that lack this hint. > > Let's provide a typical "force" module parameter that allows restoring the > old behavior. > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > --- > drivers/cpuidle/cpuidle-haltpoll.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c > index b0ce9bc78113..07e5b36076bb 100644 > --- a/drivers/cpuidle/cpuidle-haltpoll.c > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > @@ -18,6 +18,11 @@ > #include <linux/kvm_para.h> > #include <linux/cpuidle_haltpoll.h> > > +static bool force __read_mostly; > +module_param(force, bool, 0444); > +MODULE_PARM_DESC(force, > + "Load even if the host does not provide the REALTIME hint"); > + > static struct cpuidle_device __percpu *haltpoll_cpuidle_devices; > static enum cpuhp_state haltpoll_hp_state; > > @@ -90,6 +95,11 @@ static void haltpoll_uninit(void) > haltpoll_cpuidle_devices = NULL; > } > > +static bool haltpool_want(void) > +{ > + return kvm_para_has_hint(KVM_HINTS_REALTIME) || force; > +} > + > static int __init haltpoll_init(void) > { > int ret; > @@ -102,7 +112,7 @@ static int __init haltpoll_init(void) > cpuidle_poll_state_init(drv); > > if (!kvm_para_available() || > - !kvm_para_has_hint(KVM_HINTS_REALTIME)) > + !haltpool_want()) > return -ENODEV; > > ret = cpuidle_register_driver(drv); >
On Friday, February 28, 2020 6:10:18 PM CET Maciej S. Szmigiero wrote: > A friendly ping here. > > Maciej > > On 21.02.2020 18:43, Maciej S. Szmigiero wrote: > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > > > Before commit 1328edca4a14 ("cpuidle-haltpoll: Enable kvm guest polling > > when dedicated physical CPUs are available") the cpuidle-haltpoll driver > > could also be used in scenarios when the host does not advertise the > > KVM_HINTS_REALTIME hint. > > > > While the behavior introduced by the aforementioned commit makes sense as > > the default there are cases where the old behavior is desired, for example, > > when other kernel changes triggered by presence by this hint are unwanted, > > for some workloads where the latency benefit from polling overweights the > > loss from idle CPU capacity that otherwise would be available, or just when > > running under older Qemu versions that lack this hint. > > > > Let's provide a typical "force" module parameter that allows restoring the > > old behavior. > > > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > > --- > > drivers/cpuidle/cpuidle-haltpoll.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c > > index b0ce9bc78113..07e5b36076bb 100644 > > --- a/drivers/cpuidle/cpuidle-haltpoll.c > > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > > @@ -18,6 +18,11 @@ > > #include <linux/kvm_para.h> > > #include <linux/cpuidle_haltpoll.h> > > > > +static bool force __read_mostly; > > +module_param(force, bool, 0444); > > +MODULE_PARM_DESC(force, > > + "Load even if the host does not provide the REALTIME hint"); Why not to say "Load unconditionally" here? As is, one needs to know what "the REALTIME hint" is to understand it. > > + > > static struct cpuidle_device __percpu *haltpoll_cpuidle_devices; > > static enum cpuhp_state haltpoll_hp_state; > > > > @@ -90,6 +95,11 @@ static void haltpoll_uninit(void) > > haltpoll_cpuidle_devices = NULL; > > } > > > > +static bool haltpool_want(void) > > +{ > > + return kvm_para_has_hint(KVM_HINTS_REALTIME) || force; > > +} > > + > > static int __init haltpoll_init(void) > > { > > int ret; > > @@ -102,7 +112,7 @@ static int __init haltpoll_init(void) > > cpuidle_poll_state_init(drv); > > > > if (!kvm_para_available() || > > - !kvm_para_has_hint(KVM_HINTS_REALTIME)) > > + !haltpool_want()) And you don't need to break this line. > > return -ENODEV; > > > > ret = cpuidle_register_driver(drv); > > > > Thanks!
On Mon, Feb 24, 2020 at 09:10:18PM +0100, Maciej S. Szmigiero wrote: > (CC'ing also Marcelo as the cpuidle-haltpoll driver author and the KVM ML). Hi Maciej, > On 21.02.2020 18:43, Maciej S. Szmigiero wrote: > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > > > Before commit 1328edca4a14 ("cpuidle-haltpoll: Enable kvm guest polling > > when dedicated physical CPUs are available") the cpuidle-haltpoll driver > > could also be used in scenarios when the host does not advertise the > > KVM_HINTS_REALTIME hint. > > > > While the behavior introduced by the aforementioned commit makes sense as > > the default It makes sense for the pCPU overcommitted case only. > > there are cases where the old behavior is desired, for example, > > when other kernel changes triggered by presence by this hint are unwanted, > > for some workloads where the latency benefit from polling overweights the > > loss from idle CPU capacity that otherwise would be available, or just when > > running under older Qemu versions that lack this hint. > > > > Let's provide a typical "force" module parameter that allows restoring the > > old behavior. > > > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> I would rather: 1) Switch back to the default non-overcommitted case. or even better (but requires more investment) 2) Make on the flight dynamic configuration (after all pCPU overcommitment=true/false is a property that changes during the day, depending on system load). But its up to Paolo to decide, really. > > --- > > drivers/cpuidle/cpuidle-haltpoll.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c > > index b0ce9bc78113..07e5b36076bb 100644 > > --- a/drivers/cpuidle/cpuidle-haltpoll.c > > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > > @@ -18,6 +18,11 @@ > > #include <linux/kvm_para.h> > > #include <linux/cpuidle_haltpoll.h> > > > > +static bool force __read_mostly; > > +module_param(force, bool, 0444); > > +MODULE_PARM_DESC(force, > > + "Load even if the host does not provide the REALTIME hint"); > > + > > static struct cpuidle_device __percpu *haltpoll_cpuidle_devices; > > static enum cpuhp_state haltpoll_hp_state; > > > > @@ -90,6 +95,11 @@ static void haltpoll_uninit(void) > > haltpoll_cpuidle_devices = NULL; > > } > > > > +static bool haltpool_want(void) > > +{ > > + return kvm_para_has_hint(KVM_HINTS_REALTIME) || force; > > +} > > + > > static int __init haltpoll_init(void) > > { > > int ret; > > @@ -102,7 +112,7 @@ static int __init haltpoll_init(void) > > cpuidle_poll_state_init(drv); > > > > if (!kvm_para_available() || > > - !kvm_para_has_hint(KVM_HINTS_REALTIME)) > > + !haltpool_want()) > > return -ENODEV; > > > > ret = cpuidle_register_driver(drv); > >
Hi Marcelo, Thanks for your input. On 06.03.2020 10:56, Marcelo Tosatti wrote: (..) > or even better (but requires more investment) > > 2) Make on the flight dynamic configuration (after all pCPU > overcommitment=true/false is a property that changes during > the day, depending on system load). > > But its up to Paolo to decide, really. I think that for now, to cover the use cases that are described in the commit message and bring back the functionality that was kind of lost after commit 1328edca4a14, we should add this module parameter. Then we can think about some more comprehensive solution. Thanks, Maciej
On Wed, Mar 04, 2020 at 11:31:31AM +0100, Rafael J. Wysocki wrote: > On Friday, February 28, 2020 6:10:18 PM CET Maciej S. Szmigiero wrote: > > A friendly ping here. > > > > Maciej > > > > On 21.02.2020 18:43, Maciej S. Szmigiero wrote: > > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> > > > > > > Before commit 1328edca4a14 ("cpuidle-haltpoll: Enable kvm guest polling > > > when dedicated physical CPUs are available") the cpuidle-haltpoll driver > > > could also be used in scenarios when the host does not advertise the > > > KVM_HINTS_REALTIME hint. > > > > > > While the behavior introduced by the aforementioned commit makes sense as > > > the default there are cases where the old behavior is desired, for example, > > > when other kernel changes triggered by presence by this hint are unwanted, > > > for some workloads where the latency benefit from polling overweights the > > > loss from idle CPU capacity that otherwise would be available, or just when > > > running under older Qemu versions that lack this hint. > > > > > > Let's provide a typical "force" module parameter that allows restoring the > > > old behavior. > > > > > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> > > > --- > > > drivers/cpuidle/cpuidle-haltpoll.c | 12 +++++++++++- > > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c > > > index b0ce9bc78113..07e5b36076bb 100644 > > > --- a/drivers/cpuidle/cpuidle-haltpoll.c > > > +++ b/drivers/cpuidle/cpuidle-haltpoll.c > > > @@ -18,6 +18,11 @@ > > > #include <linux/kvm_para.h> > > > #include <linux/cpuidle_haltpoll.h> > > > > > > +static bool force __read_mostly; > > > +module_param(force, bool, 0444); > > > +MODULE_PARM_DESC(force, > > > + "Load even if the host does not provide the REALTIME hint"); > > Why not to say "Load unconditionally" here? Makes sense to me. > As is, one needs to know what "the REALTIME hint" is to understand it. > > > > + > > > static struct cpuidle_device __percpu *haltpoll_cpuidle_devices; > > > static enum cpuhp_state haltpoll_hp_state; > > > > > > @@ -90,6 +95,11 @@ static void haltpoll_uninit(void) > > > haltpoll_cpuidle_devices = NULL; > > > } > > > > > > +static bool haltpool_want(void) > > > +{ > > > + return kvm_para_has_hint(KVM_HINTS_REALTIME) || force; > > > +} > > > + > > > static int __init haltpoll_init(void) > > > { > > > int ret; > > > @@ -102,7 +112,7 @@ static int __init haltpoll_init(void) > > > cpuidle_poll_state_init(drv); > > > > > > if (!kvm_para_available() || > > > - !kvm_para_has_hint(KVM_HINTS_REALTIME)) > > > + !haltpool_want()) > > And you don't need to break this line. > > > > return -ENODEV; > > > > > > ret = cpuidle_register_driver(drv); > > > > > > > > > Thanks! > >
Hi Marcelo, On 10.03.2020 14:50, Marcelo Tosatti wrote: > On Wed, Mar 04, 2020 at 11:31:31AM +0100, Rafael J. Wysocki wrote: >> On Friday, February 28, 2020 6:10:18 PM CET Maciej S. Szmigiero wrote: >>> A friendly ping here. >>> >>> Maciej >>> >>> On 21.02.2020 18:43, Maciej S. Szmigiero wrote: >>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com> >>>> >>>> Before commit 1328edca4a14 ("cpuidle-haltpoll: Enable kvm guest polling >>>> when dedicated physical CPUs are available") the cpuidle-haltpoll driver >>>> could also be used in scenarios when the host does not advertise the >>>> KVM_HINTS_REALTIME hint. >>>> >>>> While the behavior introduced by the aforementioned commit makes sense as >>>> the default there are cases where the old behavior is desired, for example, >>>> when other kernel changes triggered by presence by this hint are unwanted, >>>> for some workloads where the latency benefit from polling overweights the >>>> loss from idle CPU capacity that otherwise would be available, or just when >>>> running under older Qemu versions that lack this hint. >>>> >>>> Let's provide a typical "force" module parameter that allows restoring the >>>> old behavior. >>>> >>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> >>>> --- >>>> drivers/cpuidle/cpuidle-haltpoll.c | 12 +++++++++++- >>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c >>>> index b0ce9bc78113..07e5b36076bb 100644 >>>> --- a/drivers/cpuidle/cpuidle-haltpoll.c >>>> +++ b/drivers/cpuidle/cpuidle-haltpoll.c >>>> @@ -18,6 +18,11 @@ >>>> #include <linux/kvm_para.h> >>>> #include <linux/cpuidle_haltpoll.h> >>>> >>>> +static bool force __read_mostly; >>>> +module_param(force, bool, 0444); >>>> +MODULE_PARM_DESC(force, >>>> + "Load even if the host does not provide the REALTIME hint"); >> >> Why not to say "Load unconditionally" here? > > Makes sense to me. Can you ack the v2 patch that has all the review comments resolved then? It is available here: https://lore.kernel.org/kvm/20200304113248.1143057-1-mail@maciej.szmigiero.name/ Thanks, Maciej
diff --git a/drivers/cpuidle/cpuidle-haltpoll.c b/drivers/cpuidle/cpuidle-haltpoll.c index b0ce9bc78113..07e5b36076bb 100644 --- a/drivers/cpuidle/cpuidle-haltpoll.c +++ b/drivers/cpuidle/cpuidle-haltpoll.c @@ -18,6 +18,11 @@ #include <linux/kvm_para.h> #include <linux/cpuidle_haltpoll.h> +static bool force __read_mostly; +module_param(force, bool, 0444); +MODULE_PARM_DESC(force, + "Load even if the host does not provide the REALTIME hint"); + static struct cpuidle_device __percpu *haltpoll_cpuidle_devices; static enum cpuhp_state haltpoll_hp_state; @@ -90,6 +95,11 @@ static void haltpoll_uninit(void) haltpoll_cpuidle_devices = NULL; } +static bool haltpool_want(void) +{ + return kvm_para_has_hint(KVM_HINTS_REALTIME) || force; +} + static int __init haltpoll_init(void) { int ret; @@ -102,7 +112,7 @@ static int __init haltpoll_init(void) cpuidle_poll_state_init(drv); if (!kvm_para_available() || - !kvm_para_has_hint(KVM_HINTS_REALTIME)) + !haltpool_want()) return -ENODEV; ret = cpuidle_register_driver(drv);