| Message ID | 20221117160015.344528-3-umang.jain@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vc04_services: Promote bool usage | expand |
> struct vchiq_mmal_component { > - u32 in_use:1; > + bool in_use:1; > bool enabled:1; The patch you referenced says: +If a structure has many true/false values, consider consolidating them into a +bitfield with 1 bit members, or using an appropriate fixed width type, such as +u8. The code did exactly this, using two bits fields, in one u32. A bool probably takes up 4 bytes, maybe 8 bytes, so this change probably doubles the storage size for these two fields. Are these fields on the hot path, where an extra AND instruction would make a difference? Andrew
On Fri, Nov 18, 2022 at 01:23:36AM +0100, Andrew Lunn wrote: > > struct vchiq_mmal_component { > > - u32 in_use:1; > > + bool in_use:1; > > bool enabled:1; > > The patch you referenced says: > > +If a structure has many true/false values, consider consolidating them into a > +bitfield with 1 bit members, or using an appropriate fixed width type, such as > +u8. > > The code did exactly this, using two bits fields, in one u32. A bool > probably takes up 4 bytes, maybe 8 bytes, so this change probably > doubles the storage size for these two fields. In GCC and Clang bools take a byte, but the C language is vague and other compilers are free to do it differently. > Are these fields on the > hot path, where an extra AND instruction would make a difference? This patch takes the first u32 for "in_use" and squeezes it into the same byte as "enabled" so it makes the struct four bytes smaller. There is still a 3 byte struct hole between "enabled" and "handle" so we could add more 62 bool bitfields if we wanted. In the v2 patch these become: bool in_use; bool enabled; One byte each and there is a two byte gap before "handle". regards, dan carpenter
diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c index 4abb6178cb9f..294b184d4a49 100644 --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c @@ -1648,7 +1648,7 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance, for (idx = 0; idx < VCHIQ_MMAL_MAX_COMPONENTS; idx++) { if (!instance->component[idx].in_use) { component = &instance->component[idx]; - component->in_use = 1; + component->in_use = true; break; } } @@ -1724,7 +1724,7 @@ int vchiq_mmal_component_init(struct vchiq_mmal_instance *instance, destroy_component(instance, component); unlock: if (component) - component->in_use = 0; + component->in_use = false; mutex_unlock(&instance->vchiq_mutex); return ret; @@ -1747,7 +1747,7 @@ int vchiq_mmal_component_finalise(struct vchiq_mmal_instance *instance, ret = destroy_component(instance, component); - component->in_use = 0; + component->in_use = false; mutex_unlock(&instance->vchiq_mutex); diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h index 70eda6cac1c6..c5be86b0479d 100644 --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h @@ -82,7 +82,7 @@ struct vchiq_mmal_port { }; struct vchiq_mmal_component { - u32 in_use:1; + bool in_use:1; bool enabled:1; u32 handle; /* VideoCore handle for component */ u32 inputs; /* Number of input ports */
In commit 7967656ffbfa ("coding-style: Clarify the expectations around bool") the check to dis-allow bool structure members was removed from checkpatch.pl. It promotes bool structure members to store boolean values. This enhances code readability. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 6 +++--- drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)