linux-kernel - Re: [PATCH 1/3] tracing: Merge irqflags + preempt counter.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210125140551.hlpbreks4f7ytuza@linutronix.de>
Date:   Mon, 25 Jan 2021 15:05:51 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 1/3] tracing: Merge irqflags + preempt counter.

On 2021-01-22 17:07:50 [-0500], Steven Rostedt wrote:
> On Wed, 13 Jan 2021 00:00:55 +0100
> Sebastian Andrzej Siewior <bigeasy@...utronix.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