[PATCH] net-timestamp: Fix a documentation typo

[PATCH] net-timestamp: Fix a documentation typo

[Andy Lutomirski]

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(-)

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).
-- 
1.9.3

Re: [PATCH] net-timestamp: Fix a documentation typo

[Willem de Bruijn]

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.

Re: [PATCH] net-timestamp: Fix a documentation typo

[Andy Lutomirski]

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

Re: [PATCH] net-timestamp: Fix a documentation typo

[Willem de Bruijn]

>>> 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

Re: [PATCH] net-timestamp: Fix a documentation typo

[Richard Cochran]

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

Re: [PATCH] net-timestamp: Fix a documentation typo

[Willem de Bruijn]

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.

Re: [PATCH] net-timestamp: Fix a documentation typo

[David Miller]

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.