ng_pktbuf: change semantics for received packets

[miri64]

Sending packets remain in the same order, receiving packets are reversed. This way we save memory on ng_pktbuf_start_write(), since only the parts of the packets of interest for later layers in the line are duplicated:

Some extreme examples below (next pointer always points up). Normally, with sending this would only apply for netif headers in multicast and L3 in some corner cases. For receiving this can only happen if there are sniffers or dual stacks involved.

Sending                          Receiving
=======                          =========

* Payload                        * netif header
|\                               |\
| * L4 header 1                  | * L2.5 header 1
| * L3 header 1                  | * L3 header 1 
| * netif header 1               | * L4 header 1
*   L4 header 2                  | * Payload 1
|\                               *   L3 header 2
| * Network header 2             |\
| * netif header 2               | * L4 header 2
*   Network header 3             | * Payload 2
|\                               *   Payload 3
| * netif header 3
*   netif header 4

[haukepetersen]

s/payload/next/

[miri64]

I think this is confusing.

[miri64]

(see https://github.com/RIOT-OS/RIOT/pull/2563/files#r26119214)

[haukepetersen]

hm, I don't really like the re-naming of the next pointer to payload as I think payload has a different meaning... The next pointer is more general and I think it reflects the general concept better, of the pktbuf being a generic module that just stores (and links) any number of pktsnips, without really caring about the contents of the single pktsnips...

[miri64]

But that was never the intent of the argument next in ng_pktbuf_add(). The idea for it was, that you can mark a header in the pktsnip_t you currently have, to avoid data duplication (the blocks are actual chunks of memory, not the implied structure of the packet):

Before hdr = ng_pktbuf_add(pkt, pkt->data, sizeof(ng_ipv6_hdr_t), NG_NETTYPE_IPV6):

                pkt->data
                v
               +--------------------------------------+
               +--------------------------------------+


After hdr = ng_pktbuf_add(pkt, pkt->data, sizeof(ng_ipv6_hdr_t), NG_NETTYPE_IPV6):

                hdr->data       pkt->data
                v               v
               +---------------+----------------------+
               +---------------+----------------------+

Before it really reflected how the ng_pktsnip_t list is build up (hdr->next would have been pkt), but now since for receiving the order is reversed, we don't have this anymore (pkt->next must now be hdr).

[miri64]

Just for comparison: If you just would call (as I imagine you understand what ng_pktbuf_t does) to mark out a received packet:

hdr = ng_pktbuf_add(NULL, pkt->data, sizeof(ng_ipv6_hdr_t), NG_NETTYPE_IPV6);
pkt->data += sizeof(ng_ipv6_hdr_t);
pkt->next = hdr;

you would get:

Before hdr = ng_pktbuf_add(NULL, pkt->data, sizeof(ng_ipv6_hdr_t), NG_NETTYPE_IPV6):

                pkt->data
                v
               +--------------------------------------+
               +--------------------------------------+


After hdr = ng_pktbuf_add(NULL, pkt->data, sizeof(ng_ipv6_hdr_t), NG_NETTYPE_IPV6):

                leaked memory   pkt->data                  copy of leaked memory
                v               v                          v
               +---------------+----------------------+...+---------------+
               +---------------+----------------------+...+---------------+

[miri64]

Actually the data will not be leaked or copied with this fix, I realized now. Both pkt->data and hdr->data would point to where pkt->data was before the call. But it seems very wrong to me to make this pointer pushing outside the scope of the packet buffer.

[miri64]

I'll adapt the documentation of ng_pkt.h for the change and also try to make the documentation of ng_pktsnip_add() clearer.

[miri64]

@haukepetersen better understandable now?

[miri64]

@haukepetersen ping?

[haukepetersen]

this can be removed (as application protocols are identified on their endpoint, e.g. UDP, port 1234)

[miri64]

done

[haukepetersen]

don't we need to call _pktbuf_internal_free(pkt) always if pkt->users == 0, independent of pkt->data? So this if condition needs to be split up in my opinion:

if (pkt->users == 0) {
    if (_pktbuf_internal_contains(pkt->data)) {
        _pktbuf_internal_free(pk->data);
    }
    _pktbuf_internal_free(pkt);
}

[miri64]

Done + added a check if pkt->users is 0 before being decremented (already had a memleak in #2555 because of this).

[haukepetersen]

looking good. ACK when squashed and Travis is happy.

[miri64]

Squashed

[miri64]

Forgot to adapt the unittests >.<

[haukepetersen]

and go.