Message-ID: <547E842E.7030202@sonymobile.com>
Date:	Tue, 2 Dec 2014 19:31:58 -0800
From:	Tim Bird <tim.bird@...ymobile.com>
To:	Josh Triplett <josh@...htriplett.org>
CC:	Shuah Khan <shuahkh@....samsung.com>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-embedded@...r.kernel.org" <linux-embedded@...r.kernel.org>
Subject: Re: [PATCH v4] selftest: size: Add size test for Linux kernel



On 11/26/2014 10:04 PM, Josh Triplett wrote:
> On Wed, Nov 26, 2014 at 08:27:23PM -0800, Tim Bird wrote:
>> --- /dev/null
>> +++ b/tools/testing/selftests/size/Makefile
> [...]
>> +LIBGCC=$(shell $(CC) -print-libgcc-file-name)
>> +
>> +get_size: get_size.c
>> +	$(CC) --static -ffreestanding -nostartfiles \
>> +		-Wl,--entry=_start get_size.c $(LIBGCC) \
>> +		-o get_size
> 
> You don't need -Wl,--entry=_start; that's the default.
OK - it works without this.  Thanks.

> You shouldn't need to manually find libgcc, either; the compiler should
> do that for you.  What goes wrong if you don't include that?  If you're
> trying to link libgcc statically, try -static-libgcc.
> 
> Also, static is normally spelled -static, not --static.

Hmm.  Not sure where I got --static from, but it worked.
But if -static is the norm I'm fine changing to that.

Upon experimentation, I don't need the explicit libgcc or for that
matter the -static-libgcc either.

>> --- /dev/null
>> +++ b/tools/testing/selftests/size/get_size.c
> [...]
>> +int print(const char *s)
> 
> This function, and all the others apart from _start, should be declared
> static.

OK will do.

>> +void num_to_str(unsigned long num, char *s)
> 
> Likewise, static.
OK

>> +{
>> +	unsigned long long temp, div;
>> +	int started;
>> +
>> +	temp = num;
>> +	div = 1000000000000000000LL;
>> +	started = 0;
>> +	while (div) {
>> +		if (temp/div || started) {
>> +			*s++ = (unsigned char)(temp/div + '0');
>> +			started = 1;
>> +		}
>> +		temp -= (temp/div)*div;
>> +		div /= 10;
>> +	}
>> +	*s = 0;
>> +}
> 
> You'd probably end up with significantly smaller code (and no divisions,
> and thus no corner cases on architectures that need a special function
> to do unsigned long long division) if you print in hex.  You could also
> drop the "no leading zeros" logic, and just *always* print a 64-bit
> value as 16 hex digits.

I'd like to keep base-10 output.  As far as size is concerned, the
code is now at well under 2 pages (8k), which is much smaller than
any actually useful program.  The only noticeable change in size would
be if I got it under 4096, but I don't want to sacrifice too many
features to get there (as that's still only 1 page difference in
memory usage.)

BTW - using sstrip, I can get it to 4096 already, as is.

Unfortunately, 'sstrip -z', which gets it to 2061, makes
it not work.  I need to check out what the problem is there.
Also, the ELF file still has an unneeded note section.  I think
easier reductions, without sacrificing functionality, are available
by tweaking the ELF header (ie fixing sstrip, for those that
want to use it).
 
>> +int print_num(unsigned long num)
> 
> Likewise, static.

OK

>> +{
>> +	char num_buf[30];
>> +
>> +	num_to_str(num, num_buf);
>> +	return print(num_buf);
>> +}
>> +
>> +int print_k_value(const char *s, unsigned long num, unsigned long units)
>> +{
>> +	unsigned long long temp;
>> +	int ccode;
>> +
>> +	print(s);
>> +
>> +	temp = num;
>> +	temp = (temp * units)/1024;
>> +	num = temp;
>> +	ccode = print_num(num);
>> +	print("\n");
>> +	return ccode;
>> +}
> 
> I'd suggest dropping this entirely, and just always printing the exact
> values returned by sysinfo.  Drop the multiply, too, and just print
> info.mem_unit as well.  It's easy to post-process the value in a more
> capable environment.

I'd prefer to keep the output easily human-readable.
 
>> +/* this program has no main(), as startup libraries are not used */
>> +void _start(void)
>> +{
>> +	int ccode;
>> +	struct sysinfo info;
>> +	unsigned long used;
>> +
>> +	print("Testing system size.\n");
>> +	print("1..1\n");
>> +
>> +	ccode = sysinfo(&info);
>> +	if (ccode < 0) {
>> +		print("not ok 1 get size runtime size\n");
> 
> Shouldn't the "not ok" here and the "ok" below have the same test
> description?
Yes.  Thanks.  Good catch.
 
>> +		print("# could not get sysinfo\n");
>> +		_exit(ccode);
>> +	}
>> +	/* ignore cache complexities for now */
>> +	used = info.totalram - info.freeram - info.bufferram;
>> +	print_k_value("ok 1 get runtime memory use # size = ", used,
>> +		info.mem_unit);
>> +
>> +	print("# System runtime memory report (units in Kilobytes):\n");
>> +	print_k_value("#   Total:  ", info.totalram, info.mem_unit);
>> +	print_k_value("#   Free:   ", info.freeram, info.mem_unit);
>> +	print_k_value("#   Buffer: ", info.bufferram, info.mem_unit);
>> +	print_k_value("#   In use: ", used, info.mem_unit);
>> +
>> +	_exit(0);
>> +}
>> -- 
>> 1.8.2.2

OK - look for a new version shortly (maybe not today, though).
 -- Tim

--
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/

Powered by blists - more mailing lists