On Mon, 7 Jan 2019 23:27:48 +0530
Himanshu Jha <himanshujha199640@xxxxxxxxx> wrote:
> 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.
>
The one additional thing that is relevant here is that we will probably
drop the whole structure anyway as part of moving it out of staging.
There might be some equivalent elements stored elsewhere, but quite
likely it won't be all of them. As a result I'm not particularly keen
on patches to clean platform data up.
+ we really are dealing with trivial amounts of data here..
J
