Skip to content
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

Jump to bottom

gnrc: Use exponent to configure message queue sizes #14086

Merged
merged 10 commits into from Jun 15, 2020
Merged

gnrc: Use exponent to configure message queue sizes #14086

merged 10 commits into from Jun 15, 2020

Conversation

leandrolanzieri
Copy link
Contributor

@leandrolanzieri leandrolanzieri commented May 15, 2020 •
edited

Contribution description

This changes the definition of the macros that set the sizes of the message queues for the following GNRC modules:

  • ipv6
  • nettest
  • rpl
  • udp
  • gomach
  • lwmac
  • netif
  • sixlowpan
  • tcp

Using the exponent to define the queue size helps to enforce the condition that it should be power of two. The correspondent Kconfig files were updated as well.

Also, I added Doxygen config groups for some of the modules which did not have it.

Testing procedure

Applications using these modules should still be compiling and working.

Issues/PRs references

Change proposed by @miri64 here.

@leandrolanzieri leandrolanzieri added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation TF: Config Marks issues and PRs related to the work of the Configuration Task Force labels May 15, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone May 15, 2020
@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 15, 2020
cgundogan
cgundogan previously requested changes May 15, 2020
View reviewed changes
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good so far. Configuration for TCP seems to be missing:

#define GNRC_TCP_TCB_MBOX_SIZE (8U)

in tcb.h

@akshaim
Copy link
Member

akshaim commented May 15, 2020 •
edited

Also @leandrolanzieri please note that nettest is planned to be removed after the 2020.07 release.

@leandrolanzieri
Copy link
Contributor Author

Changes look good so far. Configuration for TCP seems to be missing:

Added TCP TCB mbox size. Also the doc group for it.

@miri64
Copy link
Member

miri64 commented May 15, 2020

LGTM. On a different note, because I noticed the thread stack size of gnrc_udp is not adopted: Will there be a PR for that as well?

@akshaim akshaim mentioned this pull request May 15, 2020
Merged
@leandrolanzieri
Copy link
Contributor Author

Will there be a PR for that as well?

Yes. For now stack sizes were not exposed because THREAD_STACKSIZE_DEFAULT needs a default per CPU (or group of CPUs). But when we place the Kconfig files in the CPUs then it definitely needs to be exposed.

@miri64
Copy link
Member

miri64 commented May 18, 2020

Changes look good so far. Configuration for TCP seems to be missing:

#define GNRC_TCP_TCB_MBOX_SIZE (8U)

in tcb.h

I guess the GNRC implementation of sock should also be included then:

#ifndef SOCK_MBOX_SIZE
#define SOCK_MBOX_SIZE      (8)         /**< Size for gnrc_sock_reg_t::mbox_queue */
#endif

in sys/net/gnrc/sock/include/sock_types.h

@leandrolanzieri
Copy link
Contributor Author

I guess the GNRC implementation of sock should also be included then:

Will do

@akshaim akshaim mentioned this pull request May 20, 2020
Merged
@leandrolanzieri
Copy link
Contributor Author

Are we OK to continue as is?

@miri64
Copy link
Member

miri64 commented May 20, 2020

Many of the actual queue sizes are still not #ifdef'd anymore.

@miri64
Copy link
Member

miri64 commented May 20, 2020

But apart from that: from my side, yes.

@leandrolanzieri
Copy link
Contributor Author

Many of the actual queue sizes are still not #ifdef'd anymore.

Ok, I though you referred only to IPv6. I re-added the #ifdefs to the rest for backwards compatibility.

@leandrolanzieri leandrolanzieri removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 20, 2020
miri64
miri64 previously requested changes May 22, 2020
View reviewed changes
sys/net/gnrc/netif/Kconfig Outdated Show resolved Hide resolved
miri64
miri64 reviewed May 22, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash

@miri64
Copy link
Member

miri64 commented May 22, 2020

@cgundogan are you ok with the current state of the PR? You are atm requesting changes.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 12, 2020
@leandrolanzieri leandrolanzieri added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 13, 2020
@leandrolanzieri
Copy link
Contributor Author

I can't reproduce locally, so I skip compile tests for now while trying to fix this.

@leandrolanzieri leandrolanzieri force-pushed the pr/gnrc/queue_size_exp branch 2 times, most recently from d03fd57 to 3372e1f Compare June 13, 2020 08:17
@leandrolanzieri
Copy link
Contributor Author

It seems to be working now. I added the Mbox size parameter to Kconfig now that TCP has its configurations there. @miri64 care to take another look?

@miri64
Copy link
Member

miri64 commented Jun 13, 2020

Looking good. Please squash!

@leandrolanzieri
gnrc/ipv6: Configure queue size with exponent
b0168f5 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.
@leandrolanzieri
gnrc/nettest: Configure queue size with exponent
e02c338 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.
@leandrolanzieri
gnrc/rpl: Configure queue size with exponent
3dc7865 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.
@leandrolanzieri
gnrc/udp: Configure queue size with exponent
4998b90 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.
@leandrolanzieri
gnrc/gomach: Configure queue size with exponent and add config doc group
148c918 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.

Also a compile configuration documentation group is created.
@leandrolanzieri
gnrc/lwmac: Configure queue size with the exponent and add config group
f62623c 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.

Also a compile configuration documentation group is created.
@leandrolanzieri
gnrc/netif: Configure queue size with exponent
05962fe 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.
@leandrolanzieri
gnrc/sixlowpan: Configure queue size with exponent
375cad3 
This changes the configuration macro to be the exponent of 2^n, as the
message queue size needs to be always power of 2.
@leandrolanzieri
gnrc/tcp: Configure mbox size with exponent and add config doc group
d4cac38 
This changes the configuration macro to be the exponent of 2^n, as the
mbox buffer size needs to be always power of 2.

Also a compile configuration documentation group is created.
@leandrolanzieri
gnrc/sock: Configure mbox size with exponent and add config doc group
dbfe0d8 
This changes the configuration macro to be the exponent of 2^n, as the
mbox buffer size needs to be always power of 2. The macro now has the
GNRC prefix.

Also a compile configuration documentation group is created.
@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Jun 15, 2020
@miri64
Copy link
Member

miri64 commented Jun 15, 2020

Let's goooooo!

Mario

@miri64 miri64 merged commit 1ad2d07 into RIOT-OS:master Jun 15, 2020
@leandrolanzieri leandrolanzieri deleted the pr/gnrc/queue_size_exp branch August 7, 2020 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

@kaspar030 kaspar030 kaspar030 left review comments

@cgundogan cgundogan cgundogan left review comments

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines TF: Config Marks issues and PRs related to the work of the Configuration Task Force Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.07
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@leandrolanzieri @akshaim @miri64 @cgundogan @kaspar030