On 2021-01-22 17:07:50 [-0500], Steven Rostedt wrote:
> On Wed, 13 Jan 2021 00:00:55 +0100
> Sebastian Andrzej Siewior <bige...@linutronix.de> wrote:
>
> > +unsigned int _tracing_gen_ctx_flags(unsigned long irqflags);
> > +unsigned int tracing_gen_ctx_flags(void);
> > +unsigned int tracing_gen_ctx_flags_dect(void);
>
> Since I'm now trying to avoid underscore functions when possible, the above
> should be:
>
> _tracing_gen_ctx_flags() --> tracing_gen_ctx_flags()
> tracing_gen_ctx_flags() --> tracing_gen_ctx()
> tracing_gen_ctx_flags_dect --> tracing_gen_ctx_dec()
>
> Note, "_dec()" should be used instead of "_dect()" as "_dec" is commonly
> used around the kernel, and I had really no idea what "_dect" was, until I
> looked at the two functions.
Okay, let me do the renames.
> And looking at the implementation, I wonder if we should make this into
> static inlines in the header as follows:
>
> unsigned int tracing_gen_ctx_irq_test(int irqs_status);
>
> #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
> static inline unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
> {
> unsigned int irq_status = irqs_disabled(irqflags) ?
> TRACE_FLAG_IRQS_OFF : 0;
> return tracing_gen_ctx_irq_test(irq_status);
> }
> static inline unsigned int tracing_gen_ctx(void)
> {
> unsigned long irqflags;
>
> local_save_flags(irqflags);
> return tracing_gen_ctx_flags(irqflags);
> }
> #else
> static inline unsigned int tracing_gen_ctx_flags(unsigned long irqflags)
> {
> return tracing_gen_ctx_irq_test(TRACE_FLAG_IRQS_NOSUPPORT);
> }
> static inline unsigned int tracing_gen_ctx(void)
> {
> return tracing_gen_ctx_irq_test(TRACE_FLAG_IRQS_NOSUPPORT);
> }
> #endif
>
> static inline unsigned int tracing_gen_ctx_dec(void)
> {
> unsigned int trace_ctx;
>
> trace_ctx = tracing_gen_ctx();
> /*** Comment about buffer_reserve here ***/
> if (IS_ENABLED(CONFIG_PREEMPTION))
> trace_ctx--;
> return trace_ctx;
> }
>
> Then all you need to do is implement the tracing_gen_ctx_irq_test() without
> adding and #ifdefs in it, and just or in the "irq_status" to trace_flags,
> without any conditionals.
You just moved that from one place to another. I had to move enum
trace_flag_type just before tracing_gen_ctx_irq_test() so it is
available by tracing_gen_ctx_flags(). I don't know if you earn by
inlining much, the gcc numbers for vmlinux:
text data bss dec filename
24306853 21869070 14205180 60381103 vmlinux-rc5
24306482 21869070 14205180 60380732 vmlinux-rc5-merge-irqflags
24306852 21869070 14205180 60381102 vmlinux-rc5-merge-irqflags-inline
I know that a reduction by ~300 byte isn't exactly breath taking. I run
the test twice because the reduction in text by one byte looked odd but
it is what it is. Inlining only tracing_gen_ctx_dec() probably makes
sense especially since there is one user.
I can post the irqflags-merge and the inlinining as two seprate patches
and you can then decide if you merge the two patches or drop the
inlining.
> -- Steve
Sebastian
