Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping

From: Jakub Kicinski <kuba@kernel.org>
To: Yangbo Lu <yangbo.lu@nxp.com>
Cc: netdev@vger.kernel.org, "David S . Miller" <davem@davemloft.net>,
	Richard Cochran <richardcochran@gmail.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Vladimir Oltean <vladimir.oltean@nxp.com>,
	Russell King <linux@armlinux.org.uk>
Subject: Re: [net-next, v2, 2/2] enetc: support PTP Sync packet one-step timestamping
Date: Thu, 8 Apr 2021 09:02:50 -0700	[thread overview]
Message-ID: <20210408090250.21dee5c6@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210408111350.3817-3-yangbo.lu@nxp.com>

On Thu,  8 Apr 2021 19:13:50 +0800 Yangbo Lu wrote:
> This patch is to add support for PTP Sync packet one-step timestamping.
> Since ENETC single-step register has to be configured dynamically per
> packet for correctionField offeset and UDP checksum update, current
> one-step timestamping packet has to be sent only when the last one
> completes transmitting on hardware. So, on the TX below things are done
> by the patch:
> 
> - For one-step timestamping packet, queue to skb queue.
> - Start a work to transmit skbs in queue.
> - For other skbs, transmit immediately.
> - mutex lock used to ensure the last one-step timestamping packet has
>   already been transmitted on hardware before transmitting current one.
> 
> And the configuration for one-step timestamping on ENETC before
> transmitting is,
> 
> - Set one-step timestamping flag in extension BD.
> - Write 30 bits current timestamp in tstamp field of extension BD.
> - Update PTP Sync packet originTimestamp field with current timestamp.
> - Configure single-step register for correctionField offeset and UDP
>   checksum update.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>

> @@ -432,9 +544,12 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)
>  			xdp_return_frame(xdp_frame);
>  			tx_swbd->xdp_frame = NULL;
>  		} else if (skb) {
> -			if (unlikely(do_tstamp)) {
> +			if (unlikely(tx_swbd->skb->cb[0] &
> +				     ENETC_F_TX_ONESTEP_SYNC_TSTAMP)) {
> +				mutex_unlock(&priv->onestep_tstamp_lock);
> +			} else if (unlikely(do_twostep_tstamp)) {
>  				enetc_tstamp_tx(skb, tstamp);
> -				do_tstamp = false;
> +				do_twostep_tstamp = false;
>  			}
>  			napi_consume_skb(skb, napi_budget);
>  			tx_swbd->skb = NULL;
> @@ -1863,6 +1978,47 @@ static int enetc_phylink_connect(struct net_device *ndev)
>  	return 0;
>  }
>  
> +static void enetc_tx_onestep_tstamp(struct work_struct *work)
> +{
> +	struct enetc_ndev_priv *priv;
> +	struct sk_buff *skb;
> +
> +	priv = container_of(work, struct enetc_ndev_priv, tx_onestep_tstamp);
> +
> +	while (true) {
> +		skb = skb_dequeue(&priv->tx_skbs);
> +		if (!skb)
> +			return;
> +
> +		/* Lock before TX one-step timestamping packet, and release
> +		 * when the packet has been sent on hardware, or transmit
> +		 * failure.
> +		 */
> +		mutex_lock(&priv->onestep_tstamp_lock);

Using a lock to wake up a producer is not a great idea. It usually
breaks advanced features like priority inheritance. Probably doesn't
matter for a struct mutex, but I think it may still make lockdep
complain.

Why not make it work with a flag?

start_xmit:

	if (skb->cb[0] & ONESTEP) {
		if (priv->flags & ONESTEP_BUSY) {
			skb_queue_tail(&priv->tx_skbs, skb);
			return ...;
		}
		priv->flags |= ONESTEP_BUSY;
	}

clean_tx:

	/* don't clear ONESTEP_BUSY, we need the tx lock */
	if (skb->cb[0] & ONESTEP)
		queue_work(...);

work:

	netif_tx_lock()
	skb = skb_dequeue();
	if (skb)
		start_xmit(skb)
	else
		priv->flags &= ~ONESTEP_BUSY;
	netif_tx_unlock()

> +		netif_tx_lock(priv->ndev);
> +		enetc_start_xmit(skb, priv->ndev);
> +		netif_tx_unlock(priv->ndev);
> +	}
> +}
> +
> +static int enetc_tx_onestep_tstamp_init(struct enetc_ndev_priv *priv)
> +{
> +	priv->enetc_ptp_wq = alloc_workqueue("enetc_ptp_wq", 0, 0);
> +	if (!priv->enetc_ptp_wq)
> +		return -ENOMEM;
> +
> +	INIT_WORK(&priv->tx_onestep_tstamp, enetc_tx_onestep_tstamp);
> +	skb_queue_head_init(&priv->tx_skbs);
> +
> +	return 0;
> +}
> +
> +static void enetc_tx_onestep_tstamp_deinit(struct enetc_ndev_priv *priv)
> +{
> +	destroy_workqueue(priv->enetc_ptp_wq);
> +}

Why allocate a separate workqueue for one work? You can queue your
work on the system workqueue.