* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint [not found] <20200221174331.1480468-1-mail@maciej.szmigiero.name> @ 2020-02-24 20:10 ` Maciej S. Szmigiero 2020-03-06 9:56 ` Marcelo Tosatti 2020-02-28 17:10 ` Maciej S. Szmigiero 1 sibling, 1 reply; 7+ messages in thread From: Maciej S. Szmigiero @ 2020-02-24 20:10 UTC (permalink / raw) To: Rafael J. Wysocki, Daniel Lezcano Cc: Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, Marcelo Tosatti, kvm (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); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint 2020-02-24 20:10 ` [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint Maciej S. Szmigiero @ 2020-03-06 9:56 ` Marcelo Tosatti 2020-03-06 20:11 ` Maciej S. Szmigiero 0 siblings, 1 reply; 7+ messages in thread From: Marcelo Tosatti @ 2020-03-06 9:56 UTC (permalink / raw) To: Maciej S. Szmigiero, Paolo Bonzini Cc: Rafael J. Wysocki, Daniel Lezcano, Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, kvm 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); > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint 2020-03-06 9:56 ` Marcelo Tosatti @ 2020-03-06 20:11 ` Maciej S. Szmigiero 0 siblings, 0 replies; 7+ messages in thread From: Maciej S. Szmigiero @ 2020-03-06 20:11 UTC (permalink / raw) To: Marcelo Tosatti Cc: Paolo Bonzini, Rafael J. Wysocki, Daniel Lezcano, Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, kvm, Konrad Rzeszutek Wilk 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint [not found] <20200221174331.1480468-1-mail@maciej.szmigiero.name> 2020-02-24 20:10 ` [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint Maciej S. Szmigiero @ 2020-02-28 17:10 ` Maciej S. Szmigiero 2020-03-04 10:31 ` Rafael J. Wysocki 1 sibling, 1 reply; 7+ messages in thread From: Maciej S. Szmigiero @ 2020-02-28 17:10 UTC (permalink / raw) To: Rafael J. Wysocki, Daniel Lezcano Cc: Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, Marcelo Tosatti, kvm 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); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint 2020-02-28 17:10 ` Maciej S. Szmigiero @ 2020-03-04 10:31 ` Rafael J. Wysocki 2020-03-10 13:50 ` Marcelo Tosatti 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2020-03-04 10:31 UTC (permalink / raw) To: Maciej S. Szmigiero Cc: Daniel Lezcano, Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, Marcelo Tosatti, kvm 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint 2020-03-04 10:31 ` Rafael J. Wysocki @ 2020-03-10 13:50 ` Marcelo Tosatti 2020-03-11 21:30 ` Maciej S. Szmigiero 0 siblings, 1 reply; 7+ messages in thread From: Marcelo Tosatti @ 2020-03-10 13:50 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Maciej S. Szmigiero, Daniel Lezcano, Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, kvm 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! > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint 2020-03-10 13:50 ` Marcelo Tosatti @ 2020-03-11 21:30 ` Maciej S. Szmigiero 0 siblings, 0 replies; 7+ messages in thread From: Maciej S. Szmigiero @ 2020-03-11 21:30 UTC (permalink / raw) To: Marcelo Tosatti Cc: Rafael J. Wysocki, Daniel Lezcano, Joao Martins, Boris Ostrovsky, linux-pm, linux-kernel, kvm, Konrad Rzeszutek Wilk 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-11 21:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200221174331.1480468-1-mail@maciej.szmigiero.name>
2020-02-24 20:10 ` [PATCH] cpuidle-haltpoll: allow force loading on hosts without the REALTIME hint Maciej S. Szmigiero
2020-03-06 9:56 ` Marcelo Tosatti
2020-03-06 20:11 ` Maciej S. Szmigiero
2020-02-28 17:10 ` Maciej S. Szmigiero
2020-03-04 10:31 ` Rafael J. Wysocki
2020-03-10 13:50 ` Marcelo Tosatti
2020-03-11 21:30 ` Maciej S. Szmigiero
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).