[PATCH v3 0/1] Add software TX timestamps to the CAN devices

[PATCH v3 0/1] Add software TX timestamps to the CAN devices

[Vincent Mailhol]

With the ongoing work to add BQL to Socket CAN, I figured out that it
would be nice to have an easy way to mesure the latency.

And one easy way to do so it to check the round trip time of the
packet by doing the difference between the software rx timestamp and
the software tx timestamp.

rx timestamps are already available. This patch gives the missing
piece: add a tx software timestamp feature to the CAN devices.

Of course, the tx software timestamp might also be used for other
purposes such as performance measurements of the different queuing
disciplines (e.g. by checking the difference between the kernel tx
software timestamp and the userland tx software timestamp).

v2 was a mistake, please ignore it (fogot to do git add, changes were
not reflected...)

v3 reflects the comments that Jeroen made in
https://lkml.org/lkml/2021/1/10/54

Vincent Mailhol (1):
  can: dev: add software tx timestamps

 drivers/net/can/dev.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.26.2

[PATCH v3 1/1] can: dev: add software tx timestamps

[Vincent Mailhol]

Call skb_tx_timestamp() within can_put_echo_skb() so that a software
tx timestamp gets attached on the skb.

There two main reasons to include this call in can_put_echo_skb():

  * It easily allow to enable the tx timestamp on all devices with
    just one small change.

  * According to Documentation/networking/timestamping.rst, the tx
    timestamps should be generated in the device driver as close as
    possible, but always prior to passing the packet to the network
    interface. During the call to can_put_echo_skb(), the skb gets
    cloned meaning that the driver should not dereference the skb
    variable anymore after can_put_echo_skb() returns. This makes
    can_put_echo_skb() the very last place we can use the skb without
    having to access the echo_skb[] array.

Remarks:

  * By default, skb_tx_timestamp() does nothing. It needs to be
    activated by passing the SOF_TIMESTAMPING_TX_SOFTWARE flag either
    through socket options or control messages.

  * The hardware rx timestamp of a local loopback message is the
    hardware tx timestamp. This means that there are no needs to
    implement SOF_TIMESTAMPING_TX_HARDWARE for CAN sockets.

References:

Support for the error queue in CAN RAW sockets (which is needed for tx
timestamps) was introduced in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eb88531bdbfaafb827192d1fc6c5a3fcc4fadd96

Put the call to skb_tx_timestamp() just before adding it to the array:
https://lkml.org/lkml/2021/1/10/54

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3486704c8a95..850759c7677f 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -481,6 +481,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		skb->pkt_type = PACKET_BROADCAST;
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 		skb->dev = dev;
+		skb_tx_timestamp(skb);
 
 		/* save this skb for tx interrupt echo handling */
 		priv->echo_skb[idx] = skb;
-- 
2.26.2

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2099 bytes --]

On 1/10/21 1:49 PM, Vincent Mailhol wrote:
> Call skb_tx_timestamp() within can_put_echo_skb() so that a software
> tx timestamp gets attached on the skb.
> 
> There two main reasons to include this call in can_put_echo_skb():
> 
>   * It easily allow to enable the tx timestamp on all devices with
>     just one small change.
> 
>   * According to Documentation/networking/timestamping.rst, the tx
>     timestamps should be generated in the device driver as close as
>     possible, but always prior to passing the packet to the network
>     interface. During the call to can_put_echo_skb(), the skb gets
>     cloned meaning that the driver should not dereference the skb
>     variable anymore after can_put_echo_skb() returns. This makes
>     can_put_echo_skb() the very last place we can use the skb without
>     having to access the echo_skb[] array.
> 
> Remarks:
> 
>   * By default, skb_tx_timestamp() does nothing. It needs to be
>     activated by passing the SOF_TIMESTAMPING_TX_SOFTWARE flag either
>     through socket options or control messages.
> 
>   * The hardware rx timestamp of a local loopback message is the
>     hardware tx timestamp. This means that there are no needs to
>     implement SOF_TIMESTAMPING_TX_HARDWARE for CAN sockets.
> 
> References:
> 
> Support for the error queue in CAN RAW sockets (which is needed for tx
> timestamps) was introduced in:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eb88531bdbfaafb827192d1fc6c5a3fcc4fadd96
> 
> Put the call to skb_tx_timestamp() just before adding it to the array:
> https://lkml.org/lkml/2021/1/10/54
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Applied to linux-can-next/testing (ontop of my dev infrastructure cleanup).

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Richard Cochran]

On Sun, Jan 10, 2021 at 09:49:03PM +0900, Vincent Mailhol wrote:
>   * The hardware rx timestamp of a local loopback message is the
>     hardware tx timestamp. This means that there are no needs to
>     implement SOF_TIMESTAMPING_TX_HARDWARE for CAN sockets.

I can't agree with that statement.  The local loopback is a special
"feature" of CAN sockets, and some programs turn it off.  Furthermore,
requiring user space to handle CAN sockets differently WRT Tx time
stamps is user-unfriendly.  So I would strongly support adding
SOF_TIMESTAMPING_TX_HARDWARE to the CAN layer in the future.

(This isn't a criticism of the current patch, though.)

Thanks,
Richard

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Vincent MAILHOL]

On Tue. 12 Jan 2021 at 02:11, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Sun, Jan 10, 2021 at 09:49:03PM +0900, Vincent Mailhol wrote:
> >   * The hardware rx timestamp of a local loopback message is the
> >     hardware tx timestamp. This means that there are no needs to
> >     implement SOF_TIMESTAMPING_TX_HARDWARE for CAN sockets.
>
> I can't agree with that statement.  The local loopback is a special
> "feature" of CAN sockets, and some programs turn it off.  Furthermore,
> requiring user space to handle CAN sockets differently WRT Tx time
> stamps is user-unfriendly.  So I would strongly support adding
> SOF_TIMESTAMPING_TX_HARDWARE to the CAN layer in the future.
>
> (This isn't a criticism of the current patch, though.)

Fair enough. Implementing SOF_TIMESTAMPING_TX_HARDWARE would
result into having the timestamp being duplicated for the
loopback frames but allowing existing programs to work as
with no modifications is a good enough reason.

Out of curiosity, which programs do you use? I guess wireshark
but please let me know if you use any other programs (I just use
to write a small C program to do the stuff).

Mark: do you want me to send a v4 of that patch with above
comment removed or can you directly do the change in your testing
branch?


Yours sincerely,
Vincent

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Richard Cochran]

On Tue, Jan 12, 2021 at 09:00:33AM +0900, Vincent MAILHOL wrote:
> Out of curiosity, which programs do you use? I guess wireshark
> but please let me know if you use any other programs (I just use
> to write a small C program to do the stuff).

I was thinking of PTP over DeviceNET (which, in turn, is over CAN).
This is specified in Annex G of IEEE 1588.

The linuxptp stack has modular design and could one day support
DeviceNET.  It would be much easier for linuxptp if CAN interfaces
support hardware time stamping in the same way as other network
interfaces.

Thanks,
Richard

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Vincent MAILHOL]

On Tue. 12 Jan 2021 at 11:14, Richard Cochran <richardcochran@gmail.com> wrote:
>
> On Tue, Jan 12, 2021 at 09:00:33AM +0900, Vincent MAILHOL wrote:
> > Out of curiosity, which programs do you use? I guess wireshark
> > but please let me know if you use any other programs (I just use
> > to write a small C program to do the stuff).
>
> I was thinking of PTP over DeviceNET (which, in turn, is over CAN).
> This is specified in Annex G of IEEE 1588.
>
> The linuxptp stack has modular design and could one day support
> DeviceNET.  It would be much easier for linuxptp if CAN interfaces
> support hardware time stamping in the same way as other network
> interfaces.

I actually also thought of implementing PTP but for a slightly
different goal: synchronise the clock of the different CAN
controllers connected to a same Linux host. But so far, it is
just a rough idea and I never looked really deep into the
technical details of linuxptp.

However, I did not know about DeviceNET. I am not really aware of
the use cases for industrial automation applications.

Glad to have asked, learnt something :)


Yours sincerely,
Vincent

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 532 bytes --]

On 1/12/21 1:00 AM, Vincent MAILHOL wrote:
[...]

> Mark: do you want me to send a v4 of that patch with above
> comment removed or can you directly do the change in your testing
> branch?

Please send a patch on-top of linux-can-next/testing

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] can: dev: add software tx timestamps

[Vincent MAILHOL]

On Tue. 12 Jan 2021 at 16:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 1/12/21 1:00 AM, Vincent MAILHOL wrote:
> [...]
>
> > Mark: do you want me to send a v4 of that patch with above
> > comment removed or can you directly do the change in your testing
> > branch?
>
> Please send a patch on-top of linux-can-next/testing

Done: https://lore.kernel.org/linux-can/20210112095437.6488-1-mailhol.vincent@wanadoo.fr/


Yours sincerely,
Vincent