linux-kernel - Re: [PATCH] Staging: iio: ad7192: replaced bool in struct

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:   Mon, 7 Jan 2019 23:27:48 +0530
From:   Himanshu Jha <himanshujha199640@...il.com>
To:     Matt Ranostay <matt.ranostay@...sulko.com>
Cc:     Amir Mahdi Ghorbanian <indigoomega021@...il.com>, lars@...afoo.de,
        Michael.Hennerich@...log.com, jic23@...nel.org, knaack.h@....de,
        pmeerw@...erw.net, gregkh@...uxfoundation.org,
        linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: iio: ad7192: replaced bool in struct

On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
> 
> > On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@...il.com> wrote:
> > 
> >> On Fri, Dec 21, 2018 at 03:26:26PM -0800, Amir Mahdi Ghorbanian wrote:
> >> Replaced bool in struct with unsigned int bitfield to conserve space and
> >> more clearly define size of varibales
> 
> Important thing to note is depending on padding, alignment, and position of field it probably won’t save any space.

Well, then what do you say about the following results ;-)

Before:
------

himanshu@...anshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_data drivers/staging/iio/adc/ad7192.o
struct ad7192_platform_data {
	u16                        vref_mv;              /*     0     2 */
	u8                         clock_source_sel;     /*     2     1 */

	/* XXX 1 byte hole, try to pack */

	u32                        ext_clk_hz;           /*     4     4 */
	bool                       refin2_en;            /*     8     1 */
	bool                       rej60_en;             /*     9     1 */
	bool                       sinc3_en;             /*    10     1 */
	bool                       chop_en;              /*    11     1 */
	bool                       buf_en;               /*    12     1 */
	bool                       unipolar_en;          /*    13     1 */
	bool                       burnout_curr_en;      /*    14     1 */

	/* size: 16, cachelines: 1, members: 10 */
	/* sum members: 14, holes: 1, sum holes: 1 */
	/* padding: 1 */
	/* last cacheline: 16 bytes */
};

After:
-----

himanshu@...anshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_data drivers/staging/iio/adc/ad7192.o
struct ad7192_platform_data {
	u16                        vref_mv;              /*     0     2 */
	u8                         clock_source_sel;     /*     2     1 */

	/* XXX 1 byte hole, try to pack */

	u32                        ext_clk_hz;           /*     4     4 */
	unsigned int               refin2_en:1;          /*     8:31  4 */
	unsigned int               rej60_en:1;           /*     8:30  4 */
	unsigned int               sinc3_en:1;           /*     8:29  4 */
	unsigned int               chop_en:1;            /*     8:28  4 */
	unsigned int               buf_en:1;             /*     8:27  4 */
	unsigned int               unipolar_en:1;        /*     8:26  4 */
	unsigned int               burnout_curr_en:1;    /*     8:25  4 */

	/* size: 12, cachelines: 1, members: 10 */
	/* sum members: 11, holes: 1, sum holes: 1 */
	/* bit_padding: 25 bits */
	/* last cacheline: 12 bytes */
};

> Also for internal unpacked structures it really makes little sense to save a few bytes of data. Don’t read into that packed structures are good.. they usually aren’t :)

Yes, agreed, but I often see patches to save few bytes by marking
array as static.

There is some effort in this direction:
https://lore.kernel.org/lkml/20181221235230.GC13168@ziepe.ca/

Very likely to get more patches if the above patch gets merged.


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology