netdev ethernet: deal with too small packet buffer

[OlegHahm]

Rationale: some drivers, e.g. the Ethernet drivers, require a minimum size for the packet buffer. This commit introduces an optional minimal size that can be overwritten from the application.

I reworked this PR to introduce a temporary buffer inside the Ethernet drivers themselves, instead of reallocating on the packet buffer. IMO that's a cleaner solution as it reduces the lines of code and makes it simpler. It introduces more statically allocated memory, but this memory is necessary with the current implementation anyway (it just moves from pktbuf to the driver), but at least fails at build time if not available.

Let the native port check for a too small pktbuf size and adjust it.

Additionally this PR removes the (now not longer needed) hack from gnrc_minimal_networking's Makefile and handle native the same as any other platform.

Without this PR the default example won't work on native (or other boards with an Ethernet device). Thanks for reporting, @LudwigKnuepfer.

[kaspar030]

This breaks if multiple modules define a minimum size.

[OlegHahm]

Any better idea?

[kaspar030]

Without this PR the default example won't work on native (or other boards with an Ethernet device).

Could you elaborate why?

Any better idea?

Define an ethernet pseudomodule, make all ethernet drivers depend on it (same with other network device types), then use those pseudomodules to fix minimum pktbuf size.

[OlegHahm]

Without this PR the default example won't work on native (or other boards with an Ethernet device).

Could you elaborate why?

Because the Ethernet drivers always allocate a full Ethernet frame in the beginning and only reallocate later.

[kaspar030]

Because the Ethernet drivers always allocate a full Ethernet frame in the beginning and only reallocate later.

Ah, I remember. ;)

[OlegHahm]

Define an ethernet pseudomodule, make all ethernet drivers depend on it (same with other network device types), then use those pseudomodules to fix minimum pktbuf size.

I don't see why I need a pseudomodule if we have already netdev2_eth, but the question is mostly: where would you put this minimum pktbuf size?

[kaspar030]

I don't see why I need a pseudomodule if we have already netdev2_eth

yeah, just use that.

but the question is mostly: where would you put this minimum pktbuf size?

Why not exactly where you put with this PR?

[OlegHahm]

So, then you propose to leave this PR as it is?

[kaspar030]

So, then you propose to leave this PR as it is?

No, I meant instead of checking for "GNRC_PKTBUF_MIN_SIZE" in pktbuf.h, check for "MODULE_NETDEV2_ETH" and if true, set ethernet's minimum size.

[OlegHahm]

Ah, but I was actually looking for a way to avoid more ifdefs and particular ifdefs checking for particular modules. Imagine we have hundred of network drivers with different requirements on the minimum packetbuffer size.

IMO the ideal solution would be to define this size somewhere in the device's header, but I don't know how to make sure that this headers gets included before pktbuf.h is parsed.

[kaspar030]

Ah, but I was actually looking for a way to avoid more ifdefs and particular ifdefs checking for particular
modules. Imagine we have hundred of network drivers with different requirements on the minimum
packetbuffer size.

I imagine. Ugly. But currently we have one, and a handful would be manageable.
The solution of this PR only works for exactly one, and IMHO ifdefs within makefiles are even more ugly than ifdefs within C files.

IMO the ideal solution would be to define this size somewhere in the device's header, but I don't know
how to make sure that this headers gets included before pktbuf.h is parsed.

Reminds me of my snippets PR.

[kaspar030]

Another solution would be to only allocate the maximum pktsnip size within the ethernet drivers (or a little bit less).

[OlegHahm]

Instead of allocating it in the packet buffer, you mean? Might be somewhat cleaner and would allow us to get rid of this realloc-call.

On 2 April 2016 09:16:43 CEST, Kaspar Schleiser notifications@github.com wrote:

Another solution would be to only allocate the maximum pktsnip size
within the ethernet drivers (or a little bit less).

---

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#5229 (comment)

Join the RIOT
http://www.riot-os.org

[kaspar030]

Instead of allocating it in the packet buffer, you mean? Might be somewhat cleaner and would allow us to get rid of this realloc-call.

No, I meant something like setting an MTU dependent on pktbuf size, or checking the minimum pktbuffer size before allocating, or sth like that.

[OlegHahm]

Hm, why not simply using a temporary static buffer inside the driver? Would simplify the code and fail at bukd time if not enough memory is available.

MTU is not a fixed value in GNRC.

On 2 April 2016 10:14:23 CEST, Kaspar Schleiser notifications@github.com wrote:

Instead of allocating it in the packet buffer, you mean? Might be
somewhat cleaner and would allow us to get rid of this realloc-call.

No, I meant something like setting an MTU dependent on pktbuf size, or
checking the minimum pktbuffer size before allocating, or sth like
that.

---

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#5229 (comment)

Join the RIOT
http://www.riot-os.org

[OlegHahm]

How about this solution?

[OlegHahm]

Hm, just realized that this won't work.

[OlegHahm]

Ah, no, my mistake. It should work. Forget the previous comment.

[kaspar030]

How about this solution?

But this allocates the needed space again in the pktbuf, right? Then it essentially doubles the needed space.

[kaspar030]

If it just wraps the buffer, it needs to be locked, and only one ethernet packet can be in flight at one time.

[OlegHahm]

But this allocates the needed space again in the pktbuf, right? Then it essentially doubles the needed space.

It allocates the spaced that is actual need in the pktbuffer, yes. I think if we waste 1.5kB in the driver anyway, that doesn't make things much worse.

Can you explain (once again) why this alloc-realloc mechanism is required for the Ethernet driver but not for other radio drivers? Maybe we can get completely rid of this hack?

[kaspar030]

What was the rationale again of lowering the default pktbuf size? IMHO we should just up it again, all our MCU's have that memory.

[miri64]

Just another idea: Since this is a native related issue, how about fixing it there: We are on native, so why not use a temporary buffer that we can malloc() in the netdev2_tap driver where we copy the data from instead of the file buffer of the TAP interface? Then recv() could return the actual length of the packet instead of the maximum length.

[OlegHahm]

@kaspar030 was against this.

[kaspar030]

the NC_SIZE is different than before, is that a problem?

[kaspar030]

solved offline

[kaspar030]

pls squash

[OlegHahm]

Removed reverted (and reverting) commits.

[OlegHahm]

There were two small glitches revealed by the CI. Can I squash directly?

[haukepetersen]

yes, please

[OlegHahm]

Squashed.

[haukepetersen]

restarted Murdock

[haukepetersen]

ACK once Murdock agrees.

[miri64]

Please provide a backport.