pktbuf: port to use pkt_t instead of void* by miri64 · Pull Request #2285 · RIOT-OS/RIOT

miri64 · 2015-01-12T16:56:56Z

~~Depends on #2158~~ (merged)
~~Depends on #2440~~ (merged)

Prevents users from misusing packet buffer as malloc replacement and allows for executing hold/replace mechanism on associated headers, too.

miri64 · 2015-01-13T03:19:56Z

Rebased to #2158.

miri64 · 2015-01-14T11:16:45Z

Rebased to current master

haukepetersen · 2015-01-15T21:09:49Z

sys/net/crosslayer/pktbuf/pktbuf.c

alignment problem, (header_ptr) is 8-bit off?

The struct is still packed, can you maybe test this on the platforms you are concerned this might occur? The unittests for pktbuf have a test for alignment problems (test_pktbuf_insert_packed_struct()). In general I find it difficult to move the processing counter (and think int would be overkill), since the current implementation expects the pkt_t struct always directly before the data block (so you can find it very fast, if you only have the data pointer for e. g. copy).

I just realized that this might be a problem if you received a packet and want to move the data pointer forward as the headers proceed... -.- I have to rethink the whole pkt_t concept now, or at least it's relation with the packet buffer because this also leads to some sever problems in thread-safety...

haukepetersen · 2015-01-15T21:55:29Z

The code looks clean good to me for the changes.

By looking at the packet buffer in it's current form I am wondering however if there is a way to simplify the design by merging _pkt_internal_t, pkt_header_t, pkt_t together and just work on one unified data structure. I would yield less code size and for my opinion the API would not be completely un-understandable... I mean it's not like a high-level user API as sockets or the messaging or the threading...

miri64 · 2015-01-16T01:02:23Z

About the difference in pkt_t and pkt_hlist_t we discussed previously, that they have a semantic difference (which becomes apparent in pktbuf_add_header() and pktbuf_remove_header(). Even for users of higher level APIs this might become apparent, since

The data chunk of pkt_t is usually bigger than that of pkt_hlist_t
It just seems weird to apparently add packets to packets, though they are headers. It just makes the code using the pkt API more readable IMHO and reduces the risk of future developers asking weirded out question (even if it might be well documented... If I just want to fix something in UDP I might not want to have to read the doc for pkt also)

The reason for _pkt_internal_t was just a design choice to leave the garbage collecting abilities invisible (the processing counter in particular). At least I myself find fields in structs that are not meant to be used publicly always confusing.

miri64 · 2015-01-16T01:36:47Z

Problem I just discovered with this implementation (and pkt in general): as a packet moves up the network-stack the pkt->payload_data will start to move forward. If other threads use that they might not expect that... With the void pointer solution this was not a problem, since they only where of concern of two adjacent layers. The simplest solution I can think of right now while keeping the original goal of the pkt API (to reduce packet related code-duplication) is to revert the pktbuf to its original state (as it is in master right now) and put the packet-related pktbuf functions (allocate + add / remove + free header data; allocate packet data + header data) to the pkt module. What do you think?

miri64 · 2015-01-16T01:42:29Z

(to be clear: the pkt_t struct wouldn't be stored in the pktbuf with approach, but would be something the layers would be using locally and would only be used as a means to abstract the (headers, data, data_len) tuple of the current API. The pkt_hlist_t nodes however would be stored as chunks in the pktbuf but not as they are now. In general: the pktbuf would be more of a chunk buffer)

haukepetersen · 2015-01-16T12:52:31Z

I think your last proposal seems for me to be a huge step backwards. I think to identify packet data by a unified pointer (i.e. pkt_t) and keeping that datastructure in the packet buffer was a very good idea! As wrote earlier, I would even go so far to unify everything down to this structure (that's why I was throwing in the idea of renaming it to chunk_t or something).

I think what we need to do is meet and put our ideas side-to-side on a white board to get on the same page. In the end I think we move in the right direction but we need to synchronize first before we spend more implementation efforts on this. Does this sound feasible?

miri64 · 2015-01-16T19:16:06Z

I don't think it of a step backwards, just in another direction. Have a look at my scetched-up version: miri64@9b6c9f3

miri64 · 2015-01-16T21:01:36Z

Maybe I should also point out that currently stuff like pointing out headers on receive is not possible with the implementation in this PR, but I think for your ideas we need that ;-)

miri64 · 2015-01-19T03:51:56Z

Since this implementation is flawed, I close this PR and offer #2322 as an alternative.

miri64 · 2015-02-11T19:01:55Z

And constantly changing commit hashes don't?

LudwigKnuepfer · 2015-02-11T19:06:35Z

Not for me at least.

LudwigKnuepfer · 2015-02-11T19:07:31Z

(Maybe it's because I'm not a computer and don't read hashes unless I really have to.)

miri64 · 2015-02-11T19:09:47Z

(That's not a hash problem: changing the hash changes the commit date, putting it in another place in the history as it is presented here. But let's discuss this over lunch sometime or in another context… not here ;-))

miri64 · 2015-02-12T15:16:30Z

Rebased to current master

haukepetersen · 2015-02-12T16:01:32Z

sys/include/net/ng_pktbuf.h

Do you have any special use case in mind to make this variable global? I think a function like ng_pktbuf_stats() that is just available if compiled with DEVELHELP and that just prints this kind of data should be sufficient.

miri64 · 2015-02-13T10:12:40Z

Rebased to master and addressed comments

miri64 · 2015-02-16T17:29:56Z

Rebased to current master. Somebody cares to ACK? There are people waiting for this PR to get merged!

haukepetersen · 2015-02-19T10:55:05Z

I think there might be still room for improvement in the implementation in terms of code efficiency - but that is something for later. From my perspective the code looks fine -> ACK when squashed.

miri64 · 2015-02-19T12:52:59Z

Squashed, rebased and changed commit summary

LudwigKnuepfer · 2015-02-19T13:45:40Z

Travis only failed due to the label still being present..

pktbuf: port to use pkt_t instead of void*

miri64 assigned haukepetersen Jan 12, 2015

miri64 added this to the Release NEXT MAJOR milestone Jan 12, 2015

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Jan 12, 2015

miri64 force-pushed the pktbuf/api/use-pkt branch from bc02a0e to 97ec2de Compare January 13, 2015 03:19

miri64 mentioned this pull request Jan 13, 2015

pktqueue: adapt to pkt_t #2287

Merged

miri64 force-pushed the pktbuf/api/use-pkt branch from 97ec2de to fef162f Compare January 14, 2015 08:30

miri64 mentioned this pull request Jan 14, 2015

netapi: adapt to pkt_t #2286

Closed

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 15, 2015

haukepetersen reviewed Jan 15, 2015
View reviewed changes

miri64 mentioned this pull request Jan 16, 2015

net: passing of addresses through the network stack #2311

Closed

miri64 mentioned this pull request Jan 19, 2015

pkt: add functions to allocate packet data [WIP] #2322

Closed

miri64 closed this Jan 19, 2015

miri64 deleted the pktbuf/api/use-pkt branch January 19, 2015 03:52

miri64 restored the pktbuf/api/use-pkt branch January 22, 2015 11:59

miri64 reopened this Jan 22, 2015

miri64 force-pushed the pktbuf/api/use-pkt branch 4 times, most recently from 9c43028 to 0d898c6 Compare January 27, 2015 16:34

miri64 force-pushed the pktbuf/api/use-pkt branch 2 times, most recently from 11c4cf8 to e302574 Compare February 12, 2015 15:15

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 12, 2015

haukepetersen reviewed Feb 12, 2015
View reviewed changes

miri64 force-pushed the pktbuf/api/use-pkt branch 2 times, most recently from de2c34b to 4c067bb Compare February 13, 2015 10:12

miri64 mentioned this pull request Feb 13, 2015

ng_ipv6: initial import #2454

Merged

thomaseichinger mentioned this pull request Feb 14, 2015

iot-lab_M3: hard fault with RPL_UDP example on the testbed #2418

Closed

miri64 force-pushed the pktbuf/api/use-pkt branch from 4c067bb to 1316753 Compare February 16, 2015 17:29

pktbuf: port to use ng_ version

41f60ac

miri64 force-pushed the pktbuf/api/use-pkt branch from 2fa8a01 to 41f60ac Compare February 19, 2015 12:51

LudwigKnuepfer removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 19, 2015

LudwigKnuepfer pushed a commit that referenced this pull request Feb 19, 2015

Merge pull request #2285 from authmillenon/pktbuf/api/use-pkt

4f29a77

pktbuf: port to use pkt_t instead of void*

LudwigKnuepfer merged commit 4f29a77 into RIOT-OS:master Feb 19, 2015

miri64 deleted the pktbuf/api/use-pkt branch February 19, 2015 13:55

Conversation

miri64 commented Jan 12, 2015

Uh oh!

miri64 commented Jan 13, 2015

Uh oh!

miri64 commented Jan 14, 2015

Uh oh!

haukepetersen Jan 15, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 16, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 16, 2015

Choose a reason for hiding this comment

Uh oh!

haukepetersen commented Jan 15, 2015

Uh oh!

miri64 commented Jan 16, 2015

Uh oh!

miri64 commented Jan 16, 2015

Uh oh!

miri64 commented Jan 16, 2015

Uh oh!

haukepetersen commented Jan 16, 2015

Uh oh!

miri64 commented Jan 16, 2015

Uh oh!

miri64 commented Jan 16, 2015

Uh oh!

miri64 commented Jan 19, 2015

Uh oh!

miri64 commented Feb 11, 2015

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

LudwigKnuepfer commented Feb 11, 2015

Uh oh!

miri64 commented Feb 11, 2015

Uh oh!

miri64 commented Feb 12, 2015

Uh oh!

haukepetersen Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 commented Feb 13, 2015

Uh oh!

miri64 commented Feb 16, 2015

Uh oh!

haukepetersen commented Feb 19, 2015

Uh oh!

miri64 commented Feb 19, 2015

Uh oh!

LudwigKnuepfer commented Feb 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants