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.

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

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

Closed
wants to merge 5 commits into from

Conversation

OlegHahm
Copy link
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
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
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 notifications@github.com 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

@@ -132,7 +133,15 @@ static void _receive(gnrc_pktsnip_t *pkt)
/* send payload to receivers */
if (!gnrc_netapi_dispatch_receive(GNRC_NETTYPE_UDP, port, pkt)) {
DEBUG("udp: unable to forward packet as no one is interested in it\n");
#ifdef MODULE_GNRC_CONN
Copy link
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
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
Member

miri64 commented Nov 23, 2015

Then we need a similar solution for conn_ip

@kaspar030
Copy link
Contributor

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

msg_receive(&msg);
int idx = -1;
/* first check if there's packet waiting in the queue */
if ((idx = cib_get(&_conn_buf_cib)) != -1) {
Copy link
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
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
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
Member Author

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

@kaspar030
Copy link
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
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
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
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
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
Member

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

@OlegHahm
Copy link
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
Member Author

Are you okay with these comments? Should I squash?

int gnrc_conn_recvfrom(conn_t *conn, void *data, size_t max_len, void *addr, size_t *addr_len,
uint16_t *port)
{
msg_t msg;
/* TODO: this is a rather arbirtrary value and creates the risk of starvation. */
Copy link
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
Member Author

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

@kaspar030
Copy link
Contributor

on closing of socket?

@OlegHahm
Copy link
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
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