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

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

Willem de Bruijn wrote:
> 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?

Let me ask differently: why not queue the tx timestamp directly in
that first branch?