Re: [PATCH v4 net-next 6/7] net: allow simultaneous SW and HW transmit timestamping

[Willem de Bruijn]

On Fri, May 19, 2017 at 6:00 AM, Miroslav Lichvar  wrote:
> On Thu, May 18, 2017 at 04:16:26PM -0400, Willem de Bruijn wrote:
>> On Thu, May 18, 2017 at 9:06 AM, Miroslav Lichvar  
>> wrote:
>> > +/* On transmit, software and hardware timestamps are returned 
>> > independently.
>> > + * As the two skb clones share the hardware timestamp, which may be 
>> > updated
>> > + * before the software timestamp is received, a hardware TX timestamp may 
>> > be
>> > + * returned only if there is no software TX timestamp. A false software
>> > + * timestamp made for SOCK_RCVTSTAMP when a real timestamp is missing must
>> > + * be ignored.
>>
>> Please expand on why this case can be ignored. It is quite subtle. How about
>> something like
>>
>> *
>> * A false software timestamp is one made inside the __sock_recv_timestamp
>> * call itself. These are generated whenever SO_TIMESTAMP(NS) is enabled
>> * on the socket, even when the timestamp reported is for another option, such
>> * as hardware tx timestamp.
>> *
>> * Ignore these when deciding whether a timestamp source is hw or sw.
>> */
>
> That seems a bit too verbose to me. :) Would the following work?
>
> /* On transmit, software and hardware timestamps are returned independently.
>  * As the two skb clones share the hardware timestamp, which may be updated
>  * before the software timestamp is received, a hardware TX timestamp may be
>  * returned only if there is no software TX timestamp. Ignore false software
>  * timestamps, which may be made in the __sock_recv_timestamp() call when the
>  * option SO_TIMESTAMP(NS) is enabled on the socket, even when the skb has a
>  * hardware timestamp.
>  */

Looks great, thanks.

>
>> > +static bool skb_is_swtx_tstamp(const struct sk_buff *skb,
>> > +  const struct sock *sk, int false_tstamp)
>> > +{
>> > +   if (false_tstamp && sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)
>>
>> Also, why is it ignored only for the new mode?
>
> Good point. That should not be there. The function can be now reduced
> to a single line again. I originally tried a different approach,
> disabling false timestamps in the new mode, but then I thought it's
> better to not complicate it unnecessarily and keep it consistent.
>
> --
> Miroslav Lichvar

Re: [PATCH v4 net-next 6/7] net: allow simultaneous SW and HW transmit timestamping

[Miroslav Lichvar]

On Thu, May 18, 2017 at 04:16:26PM -0400, Willem de Bruijn wrote:
> On Thu, May 18, 2017 at 9:06 AM, Miroslav Lichvar  wrote:
> > +/* On transmit, software and hardware timestamps are returned 
> > independently.
> > + * As the two skb clones share the hardware timestamp, which may be updated
> > + * before the software timestamp is received, a hardware TX timestamp may 
> > be
> > + * returned only if there is no software TX timestamp. A false software
> > + * timestamp made for SOCK_RCVTSTAMP when a real timestamp is missing must
> > + * be ignored.
> 
> Please expand on why this case can be ignored. It is quite subtle. How about
> something like
> 
> *
> * A false software timestamp is one made inside the __sock_recv_timestamp
> * call itself. These are generated whenever SO_TIMESTAMP(NS) is enabled
> * on the socket, even when the timestamp reported is for another option, such
> * as hardware tx timestamp.
> *
> * Ignore these when deciding whether a timestamp source is hw or sw.
> */

That seems a bit too verbose to me. :) Would the following work?

/* On transmit, software and hardware timestamps are returned independently.
 * As the two skb clones share the hardware timestamp, which may be updated
 * before the software timestamp is received, a hardware TX timestamp may be
 * returned only if there is no software TX timestamp. Ignore false software
 * timestamps, which may be made in the __sock_recv_timestamp() call when the
 * option SO_TIMESTAMP(NS) is enabled on the socket, even when the skb has a
 * hardware timestamp.
 */

> > +static bool skb_is_swtx_tstamp(const struct sk_buff *skb,
> > +  const struct sock *sk, int false_tstamp)
> > +{
> > +   if (false_tstamp && sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)
> 
> Also, why is it ignored only for the new mode?

Good point. That should not be there. The function can be now reduced
to a single line again. I originally tried a different approach,
disabling false timestamps in the new mode, but then I thought it's
better to not complicate it unnecessarily and keep it consistent.

-- 
Miroslav Lichvar

Re: [PATCH v4 net-next 6/7] net: allow simultaneous SW and HW transmit timestamping

[Willem de Bruijn]

On Thu, May 18, 2017 at 9:06 AM, Miroslav Lichvar  wrote:
> Add SOF_TIMESTAMPING_OPT_TX_SWHW option to allow an outgoing packet to
> be looped to the socket's error queue with a software timestamp even
> when a hardware transmit timestamp is expected to be provided by the
> driver.
>
> Applications using this option will receive two separate messages from
> the error queue, one with a software timestamp and the other with a
> hardware timestamp. As the hardware timestamp is saved to the shared skb
> info, which may happen before the first message with software timestamp
> is received by the application, the hardware timestamp is copied to the
> SCM_TIMESTAMPING control message only when the skb has no software
> timestamp or it is an incoming packet.
>
> While changing sw_tx_timestamp(), inline it in skb_tx_timestamp() as
> there are no other users.
>
> CC: Richard Cochran 
> CC: Willem de Bruijn 
> Signed-off-by: Miroslav Lichvar 
> ---

> +/* On transmit, software and hardware timestamps are returned independently.
> + * As the two skb clones share the hardware timestamp, which may be updated
> + * before the software timestamp is received, a hardware TX timestamp may be
> + * returned only if there is no software TX timestamp. A false software
> + * timestamp made for SOCK_RCVTSTAMP when a real timestamp is missing must
> + * be ignored.

Please expand on why this case can be ignored. It is quite subtle. How about
something like

*
* A false software timestamp is one made inside the __sock_recv_timestamp
* call itself. These are generated whenever SO_TIMESTAMP(NS) is enabled
* on the socket, even when the timestamp reported is for another option, such
* as hardware tx timestamp.
*
* Ignore these when deciding whether a timestamp source is hw or sw.
*/

And perhaps move the comment to the branch itself.

> + */
> +static bool skb_is_swtx_tstamp(const struct sk_buff *skb,
> +  const struct sock *sk, int false_tstamp)
> +{
> +   if (false_tstamp && sk->sk_tsflags & SOF_TIMESTAMPING_OPT_TX_SWHW)

Also, why is it ignored only for the new mode?

> +   return 0;
> +
> +   return skb->tstamp && skb_is_err_queue(skb);
> +}