| Message ID | 20170516124425.6294-6-mlichvar@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 16, 2017 at 8:44 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote: > If software timestamping is enabled by the SO_TIMESTAMP(NS) option > when a message without timestamp is already waiting in the queue, the > __sock_recv_timestamp() function will read the current time to make a > timestamp in order to always have something for the application. > > However, this applies also to outgoing packets looped back to the error > queue when hardware timestamping is enabled by the SO_TIMESTAMPING > option. This is already the case for sockets that have both software receive timestamps and hardware tx timestamps enabled, independent from the new option SOF_TIMESTAMPING_OPT_TX_SWHW, right? If so, then this behavior must remain.
On Tue, May 16, 2017 at 06:34:38PM -0400, Willem de Bruijn wrote: > On Tue, May 16, 2017 at 8:44 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote: > > If software timestamping is enabled by the SO_TIMESTAMP(NS) option > > when a message without timestamp is already waiting in the queue, the > > __sock_recv_timestamp() function will read the current time to make a > > timestamp in order to always have something for the application. > > > > However, this applies also to outgoing packets looped back to the error > > queue when hardware timestamping is enabled by the SO_TIMESTAMPING > > option. > > This is already the case for sockets that have both software receive > timestamps and hardware tx timestamps enabled, independent from > the new option SOF_TIMESTAMPING_OPT_TX_SWHW, right? If so, > then this behavior must remain. Even if we consider that it's not actually returning a valid TX timestamp and it didn't behave as documented ("Only one field is non-zero at any time")? On the RX side this timestamp does make some sense as it could be viewed as a very late timestamp, correctly ordered after the HW timestamp, but on the TX side the order is reversed and returning a timestamp later than the actual transmission might break a protocol. If you don't see it as a bug fix, I think this weird interaction between the SO_TIMESTAMPING(NS) and SO_TIMESTAMPING options needs to be documented.
On Wed, May 17, 2017 at 6:27 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote: > On Tue, May 16, 2017 at 06:34:38PM -0400, Willem de Bruijn wrote: >> On Tue, May 16, 2017 at 8:44 AM, Miroslav Lichvar <mlichvar@redhat.com> wrote: >> > If software timestamping is enabled by the SO_TIMESTAMP(NS) option >> > when a message without timestamp is already waiting in the queue, the >> > __sock_recv_timestamp() function will read the current time to make a >> > timestamp in order to always have something for the application. >> > >> > However, this applies also to outgoing packets looped back to the error >> > queue when hardware timestamping is enabled by the SO_TIMESTAMPING >> > option. >> >> This is already the case for sockets that have both software receive >> timestamps and hardware tx timestamps enabled, independent from >> the new option SOF_TIMESTAMPING_OPT_TX_SWHW, right? If so, >> then this behavior must remain. > > Even if we consider that it's not actually returning a valid TX > timestamp and it didn't behave as documented ("Only one field is > non-zero at any time")? Even if it is not a very useful timestamp. Applications may read it nonetheless. I think that the documentation is newer than the feature, and wrong in this case. > On the RX side this timestamp does make some sense as it could be > viewed as a very late timestamp, correctly ordered after the HW > timestamp, Actually, on Rx it is equally problematic. It can easily generate out of order timestamps. When enabling timestamping, the first packets will have a timestamp generated at recvmsg while the following have one generated at __netif_receive_skb_core. > but on the TX side the order is reversed and returning a > timestamp later than the actual transmission might break a protocol. > > If you don't see it as a bug fix, I think this weird interaction > between the SO_TIMESTAMPING(NS) and SO_TIMESTAMPING options needs to > be documented. I agree. I don't think that it's all that useful, but it is established behavior.
diff --git a/net/socket.c b/net/socket.c index ee1f4ec..879df37 100644 --- a/net/socket.c +++ b/net/socket.c @@ -689,7 +689,8 @@ static void put_ts_pktinfo(struct msghdr *msg, struct sk_buff *skb) void __sock_recv_timestamp(struct msghdr *msg, struct sock *sk, struct sk_buff *skb) { - int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP); + int need_software_tstamp = sock_flag(sk, SOCK_RCVTSTAMP) && + !skb_is_err_queue(skb); struct scm_timestamping tss; int empty = 1; struct skb_shared_hwtstamps *shhwtstamps =
If software timestamping is enabled by the SO_TIMESTAMP(NS) option when a message without timestamp is already waiting in the queue, the __sock_recv_timestamp() function will read the current time to make a timestamp in order to always have something for the application. However, this applies also to outgoing packets looped back to the error queue when hardware timestamping is enabled by the SO_TIMESTAMPING option. A software transmit timestamp made after the actual transmission is added to messages with hardware timestamps. Modify the function to save the current time as a software timestamp only if it's for a received packet (i.e. it's not in the error queue). CC: Richard Cochran <richardcochran@gmail.com> CC: Willem de Bruijn <willemb@google.com> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com> --- net/socket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)