linux-kernel - Re: [PATCH] tracing/irq: use softirq_to_name instead of __print

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] [day] [month] [year] [list]

Date:	Tue, 02 Jun 2009 09:31:00 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	Christoph Hellwig <hch@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tracing/irq: use softirq_to_name instead of __print_symbolic

Steven Rostedt wrote:
> On Mon, 1 Jun 2009, Christoph Hellwig wrote:
> 
>> On Mon, Jun 01, 2009 at 04:52:23PM +0800, Li Zefan wrote:
>>> It's great to boost recording of softirq events, but why not simply
>>> use softirq_to_name in TP_printk()?
>> Because userspace programs using the binary trace buffer have no chance to
>> retrieve the values from softirq_to_name.
>>

Do you mean, by parsing the format file?

# cat events/irq/softirq_entry/format
	...
print fmt: "softirq=%d action=%s", REC->vec, ({ static const struct trace_print_flags
symbols[] =  { { HI_SOFTIRQ, "HI_SOFTIRQ" }, { TIMER_SOFTIRQ, "TIMER_SOFTIRQ" },
{ NET_TX_SOFTIRQ, "NET_TX_SOFTIRQ" }, { NET_RX_SOFTIRQ, "NET_RX_SOFTIRQ" },
{ BLOCK_SOFTIRQ, "BLOCK_SOFTIRQ" }, { TASKLET_SOFTIRQ, "TASKLET_SOFTIRQ" }, 
{ SCHED_SOFTIRQ, "SCHED_SOFTIRQ" }, { HRTIMER_SOFTIRQ, "HRTIMER_SOFTIRQ" },
{ RCU_SOFTIRQ, "RCU_SOFTIRQ" },  { -1, ((void *)0) }};
ftrace_print_symbols_seq(p, REC->vec, symbols); })

>>> The above commit has 2 flaws:
>>>
>>>   - we waste memory defining local static struct trace_print_flags array
>>>     in each softirq TRACE_EVENT
>> That could be solved by declaring the array manually and just passing
>> the address to __print_symbolic.  Steve, would that work?  (also for
>> __print_flags)
> 
> I added the code to make it an array, but we still need to allocate it for 
> every trace call. Otherwise, how do we export the information to 
> userspace. One trace event (currently) has no way of depending on another 
> trace event. If two trace events want the same array, currently they both 
> must allocate it. I haven't looked too deeply into seeing how to manage 
> that. It's not that much memory duplicated. I hate to make the code even 
> more complex just to save a couple of hundred of bytes.
> 
>>>   - if someone adds/removes a softirq, he may not know show_softirq_name()
>>>     needs to be updated
>> Just make sure you have the translation defined next to the actual
>> flags.  This is what I do in the XFS tracer:
>>
>> typedef enum xfs_alloctype
>> {
>> 	XFS_ALLOCTYPE_ANY_AG,           /* allocate anywhere, use rotor */
>> 	XFS_ALLOCTYPE_FIRST_AG,         /* ... start at ag 0 */
>> 	XFS_ALLOCTYPE_START_AG,         /* anywhere, start in this a.g. */
>> 	XFS_ALLOCTYPE_THIS_AG,          /* anywhere in this a.g. */
>> 	XFS_ALLOCTYPE_START_BNO,	/* near this block else anywhere */
>> 	XFS_ALLOCTYPE_NEAR_BNO,		/* in this a.g. and near this block */
>> 	XFS_ALLOCTYPE_THIS_BNO		/* at exactly this block */
>> } xfs_alloctype_t;
>>
>> #define XFS_ALLOC_TYPES \
>> 	{ XFS_ALLOCTYPE_ANY_AG,         "ANY_AG" }, \
>> 	{ XFS_ALLOCTYPE_FIRST_AG,       "FIRST_AG" }, \
>> 	{ XFS_ALLOCTYPE_START_AG,       "START_AG" }, \
>> 	{ XFS_ALLOCTYPE_THIS_AG,	"THIS_AG" }, \
>> 	{ XFS_ALLOCTYPE_START_BNO,	"START_BNO" }, \
>> 	{ XFS_ALLOCTYPE_NEAR_BNO,	"NEAR_BNO" }, \
>> 	{ XFS_ALLOCTYPE_THIS_BNO,	"THIS_BNO" }
>>
>>> Another issue with the above commit, that the output of softirq events
>>> becomes:
>>>
>>>   X-1701  [000]  1595.220739: softirq_entry: softirq=1 action=TIMER_SOFTIRQ
>>>
>>> Compared to the original output:
>>>
>>>   X-1701  [000]  1595.220739: softirq_entry: softirq=1 action=TIMER
>> Which is trivially fixable, see above :)
> 
> Hmm, I thought I fixed that already. Perhaps it is in the code that Ingo 
> has not pulled.
> 

I guess you forgot to fix it. ;)

I made a comment on that patch, but seems you didn't update the patch:
	http://lkml.org/lkml/2009/5/21/9

I'll make a patch for this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/