Skip to content

gnrc conn: enqueue packets if noone's waiting#4327

Closed
OlegHahm wants to merge 5 commits intoRIOT-OS:masterfrom
OlegHahm:gnrc_conn_enqueue
Closed

gnrc conn: enqueue packets if noone's waiting#4327
OlegHahm wants to merge 5 commits intoRIOT-OS:masterfrom
OlegHahm:gnrc_conn_enqueue

Conversation

@OlegHahm
Copy link
Copy Markdown
Member

Currently gnrc in combination with POSIX sockets will discard packets if recvfrom() is not called before.

Another part for fixing #4320.

@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: POSIX Area: POSIX API wrapper Area: network Area: Networking labels Nov 22, 2015
@OlegHahm OlegHahm added this to the Release 2015.12 milestone Nov 22, 2015
@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 23, 2015

There must be another way of doing this without changing gnrc_udp. Because this is somewhat beside the point of conn, IMHO.

@OlegHahm
Copy link
Copy Markdown
Member Author

The only way to keep gnrc_udp completely untouched would require to run conn or POSIX in its own thread which would be worse. I'm also unhappy with this dependency but without this patch sockets won't work properly.

On 23 November 2015 09:17:02 CET, Martine Lenders [email protected] wrote:

There must be another way of doing this without changing gnrc_udp.
Because this is somewhat beside the point of conn, IMHO.


Reply to this email directly or view it on GitHub:
#4327 (comment)

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

GNRC_CONN_UDP

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can one include GNRC_CONN and GNRC_UDP without GNRC_CONN_UDP?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 23, 2015

Then we need a similar solution for conn_ip

@kaspar030
Copy link
Copy Markdown
Contributor

This seems like a hack. @OlegHahm Could you elaborate on the problem?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't this also dequeue packets destined for other ports since you use only one universal cib?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but an (unbound) POSIX recvfrom() doesn't specify a port to receive on. However, this rises two questions:
1.) do we want this queue only in combination with POSIX sockets?
2.) what to do with bound sockets here?

@OlegHahm
Copy link
Copy Markdown
Member Author

This seems like a hack. @OlegHahm Could you elaborate on the problem?

If a library uses POSIX sockets (like ccn-lite or tinydtls) and calls recvfrom() it expects that packets are buffered in the stack. Hence, one can do something like

sendto(...);
do_some_computation(); 
recvfrom(...);

expecting to receive the response to the sendto-call even if do_some_computation() takes a long time. Since gnrc_udp cannot forward the pointer to the packet to anyone if noone is registered, it can only discard the packets (what is currently happening and makes POSIX sockets on RIOT pretty useless for porting) or queue the packets somewhere.

@OlegHahm
Copy link
Copy Markdown
Member Author

@cgundogan, what about the proposed solution in OlegHahm@65506e5 ?

@kaspar030
Copy link
Copy Markdown
Contributor

@OlegHahm thanks, I get it.

This points to something that should be solved at the core IPC: decouple message endpoints from threads. That way conn_* could have it's own message queue and register that when *_bind()ing. I've got code lying around for that, but that will probably need time to get in, as it's at the heart of core/.

If this PR fixes posix sockets for now, I'd say, lets get it in.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Nov 24, 2015

I thought about it, too. This doesn't necessarily have to be a replacement for msg but can also be an alternative (maybe a good idea for a first step anyway). I'm thinking of something like lwIPs mboxes.

@OlegHahm
Copy link
Copy Markdown
Member Author

I agree that this is a workaround for now, but we would need a more generic (no pun intended) concept to solve this. However, if you agree to take this workaround for now, I would implement the missing RAW IP part and we can merge this.

@cgundogan
Copy link
Copy Markdown
Member

@OlegHahm I am fine with your proposed enqueuing strategy. However: In case of multiple threads that use gnrc_udp with different ports it may be possible that a message gets dequeued before it is checked by the target thread and put into the queue afterwards. This can repeat and lead to starvation (if e.g. both threads call receive in a loop)

@OlegHahm
Copy link
Copy Markdown
Member Author

@cgundogan, probably you're right, but I guess this can already happen without this patch, if there's an RAW IP and a UDP socket opened. I think we need to find a better solution in the long run anyway.

@cgundogan
Copy link
Copy Markdown
Member

As long as this woraround is somehow marked as such in the code I join the others and ACK it

@OlegHahm
Copy link
Copy Markdown
Member Author

Okay, will do.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 25, 2015
@OlegHahm
Copy link
Copy Markdown
Member Author

Are you okay with these comments? Should I squash?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

arbitrary (non-commit comment)

@OlegHahm
Copy link
Copy Markdown
Member Author

Hm, still don't know how to garbage collect enqueued datagrams?

@kaspar030
Copy link
Copy Markdown
Contributor

on closing of socket?

@OlegHahm
Copy link
Copy Markdown
Member Author

Might be way too late. One may open a socket just for sending period packets without ever calling receive. I think the best way is to switch to a different data structure, e.g. a lifo, and call a timer based garbage collector.

@OlegHahm
Copy link
Copy Markdown
Member Author

Thanks to the discussion with @cgundogan in #4320 I came to the conclusion that this is actually not required if you do the implicit binding during the sendto() call.

@OlegHahm OlegHahm closed this Nov 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Area: POSIX Area: POSIX API wrapper CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants