| Message ID | a804f2e91bc32cc5e17e012ed90972415606db4e.1687075665.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | HID: Reorder fields in 'struct hid_field' | expand |
On Sun, Jun 18, 2023 at 10:08:07AM +0200, Christophe JAILLET wrote: > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 39e21e3815ad..5be5e671c263 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -480,9 +480,9 @@ struct hid_field { > __s32 physical_maximum; > __s32 unit_exponent; > unsigned unit; > - bool ignored; /* this field is ignored in this event */ > struct hid_report *report; /* associated report */ > unsigned index; /* index into report->field[] */ > + bool ignored; /* this field is ignored in this event */ > /* hidinput data */ > struct hid_input *hidinput; /* associated input structure */ > __u16 dpad; /* dpad input code */ You could move the dpad next to the ignored to save another 4 bytes. I think it is still grouped logically that way but I don't really know what dpad is so I might be wrong. struct hid_report *report; /* associated report */ unsigned index; /* index into report->field[] */ bool ignored; /* this field is ignored in this event */ /* hidinput data */ __u16 dpad; /* dpad input code */ struct hid_input *hidinput; /* associated input structure */ regards, dan carpenter
Le 19/06/2023 à 07:18, Dan Carpenter a écrit : > On Sun, Jun 18, 2023 at 10:08:07AM +0200, Christophe JAILLET wrote: >> diff --git a/include/linux/hid.h b/include/linux/hid.h >> index 39e21e3815ad..5be5e671c263 100644 >> --- a/include/linux/hid.h >> +++ b/include/linux/hid.h >> @@ -480,9 +480,9 @@ struct hid_field { >> __s32 physical_maximum; >> __s32 unit_exponent; >> unsigned unit; >> - bool ignored; /* this field is ignored in this event */ >> struct hid_report *report; /* associated report */ >> unsigned index; /* index into report->field[] */ >> + bool ignored; /* this field is ignored in this event */ >> /* hidinput data */ >> struct hid_input *hidinput; /* associated input structure */ >> __u16 dpad; /* dpad input code */ > > You could move the dpad next to the ignored to save another 4 bytes. > I think it is still grouped logically that way but I don't really know > what dpad is so I might be wrong. > > struct hid_report *report; /* associated report */ > unsigned index; /* index into report->field[] */ > bool ignored; /* this field is ignored in this event */ > /* hidinput data */ > __u16 dpad; /* dpad input code */ > struct hid_input *hidinput; /* associated input structure */ > > regards, > dan carpenter > > In fact, not really, It would fill a hole better, but would generate some padding at the end of the struct: bool ignored; /* 108 1 */ /* XXX 1 byte hole, try to pack */ __u16 dpad; /* 110 2 */ struct hid_input * hidinput; /* 112 8 */ unsigned int slot_idx; /* 120 4 */ /* size: 128, cachelines: 2, members: 25 */ /* sum members: 119, holes: 2, sum holes: 5 */ /* padding: 4 */ }; CJ
On Mon, Jun 19, 2023 at 08:20:08PM +0200, Christophe JAILLET wrote: > Le 19/06/2023 à 07:18, Dan Carpenter a écrit : > > On Sun, Jun 18, 2023 at 10:08:07AM +0200, Christophe JAILLET wrote: > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > > index 39e21e3815ad..5be5e671c263 100644 > > > --- a/include/linux/hid.h > > > +++ b/include/linux/hid.h > > > @@ -480,9 +480,9 @@ struct hid_field { > > > __s32 physical_maximum; > > > __s32 unit_exponent; > > > unsigned unit; > > > - bool ignored; /* this field is ignored in this event */ > > > struct hid_report *report; /* associated report */ > > > unsigned index; /* index into report->field[] */ > > > + bool ignored; /* this field is ignored in this event */ > > > /* hidinput data */ > > > struct hid_input *hidinput; /* associated input structure */ > > > __u16 dpad; /* dpad input code */ > > > > You could move the dpad next to the ignored to save another 4 bytes. > > I think it is still grouped logically that way but I don't really know > > what dpad is so I might be wrong. > > > > struct hid_report *report; /* associated report */ > > unsigned index; /* index into report->field[] */ > > bool ignored; /* this field is ignored in this event */ > > /* hidinput data */ > > __u16 dpad; /* dpad input code */ > > struct hid_input *hidinput; /* associated input structure */ > > > > regards, > > dan carpenter > > > > > > In fact, not really, > It would fill a hole better, but would generate some padding at the end of > the struct: > > bool ignored; /* 108 1 */ > > /* XXX 1 byte hole, try to pack */ > > __u16 dpad; /* 110 2 */ > struct hid_input * hidinput; /* 112 8 */ > unsigned int slot_idx; /* 120 4 */ > > /* size: 128, cachelines: 2, members: 25 */ > /* sum members: 119, holes: 2, sum holes: 5 */ > /* padding: 4 */ > }; Ah. Right. I should have looked at that more closely. regards, dan carpenter
diff --git a/include/linux/hid.h b/include/linux/hid.h index 39e21e3815ad..5be5e671c263 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -480,9 +480,9 @@ struct hid_field { __s32 physical_maximum; __s32 unit_exponent; unsigned unit; - bool ignored; /* this field is ignored in this event */ struct hid_report *report; /* associated report */ unsigned index; /* index into report->field[] */ + bool ignored; /* this field is ignored in this event */ /* hidinput data */ struct hid_input *hidinput; /* associated input structure */ __u16 dpad; /* dpad input code */
Group some variables based on their sizes to reduce hole and avoid padding. On x86_64, this shrinks the size of 'struct hid_field' from 136 to 128 bytes. It saves a few bytes of memory and is more cache-line friendly. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Using pahole Before: ====== struct hid_field { unsigned int physical; /* 0 4 */ unsigned int logical; /* 4 4 */ unsigned int application; /* 8 4 */ /* XXX 4 bytes hole, try to pack */ struct hid_usage * usage; /* 16 8 */ unsigned int maxusage; /* 24 4 */ unsigned int flags; /* 28 4 */ unsigned int report_offset; /* 32 4 */ unsigned int report_size; /* 36 4 */ unsigned int report_count; /* 40 4 */ unsigned int report_type; /* 44 4 */ __s32 * value; /* 48 8 */ __s32 * new_value; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ __s32 * usages_priorities; /* 64 8 */ __s32 logical_minimum; /* 72 4 */ __s32 logical_maximum; /* 76 4 */ __s32 physical_minimum; /* 80 4 */ __s32 physical_maximum; /* 84 4 */ __s32 unit_exponent; /* 88 4 */ unsigned int unit; /* 92 4 */ bool ignored; /* 96 1 */ /* XXX 7 bytes hole, try to pack */ struct hid_report * report; /* 104 8 */ unsigned int index; /* 112 4 */ /* XXX 4 bytes hole, try to pack */ struct hid_input * hidinput; /* 120 8 */ /* --- cacheline 2 boundary (128 bytes) --- */ __u16 dpad; /* 128 2 */ /* XXX 2 bytes hole, try to pack */ unsigned int slot_idx; /* 132 4 */ /* size: 136, cachelines: 3, members: 25 */ /* sum members: 119, holes: 4, sum holes: 17 */ /* last cacheline: 8 bytes */ }; After: ===== struct hid_field { unsigned int physical; /* 0 4 */ unsigned int logical; /* 4 4 */ unsigned int application; /* 8 4 */ /* XXX 4 bytes hole, try to pack */ struct hid_usage * usage; /* 16 8 */ unsigned int maxusage; /* 24 4 */ unsigned int flags; /* 28 4 */ unsigned int report_offset; /* 32 4 */ unsigned int report_size; /* 36 4 */ unsigned int report_count; /* 40 4 */ unsigned int report_type; /* 44 4 */ __s32 * value; /* 48 8 */ __s32 * new_value; /* 56 8 */ /* --- cacheline 1 boundary (64 bytes) --- */ __s32 * usages_priorities; /* 64 8 */ __s32 logical_minimum; /* 72 4 */ __s32 logical_maximum; /* 76 4 */ __s32 physical_minimum; /* 80 4 */ __s32 physical_maximum; /* 84 4 */ __s32 unit_exponent; /* 88 4 */ unsigned int unit; /* 92 4 */ struct hid_report * report; /* 96 8 */ unsigned int index; /* 104 4 */ bool ignored; /* 108 1 */ /* XXX 3 bytes hole, try to pack */ struct hid_input * hidinput; /* 112 8 */ __u16 dpad; /* 120 2 */ /* XXX 2 bytes hole, try to pack */ unsigned int slot_idx; /* 124 4 */ /* size: 128, cachelines: 2, members: 25 */ /* sum members: 119, holes: 3, sum holes: 9 */ }; --- include/linux/hid.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)