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

[Himanshu Jha]

On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
> 
> > On Dec 24, 2018, at 01:58, Himanshu Jha <himanshujha199640@xxxxxxxxx> 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@himanshu-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@himanshu-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@xxxxxxxx/

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