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

gnrc_netif: expose message queue size configurable #11067

Merged
merged 1 commit into from Feb 26, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 26, 2019

Contribution description

In some offline discussions regarding #10268 we noticed, that the message queue size is too small for some network devices to handle large fragmented packages. Not just the driver in #10268 is affected, but also e.g. the kw2x on pba-d-01-kw2x. This change

  1. Increases the queue size from 8 to 16 for gnrc_netif threads and
  2. Exposes the macro for the message queue size and makes it configurable. (see gnrc_netif: expose message queue size configurable #11067 (comment)).

Testing procedure

Compile and flash examples/gnrc_networking to pba-d-01-kw2x with CFLAGS=-DGNRC_NETIF_MSG_QUEUE_SIZE=16, try to ping it from another IEEE-802.15.4-capable board with large payloads:

ping6 -s 1232 "<link-local address>"

Without this PR problems might arise, with it, it should work. Also try and see if for smaller boards (e.g. z1 or arduino-mega2560) examples/gnrc_minimal still works without stack overflows (you can't send too huge packets though, as the packet buffer size is set to 512 in gnrc_minimal) (gnrc_minimal does not work in master, see #11067 (comment)).

Issues/PRs references

Discovered in #10268.

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 26, 2019
@miri64 miri64 added this to the Release 2019.04 milestone Feb 26, 2019
* changed.
*/
#ifndef GNRC_NETIF_MSG_QUEUE_SIZE
#define GNRC_NETIF_MSG_QUEUE_SIZE (16U)
Copy link
Contributor

Choose a reason for hiding this comment

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

Previous setting was 8. Would make sense to keep that value.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, nvm, PR text describes the change. ok as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

See OP:

  1. Increases the queue size from 8 to 16 for gnrc_netif threads and

Also I put the change of the message queue size in a separate commit + added testing procedure description to account for the change ;-).

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

gnrc_minimal doesn't work on z1 :-/.

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

At least (on master) it doesn't reply to ping requests :-/.

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

gnrc_minimal is also not pingable on samr21-xpro... :(

@miri64 miri64 force-pushed the gnrc_netif/enh/msg-size-config branch from b00044c to b5028e1 Compare February 26, 2019 11:12
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

Better safe than sorry. I reverted the message queue size change...

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

@kaspar030 can you have another look. The change now should be very simple.

@SemjonWilke
Copy link
Member

SemjonWilke commented Feb 26, 2019

Successfully tested with ping from samr21-xpro -> pba-d-01-kw2x and samr21-xpro -> nrf52840dk (#10268) both with 1232 Bytes.
ping6 "<link-local address>" -s 1232

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

(re-)ACK.

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

waidaminute. shouldn't the original define be removed somewhere?

@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

Sorry... must have happened when I removed the size change >_<

@miri64 miri64 force-pushed the gnrc_netif/enh/msg-size-config branch from b5028e1 to 13030d5 Compare February 26, 2019 12:26
@miri64
Copy link
Member Author

miri64 commented Feb 26, 2019

Fixed and amended immediately.

@kaspar030
Copy link
Contributor

&go.

@kaspar030 kaspar030 merged commit 16e4722 into RIOT-OS:master Feb 26, 2019
@miri64 miri64 deleted the gnrc_netif/enh/msg-size-config branch February 26, 2019 12:46
@miri64
Copy link
Member Author

miri64 commented Mar 7, 2019

gnrc_minimal is also not pingable on samr21-xpro... :(

For reference: this is because gnrc_minimal is not including gnrc_sixlowpan_iphc is not including IPHC, so the pinging node needs to deactivate iphc. This is also documented in the example's README

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants