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.

Sign up for GitHub

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

netreg: Initial import #2404

Merged
merged 2 commits into from
Feb 12, 2015
Merged

netreg: Initial import #2404

merged 2 commits into from
Feb 12, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 7, 2015

Depends on #2397 (merged) #2440 (merged). Tests will follow when review is somewhat positive ;-)

This was referenced Feb 7, 2015
Merged
Closed
@LudwigKnuepfer LudwigKnuepfer added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: network Area: Networking NSTF Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Feb 7, 2015
haukepetersen
haukepetersen reviewed Feb 7, 2015
View reviewed changes
sys/include/net/ng_netreg.h
* E. g. protocol numbers / next header numbers in IPv4/IPv6,
* ports in UDP/TCP, or similar.
*/
uint32_t demux_ctx;
Copy link
Contributor

Choose a reason for hiding this comment

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

we put this in netapi and netdev to uint16_t...

Copy link
Member Author

Choose a reason for hiding this comment

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

Question is: is this enough?

@haukepetersen
Copy link
Contributor

I am not quite convinced of this implementation, I think the lookup is still too expensive! (Compare to #2310)

@OlegHahm
Copy link
Member

OlegHahm commented Feb 7, 2015

What*s the relation to #2310?

OlegHahm
OlegHahm reviewed Feb 7, 2015
View reviewed changes
sys/net/crosslayer/ng_netreg/ng_netreg.c
ng_netreg_entry_t *start = entry;

do {
if (entry->proto_pid == proto_pid && entry->demux_ctx == demux_ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

parens

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2015

@haukepetersen I was thinking about doing a lookup on a two-dimensional array (1 axis: type, other axis demux_type % n), but that seemed to expensive memory-wise. The lookup is in the very common case of 1:1 mappings nearly as fast as your implementation (I have just two comparisons more), since the list will be only one element long. Only for 1:n mappings there will be an iteration, but I don't expect more than two or maybe three protocols talking to the same lower layer in practice. On the network layer we have only IPv4 and IPv6, and we both already argued, that more network protocols are not very realistic. On the transport layer we currently have UDP and TCP and maybe we implement DCCP and SCTP someday. But it is not to expect that you need more than two of them for your application (either you use UDP and TCP together, or you use the benefits of either DCCP or SCTP to not having to do that ;-)). On application layer you might have more than that, but in general you will use the socket API as a "normal" user. And that is one thread only, too.

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2015

@haukepetersen I just looked at your implementation again: How does your implementation caters to demultiplexing contexts?

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2015

Utilizes utlist instead of clist now

@haukepetersen
Copy link
Contributor

I just looked at your implementation again: How does your implementation caters to demultiplexing contexts?

it doesn't. The context stuff was introduced after I did the PR...

@miri64
Copy link
Member Author

miri64 commented Feb 8, 2015

The context stuff was introduced after I did the PR...

It was not (it was already in the introductory PR of netapi), but this is not the point: With the demultiplexing context we either have to decide for a slower, but more memory efficient list implementation or a more memory-wasting, but faster, hash table implementation. A simple array-lookup is just not possible.

@miri64
Copy link
Member Author

miri64 commented Feb 8, 2015

A tree-based data structure would also be possible, but would need at least one extra pointer + the ROM size would be increasing since it would have a more complex code.

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 9, 2015
@miri64
Copy link
Member Author

miri64 commented Feb 9, 2015

Changed semantic, added tests and fixes following from them

@haukepetersen
Copy link
Contributor

Concerning the underlying data-structure: I think we could just provide more then one implementation in the future, one optimized for run-time and one for memory...

Otherwise this PR looks good to me: ACK

Lotterleben
Lotterleben reviewed Feb 11, 2015
View reviewed changes
sys/include/net/ng_netreg.h
* E. g. protocol numbers / next header numbers in IPv4/IPv6,
* ports in UDP/TCP, or similar.
*/
uint16_t demux_ctx;
Copy link
Member

Choose a reason for hiding this comment

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

If I understood it correctly, the demultiplexing context here works similar to the one from the netapi– why does the demultiplexing context in netapi gets its own typedef'ed type (netapi_reg_demux_ctx_t) and here it's a uint16_t?

Copy link
Member Author

Choose a reason for hiding this comment

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

This type does not exist in the most current version of netapi, since we moved all of the registration to this module. netapi_reg_demux_ctx_t is quite a mouthful compared to a simple uint16_t, so I left it out ;)

@Lotterleben
Copy link
Member

(I've had a bit of a hard time understanding ng_netreg_register(), so I added some suggestions for explanatory comments)

@miri64
Copy link
Member Author

miri64 commented Feb 12, 2015

Addressed comments and moved from (only available during testing) reset- to init-based setting of the values, just in case someone decides KERNEL_PID_UNDEF can't be 0 anymore ;-)

@Lotterleben
Copy link
Member

👍 I'll run the tests on a SAMR21 after lunch and if that goes through and Travis is happy, I'd ACK, too.

@haukepetersen
Copy link
Contributor

please squash.

@miri64
Copy link
Member Author

miri64 commented Feb 12, 2015

I'll wait till #2440 is merged.

@miri64 miri64 mentioned this pull request Feb 12, 2015
Merged
@haukepetersen
Copy link
Contributor

sorry, overlooked that dependency...

@haukepetersen
Copy link
Contributor

now now now :-)

@miri64
Copy link
Member Author

miri64 commented Feb 12, 2015

Rebased and squashed. @Lotterleben asked if she can merge ;-)

@haukepetersen
Copy link
Contributor

let's just give Travis another chance to complain... @Lotterleben merge at will when Travis is happy!

@miri64 miri64 removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 12, 2015
@Lotterleben
Copy link
Member

Travis is happy, the tests on the SAMR21are happy... Imma merge this now ✨

Lotterleben added a commit that referenced this pull request Feb 12, 2015
@Lotterleben
Merge pull request #2404 from authmillenon/netreg/feat/initial
e9afa1e 
netreg: Initial import
@Lotterleben Lotterleben merged commit e9afa1e into RIOT-OS:master Feb 12, 2015
@haukepetersen
Copy link
Contributor

yeah!

@OlegHahm OlegHahm mentioned this pull request Feb 13, 2015
Closed
@miri64 miri64 deleted the netreg/feat/initial branch February 13, 2015 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Lotterleben Lotterleben

Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Network Stack Task Force
Development

Successfully merging this pull request may close these issues.
6 participants
@miri64 @haukepetersen @OlegHahm @cgundogan @Lotterleben @LudwigKnuepfer