Use new VIRTIO_{DRIVER,DEVICE}_SUPPORT compiler define to improve code readability #560

glneo · 2024-03-13T16:10:09Z

Currently compiler defines are defined when support for driver or device is the only support being built. This is a negative define, it surrounds the code to not be built and we use ifndef. This is confusing. It also leaves ifndefs all throughout the code-base. Instead, define a macro that is set to 1 if support is enabled. Use this inline in if statements where possible. Any sane compiler will optimize away the code in the branch when support is not enabled just the same as when using the preprocessor so we keep the same binary size.

arnopo · 2024-03-14T10:10:17Z

Hello @glneo

Your proposal seems making code more readable. Nevertheless I have few concerns

can we ensure that all compilers will do the optimisation (gcc, armcc, ewarm,...). If not I would prefer to keep #ifdef
VIRTIO_DEVICE_ONLY VIRTIO_DRIVER_ONLY are currently used in projects They have to be tagged as deprecated and supported 2 years ( as currently done for VIRTIO_MASTER_ONLY)
applying your patch i'm facing warning in zephyr

 west build -b stm32mp157c_dk2 samples/subsys/ipc/openamp_rsc_table
 [...]
 /local/home/frq07267/views/zephyrproject/modules/lib/open-amp/open-amp/lib/remoteproc/remoteproc_virtio.c:332:17: warning: implicit declaration of function 'rproc_virtio_negotiate_features'; did you mean 'virtio_negotiate_features'? [-Wimplicit-function-declaration]
  332 |                 rproc_virtio_negotiate_features(vdev, dfeatures);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                 virtio_negotiate_features
 [...]

glneo · 2024-03-14T14:29:44Z

Hi @arnopo,

All compilers I have found make this trivial dead code elimination, even on the lowest optimization level setting (-O0). If some compiler exists that cannot do that elimination then that compiler is probably generating extremely large and optimized code all over and this micro-optimization for size wasn't helping them much anyway.
I can somewhat understand keeping WITH_VIRTIO_MASTER around for a bit longer as it was a user facing setting, so it might have been used in some builder script. But VIRTIO_DRIVER_ONLY is an internal-only flag generated by the external CMake setting WITH_VIRTIO_DRIVER. It is not even included in a configuration header, it is simply injected into the compiler at build time. Nothing outside of internal source files could have made use of this flag.

Unless what you are saying is that you have changes in a downstream fork of this project that makes use of this flag. In which case I'd like to know the policy on that. For instance the Linux kernel is very clear that downstream forks need not be considered when making changes to internal code. Otherwise how could one make any change without risking breaking some fork that they have no visibility into?

Fixed.

arnopo · 2024-03-18T10:26:27Z

Hi @arnopo,

All compilers I have found make this trivial dead code elimination, even on the lowest optimization level setting (-O0). If some compiler exists that cannot do that elimination then that compiler is probably generating extremely large and optimized code all over and this micro-optimization for size wasn't helping them much anyway.

Having more optimized code for device mode is required for device role as coprocessor can be limited in term of memories.
keeping #if have the advantage to guaranty that all compilers do the job, but also make the code more clear that the use of a define, that mask the optimization.
So I would more in favour in keep #if

I can somewhat understand keeping WITH_VIRTIO_MASTER around for a bit longer as it was a user facing setting, so it might have been used in some builder script. But VIRTIO_DRIVER_ONLY is an internal-only flag generated by the external CMake setting WITH_VIRTIO_DRIVER. It is not even included in a configuration header, it is simply injected into the compiler at build time. Nothing outside of internal source files could have made use of this flag.

That true for project based on cmake but not for project that use the code without cmake pre-build step.
This is the case for instance for some ST projects.

Unless what you are saying is that you have changes in a downstream fork of this project that makes use of this flag. In which case I'd like to know the policy on that. For instance the Linux kernel is very clear that downstream forks need not be considered when making changes to internal code. Otherwise how could one make any change without risking breaking some fork that they have no visibility into?

You can refer to https://www.openampproject.org/governance/ for the rule we expect to apply on OpenAMP.
As mentioned above these preprocess defines have to be considered as API for some projects that are not cmake based.

In this PR following the deprecated rule seems quite simple, so I don't see a a reason to make an exception.
You can take inspiration from VIRTIO_MASTER_ONLY to do it.

arnopo

VIRTIO_DEVICE_ONLY and VIRTIO_DRIVER_ONLY have to be marked as deprecated
VIRTIO_MASTER_ONLY and VIRTIO_MASTER_ONLY can be removed (in a separate commit) as now deprecated since 2 years (Replace potentially objectionable terms in OpenAMP variable names. #323)

lib/remoteproc/remoteproc_virtio.c

arnopo · 2024-03-18T10:29:31Z

lib/remoteproc/remoteproc.c

-#ifdef VIRTIO_DRIVER_ONLY
-	role = (role != VIRTIO_DEV_DRIVER) ? 0xFFFFFFFFUL : role;
-#endif
+	if (role == VIRTIO_DEV_DRIVER && !VIRTIO_DRIVER_SUPPORT) {


would prefer to keep #if

Sure, no problem.

arnopo · 2024-03-18T10:39:19Z

lib/remoteproc/remoteproc.c

-	role = (role != VIRTIO_DEV_DRIVER) ? 0xFFFFFFFFUL : role;
-#endif
+	if (role == VIRTIO_DEV_DRIVER && !VIRTIO_DRIVER_SUPPORT) {
+		metal_log(METAL_LOG_ERROR, "VirtIO role is set to Driver, but Driver support is not enabled\n");


Do we really need this debug message? It adds extra footprint for a prebuild configuration issue.

Not sure what you mean by a prebuild configuration issue? The variable role is passed in by the user of this API function remoteproc_create_virtio() at runtime. If the library was built without VIRTIO_DRIVER_SUPPORT and later the user wants the created virtio to have the "driver" role, then it will fail at runtime.

There is another check that both driver and device support have been disabled (which would be a build configuration issue), that is a different check.

Either way, I can remove this message and keep the old behavior of just quietly returning NULL here.

glneo · 2024-03-18T16:37:13Z

You can refer to https://www.openampproject.org/governance/ for the rule we expect to apply on OpenAMP. As mentioned above these preprocess defines have to be considered as API for some projects that are not cmake based.

What projects? This project is cmake based. It builds a library that can be used by other projects that may not be cmake based, but that doesn't change this project. Unless you are saying you are directly building/copy/pasting the source from this repo into the build of another project. The API must be defined as the function prototypes in include/openamp/open_amp.h. The code itself, nor the build system, can be considered API. If they were, then nothing here could ever be changed again, every line of code would have to be considered an API for some downstream project that might make some use of it.

In this PR following the deprecated rule seems quite simple, so I don't see a a reason to make an exception. You can take inspiration from VIRTIO_MASTER_ONLY to do it.

I'll make the change to keep this PR moving along, but as stated above, I do not agree with the reasoning. Might be worth chatting out what constitutes the "API" in the next meeting.

glneo · 2024-03-18T16:47:15Z

Having more optimized code for device mode is required for device role as coprocessor can be limited in term of memories. keeping #if have the advantage to guaranty that all compilers do the job, but also make the code more clear that the use of a define, that mask the optimization. So I would more in favour in keep #if

If the issue is that the define is not obvious or masks that the optimization is intended, we could use that IS_ENABLED() macro suggested above, then the lines become:

if (IS_ENABLED(VIRTIO_DRIVER_SUPPORT) && vdev->role == VIRTIO_DEV_DRIVER)

Which I think makes it very clear now that this is a preprocessor level check. It also matches what U-Boot is now doing to remove its #ifdef messiness.

arnopo · 2024-03-19T17:30:14Z

You can refer to https://www.openampproject.org/governance/ for the rule we expect to apply on OpenAMP. As mentioned above these preprocess defines have to be considered as API for some projects that are not cmake based.

What projects? This project is cmake based. It builds a library that can be used by other projects that may not be cmake based, but that doesn't change this project. Unless you are saying you are directly building/copy/pasting the source from this repo into the build of another project. The API must be defined as the function prototypes in include/openamp/open_amp.h. The code itself, nor the build system, can be considered API. If they were, then nothing here could ever be changed again, every line of code would have to be considered an API for some downstream project that might make some use of it.

CMake can be used to configure a project, but it does not necessarily impose building a library.
Some project environment does not support cmake build project, in such case you have to include open-amp source code.

The objective here is to not break compatibility with some existing legacy projects, when possible. We don't know all the users. In such an environment, the work you are doing to rationalize the APIs is very important because it will help with maintainability. We just need to move forward step by step.

In this PR following the deprecated rule seems quite simple, so I don't see a a reason to make an exception. You can take inspiration from VIRTIO_MASTER_ONLY to do it.

I'll make the change to keep this PR moving along, but as stated above, I do not agree with the reasoning. Might be worth chatting out what constitutes the "API" in the next meeting.

That's a good Idea, let's put this on the agenda for next meeting.

arnopo

Need to fix zephyr issue else as discussed in meeting ok to go in this way . If we detect that this generate extra footprint we some compiler we will come back to #if solution

arnopo · 2024-04-17T13:49:15Z

lib/include/openamp/virtio.h

@@ -101,6 +101,8 @@ __deprecated static inline int deprecated_virtio_dev_slave(void)
 #warning "VIRTIO_DEVICE_ONLY is deprecated, please use VIRTIO_DRIVER_SUPPORT=0"
 #endif

+#define IS_ENABLED(option) (option == 1)


Seems that this creates a redefinition issue in Zephyr context.
Perhaps this define should be in a separate include that only been includes by open_amp source file.
What about adding a include/internal/utilities.h file for that?

Seems that doesn't work either, if any source file includes the new utilities.h but also is built in the Zephyr context which also has IS_ENABLED() defined due to inclusions from libmetal, we get the same issue.

Simple solution is to change this to a unique name, since this is for checking if virtio options are enabled, I'll just rename it to VIRTIO_ENABLED.

Fair enough, if we need a more generic macro for some other config we will address that later.

glneo · 2024-04-17T21:31:29Z

Also, the long line checkpatch warnings will be fixed in the next PR.

arnopo · 2024-04-19T09:35:06Z

Also, the long line checkpatch warnings will be fixed in the next PR.

Please fix that is in this PR, checkpatch success is requested before we merge the PR

arnopo

LGTM after fixing checkpatch report

Currently compiler defines are defined when support for driver or device is the only support being built. This is a negative define, it surrounds the code to not be built and we use ifndef. This is confusing. It also leaves ifndefs all throughout the code-base. Instead, define a macro that is set to 1 if support is enabled. Use this inline in if statements where possible. Any sane compiler will optimize away the code in the branch when support is not enabled just the same as when using the preprocessor so we keep the same binary size. Signed-off-by: Andrew Davis <[email protected]>

Checks if this symbol is defined and set to equal 1. Used to make it more clear that a preprocessor level check is intended. Signed-off-by: Andrew Davis <[email protected]>

There is a common pattern of checking the virtio role while also checking that this role is supported in this build, which can help optimize away unusable code. Add a couple macros for this. This has two main benefits, first being shorter and easier to read if statements, and also makes it easier to not forget to always do both checks. Use these everywhere except rpmsg_virtio.c which needs one more refactor before we can switch it over. Signed-off-by: Andrew Davis <[email protected]>

glneo · 2024-04-19T13:37:31Z

Also, the long line checkpatch warnings will be fixed in the next PR.

Please fix that is in this PR, checkpatch success is requested before we merge the PR

Fair enough, I've added one extra patch to this series which I was going to send as part of a different PR, but it fixes the long line warnings and hopefully is trivial enough.

arnopo requested review from arnopo, edmooring, tammyleino and tnmysh and removed request for arnopo, edmooring and tammyleino March 14, 2024 10:27

glneo force-pushed the reverse-macro-def branch from c17bba3 to e8d3a7e Compare March 14, 2024 14:28

arnopo requested changes Mar 18, 2024

View reviewed changes

glneo force-pushed the reverse-macro-def branch 2 times, most recently from 7f4cda0 to e4842e6 Compare March 19, 2024 16:13

glneo force-pushed the reverse-macro-def branch 3 times, most recently from 9574620 to 3b609d4 Compare April 3, 2024 16:25

arnopo requested changes Apr 17, 2024

View reviewed changes

glneo force-pushed the reverse-macro-def branch from 3b609d4 to c7c8f23 Compare April 17, 2024 20:08

arnopo approved these changes Apr 19, 2024

View reviewed changes

glneo added 3 commits April 19, 2024 07:57

lib: Add and use VIRTIO_ENABLED() macro

3a67e50

Checks if this symbol is defined and set to equal 1. Used to make it more clear that a preprocessor level check is intended. Signed-off-by: Andrew Davis <[email protected]>

glneo force-pushed the reverse-macro-def branch from c7c8f23 to e969802 Compare April 19, 2024 13:34

tnmysh approved these changes Apr 19, 2024

View reviewed changes

arnopo merged commit f939a86 into OpenAMP:main Apr 23, 2024
3 checks passed

glneo mentioned this pull request Apr 23, 2024

Finish converting rpmsg_virtio to use VIRTIO_ROLE_IS_{DRIVER,DEVICE}() #581

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new VIRTIO_{DRIVER,DEVICE}_SUPPORT compiler define to improve code readability #560

Use new VIRTIO_{DRIVER,DEVICE}_SUPPORT compiler define to improve code readability #560

glneo commented Mar 13, 2024

arnopo commented Mar 14, 2024

glneo commented Mar 14, 2024

arnopo commented Mar 18, 2024

arnopo left a comment

arnopo Mar 18, 2024

glneo Mar 18, 2024

arnopo Mar 18, 2024

glneo Mar 18, 2024

glneo commented Mar 18, 2024

glneo commented Mar 18, 2024

arnopo commented Mar 19, 2024

arnopo left a comment

arnopo Apr 17, 2024

glneo Apr 17, 2024

arnopo Apr 19, 2024

glneo commented Apr 17, 2024

arnopo commented Apr 19, 2024

arnopo left a comment

glneo commented Apr 19, 2024

Use new VIRTIO_{DRIVER,DEVICE}_SUPPORT compiler define to improve code readability #560

Use new VIRTIO_{DRIVER,DEVICE}_SUPPORT compiler define to improve code readability #560

Conversation

glneo commented Mar 13, 2024

arnopo commented Mar 14, 2024

glneo commented Mar 14, 2024

arnopo commented Mar 18, 2024

arnopo left a comment

Choose a reason for hiding this comment

arnopo Mar 18, 2024

Choose a reason for hiding this comment

glneo Mar 18, 2024

Choose a reason for hiding this comment

arnopo Mar 18, 2024

Choose a reason for hiding this comment

glneo Mar 18, 2024

Choose a reason for hiding this comment

glneo commented Mar 18, 2024

glneo commented Mar 18, 2024

arnopo commented Mar 19, 2024

arnopo left a comment

Choose a reason for hiding this comment

arnopo Apr 17, 2024

Choose a reason for hiding this comment

glneo Apr 17, 2024

Choose a reason for hiding this comment

arnopo Apr 19, 2024

Choose a reason for hiding this comment

glneo commented Apr 17, 2024

arnopo commented Apr 19, 2024

arnopo left a comment

Choose a reason for hiding this comment

glneo commented Apr 19, 2024