Re: [PATCH v3 net-next 2/3] netdevsim: Implement basic ptp support

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Maciek Machnikowski wrote:
> Add support for virtual timestamping inside the netdevsim driver.
> The implementation uses two attached ptp_mock clocks, reads the timestamps
> of the ones attached either to the netdevsim or its peer and returns
> timestamps using standard timestamps APIs.
> 
> This implementation enables running ptp4l on netdevsim adapters and
> introduces a new ptp selftest.
> 
> Co-developed-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Milena Olech <milena.olech@intel.com>
> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
> ---
>  drivers/net/netdevsim/ethtool.c   | 11 ++++
>  drivers/net/netdevsim/netdev.c    | 90 +++++++++++++++++++++++++++++++
>  drivers/net/netdevsim/netdevsim.h |  1 +
>  3 files changed, 102 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
> index 36a201533..5b709033c 100644
> --- a/drivers/net/netdevsim/ethtool.c
> +++ b/drivers/net/netdevsim/ethtool.c
> @@ -200,7 +200,18 @@ static int nsim_get_ts_info(struct net_device *dev,
>  {
>  	struct netdevsim *ns = netdev_priv(dev);
>  
> +	ethtool_op_get_ts_info(dev, info);
> +
>  	info->phc_index = mock_phc_index(ns->phc);
> +	if (info->phc_index < 0)
> +		return 0;
> +
> +	info->so_timestamping |= SOF_TIMESTAMPING_TX_HARDWARE |
> +				 SOF_TIMESTAMPING_RX_HARDWARE |
> +				 SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +	info->tx_types = BIT(HWTSTAMP_TX_OFF) | BIT(HWTSTAMP_TX_ON);
> +	info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) | BIT(HWTSTAMP_FILTER_ALL);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 6285fbefe..b1161c1ca 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -30,6 +30,8 @@
>  #include <net/rtnetlink.h>
>  #include <net/udp_tunnel.h>
>  #include <net/busy_poll.h>
> +#include <linux/ptp_clock_kernel.h>
> +#include <linux/timecounter.h>
>  
>  #include "netdevsim.h"
>  
> @@ -119,12 +121,17 @@ static int nsim_forward_skb(struct net_device *tx_dev,
>  
>  static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
> +	struct skb_shared_hwtstamps shhwtstamps = {};
>  	struct netdevsim *ns = netdev_priv(dev);
> +	struct ptp_clock_info *ptp_info;
> +	struct timespec64 tx_ts, rx_ts;
> +	struct sk_buff *skb_orig = skb;
>  	struct skb_ext *psp_ext = NULL;
>  	struct net_device *peer_dev;
>  	unsigned int len = skb->len;
>  	struct netdevsim *peer_ns;
>  	struct netdev_config *cfg;
> +	bool gen_tx_tstamp = false;
>  	struct nsim_rq *rq;
>  	int rxq;
>  	int dr;
> @@ -161,6 +168,27 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		skb_linearize(skb);
>  
>  	skb_tx_timestamp(skb);
> +	gen_tx_tstamp = skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP;
> +	if (gen_tx_tstamp) {
> +		ptp_info = mock_phc_get_ptp_info(ns->phc);
> +
> +		/* Create a copy of tx skb to keep the tx reference */
> +		skb_orig = skb;
> +		skb = skb_copy(skb_orig, GFP_ATOMIC);
> +		skb_shinfo(skb_orig)->tx_flags |= SKBTX_IN_PROGRESS;
> +
> +		/* Timestamp as late as possible */
> +		if (ptp_info)
> +			ptp_info->gettime64(ptp_info, &tx_ts);
> +	}
> +
> +	/* Generate Rx tstamp based on the peer clock */
> +	ptp_info = mock_phc_get_ptp_info(peer_ns->phc);
> +	if (ptp_info) {
> +		ptp_info->gettime64(ptp_info, &rx_ts);
> +		skb_hwtstamps(skb)->hwtstamp = timespec64_to_ktime(rx_ts);
> +	}
> +
>  	if (unlikely(nsim_forward_skb(dev, peer_dev,
>  				      skb, rq, psp_ext) == NET_RX_DROP))
>  		goto out_drop_cnt;
> @@ -168,6 +196,13 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	if (!hrtimer_active(&rq->napi_timer))
>  		hrtimer_start(&rq->napi_timer, us_to_ktime(5), HRTIMER_MODE_REL);
>  
> +	/* only timestamp the outbound packet if user requested it */
> +	if (gen_tx_tstamp) {
> +		shhwtstamps.hwtstamp = timespec64_to_ktime(tx_ts);
> +		skb_tstamp_tx(skb_orig, &shhwtstamps);
> +		dev_kfree_skb_any(skb_orig);
> +	}
> +

I still don't understand this tx timestamp path, which is
non-standard.

The comment says timestamp as late as possible, which would be calling
gettime64 in the second branch, rather than the first. That would mean
that the tx timestamp is taken after the rx timestamp at the peer.
Confusing, but something that may actually happen on real NICs too in
some cases (e.g., PCI backpressure).

If goal is to maintain ordering between the two, remove the comment
about as late as possible.

More importantly, since skb_tstamp_tx internally will call alloc_skb
or skb_clone, why is this skb_copy here needed?

I probably should have replied to your previous response:

> If I queue the tx timestamp before forwarding - the same skb would
> be freed when the timestamp is delivered.

That is not the case since it is a clone or separate skb that is
queued to the error queue?