| Message ID | ca6ac789334c26905d86f2a596878d2dc18e69e9.1416859110.git.luto@amacapital.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Nov 24, 2014 at 3:02 PM, Andy Lutomirski <luto@amacapital.net> wrote: > SOF_TIMESTAMPING_OPT_ID puts the id in ee_data, not ee_info. > > Cc: Willem de Bruijn <willemb@google.com> > Signed-off-by: Andy Lutomirski <luto@amacapital.net> Acked-by: Willem de Bruijn <willemb@google.com> > --- Thanks for sending a fix. > While I'm here, the docs say: > > In practice, it [ee_data] is a monotonically increasing u32 (that wraps). > > Is user code supposed to rely on this and, further, on the fact that the > counter starts at zero? If not, how else is user code supposed to match > outgoing data to timestamps? That is correct. The per-socket counter is reset when SOF_TIMESTAMPING_OPT_ID is set. On datagram sockets, it returns the packet number since the reset. On stream sockets, it returns the byte offset since the reset. > Also, is it intentional that the payload data associated with the tx > timestamp is (I think) the full outgoing packet including lower-layer > headers? Absolutely not. I'll look into that right away. It doesn't on ACK, and should certainly not expose this info in the other cases, either. > And, finally, would it be possible to attach IP_PKTINFO to the looped > timestamp? That way I could finally update my fancy ping program to > track which outgoing interface was used for a request. If socket option IP_PKTINFO is set, you want to receive in_pktinfo for any packet that happens to be queued onto the error queue? Both SKB_EXT_ERR(skb) and PKTINFO_SKB_CB(skb) use the control block to store data that is later encoded in a cmsg, so there may not be enough room to hold both. I'll take a look. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 24, 2014 at 2:11 PM, Willem de Bruijn <willemb@google.com> wrote: > On Mon, Nov 24, 2014 at 3:02 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> SOF_TIMESTAMPING_OPT_ID puts the id in ee_data, not ee_info. >> >> Cc: Willem de Bruijn <willemb@google.com> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> > > Acked-by: Willem de Bruijn <willemb@google.com> > >> --- > > Thanks for sending a fix. > >> While I'm here, the docs say: >> >> In practice, it [ee_data] is a monotonically increasing u32 (that wraps). >> >> Is user code supposed to rely on this and, further, on the fact that the >> counter starts at zero? If not, how else is user code supposed to match >> outgoing data to timestamps? > > That is correct. The per-socket counter is reset when > SOF_TIMESTAMPING_OPT_ID is set. On datagram sockets, it returns the > packet number since the reset. On stream sockets, it returns the byte > offset since the reset. > It might be worth tweaking the docs at some point to make this clearer. >> Also, is it intentional that the payload data associated with the tx >> timestamp is (I think) the full outgoing packet including lower-layer >> headers? > > Absolutely not. I'll look into that right away. It doesn't on ACK, and > should certainly not expose this info in the other cases, either. Then I won't start trying to decode it :) TBH, all I looked at was the packet size, which matched the full link-layer packet. Also, the address returned by recvmsg appeared to be garbage instead of 0.0.0.0 (or something meaningful, whatever that would be). > >> And, finally, would it be possible to attach IP_PKTINFO to the looped >> timestamp? That way I could finally update my fancy ping program to >> track which outgoing interface was used for a request. > > If socket option IP_PKTINFO is set, you want to receive in_pktinfo for > any packet that happens to be queued onto the error queue? Both > SKB_EXT_ERR(skb) and PKTINFO_SKB_CB(skb) use the control block to > store data that is later encoded in a cmsg, so there may not be enough > room to hold both. I'll take a look. I don't really care what the mechanism is, but it would be really nice if I could see what interface the send timestamp is associated with. Also, thanks for this new feature. It's great! --Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Is user code supposed to rely on this and, further, on the fact that the >>> counter starts at zero? If not, how else is user code supposed to match >>> outgoing data to timestamps? >> >> That is correct. The per-socket counter is reset when >> SOF_TIMESTAMPING_OPT_ID is set. On datagram sockets, it returns the >> packet number since the reset. On stream sockets, it returns the byte >> offset since the reset. >> > > It might be worth tweaking the docs at some point to make this clearer. Good point. The commit message is apparently more informative than the actual documentation. >>> Also, is it intentional that the payload data associated with the tx >>> timestamp is (I think) the full outgoing packet including lower-layer >>> headers? >> >> Absolutely not. I'll look into that right away. It doesn't on ACK, and >> should certainly not expose this info in the other cases, either. > > Then I won't start trying to decode it :) The datagram feature existed before I added the counter and stream support, so returning the entire packet in that case is legacy behavior, I suppose. I did not intend to expose network headers for the new stream socket interface, though. > TBH, all I looked at was the packet size, which matched the full > link-layer packet. > > Also, the address returned by recvmsg appeared to be garbage instead > of 0.0.0.0 (or something meaningful, whatever that would be). > >> >>> And, finally, would it be possible to attach IP_PKTINFO to the looped >>> timestamp? That way I could finally update my fancy ping program to >>> track which outgoing interface was used for a request. >> >> If socket option IP_PKTINFO is set, you want to receive in_pktinfo for >> any packet that happens to be queued onto the error queue? Both >> SKB_EXT_ERR(skb) and PKTINFO_SKB_CB(skb) use the control block to >> store data that is later encoded in a cmsg, so there may not be enough >> room to hold both. I'll take a look. > > I don't really care what the mechanism is, but it would be really nice > if I could see what interface the send timestamp is associated with. Okay. I'll see if I can cook something up. > Also, thanks for this new feature. It's great! Thanks! > --Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 24, 2014 at 05:38:21PM -0500, Willem de Bruijn wrote: > The datagram feature existed before I added the counter and stream > support, so returning the entire packet in that case is legacy > behavior, I suppose. I did not intend to expose network headers for > the new stream socket interface, though. I suppose that idea (not mine) behind returning the entire frame was to allow the user to send multiple frames, and then figure out which time stamp goes with which sent frame. The whole thing is weak for two reasons, firstly you aren't able to send two identical frames, and secondly there are quite a few time stamping cards out there which only handle one transmit time stamp at a time. Thus the ptp4l application always waits for the hardware time stamp before sending another frame and never looks at the returned frame data. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I just sent an initial patch set for comments. On Mon, Nov 24, 2014 at 5:38 PM, Willem de Bruijn <willemb@google.com> wrote: >>>> Is user code supposed to rely on this and, further, on the fact that the >>>> counter starts at zero? If not, how else is user code supposed to match >>>> outgoing data to timestamps? >>> >>> That is correct. The per-socket counter is reset when >>> SOF_TIMESTAMPING_OPT_ID is set. On datagram sockets, it returns the >>> packet number since the reset. On stream sockets, it returns the byte >>> offset since the reset. >>> >> >> It might be worth tweaking the docs at some point to make this clearer. > > Good point. The commit message is apparently more informative than the > actual documentation. I did not update the documentation yet, as that would conflict with your patch, Andy. >>>> Also, is it intentional that the payload data associated with the tx >>>> timestamp is (I think) the full outgoing packet including lower-layer >>>> headers? >>> >>> Absolutely not. I'll look into that right away. It doesn't on ACK, and >>> should certainly not expose this info in the other cases, either. >> >> Then I won't start trying to decode it :) > > The datagram feature existed before I added the counter and stream > support, so returning the entire packet in that case is legacy > behavior, I suppose. I did not intend to expose network headers for > the new stream socket interface, though. Since the interface with headers is legacy and cannot be changed, you could actually use it. The counter + PKTINFO hopefully give the same information in a cleaner way. >> TBH, all I looked at was the packet size, which matched the full >> link-layer packet. >> >> Also, the address returned by recvmsg appeared to be garbage instead >> of 0.0.0.0 (or something meaningful, whatever that would be). >> >>> >>>> And, finally, would it be possible to attach IP_PKTINFO to the looped >>>> timestamp? That way I could finally update my fancy ping program to >>>> track which outgoing interface was used for a request. >>> >>> If socket option IP_PKTINFO is set, you want to receive in_pktinfo for >>> any packet that happens to be queued onto the error queue? Both >>> SKB_EXT_ERR(skb) and PKTINFO_SKB_CB(skb) use the control block to >>> store data that is later encoded in a cmsg, so there may not be enough >>> room to hold both. I'll take a look. >> >> I don't really care what the mechanism is, but it would be really nice >> if I could see what interface the send timestamp is associated with. > > Okay. I'll see if I can cook something up. I decided to make the feature independent from the timestamps. There may be other uses for finding out the egress device. The two metadata structures are looped back together with the same payload, though, so it still allows correlation of all fields. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andy Lutomirski <luto@amacapital.net> Date: Mon, 24 Nov 2014 12:02:29 -0800 > SOF_TIMESTAMPING_OPT_ID puts the id in ee_data, not ee_info. > > Cc: Willem de Bruijn <willemb@google.com> > Signed-off-by: Andy Lutomirski <luto@amacapital.net> Applied, thanks Andy. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/networking/timestamping.txt b/Documentation/networking/timestamping.txt index 412f45ca2d73..1d6d02d6ba52 100644 --- a/Documentation/networking/timestamping.txt +++ b/Documentation/networking/timestamping.txt @@ -136,7 +136,7 @@ SOF_TIMESTAMPING_OPT_ID: This option is implemented only for transmit timestamps. There, the timestamp is always looped along with a struct sock_extended_err. - The option modifies field ee_info to pass an id that is unique + The option modifies field ee_data to pass an id that is unique among all possibly concurrently outstanding timestamp requests for that socket. In practice, it is a monotonically increasing u32 (that wraps).
SOF_TIMESTAMPING_OPT_ID puts the id in ee_data, not ee_info. Cc: Willem de Bruijn <willemb@google.com> Signed-off-by: Andy Lutomirski <luto@amacapital.net> --- While I'm here, the docs say: In practice, it [ee_data] is a monotonically increasing u32 (that wraps). Is user code supposed to rely on this and, further, on the fact that the counter starts at zero? If not, how else is user code supposed to match outgoing data to timestamps? Also, is it intentional that the payload data associated with the tx timestamp is (I think) the full outgoing packet including lower-layer headers? And, finally, would it be possible to attach IP_PKTINFO to the looped timestamp? That way I could finally update my fancy ping program to track which outgoing interface was used for a request. Documentation/networking/timestamping.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)