linux-kernel - Re: [RFC][PATCH] writeback: Unduplicate writeback reason

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: <20111214131619.GA17225@localhost>
Date:	Wed, 14 Dec 2011 21:16:19 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	Curt Wohlgemuth <curtw@...gle.com>, Jan Kara <jack@...e.cz>,
	Christoph Hellwig <hch@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH] writeback: Unduplicate writeback reason

On Wed, Dec 14, 2011 at 11:28:44AM +0800, Dave Chinner wrote:
> On Tue, Dec 13, 2011 at 08:14:00PM -0500, Steven Rostedt wrote:
> > Names of the writeback reasons are used in both the main kernel as well
> > as for parsing the tracepoint format file. Instead of duplicating the
> > names in two locations making it likely that they may become out of
> > sync, use some macro magic to make sure all the names stay in sync. Any
> > update only needs to happen in one spot for it to take place in all
> > locations.
> > 
> > Note, this is an RFC patch, and it probably needs much better comments
> > (well, it currently has no comments), and the C() macro probably should
> > have a different name too.
> 
> I'm not sure this is a pattern we want to repeat all over the place -
> print_symbolic() is quite widely used and adding macro redefinitions
> all over the place doesn't fill me with joy.

Yeah, unfortunately...

> AFAICT this code doesn't need a declared array to work so you can

You mean the string array wb_reason_name[]? Ah it's actually not used
for now -- until there comes the (planned) writeback stats patch to
show the reason names in some debugfs/sysfs interface.

So for the upcoming 3.2, wb_reason_name[] can be removed to avoid the
duplication. However the question still remains how exactly are we
going to re-introduce it in future?

> just use a preprocessor construct like this (as used in XFS):
> 
> #define value_1 	1
> #define value_2		2
> .....
> 
> or
> 
> enum {
> 	value_1	= 1,
> 	value_2 = 2,
> 	.....
> }
> 
> followed by:
> 
> #define VALUES	\
> 	{ value_1,	"Value 1" }, \
> 	{ value_2,	"Value 2" }, \
> 	.....
> 
> And it just uses print_symbolic(__entry->value, VALUES); to print
> them out.

If using the above macros, wb_reason_name[] can be defined as

        static const struct trace_print_flags wb_reason_name[] = { VALUES };

and reference it in this way

        wb_reason_name[reason][1]

The first element is redundant info and will be ignored, because

        wb_reason_name[reason][0] == reason

> If this construct does everything requiredi, then I think it is a
> much better pattern to use because it's easy to maintain, doesn't
> require an array to be declared in a C file and doesn't require
> macro tricks to do it's job....

Hmm, I looked through XFS tracing code and find no use case like the
wb_reason_name[]. That is, the XFS symbolic names are only used for
tracing output and there is no sharing with debugfs/sysfs outputs.
So we may be talking about different situations.

Thanks,
Fengguang
--
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/