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

net/sock: Add access to auxiliary data (API only) #14703

Merged
merged 3 commits into from
Dec 3, 2020

Conversation

maribu
Copy link
Member

@maribu maribu commented Aug 5, 2020

Contribution description

Add variants of sock_<PROTO>_send() and sock_<PROTO>_recv() that allow fetching auxiliary data. As a motivating example, the local address is provided as auxiliary data for reception and the timestamp of reception/transmission as auxiliary data for reception/transmission.

The local address could be used to allow virtual hosting to be implemented on top of the sock API. The timestamps can be used to more precisely estimate network delay (without the processing time of the network stack influencing the results). This is e.g. needed for accurate time synchronization, such as used in the Precision Time Protocol (PTP).

These are the functions that were added for the UDP protocol:

ssize_t sock_udp_recv_aux(sock_udp_t *sock, void *data, size_t max_len,
                          uint32_t timeout, sock_udp_ep_t *remote,
                          sock_udp_aux_rx_t *aux);
ssize_t sock_udp_recv_buf_aux(sock_udp_t *sock, void **data, void **buf_ctx,
                              uint32_t timeout, sock_udp_ep_t *remote,
                              sock_udp_aux_rx_t *aux);
ssize_t sock_udp_send_aux(sock_udp_t *sock, const void *data, size_t len,
                          const sock_udp_ep_t *remote, sock_udp_aux_tx_t *aux);

The same was provided for IP and DTLS.

Testing procedure

This PR only contains the API, no implementations are provided. Testing has to be done with the follow up PRs.

Issues/PRs references

Split out of: #14622

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

In general I have no problem with this API extension. The only issue creeping in my mind is: How would the problem you are trying to solve be solved with the classic socket API? Or is it out of scope of that API? Since this is the direct inspiration for sock this questions is always the first raised in my head with such an API change.

@maribu
Copy link
Member Author

maribu commented Aug 5, 2020

How would the problem you are trying to solve be solved with the classic socket API?

It can be done using recvmsg() and sendmsg(). Access is done using cmsg().

But IMO that API is not a pleasure to work with.

@miri64
Copy link
Member

miri64 commented Aug 5, 2020

I honestly forgot about those ^^ Ok then, this API extension gets my 🌟 ;-). I will look into it in detail when I find the time to give it my full approval.

@benpicco
Copy link
Contributor

If cmsg calls it "ancillary data" maybe we should too?
Although I must say that is an unusual word.

@maribu
Copy link
Member Author

maribu commented Sep 10, 2020

If cmsg calls it "ancillary data" maybe we should too?

If I recall correctly, it uses both "ancillary data" and "auxiliary data" as synonyms. E.g. from the man page

/* Receive auxiliary data in msgh */
for (cmsg = CMSG_FIRSTHDR(&msgh); cmsg != NULL;
        cmsg = CMSG_NXTHDR(&msgh,cmsg)) {

I personally like "auxiliary" more, as it seems to be more commonly used. But I could live with "ancillary data" just as well.

@miri64
Copy link
Member

miri64 commented Sep 30, 2020

I personally like "auxiliary" more, as it seems to be more commonly used. But I could live with "ancillary data" just as well.

+1 for auxiliary.

@chrysn
Copy link
Member

chrysn commented Oct 1, 2020

I appreciate this addition, as it allows doing CoAP servers right. (See CoAP FAQ "My application does not process the responses, even though I see them arrive in a network sniffer. Why is that?"; CoI declaration: I wrote that).

In receiving, I understand the bits to be set by the stack for whichever item the stack populates. Is any of those operations costly? The POSIX socket API sets socket flags for the receipt of ancillary data (eg. RECVPKTINFO); as we have better control over our data structures we could do this in place, eg. by having the callers set all bits they care about, so the socket implementation only fills them on demand (and then clears the bits, so that the client can either just check == 0 to assert that all requested data was received, or check individual bits before accessing fields).

(In sending, the analogous construction would beg the question of whether a submission can be done or should be aborted if there's a flag present the stack doesn't understand).

I was briefly worried about the struct increasing the stack size when it grows new features (and would have asked to consider replacing the flags with another indirection so that the user does sock_udp_ep_t sender; sock_udp_aux_rx_t {.sender = &sender};), but given all the fields are pulled in by the application by setting features, that should be alright.

Would it make sense, while creating an API for receiving all things about a message, to group the remote with the other auxiliary / ancillary data?

Possibly OT, but may also help get a grasp of this new API: Could this be used to get something like the (Linux-only) RECVERR recvmsg functionality as well? Where the receiving client would set an aux flag to indicate it also wants to receive ICMP errors on this socket, and then (in a more straightforward fashion than RECVERR's way) would also receive the payload of ICMP errors arriving "on" (more: "about") the socket. (The client would then check the respective aux field to see whether it's a received datagram or an ICMP response).

@maribu
Copy link
Member Author

maribu commented Oct 1, 2020

It would certainly possible to allow users of the API to ask for specific auxiliary data on a case by case base by instead having callers set the corresponding flags for which data they care about.

This would however increase the complexity of the socket implementation a bit. As proposed in this PR, the code would only need to check for the pointer to the auxiliary data struct to not be NULL and only if so populate it with all information available and unlocked by corresponding pseudo-module.

Adding one additional check for each information would slightly increase complexity (and ROM). If an auxiliary information item would be costly to retrieve and only in some cases needed, that would certainly be a good trade off. But I wonder what that would be. E.g. if some low layer information such as RSSI, RX timestamp etc. is needed occasionally, one would still have to always capture them: You never know if those information is needed for an incoming frame until it was passed way up the network stack; which would be too late to capture them.

Could this be used to get something like the (Linux-only) RECVERR recvmsg functionality as well?

Certainly. But I personally would implement it using a separate SOCK API for ICMP errors and have users opening two SOCK sockets. (Note that with the not yet stable async SOCK API you don't need two threads to handle both sockets.)

@chrysn
Copy link
Member

chrysn commented Oct 1, 2020

The ICMP errors on a separate channel could work. I think an option here would be easier to implement (because if the second socket is just for this one socket the stack would need to find the original socket in the first place anyway, and if not (and the second socket is a catch-all ICMP) it'd be hard on the application), but anyway, I digress.

On the matter of cost, one example I had in mind was a 6lo stack that dealt in compressed addresses wherever possible, and would have to expand the address for the local (or even remote if considered for auxiliary data) field. I do now know how realistic that is, however. On added complexity, implementations could be still at leisure to populate fields if that is easier for them than checking the bit. Thus, all the change would be that the |= SOCK_AUX_HAS_LOCAL becomes &= ~SOCK_AUX_HAS_LOCAL, and that the client needs to request them explicitly (one more byte set) -- but could later just use all they requested if == 0 instead of checking for whether SOCK_AUX_HAS_LOCAL is set before accessing the local address.

@maribu
Copy link
Member Author

maribu commented Oct 23, 2020

@chrysn: Rebased against current master and amended as suggested. Please have a second look.

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I'm generally happy with this; the comments all pertain to documentation. I think the docs might be easier to read if the aux mechanism was tied into a central place (probably sock_aux_flags_t), where documentations of the aux field could link to and which would link back to the types and locations where it is used. (But take this as a suggestion, not a review condition).

On the review checklist, what I don't feel qualified to judge is "1: Fundamentals"; @miri64, if you (or someone else with a good roadmap-level eye on the sockets) can tick that box, I can take a sweep at the others (eg. I didn't test this yet).

sys/include/net/sock/udp.h Show resolved Hide resolved
sys/include/net/sock.h Outdated Show resolved Hide resolved
@chrysn
Copy link
Member

chrysn commented Nov 4, 2020

Looking back I just saw that it has the fundamental-API OK already in #14703 (comment). I'll start a final review pass later today, but expect that this can go ahead.

@chrysn
Copy link
Member

chrysn commented Nov 6, 2020

On the time stamps in sending: Do the socket send functions necessarily block until the the data was sent through the physical interface? If not, I'd assume that those implementations just won't accept (ie. clear) the SOCK_AUX_GET_TIMESTAMP, as they can't know whether the aux structure is even still active. Or would they rather block the send operation until the data is out if the flag is set?

@maribu
Copy link
Member Author

maribu commented Nov 6, 2020

On the time stamps in sending: Do the socket send functions necessarily block until the the data was sent through the physical interface?

E.g. sock_udp_send() returns:

The number of bytes sent on success.

And a negative errno code on failure. Thus, those have to block until the network driver reports that the transmission has succeeded. And by that time the timestamp is also available.

@chrysn
Copy link
Member

chrysn commented Nov 6, 2020

In the raw and UDP sockets, why is there no sock_ip_ep_t in the sock_{ip,udp}_aux_tx_t? (I grant that it can still be added later, but for symmetry and responding via the same address, I think it'd make sense).

@benpicco
Copy link
Contributor

benpicco commented Nov 6, 2020

E.g. sock_udp_send() returns:

The number of bytes sent on success.

And a negative errno code on failure. Thus, those have to block until the network driver reports that the transmission has succeeded. And by that time the timestamp is also available.

That's what I thought too. Turns out it's not the case. #11551

@miri64
Copy link
Member

miri64 commented Nov 6, 2020

Turns out it's not the case. #11551

Not always the case ;-).

@chrysn chrysn added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Nov 6, 2020
@maribu
Copy link
Member Author

maribu commented Nov 6, 2020

In the raw and UDP sockets, why is there no sock_ip_ep_t in the sock_{ip,udp}_aux_tx_t?

I assume that the network stack will use the very same local address for sending in any case - everything else seems unreasonable to me.

And to my understanding the local address used for sending can be queried using sock_udp_get_local()

@maribu
Copy link
Member Author

maribu commented Dec 2, 2020

May I squash? Anything left for me to do here?

@chrysn
Copy link
Member

chrysn commented Dec 2, 2020

Any code-anchored comments I had are addressed, so good to squash.

There's the remaining open point on whether the send time can be reliably populated, but that can be addressed where it's implemented just as well.

As for the sending address, sock_udp_get_local could give SOCK_IPVn_EP_ANY as far as I understand; setting the outgoing address then does matter when there's multiple candidate addresses. But that, too, can be addressed where it's first actually implemented.

@chrysn chrysn added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Dec 2, 2020
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Testing-wise, I've run through a few gcoap tests -- but given this primarily changes headers, the CI testing is what matters more here. I'm happy with the changes.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 2, 2020
@maribu maribu merged commit 9d46bc7 into RIOT-OS:master Dec 3, 2020
@maribu maribu deleted the sock-aux-api branch December 3, 2020 11:08
@maribu
Copy link
Member Author

maribu commented Dec 3, 2020

Thanks for the reviews :-)

@benpicco
Copy link
Contributor

Did you happen to have an implementation for sock_dtls_recv_buf_aux() somewhere already?

@maribu
Copy link
Member Author

maribu commented Jul 26, 2022

Not that I remember :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants