netdev - Re: [RFC PATCH] tg3: Convert chip type macros to inline functions

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]

Date:	Sun, 10 Feb 2013 19:39:27 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	joe@...ches.com
Cc:	netdev@...r.kernel.org, hauke@...ke-m.de, mcarlson@...adcom.com,
	mchan@...adcom.com, nsujir@...adcom.com
Subject: Re: [RFC PATCH] tg3: Convert chip type macros to inline functions

From: Joe Perches <joe@...ches.com>
Date: Wed, 06 Feb 2013 15:44:58 -0800

> To me the negative to these conversions is at
> least for gcc 4.7.2, the overall code size
> increases
> 
> $ size drivers/net/ethernet/broadcom/tg3.o*
>    text	   data	    bss	    dec	    hex	filename
>  203426	  13446	  55744	 272616	  428e8	drivers/net/ethernet/broadcom/tg3.o.new
>  202135	  13446	  55144	 270725	  42185	drivers/net/ethernet/broadcom/tg3.o.old
> 
> I'm not sure why gcc doesn't do the optimization
> and code generation the same way.

Out of curiosity I looked at the assembler difference on sparc
and one thing stood out.

You're changing how the values are evaluated, specifically you
now are forcing the compiler to work with unsigned ID numbers
whereas before it was working with signed values.

So, looking at one example, the ASIC revision test in __tg3_set_mac_addr():

	if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5703 ||
	    GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5704) {
	...
	}

The compiler previously turned the two tests into one:

	asic_id -= 1;
	if (asic_id <= 1) {
	...
	}

But this construct isn't valid for unsigned quantities, so now with
your patch this expands to two tests:

	if (asic_id == 1 ||
	    asic_id == 2) {
	...
	}

The assembler is hard to compare manually, because the inlining
changes how hard registers are allocated, and issues like the above
will change all of the branch and file offsets as well. :-/

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html