| Message ID | 20231107215742.363031-4-ankur.a.arora@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make the kernel preemptible | expand |
On Tue, 7 Nov 2023 13:56:49 -0800
Ankur Arora <ankur.a.arora@oracle.com> wrote:
> This reverts commit 089c02ae2771a14af2928c59c56abfb9b885a8d7.
I rather not revert this.
If user space can decided between various version of preemption, then the
trace should reflect that. At least state what the preemption model was when
a trace started, or currently is.
That is, the model may not be "static" per boot. Anyway, the real change here should be:
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7b4b1fcd6f93..2553c4efca15 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2208,14 +2208,6 @@ static inline void cond_resched_rcu(void)
#endif
}
-#ifdef CONFIG_PREEMPT_DYNAMIC
-
-extern bool preempt_model_none(void);
-extern bool preempt_model_voluntary(void);
-extern bool preempt_model_full(void);
-
-#else
-
static inline bool preempt_model_none(void)
{
return IS_ENABLED(CONFIG_PREEMPT_NONE);
@@ -2229,8 +2221,6 @@ static inline bool preempt_model_full(void)
return IS_ENABLED(CONFIG_PREEMPT);
}
-#endif
-
static inline bool preempt_model_rt(void)
{
return IS_ENABLED(CONFIG_PREEMPT_RT);
Then this way we can decided to make it runtime dynamic, we don't need to
fiddle with the tracing code again.
-- Steve
Steven Rostedt <rostedt@goodmis.org> writes: > On Tue, 7 Nov 2023 13:56:49 -0800 > Ankur Arora <ankur.a.arora@oracle.com> wrote: > >> This reverts commit 089c02ae2771a14af2928c59c56abfb9b885a8d7. > > I rather not revert this. > > If user space can decided between various version of preemption, then the > trace should reflect that. At least state what the preemption model was when > a trace started, or currently is. > Oh absolutely. As I mention in the cover at least these three patches would be back: 089c02ae2771 ("ftrace: Use preemption model accessors for trace header printout") cfe43f478b79 ("preempt/dynamic: Introduce preemption model accessors") 5693fa74f98a ("kcsan: Use preemption model accessors") The intent was (which I didn't do for the RFC), to do the reverts as cleanly as possible, do the changes for the series and then bring these patches back with appropriate modifications. > That is, the model may not be "static" per boot. Anyway, the real change here should be: Yeah, I intended to do something like that. Or would you prefer these not be reverted (and reapplied) at all -- just fixed as you describe here? > Then this way we can decided to make it runtime dynamic, we don't need to > fiddle with the tracing code again. Yeah, that makes sense. -- ankur
On Tue, 07 Nov 2023 15:23:05 -0800 Ankur Arora <ankur.a.arora@oracle.com> wrote: > Or would you prefer these not be reverted (and reapplied) at all -- just fixed > as you describe here? Yes, exactly that. Thanks, -- Steve
On Tue, 7 Nov 2023 18:31:54 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Tue, 07 Nov 2023 15:23:05 -0800 > Ankur Arora <ankur.a.arora@oracle.com> wrote: > > > Or would you prefer these not be reverted (and reapplied) at all -- just fixed > > as you describe here? > > Yes, exactly that. > Note, a revert usually means, "get rid of something because it's broken", it shouldn't be used for "I'm implementing this differently, and need to remove the old code first" For the latter case, just remove what you don't need for the reason why it's being removed. Reverting commits is confusing, because when you see a revert in a git log, you think that commit was broken and needed to be taken out. -- Steve
Steven Rostedt <rostedt@goodmis.org> writes: > On Tue, 7 Nov 2023 18:31:54 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> On Tue, 07 Nov 2023 15:23:05 -0800 >> Ankur Arora <ankur.a.arora@oracle.com> wrote: >> >> > Or would you prefer these not be reverted (and reapplied) at all -- just fixed >> > as you describe here? >> >> Yes, exactly that. >> > > Note, a revert usually means, "get rid of something because it's broken", it > shouldn't be used for "I'm implementing this differently, and need to > remove the old code first" > > For the latter case, just remove what you don't need for the reason why > it's being removed. Reverting commits is confusing, because when you see a > revert in a git log, you think that commit was broken and needed to be taken > out. Ack that. And, agree, it did feel pretty odd to revert so many good commits. I guess in that sense it makes sense to minimize the number of reverts. There are some that I suspect I will have to revert. Will detail specifically why they are being reverted. Thanks -- ankur
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index abaaf516fcae..7f565f0a00da 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -4392,11 +4392,17 @@ print_trace_header(struct seq_file *m, struct trace_iterator *iter) entries, total, buf->cpu, - preempt_model_none() ? "server" : - preempt_model_voluntary() ? "desktop" : - preempt_model_full() ? "preempt" : - preempt_model_rt() ? "preempt_rt" : +#if defined(CONFIG_PREEMPT_NONE) + "server", +#elif defined(CONFIG_PREEMPT_VOLUNTARY) + "desktop", +#elif defined(CONFIG_PREEMPT) + "preempt", +#elif defined(CONFIG_PREEMPT_RT) + "preempt_rt", +#else "unknown", +#endif /* These are reserved for later use */ 0, 0, 0, 0); #ifdef CONFIG_SMP
This reverts commit 089c02ae2771a14af2928c59c56abfb9b885a8d7. Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com> --- kernel/trace/trace.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)