| Message ID | 20230317134448.11082-2-mhocko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memcg, cpuisol: do not interfere pcp cache charges draining with cpuisol workloads | expand |
On Fri, Mar 17, 2023 at 02:44:47PM +0100, Michal Hocko wrote: > From: Frederic Weisbecker <frederic@kernel.org> > > Provide this new API to check if a CPU has been isolated either through > isolcpus= or nohz_full= kernel parameter. > > It aims at avoiding kernel load deemed to be safely spared on CPUs > running sensitive workload that can't bear any disturbance, such as > pcp cache draining. Hi Michal, This makes no sense to me. HK_TYPE_DOMAIN is set when isolcpus=domain is configured. HK_TYPE_TICK is set when nohz_full= is configured. The use-cases i am aware of use either: isolcpus=managed_irq,... nohz_full= OR isolcpus=domain,managed_irq,... nohz_full= So what is the point of this function again? Perhaps it made sense along with, but now does not make sense anymore: Subject: [PATCH 1/2] sched/isolation: Merge individual nohz_full features into a common housekeeping flag The individual isolation features turned on by nohz_full were initially split in order for each of them to be tunable through cpusets. However plans have changed in favour of an interface (be it cpusets or sysctl) grouping all these features to be turned on/off altogether. Then should the need ever arise, the interface can still be expanded to handle the individual isolation features. But Michal can just use housekeeping_test_cpu(cpu, HK_TYPE_TICK) and the convertion of nohz_full features into a common housekeeping flag can convert that to something else later? > > Suggested-by: Michal Hocko <mhocko@suse.com> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org> > Signed-off-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/sched/isolation.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/include/linux/sched/isolation.h b/include/linux/sched/isolation.h > index 8c15abd67aed..fe1a46f30d24 100644 > --- a/include/linux/sched/isolation.h > +++ b/include/linux/sched/isolation.h > @@ -46,6 +46,12 @@ static inline bool housekeeping_enabled(enum hk_type type) > > static inline void housekeeping_affine(struct task_struct *t, > enum hk_type type) { } > + > +static inline bool housekeeping_test_cpu(int cpu, enum hk_type type) > +{ > + return true; > +} > + > static inline void housekeeping_init(void) { } > #endif /* CONFIG_CPU_ISOLATION */ > > @@ -58,4 +64,10 @@ static inline bool housekeeping_cpu(int cpu, enum hk_type type) > return true; > } > > +static inline bool cpu_is_isolated(int cpu) > +{ > + return !housekeeping_test_cpu(cpu, HK_TYPE_DOMAIN) || > + !housekeeping_test_cpu(cpu, HK_TYPE_TICK); > +} > + > #endif /* _LINUX_SCHED_ISOLATION_H */ > -- > 2.30.2 > >
On Fri, Mar 17, 2023 at 03:33:13PM -0300, Marcelo Tosatti wrote: > On Fri, Mar 17, 2023 at 02:44:47PM +0100, Michal Hocko wrote: > > From: Frederic Weisbecker <frederic@kernel.org> > > > > Provide this new API to check if a CPU has been isolated either through > > isolcpus= or nohz_full= kernel parameter. > > > > It aims at avoiding kernel load deemed to be safely spared on CPUs > > running sensitive workload that can't bear any disturbance, such as > > pcp cache draining. > > Hi Michal, > > This makes no sense to me. > > HK_TYPE_DOMAIN is set when isolcpus=domain is configured. > HK_TYPE_TICK is set when nohz_full= is configured. > > The use-cases i am aware of use either: > > isolcpus=managed_irq,... nohz_full= > OR > isolcpus=domain,managed_irq,... nohz_full= > > So what is the point of this function again? > > Perhaps it made sense along with, but now does not make sense > anymore: > > Subject: [PATCH 1/2] sched/isolation: Merge individual nohz_full features into a common housekeeping flag > > The individual isolation features turned on by nohz_full were initially > split in order for each of them to be tunable through cpusets. However > plans have changed in favour of an interface (be it cpusets or sysctl) > grouping all these features to be turned on/off altogether. Then should > the need ever arise, the interface can still be expanded to handle the > individual isolation features. > > But Michal can just use housekeeping_test_cpu(cpu, HK_TYPE_TICK) and > the convertion of nohz_full features into a common housekeeping flag > can convert that to something else later? Actually introducing cpu_is_isolated() seems fine, but it can call housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS.
On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: > On Fri, Mar 17, 2023 at 03:33:13PM -0300, Marcelo Tosatti wrote: > > On Fri, Mar 17, 2023 at 02:44:47PM +0100, Michal Hocko wrote: > > > From: Frederic Weisbecker <frederic@kernel.org> > > > > > > Provide this new API to check if a CPU has been isolated either through > > > isolcpus= or nohz_full= kernel parameter. > > > > > > It aims at avoiding kernel load deemed to be safely spared on CPUs > > > running sensitive workload that can't bear any disturbance, such as > > > pcp cache draining. > > > > Hi Michal, > > > > This makes no sense to me. > > > > HK_TYPE_DOMAIN is set when isolcpus=domain is configured. > > HK_TYPE_TICK is set when nohz_full= is configured. > > > > The use-cases i am aware of use either: > > > > isolcpus=managed_irq,... nohz_full= > > OR > > isolcpus=domain,managed_irq,... nohz_full= > > > > So what is the point of this function again? > > > > Perhaps it made sense along with, but now does not make sense > > anymore: > > > > Subject: [PATCH 1/2] sched/isolation: Merge individual nohz_full features into a common housekeeping flag > > > > The individual isolation features turned on by nohz_full were initially > > split in order for each of them to be tunable through cpusets. However > > plans have changed in favour of an interface (be it cpusets or sysctl) > > grouping all these features to be turned on/off altogether. Then should > > the need ever arise, the interface can still be expanded to handle the > > individual isolation features. > > > > But Michal can just use housekeeping_test_cpu(cpu, HK_TYPE_TICK) and > > the convertion of nohz_full features into a common housekeeping flag > > can convert that to something else later? > > Actually introducing cpu_is_isolated() seems fine, but it can call > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. This is not really my area. Frederic, could you have a look please?
Le Sat, Mar 18, 2023 at 09:04:38AM +0100, Michal Hocko a écrit : > On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: > > On Fri, Mar 17, 2023 at 03:33:13PM -0300, Marcelo Tosatti wrote: > > > On Fri, Mar 17, 2023 at 02:44:47PM +0100, Michal Hocko wrote: > > > > From: Frederic Weisbecker <frederic@kernel.org> > > > > > > > > Provide this new API to check if a CPU has been isolated either through > > > > isolcpus= or nohz_full= kernel parameter. > > > > > > > > It aims at avoiding kernel load deemed to be safely spared on CPUs > > > > running sensitive workload that can't bear any disturbance, such as > > > > pcp cache draining. > > > > > > Hi Michal, > > > > > > This makes no sense to me. > > > > > > HK_TYPE_DOMAIN is set when isolcpus=domain is configured. > > > HK_TYPE_TICK is set when nohz_full= is configured. > > > > > > The use-cases i am aware of use either: > > > > > > isolcpus=managed_irq,... nohz_full= > > > OR > > > isolcpus=domain,managed_irq,... nohz_full= > > > > > > So what is the point of this function again? > > > > > > Perhaps it made sense along with, but now does not make sense > > > anymore: > > > > > > Subject: [PATCH 1/2] sched/isolation: Merge individual nohz_full features into a common housekeeping flag > > > > > > The individual isolation features turned on by nohz_full were initially > > > split in order for each of them to be tunable through cpusets. However > > > plans have changed in favour of an interface (be it cpusets or sysctl) > > > grouping all these features to be turned on/off altogether. Then should > > > the need ever arise, the interface can still be expanded to handle the > > > individual isolation features. > > > > > > But Michal can just use housekeeping_test_cpu(cpu, HK_TYPE_TICK) and > > > the convertion of nohz_full features into a common housekeeping flag > > > can convert that to something else later? > > > > Actually introducing cpu_is_isolated() seems fine, but it can call > > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. > > This is not really my area. Frederic, could you have a look please? The point is to have a function that tells if either nohz_full= or isolcpus=[domain] has been passed for the given CPU. Because I assumed that both would be interested in avoiding that flush noise, wouldn't it be the case?
On Fri, Mar 24, 2023 at 11:35:35PM +0100, Frederic Weisbecker wrote: > Le Sat, Mar 18, 2023 at 09:04:38AM +0100, Michal Hocko a écrit : > > On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: > > > On Fri, Mar 17, 2023 at 03:33:13PM -0300, Marcelo Tosatti wrote: > > > > On Fri, Mar 17, 2023 at 02:44:47PM +0100, Michal Hocko wrote: > > > > > From: Frederic Weisbecker <frederic@kernel.org> > > > > > > > > > > Provide this new API to check if a CPU has been isolated either through > > > > > isolcpus= or nohz_full= kernel parameter. > > > > > > > > > > It aims at avoiding kernel load deemed to be safely spared on CPUs > > > > > running sensitive workload that can't bear any disturbance, such as > > > > > pcp cache draining. > > > > > > > > Hi Michal, > > > > > > > > This makes no sense to me. > > > > > > > > HK_TYPE_DOMAIN is set when isolcpus=domain is configured. > > > > HK_TYPE_TICK is set when nohz_full= is configured. > > > > > > > > The use-cases i am aware of use either: > > > > > > > > isolcpus=managed_irq,... nohz_full= > > > > OR > > > > isolcpus=domain,managed_irq,... nohz_full= > > > > > > > > So what is the point of this function again? > > > > > > > > Perhaps it made sense along with, but now does not make sense > > > > anymore: > > > > > > > > Subject: [PATCH 1/2] sched/isolation: Merge individual nohz_full features into a common housekeeping flag > > > > > > > > The individual isolation features turned on by nohz_full were initially > > > > split in order for each of them to be tunable through cpusets. However > > > > plans have changed in favour of an interface (be it cpusets or sysctl) > > > > grouping all these features to be turned on/off altogether. Then should > > > > the need ever arise, the interface can still be expanded to handle the > > > > individual isolation features. > > > > > > > > But Michal can just use housekeeping_test_cpu(cpu, HK_TYPE_TICK) and > > > > the convertion of nohz_full features into a common housekeeping flag > > > > can convert that to something else later? > > > > > > Actually introducing cpu_is_isolated() seems fine, but it can call > > > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. > > > > This is not really my area. Frederic, could you have a look please? > > The point is to have a function that tells if either nohz_full= or > isolcpus=[domain] has been passed for the given CPU. > > Because I assumed that both would be interested in avoiding that flush > noise, wouldn't it be the case? Yes, that is the case. But as a note: for the two main types of configuration performed (one uses isolcpus=[domain] and the other cgroups, for isolating processes) nohz_full= is always set. So just testing for nohz_full= would be sufficient (which perhaps would make the code simpler).
On Mon, Mar 27, 2023 at 07:24:54AM -0300, Marcelo Tosatti wrote: > On Fri, Mar 24, 2023 at 11:35:35PM +0100, Frederic Weisbecker wrote: > > Le Sat, Mar 18, 2023 at 09:04:38AM +0100, Michal Hocko a écrit : > > > On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: > > > > On Fri, Mar 17, 2023 at 03:33:13PM -0300, Marcelo Tosatti wrote: > > > > > On Fri, Mar 17, 2023 at 02:44:47PM +0100, Michal Hocko wrote: > > > > > > From: Frederic Weisbecker <frederic@kernel.org> > > > > > > > > > > > > Provide this new API to check if a CPU has been isolated either through > > > > > > isolcpus= or nohz_full= kernel parameter. > > > > > > > > > > > > It aims at avoiding kernel load deemed to be safely spared on CPUs > > > > > > running sensitive workload that can't bear any disturbance, such as > > > > > > pcp cache draining. > > > > > > > > > > Hi Michal, > > > > > > > > > > This makes no sense to me. > > > > > > > > > > HK_TYPE_DOMAIN is set when isolcpus=domain is configured. > > > > > HK_TYPE_TICK is set when nohz_full= is configured. > > > > > > > > > > The use-cases i am aware of use either: > > > > > > > > > > isolcpus=managed_irq,... nohz_full= > > > > > OR > > > > > isolcpus=domain,managed_irq,... nohz_full= > > > > > > > > > > So what is the point of this function again? > > > > > > > > > > Perhaps it made sense along with, but now does not make sense > > > > > anymore: > > > > > > > > > > Subject: [PATCH 1/2] sched/isolation: Merge individual nohz_full features into a common housekeeping flag > > > > > > > > > > The individual isolation features turned on by nohz_full were initially > > > > > split in order for each of them to be tunable through cpusets. However > > > > > plans have changed in favour of an interface (be it cpusets or sysctl) > > > > > grouping all these features to be turned on/off altogether. Then should > > > > > the need ever arise, the interface can still be expanded to handle the > > > > > individual isolation features. > > > > > > > > > > But Michal can just use housekeeping_test_cpu(cpu, HK_TYPE_TICK) and > > > > > the convertion of nohz_full features into a common housekeeping flag > > > > > can convert that to something else later? > > > > > > > > Actually introducing cpu_is_isolated() seems fine, but it can call > > > > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. > > > > > > This is not really my area. Frederic, could you have a look please? > > > > The point is to have a function that tells if either nohz_full= or > > isolcpus=[domain] has been passed for the given CPU. > > > > Because I assumed that both would be interested in avoiding that flush > > noise, wouldn't it be the case? > > Yes, that is the case. But as a note: for the two main types of > configuration performed (one uses isolcpus=[domain] and the other > cgroups, for isolating processes) nohz_full= is always set. > > So just testing for nohz_full= would be sufficient (which perhaps would > make the code simpler). Ok then all is needed is to test tick_nohz_full_cpu(target), right?
On Mon 27-03-23 07:24:54, Marcelo Tosatti wrote: > On Fri, Mar 24, 2023 at 11:35:35PM +0100, Frederic Weisbecker wrote: > > Le Sat, Mar 18, 2023 at 09:04:38AM +0100, Michal Hocko a écrit : > > > On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: [...] > > > > Actually introducing cpu_is_isolated() seems fine, but it can call > > > > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. > > > > > > This is not really my area. Frederic, could you have a look please? > > > > The point is to have a function that tells if either nohz_full= or > > isolcpus=[domain] has been passed for the given CPU. > > > > Because I assumed that both would be interested in avoiding that flush > > noise, wouldn't it be the case? > > Yes, that is the case. But as a note: for the two main types of > configuration performed (one uses isolcpus=[domain] and the other > cgroups, for isolating processes) nohz_full= is always set. > > So just testing for nohz_full= would be sufficient (which perhaps would > make the code simpler). I do not see any mention about that assumption under Documentation/. Is this a best practice documented anywhere or it just happens to be the case with workloads you deal with?
On Tue, Mar 28, 2023 at 01:48:02PM +0200, Michal Hocko wrote: > On Mon 27-03-23 07:24:54, Marcelo Tosatti wrote: > > On Fri, Mar 24, 2023 at 11:35:35PM +0100, Frederic Weisbecker wrote: > > > Le Sat, Mar 18, 2023 at 09:04:38AM +0100, Michal Hocko a écrit : > > > > On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: > [...] > > > > > Actually introducing cpu_is_isolated() seems fine, but it can call > > > > > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. > > > > > > > > This is not really my area. Frederic, could you have a look please? > > > > > > The point is to have a function that tells if either nohz_full= or > > > isolcpus=[domain] has been passed for the given CPU. > > > > > > Because I assumed that both would be interested in avoiding that flush > > > noise, wouldn't it be the case? > > > > Yes, that is the case. But as a note: for the two main types of > > configuration performed (one uses isolcpus=[domain] and the other > > cgroups, for isolating processes) nohz_full= is always set. > > > > So just testing for nohz_full= would be sufficient (which perhaps would > > make the code simpler). > > I do not see any mention about that assumption under Documentation/. Documentation/admin-guide/kernel-per-CPU-kthreads.rst SCHED_SOFTIRQ ------------- Do all of the following: 1. Avoid sending scheduler IPIs to the CPU to be de-jittered, for example, ensure that at most one runnable kthread is present on that CPU. If a thread that expects to run on the de-jittered CPU awakens, the scheduler will send an IPI that can result in a subsequent SCHED_SOFTIRQ. 2. CONFIG_NO_HZ_FULL=y and ensure that the CPU to be de-jittered is marked as an adaptive-ticks CPU using the "nohz_full=" boot parameter. This reduces the number of scheduler-clock interrupts that the de-jittered CPU receives, minimizing its chances of being selected to do the load balancing work that runs in SCHED_SOFTIRQ context. > Is this a best practice documented anywhere or it just happens to be > the case with workloads you deal with? Option 2. However Frederic seems interested in matching the exported toggles with the known use-cases classes. For example, for this guide: http://www.comfilewiki.co.kr/en/doku.php?id=comfilepi:improving_real-time_performance:index Using nohz_full= would be a benefit (and its not being currently set, perhaps due to not knowing all the options?). http://www.comfilewiki.co.kr/en/doku.php?id=comfilepi:improving_real-time_performance:index AFAIU the workloads for which disabling nohz_full= is a benefit are those where the switching between nohz full mode and sched tick enabled mode and vice-versa (which involve programming the local timer) happens often and is therefore avoidable? For example switching between 1 runnable task and more than 1 runnable task (and vice versa).
On Wed 29-03-23 11:20:21, Marcelo Tosatti wrote: > On Tue, Mar 28, 2023 at 01:48:02PM +0200, Michal Hocko wrote: > > On Mon 27-03-23 07:24:54, Marcelo Tosatti wrote: > > > On Fri, Mar 24, 2023 at 11:35:35PM +0100, Frederic Weisbecker wrote: > > > > Le Sat, Mar 18, 2023 at 09:04:38AM +0100, Michal Hocko a écrit : > > > > > On Fri 17-03-23 15:35:05, Marcelo Tosatti wrote: > > [...] > > > > > > Actually introducing cpu_is_isolated() seems fine, but it can call > > > > > > housekeeping_test_cpu(cpu, HK_TYPE_TICK) AFAICS. > > > > > > > > > > This is not really my area. Frederic, could you have a look please? > > > > > > > > The point is to have a function that tells if either nohz_full= or > > > > isolcpus=[domain] has been passed for the given CPU. > > > > > > > > Because I assumed that both would be interested in avoiding that flush > > > > noise, wouldn't it be the case? > > > > > > Yes, that is the case. But as a note: for the two main types of > > > configuration performed (one uses isolcpus=[domain] and the other > > > cgroups, for isolating processes) nohz_full= is always set. > > > > > > So just testing for nohz_full= would be sufficient (which perhaps would > > > make the code simpler). > > > > I do not see any mention about that assumption under Documentation/. > > Documentation/admin-guide/kernel-per-CPU-kthreads.rst > > SCHED_SOFTIRQ > ------------- > > Do all of the following: > > 1. Avoid sending scheduler IPIs to the CPU to be de-jittered, > for example, ensure that at most one runnable kthread is present > on that CPU. If a thread that expects to run on the de-jittered > CPU awakens, the scheduler will send an IPI that can result in > a subsequent SCHED_SOFTIRQ. > 2. CONFIG_NO_HZ_FULL=y and ensure that the CPU to be de-jittered > is marked as an adaptive-ticks CPU using the "nohz_full=" > boot parameter. This reduces the number of scheduler-clock > interrupts that the de-jittered CPU receives, minimizing its > chances of being selected to do the load balancing work that > runs in SCHED_SOFTIRQ context. Quite hidden and easy to miss if you are only aware of isolcpus. > > Is this a best practice documented anywhere or it just happens to be > > the case with workloads you deal with? > > Option 2. However Frederic seems interested in matching the exported > toggles with the known use-cases classes. > > For example, for this guide: > http://www.comfilewiki.co.kr/en/doku.php?id=comfilepi:improving_real-time_performance:index > > Using nohz_full= would be a benefit (and its not being currently set, > perhaps due to not knowing all the options?). > > http://www.comfilewiki.co.kr/en/doku.php?id=comfilepi:improving_real-time_performance:index > > > AFAIU the workloads for which disabling nohz_full= is a benefit are those > where the switching between nohz full mode and sched tick enabled mode > and vice-versa (which involve programming the local timer) happens > often and is therefore avoidable? For example switching between 1 > runnable task and more than 1 runnable task (and vice versa). The patch from Frederic is testing for both. You seem to be arguing to reduce the test and I still do not understand why. Sure some workloads (following the above) will likely use nohz_full= as well but does it make sense to build that expectation into the higher level logic? What is an actual benefit?
On Thu, Mar 30, 2023 at 03:28:52PM +0200, Michal Hocko wrote: > > > Is this a best practice documented anywhere or it just happens to be > > > the case with workloads you deal with? > > > > Option 2. However Frederic seems interested in matching the exported > > toggles with the known use-cases classes. > > > > For example, for this guide: > > http://www.comfilewiki.co.kr/en/doku.php?id=comfilepi:improving_real-time_performance:index > > > > Using nohz_full= would be a benefit (and its not being currently set, > > perhaps due to not knowing all the options?). > > > > http://www.comfilewiki.co.kr/en/doku.php?id=comfilepi:improving_real-time_performance:index > > > > > > AFAIU the workloads for which disabling nohz_full= is a benefit are those > > where the switching between nohz full mode and sched tick enabled mode > > and vice-versa (which involve programming the local timer) happens > > often and is therefore avoidable? For example switching between 1 > > runnable task and more than 1 runnable task (and vice versa). > > The patch from Frederic is testing for both. You seem to be arguing to > reduce the test and I still do not understand why. Sure some workloads > (following the above) will likely use nohz_full= as well but does it > make sense to build that expectation into the higher level logic? What > is an actual benefit? Just thinking of simpler code. Feel free to maintain the patch as-is if you see fit.
diff --git a/include/linux/sched/isolation.h b/include/linux/sched/isolation.h index 8c15abd67aed..fe1a46f30d24 100644 --- a/include/linux/sched/isolation.h +++ b/include/linux/sched/isolation.h @@ -46,6 +46,12 @@ static inline bool housekeeping_enabled(enum hk_type type) static inline void housekeeping_affine(struct task_struct *t, enum hk_type type) { } + +static inline bool housekeeping_test_cpu(int cpu, enum hk_type type) +{ + return true; +} + static inline void housekeeping_init(void) { } #endif /* CONFIG_CPU_ISOLATION */ @@ -58,4 +64,10 @@ static inline bool housekeeping_cpu(int cpu, enum hk_type type) return true; } +static inline bool cpu_is_isolated(int cpu) +{ + return !housekeeping_test_cpu(cpu, HK_TYPE_DOMAIN) || + !housekeeping_test_cpu(cpu, HK_TYPE_TICK); +} + #endif /* _LINUX_SCHED_ISOLATION_H */