media: rc: reduce size of struct ir_raw_event
Subsystems (2)
PATCH 1/1 media: rc: reduce size of struct ir_raw_event
struct ir_raw_event currently has a size of 12 bytes on most (all?) architectures. This can be reduced to 8 bytes whilst maintaining full backwards compatibility.
This saves 2KB in size of struct ir_raw_event_ctrl (as element kfifo is reduced by 512 * 4 bytes) and it allows to copy the full struct ir_raw_event with a single 64 bit operation.
Successfully tested with the Nuvoton driver and successfully compile-tested with the ene_ir driver (as it uses the carrier / duty_cycle elements).
--- include/media/rc-core.h | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
include/media/rc-core.h +8 -18
@@ -214,27 +214,17 @@ enum raw_event_type {struct ir_raw_event {union { -u32 duration; - -struct { -u32 carrier; -u8 duty_cycle; - }; +u32 duration; +u32 carrier; }; - -unsigned pulse:1 ; -unsigned reset:1 ; -unsigned timeout:1 ; -unsigned carrier_report:1 ; +u8 duty_cycle; +u8 pulse:1 ; +u8 reset:1 ; +u8 timeout:1 ; +u8 carrier_report:1 ; }; -#define DEFINE_IR_RAW_EVENT (event) \ - struct ir_raw_event event = { \ - { .duration = 0 } , \ - .pulse = 0, \ - .reset = 0, \ - .timeout = 0, \ - .carrier_report = 0 } +#define DEFINE_IR_RAW_EVENT (event) struct ir_raw_event event = {}static inline void init_ir_raw_event (struct ir_raw_event *ev) {
Signature · 1 line
2.7.3
Sean Young
maintainerQuoted 29 lines · Heiner Kallweit
struct
ir_raw_eventcurrently has a size of 12 bytes on most (all?) architectures. This can be reduced to 8 bytes whilst maintaining full backwards compatibility.
This saves 2KB in size of structir_raw_event_ctrl(as element kfifo is reduced by 512 * 4 bytes) and it allows to copy the full structir_raw_eventwith a single 64 bit operation.Successfully tested with the Nuvoton driver and successfully compile-tested with the
ene_irdriver (as it uses the carrier /duty_cycleelements).Signed-off-by Heiner Kallweit <[email protected]>--- include/media/rc-core.h | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)include/media/rc-core.h +0
-5@@ -214,27 +214,17 @@ enum raw_event_type { struct ir_raw_event { union { - u32 duration; - - struct { - u32 carrier; - u8 duty_cycle;
include/media/rc-core.h enum raw_event_type { · line 220
Quoted 10 lines
include/media/rc-core.h +3
-6- }; +u32 duration; +u32 carrier; }; - - unsigned pulse:1; - unsigned reset:1; - unsigned timeout:1; - unsigned carrier_report:1; +u8 duty_cycle;
Moving duty_cycle does indeed reduce the structure size from 12 to 8.
include/media/rc-core.h enum raw_event_type { · line 224
Quoted 4 lines
include/media/rc-core.h +4
-0+ u8 pulse:1; + u8 reset:1; + u8 timeout:1; + u8 carrier_report:1;
Why are you changing the type of the bitfields?
include/media/rc-core.h enum raw_event_type { · line 227
Quoted 10 lines
include/media/rc-core.h +1
-7}; -#define DEFINE_IR_RAW_EVENT (event) \ - struct ir_raw_event event = { \ - { .duration = 0 } , \ - .pulse = 0, \ - .reset = 0, \ - .timeout = 0, \ - .carrier_report = 0 } +#define DEFINE_IR_RAW_EVENT (event) struct ir_raw_event event = {}
Seems fine. I've always kinda wondered why this macro is needed.
Sean
Am 16.03.2016 um 23:28 schrieb Sean Young:
Quoted 43 lines · Heiner Kallweit
Quoted 29 lines
struct
ir_raw_eventcurrently has a size of 12 bytes on most (all?) architectures. This can be reduced to 8 bytes whilst maintaining full backwards compatibility.
This saves 2KB in size of structir_raw_event_ctrl(as element kfifo is reduced by 512 * 4 bytes) and it allows to copy the full structir_raw_eventwith a single 64 bit operation.Successfully tested with the Nuvoton driver and successfully compile-tested with the
ene_irdriver (as it uses the carrier /duty_cycleelements).Signed-off-by Heiner Kallweit <[email protected]>--- include/media/rc-core.h | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)include/media/rc-core.h +0
-5@@ -214,27 +214,17 @@ enum raw_event_type { struct ir_raw_event { union { - u32 duration; - - struct { - u32 carrier; - u8 duty_cycle;include/media/rc-core.h enum raw_event_type { · line 220
Quoted 10 lines
include/media/rc-core.h +3
-6- }; +u32 duration; +u32 carrier; }; - - unsigned pulse:1; - unsigned reset:1; - unsigned timeout:1; - unsigned carrier_report:1; +u8 duty_cycle;Moving
duty_cycledoes indeed reduce the structure size from 12 to 8.
Quoted 7 lines
include/media/rc-core.h enum raw_event_type { · line 224
Quoted 4 lines
include/media/rc-core.h +4
-0+ u8 pulse:1; + u8 reset:1; + u8 timeout:1; + u8 carrier_report:1;Why are you changing the type of the bitfields?
I did this to make sure that the compiler uses one byte for the bit field. When testing gcc also used just one byte when keeping the original "unsigned" type for the bit field members. Therefore it wouldn't be strictly neeeded.
But I'm not sure whether it's guaranteed that the compiler packs a bit field to the smallest possible data type and we can rely on it. AFAIK C99 is a little more specific about this implementation detail of bit fields but C89/C90 is used for kernel compilation.
Heiner
Quoted 16 lines
include/media/rc-core.h enum raw_event_type { · line 227
Quoted 10 lines
include/media/rc-core.h +1
-7}; -#define DEFINE_IR_RAW_EVENT (event) \ - struct ir_raw_event event = { \ - { .duration = 0 } , \ - .pulse = 0, \ - .reset = 0, \ - .timeout = 0, \ - .carrier_report = 0 } +#define DEFINE_IR_RAW_EVENT (event) struct ir_raw_event event = {}Seems fine. I've always kinda wondered why this macro is needed.
Sean
Sean Young
maintainerQuoted 14 lines · Heiner Kallweit
Am 16.03.2016 um 23:28 schrieb Sean Young:
Quoted 8 lines · Heiner Kallweit
include/media/rc-core.h enum raw_event_type { · line 224
Quoted 4 lines
include/media/rc-core.h +4
-0+ u8 pulse:1; + u8 reset:1; + u8 timeout:1; + u8 carrier_report:1;Why are you changing the type of the bitfields?
I did this to make sure that the compiler uses one byte for the bit field. When testing gcc also used just one byte when keeping the original "unsigned" type for the bit field members. Therefore it wouldn't be strictly neeeded.
Quoted 4 lines
But I'm not sure whether it's guaranteed that the compiler packs a bit field to the smallest possible data type and we can rely on it. AFAIK C99 is a little more specific about this implementation detail of bit fields but C89/C90 is used for kernel compilation.
It might be worth reading about structure packing rules rather than guessing.
Sean
Am 17.03.2016 um 11:53 schrieb Sean Young:
Quoted 15 lines · Heiner Kallweit
Quoted 14 lines
Am 16.03.2016 um 23:28 schrieb Sean Young:
Quoted 8 lines · Heiner Kallweit
include/media/rc-core.h enum raw_event_type { · line 224
Quoted 4 lines
include/media/rc-core.h +4
-0+ u8 pulse:1; + u8 reset:1; + u8 timeout:1; + u8 carrier_report:1;Why are you changing the type of the bitfields?
I did this to make sure that the compiler uses one byte for the bit field. When testing gcc also used just one byte when keeping the original "unsigned" type for the bit field members. Therefore it wouldn't be strictly neeeded.
Quoted 8 lines
Quoted 4 lines
But I'm not sure whether it's guaranteed that the compiler packs a bit field to the smallest possible data type and we can rely on it. AFAIK C99 is a little more specific about this implementation detail of bit fields but C89/C90 is used for kernel compilation.
It might be worth reading about structure packing rules rather than guessing.
Whenever it became interesting when reading the statement was:
unspecified / implementation-dependent.
But at least C90 clearly states that only signed / unsigned int are acceptable for bit fields. Therefore leave the bit field as it is. I will provide a v2.
Heiner
Quoted 3 lines
Sean