diff mbox series

HID: Reorder fields in 'struct hid_field'

Message ID	a804f2e91bc32cc5e17e012ed90972415606db4e.1687075665.git.christophe.jaillet@wanadoo.fr
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> To: Jiri Kosina <jikos@kernel.org>, Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, linux-input@vger.kernel.org Subject: [PATCH] HID: Reorder fields in 'struct hid_field' Date: Sun, 18 Jun 2023 10:08:07 +0200 Message-Id: <a804f2e91bc32cc5e17e012ed90972415606db4e.1687075665.git.christophe.jaillet@wanadoo.fr> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	HID: Reorder fields in 'struct hid_field' \| HID: Reorder fields in 'struct hid_field'

Commit Message

Christophe JAILLET June 18, 2023, 8:08 a.m. UTC

  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(-)

Comments

Dan Carpenter June 19, 2023, 5:18 a.m. UTC | #1

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

Christophe JAILLET June 19, 2023, 6:20 p.m. UTC | #2

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

Dan Carpenter June 20, 2023, 5:09 a.m. UTC | #3

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 mbox series

Patch

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 */