Skip to content

evaluate complexity of moving immutable device data to substructure #26127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Jun 11, 2020

As requested to support of assessing options related to constification of struct device (#26072) this PR demonstrates the conversions for and impact of placing immutable device data in a substructure, with only the mutable material (and a pointer to the substructure) in the RAM section which remains referenced as struct device * in all API (i.e. is not const qualified).

Those who are interested can use this branch to evaluate the savings on specific applications. I picked two bluetooth samples on two boards; it appears the cost is on the order of 2 to 3 bytes of flash for each byte of RAM saved, but I wouldn't infer too much from that.

I was mistaken in the 2020-06-10 dev-review when I surmised that the conversion would be difficult. Coccinelle handles about 90% of the required modifications to inject the fixed pointer dereference; another remaining 9% are handled with a sed script; and a few need to be done by hand. The conversion is significantly simpler than converting to const struct device * in #25208, though the sed script could make inappropriate changes, and this draft PR probably still hasn't identified all the places where a change needs to be made.

The conversion does affect over 540 files, but the impact on non-driver code is minimal. The steps in the PR are fragile in the face of upstream changes: mechanical rebases will generally fail so the last two or three commits must be implemented by re-running the transformations.

For samples/bluetooth/central_hr on pca10056: -56 ram +110 flash
   text	   data	    bss	    dec	    hex	filename
 145070	   2466	  20355	 167891	  28fd3	build/zephyr/zephyr.elf   # RAM
 145280	   2410	  20355	 168045	  2906d	build/zephyr/zephyr.elf   # Flash

For samples/bluetooth/st_ble_sensor on wb55: -88 ram +260 flash
   text	   data	    bss	    dec	    hex	filename
  62096	   1956	  16823	  80875	  13beb	build/zephyr/zephyr.elf   # RAM
  62356	   1868	  16823	  81047	  13c97	build/zephyr/zephyr.elf   # Flash

@pabigot pabigot force-pushed the nordic/20200611a branch 7 times, most recently from 2f170c7 to cdbb1a7 Compare June 12, 2020 15:31
Tomasz Bursztyka and others added 13 commits June 12, 2020 13:17
Let's set the api at built time, or this will create a bug once device
instance pointers become constant.

Signed-off-by: Tomasz Bursztyka <[email protected]>
This is set when the device is defined, and should not be modified.

Signed-off-by: Peter Bigot <[email protected]>
This is set when the device is defined, and should not be modified.

Signed-off-by: Peter Bigot <[email protected]>
The driver API should be assigned at the time the device is defined.
Assigning in the init function is redundant and prevents moving the
pointer to a immutable driver structure.

Signed-off-by: Peter Bigot <[email protected]>
This header provides one public API function, but lacked the
annotations necessary to process it when generating documentation.
Also detailed documentation was provided in Z_INIT_ENTRY_DEFINE which
is an internal function excluded from documentation.

Add the necessary directives to include SYS_INIT() in generated
documentation and move the description of the level and prio
properties to it.

Signed-off-by: Peter Bigot <[email protected]>
DEVICE_AND_API_INIT and DEVICE_DEFINE are identical except that
DEVICE_DEFINE adds a parameter providing the device pm control
function, while DEVICE_AND_API_INIT does not.  This requires duplicate
implementations where if CONFIG_DEVICE_POWER_MANAGEMENT is enabled
then DEVICE_AND_API_INIT delegates to DEVICE_DEFINE with a dummy pm
control function, and if it is not enabled then DEVICE_DEFINE discards
the parameter and delegates to DEVICE_AND_API_INIT.

DEVICE_INIT is like DEVICE_AND_API_INIT but doesn't provide an API.

Clean this up by refactoring so that DEVICE_DEFINE is the core
implementation, providing with and without device power management
right next to each other where they can be compared and maintained.
Redefine DEVICE_INIT and DEVICE_AND_API_INIT delegate to
DEVICE_DEFINE.

Also remove duplicate code by extracting the variations due to
enabling device power management into macros.

Signed-off-by: Peter Bigot <[email protected]>
Zeroing the API pointer saves space but is confusing and prevents
re-initializing the device at another point.

Signed-off-by: Peter Bigot <[email protected]>
Replace comparisons of driver_api against NULL with comparisons
of initres against 0.

Related Coccinelle script:

@@
expression D;
@@
 D->
- driver_api == NULL
+ init_res != 0

@@
expression D;
@@
 D->
- driver_api != NULL
+ init_res == 0

Post-process to fix whitespace errors:

 sed -r \
    -e 's@res([!=]=)(\S)@res \1 @g' \
    -e 's@res([!=]=\s)@res \1@g'

Signed-off-by: Peter Bigot <[email protected]>
Everything in struct device except the result of attempting to
initialize the device is immutable data.  Move it to its own structure
which will be stored in flash.

Signed-off-by: Peter Bigot <[email protected]>
Insert the new intermediate in all expressions that access an
immutable member of the device structure.

Coccinelle script:

@r_base@
expression DE;
identifier ftag =~ "(?x)^
(name
|config_info
|driver_api
|driver_data
|device_pm_control
|pm
)$";
@@
DE->ftag

@r_param
 extends r_base@
identifier FN;
identifier D;
@@
FN(..., struct device *D, ...) {
 <...
 D->
+fixed->
 ftag
 ...>
}

@r_local
 extends r_base@
identifier D;
@@
 struct device *D;
 <...
 D->
+fixed->
 ftag
 ...>

@r_expr
 extends r_base@
expression E;
@@
 (struct device *)E->
+fixed->
 ftag

Signed-off-by: Peter Bigot <[email protected]>
Coccinelle doesn't find these expressions inside helper macros
or functions.

FTAG="(name|config_info|driver_api|driver_data|device_pm_control|pm)"
git grep -lP '.->'"${FTAG}"'\b' \
  | sed \
    -e '/workqueue.rst/d' \
  | xargs sed -i -r \
    -e 's@(\(\w+\))(->'"${FTAG}"'\b)@\1->fixed\2@g' \
    -e 's@\b(_?dev)(->'"${FTAG}"'\b)@\1->fixed\2@g' \
    -e 's@\b(\w+_dev)(->'"${FTAG}"'\b)@\1->fixed\2@g' \
    -e 's@\b(\w*_?device)(->'"${FTAG}"'\b)@\1->fixed\2@g'
sed -i -r \
  -e 's@(ppp_dev|at_dev|control_dev)(->'"${FTAG}"'\b)@\1->fixed\2@g' \
  drivers/modem/gsm_ppp.c

Signed-off-by: Peter Bigot <[email protected]>

fixup 6aeb622e9d
These aren't caught by Coccinnelle, and can't easily be validated in
regexp, so do them manually.

Signed-off-by: Peter Bigot <[email protected]>
@pabigot
Copy link
Collaborator Author

pabigot commented Jun 12, 2020

When rebasing on master the easiest approach is to cherry the non-automated commits, then run each of these commands before committing the result with the corresponding application commit.

Use this as the change for "DNM device: update tests of init result"

spatch --sp-file initres.cocci \
  --very-quiet --macro-file scripts/coccinelle/macros.h \
  --include-headers --dir . \
  | sed -r \
    -e 's@res([!=]=)(\S)@res \1 @g' \
    -e 's@res([!=]=\s)@res \1@g' \
  > initres.patch

Use this as the change for "DNM device: fixups for fixed data into separate structure"

spatch --sp-file devfix.cocci \
  --very-quiet --macro-file scripts/coccinelle/macros.h \
  --include-headers --dir . \
  > devfix.patch

Use this as the change for "DNM device: convert helper macros that access immutable device fields"

FTAG="(name|config_info|driver_api|driver_data|device_pm_control|pm)"
git grep -lP '.->'"${FTAG}"'\b' \
  | sed \
    -e '/workqueue.rst/d' \
  | xargs sed -i -r \
    -e 's@(\(\w+\))(->'"${FTAG}"'\b)@\1->fixed\2@g' \
    -e 's@\b(_?dev)(->'"${FTAG}"'\b)@\1->fixed\2@g' \
    -e 's@\b(\w+_dev)(->'"${FTAG}"'\b)@\1->fixed\2@g' \
    -e 's@\b(\w*_?device)(->'"${FTAG}"'\b)@\1->fixed\2@g'
sed -i -r \
  -e 's@(ppp_dev|at_dev|control_dev)(->'"${FTAG}"'\b)@\1->fixed\2@g' \
  drivers/modem/gsm_ppp.c

@github-actions github-actions bot added area: Boards area: Documentation has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020
@pabigot
Copy link
Collaborator Author

pabigot commented Jul 8, 2020

Outdated; this approach was rejected in favor of making struct device const.

@pabigot pabigot closed this Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Boards area: Documentation has-conflicts Issue/PR has conflicts with another issue/PR platform: NXP NXP platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant