RFC - size tool for kernel build system

I've been thinking about a tool that might be useful to track kernel size changes. I'm posting this Request For Comments to get feedback, and determine if this is something that would be worthwhile to pursue.

What I envision is some new kernel build targets, specifically related to gathering size information and generating a size comparison report. Some small helper scripts would be written to gather the necessary information, and generate the report.

A kernel developer would type:

1) make size-baseline

And kernel size information would be recorded for the current kernel (after a build, if needed).
I envision this saving off the .config and System.map, the result of 'size vmlinux' and several of the 'size */builtin.o' results.

Additionally (and optionally), a program could be run to acquire some size information from a running system (e.g. a newly booted system, or a system under a particular load), to include in the baseline report.

All of the gathered information would be stored
as the "size baseline".

-------

After making some modifications, either to the source or the configuration, the developer could type:

2) make size-report

The kernel size information would be recorded again, and compared with the size-baseline results. A report of differences (e.g. from bloat-o-meter and other comparison tools) would be produced. Any differences exceeding some threshhold (specified in a size-watch config file?) could be highlighted. The git commit IDs would be recorded, as well as differences between the configs used (e.g. diffconfig output).

If some designated size difference exceeds
a threshold (specified in the size-watch configuration) then the make could return an error, while still producing the report. This would mean that this could be used for git bisection to find a size regression.

Another way to look at this, would be that a developer could pick a specific size value to monitor (for example, the static size of the network sub-system, or the size of a particular slab in the dynamic memory of a newly booted kernel). They would specify that in the size-watch config, and could monitor that size over time and under various configurations.

I envision a couple of usages:
1) A developer could use this to be able to see a report about the total size increases caused by a patch they are about to submit

2) A developer could compare kernel versions for overall size changes

3) A maintainer could examine the affect of a patch on the size of their subsystem.

4) A developer could compare different kernel configs to see the impact of configuration option choices.

5) An automated tool could generate size values to associate with different config option choices (at least, starting from a consistent config set).

6) An automated tool could generate size values for each kernel version (this is what Bloatwatch does now).

Bloatwatch generates information on the static size information for various kernel versions. This would have a similar purpose, but the intent would be to integrate it into the kernel build system, to allow any developer to measure the size information, and highlight and track the information of their choice.

Any comments?
-- Tim

=============================
Tim Bird
Architecture Group Chair, CE Linux Forum Senior Staff Engineer, Sony Corporation of America
=============================

The kernel build system is supposed to be stateless, and integrating this with make would mess that up. If your goal is to get more people to use Bloatwatch so they don't make your job quite as difficult, it would probably be more appropriate to put a size analysis script in scripts/ (like checkpatch.pl) that looks at only the kernel you just built and generates thorough statistics in a format readable by both humans and Bloatwatch, preferably something easily diffed. Then developers could use that output in mailing list discussions without having to use Bloatwatch, but embedded developers who care about this enough to use Bloatwatch can be confident that they're working with the same numbers that the rest of us are discussing with the plain text on the lists.

-- Chris

Not really. Bloatwatch is a means to an end. It's only the end I care about, which is more visibility into kernel size growth over time.

The intent would be to do as you describe (although I don't really care if the format is readable by Bloatwatch). I'm a Bloatwatch proponent (I'm not the author), but this suggestion doesn't really have anything to do with Bloatwatch. Maybe I should have avoided mentioning it in my initial post.

Having a make target to run the script is just
a method of making it as easy as possible to run it. (Kind of like 'make checkstack' or 'make cscope') checkpatch.pl is different in that it operates on something not in the tree yet. It doesn't make sense as a make target so I don't think it's comparable.

Maybe it would be simpler to omit the baseline comparison capability at first, and just focus on a canonical size report (script and report format) for the kernel.

Discussing the number with the plain text on the lists is the ultimate goal. My thinking is that I'd like it to be used kind of like 'powertop' (except not as a runtime tool but as a development-time tool). The main intent is just to give developers more information about how their changes affect the kernel size.

Thanks for the feedback.
-- Tim

=============================
Tim Bird
Architecture Group Chair, CE Linux Forum Senior Staff Engineer, Sony Corporation of America
=============================

The building blocks that would be useful are IMHO:
- a make target that generates a report for one kernel (like the checkstack or export_report targets)
- a script that compares two such reports and outputs the size differences

That's also easy to do, and if that's what's wanted I can send a patch that does it.

Everything else is IMHO overdesigned.

The real problem is that dumping some scripts into the kernel sources or publishing some data on a webpage doesn't make people use them.

Like if you run "make checkstack" on the kernel today you can see that drivers allocate arrays > 1 kB on the stack despite checkstack being available...

cu
Adrian

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Please do.

Funny you should mention that. Yesterday I noticed that make checkstack had been ported to five more architectures since my last look at the code. It doesn't seem likely that those ports were required by some pointy-haired boss for feature-completeness. Someone must actually be using it.

The very beauty of make checkstack is that you don't even notice whether it is being used or not. You point to some drivers that apparently didn't use it, which is fine. But how many drivers _did_ use it? How many problems have been solved before the patches have ever been posted for review? Noone knows. And that is a good thing. We want the problems to get solved and not become visible in the first place.

Bloatwatch imo has the design flaw that it is a central tool hosted on some server somewhere and only documents the damage once it has happened. It would be much better if every developer could run something simple locally and clean up the mess before anyone else notices.

I partially agree with you in one point. It would be even better if checkstack, bloatcheck, etc. were run automatically on every kernel compile and developers were forced to look at any problems that come up. But that would increase compile time, which is bad. So there needs to be an off button as well, as there is for sparse - where off is the default.

Jörn

But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens

On Thu 9 Oct 2008 11:21, Adrian Bunk pondered:

and the report includes sizes of more than just the text section? Which is my biggest pet peeve with bloat-o-meter today, since it uses nm, not readelf - and saving data is just as important as saving instruction.

I understand the desire though - make it easier to compare two setups.

capturing a make target into a file, (make size_report > config1.size) and running a compare on two outputs seems like a more "standard" way of doing things...

I took a stab at this with the attached two scripts. These are not quite ready for prime time, but show the basic idea. I only have a partial list of subsystems, and am skipping the runtime data collection, for now.

I have only made the scripts, not any make targets for them.

I record all data into a flat namespace, which makes it easier to compare later.

One element of this design is the ability to configure the diff-size-report tool to watch only certain values, and to return a non-zero exit code under certain conditions. This makes it possible to use the tool with git-bisect to find the source of a size regression. I believe Linus asked for something like this at the last kernel summit.

Without the use of the config file, diff-size-report is very to bloat-o-meter, but provides info about additional aggregate items (like subsystems and the full kernel).

Feedback is welcome.
-- Tim

commit d4c8434396cc9a06dbd682f4eea44e2cfb44950f

Author: Tim Bird <[email protected]>
Date:   Thu Oct 9 16:50:08 2008 -0700

Add size reporting and monitoring scripts to the Linux kernel

new file mode 100755
index 0000000..40663cb
@@ -0,0 +1,237 @@
+#!/usr/bin/python
+#
+# diff-size-report - tool to show differences between two size reports
+#
+# Copyright 2008 Sony Corporation
+#
+# GPL version 2.0 applies
+#
+
+import sys, os
+
+conf_file="scripts/diff-size.conf"
+
+def usage():
+	print """Usage: %s file1 file2
+
+If a configuration file is present, then only show requested info.
+The default config file is: %s
+
+If any threshold specified in the config file is exceeded, the
+program returns a non-zero exit code.  This should be useful with
+git-bisect, to find the commit which creates a size regression.
+
+A sample config file is:
+  watch kernel_total
+  threshold kernel_total 15%%
+  watch subsys_net_text changes
+  threshold subsys_drivers_char_total +20000
+  threshold symbol___log_buf 64000
+
+This always shows the value of kernel_total, and shows a warning if
+the kernel_total increases by more than 15%% from file1 to file2.
+It only shows subsys_net_text if it's value changes.  It shows a warning
+if subsys_drivers_char_total increases more than 20000 bytes, and a
+warning if symbol___log_buf is bigger than 64000 bytes.
+""" % (os.path.basename(sys.argv[0]), conf_file)
+
+def read_report(filename):
+	lines = open(filename).readlines()
+	d = {}
+	in_block=0
+	for line in lines:
+		# accrete block, if still in one
+		if in_block:
+			if line.startswith(block_name+"_end"):
+				in_block=0
+				d[block_name] = block
+				continue
+			block += line
+			continue
+
+		# ignore empty lines and comments
+		if not line.strip() or line.startswith("#"):
+			continue
+
+		# get regular one-line value
+		if line.find("=") != -1:
+			name, value = line.split('=',1)
+			name = name.strip()
+			value = value.strip()
+			try:
+				value = int(value)
+			except:
+				pass
+			d[name] = value
+			continue
+
+		# check for start of block
+		if line.find("_start:") != -1 and not in_block:
+			in_block=1
+			block = ""
+			block_name=line.split("_start:")[0]
+			continue
+			
+		sys.stderr.write("Unrecognized line in file %s\n" % filename)
+		sys.stderr.write("line=%s"  % line)
+
+	if in_block:
+		sys.stderr.write("Error: Untermined block '%s' in file %s\n"\
+			 % (block_name, filename))
+	return d
+
+
+def show_warning(msg, value, t_str, name, o, d):
+	print "WARNING: %s of %d exceeds threshold of %s for '%s'" % \
+		(msg, value, t_str, name)
+	pchange = (float(d)/float(o))* 100
+	print "Old value: %d,  New value: %d, Change: %d (%.1f%%)" % \
+		(o, o+d, d, pchange)
+
+# allowed thresholds are:
+# +t - tree if delta > t
+# -t - tree if delta < t
+# t - true if new value > t
+# t% - true if delta > old value + t%
+def do_threshold(name, o, d, t_str):
+	rcode = 0
+	if t_str.startswith("+"):
+		t = int(t_str[1:])
+		if d > t:
+			show_warning("Change", d, t_str, name, o, d)
+			rcode = -1
+		return rcode
+
+	if t_str.startswith("-"):
+		t = int(t_str[1:])
+		if delta < t:
+			show_warning("Change", d, t_str, name, o, d)
+			rcode = -1
+		return rcode
+
+	if t_str.endswith("%"):
+		# handle percentage
+		t = o + (o*int(t_str[:-1]))/100
+		if o+d>t:
+			show_warning("Change", d, t_str, name, o, d)
+			rcode = -1
+		return rcode
+
+	t = int(t_str)
+	if o+d>t:
+		show_warning("Value", o+d, t_str, name, o, d)
+		rcode = -1
+	return rcode
+			
+
+# returns non-zero on threshold exception
+def process_report_conf(conf_file, old, delta):
+	rcode = 0
+	conf_list = open(conf_file).readlines()
+
+	# convert delta list to map
+	dmap = {}
+	for (value, name) in delta:
+		dmap[name] = value
+
+	for c in conf_list:
+		if not c.strip or c.startswith("#"):
+			continue
+		cparts = c.split()
+		cmd = cparts[0]
+		if cmd=="watch":
+			name = cparts[1]
+			if not dmap.has_key(name):
+				sys.stderr.write("Error: could not find item '%s' to watch\n" % name)
+				continue
+			d = dmap[name]
+			o = old[name]
+			
+			if len(cparts)>2 and cparts[2].startswith("change") \
+				and d==0:
+				# skip unchanged values	
+				continue
+			if d==0:
+				print "%s stayed at %d bytes" % (name, o)
+				continue
+			
+			p = (float(d)/float(o))* 100
+			print "%s changed by %d bytes (%.1f%%)" % (name, d, p)
+			continue
+
+		if cmd=="threshold":
+			name = cparts[1]
+			t_str = cparts[2]
+			if not dmap.has_key(name):
+				sys.stderr.write("Error: could not find item '%s' for threshold check\n" % name)
+				continue
+			o = old.get(name, 0)
+			d = dmap[name]
+			rcode |= do_threshold(name, o, d, t_str)
+
+	return rcode
+
+
+def main():
+	if len(sys.argv) != 3:
+		usage()
+		sys.exit(1)
+
+	old = read_report(sys.argv[1])
+	new = read_report(sys.argv[2])
+
+	# ignore kernel config (should do diffconfig eventually)
+	old_config = old["kernel_config"]
+	del(old["kernel_config"])
+	new_config =  new["kernel_config"]
+	del(new["kernel_config"])
+
+	# delta generation copied from bloat-o-meter
+	up = 0
+	down = 0
+	delta = []
+	common = {}
+
+	for a in old:
+		if a in new:
+			common[a] = 1
+
+	for name in old:
+		if name not in common:
+			down += old[name]
+			delta.append((-old[name], name))
+
+	for name in new:
+		if name not in common:
+			up += new[name]
+			delta.append((new[name], name))
+
+	for name in common:
+		d = new.get(name, 0) - old.get(name, 0)
+		if d>0: up += d
+		if d<0: down -= d
+		delta.append((d, name))
+
+	delta.sort()
+	delta.reverse()
+
+	if os.path.isfile(conf_file):
+		rcode = process_report_conf(conf_file, old, delta)
+		sys.exit(rcode)
+	else:
+		print "up: %d, down %d, net change %d" % (up, -down, up-down)
+		fmt = "%-40s %7s %7s %+7s %8s"
+		print fmt % ("item", "old", "new", "change", "percent")
+		fmt = "%-40s %7s %7s %+7s (%4.1f%%)"
+		for d, n in delta:
+			if d:
+				o = old.get(n,0)
+				if o!=0:
+					p = (float(d)/float(o))*100
+				else:
+					p = 100
+				print fmt % (n, old.get(n,"-"),
+					new.get(n,"-"), d, p)
+	sys.exit(0)
+
+main()

Seems to work fine, after I hacked my $CROSS_COMPILE prefix into the hardcoded nm launch.

Thanks!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   [email protected]
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010

On Thu 9 Oct 2008 19:56, Tim Bird pondered:

This seems to still only look at the text section (only uses nm)? not data or other section sizes increasing?