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

From: "Y.b. Lu" <yangbo.lu@nxp.com>
To: Jakub Kicinski <kuba@kernel.org>,
	Claudiu Manoil <claudiu.manoil@gmail.com>
Cc: Claudiu Manoil <claudiu.manoil@nxp.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Richard Cochran <richardcochran@gmail.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: Mon, 12 Apr 2021 09:15:14 +0000	[thread overview]
Message-ID: <AM7PR04MB6885C05634FCE45B0CE0DEBBF8709@AM7PR04MB6885.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20210409124412.3728a224@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

Hi Jakub and Claudiu,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2021年4月10日 3:44
> To: Claudiu Manoil <claudiu.manoil@gmail.com>
> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Y.b. Lu <yangbo.lu@nxp.com>;
> netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Richard
> Cochran <richardcochran@gmail.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
> 
> On Fri, 9 Apr 2021 22:32:49 +0300 Claudiu Manoil wrote:
> > On 09.04.2021 19:09, Jakub Kicinski wrote:
> > > On Fri, 9 Apr 2021 06:37:53 +0000 Claudiu Manoil wrote:
> > >> Please try test_and_set_bit_lock()/ clear_bit_unlock() based on
> > >> Jakub's suggestion, and see if it works for you / whether it can replace the
> mutex.
> > >
> > > I was thinking that with multiple queues just a bit won't be
> > > sufficient
> > > because:
> > >
> > > xmit:				work:
> > > test_bit... // already set
> > > 				dequeue // empty
> > > enqueue
> > > 				clear_bit()
> > >
> > > That frame will never get sent, no?
> >
> > I don't see any issue with Yangbo's initial design actually, I was
> > just suggesting him to replace the mutex with a bit lock, based on your
> comments.
> > That means:
> > xmit:		work:				clean_tx_ring: //Tx conf
> > skb_queue_tail()
> > 		skb_dequeue()
> > 		test_and_set_bit_lock()
> > 						clear_bit_unlock()
> >
> > The skb queue is one per device, as it needs to serialize ptp skbs for
> > that device (due to the restriction that a ptp packet cannot be
> > enqueued for transmission if there's another ptp packet waiting for
> > transmission in a h/w descriptor ring).
> >
> > If multiple ptp skbs are coming in from different xmit queues at the
> > same time (same device), they are enqueued in the common priv->tx_skbs
> > queue (skb_queue_tail() is protected by locks), and the worker thread
> > is started.
> > The worker dequeues the first ptp skb, and places the packet in the
> > h/w descriptor ring for transmission. Then dequeues the second skb and
> > waits at the lock (or mutex or whatever lock is preferred).
> > Upon transmission of the ptp packet the lock is released by the Tx
> > confirmation napi thread (clean_tx_ring()) and the next PTP skb can be
> > placed in the corresponding descriptor ring for transmission by the
> > worker thread.
> 
> I see. I thought you were commenting on my scheme. Yes, if only the worker is
> allowed to send there is no race, that should work well.
> 
> In my suggestion I was trying to allow the first frame to be sent directly
> without going via the queue and requiring the worker to be scheduled in.
> 
> > So the way I understood your comments is that you'd rather use a spin
> > lock in the worker thread instead of a mutex.
> 
> Not exactly, my main objection was that the mutex was used for wake up.
> Worker locks it, completion path unlocks it.
> 
> Your suggestion of using a bit works well. Just Instead of a loop the worker
> needs to send a single skb, and completion needs to schedule it again.

Thank you so much for your suggestion and guide. So I used bit lock, and also followed Jakub's suggestion.
The difference was, I moved the flag cleaning before transmitting single skb from skb queue.
Otherwise, skb in queue would never be transmitted in start_xmit().
Please help to review v3 I sent:)

work:

	netif_tx_lock()
    clear_bit_unlock(); // put cleaning here
	skb = skb_dequeue();
	if (skb)
		start_xmit(skb)
//	else
//		priv->flags &= ~ONESTEP_BUSY;
	netif_tx_unlock()

Thanks.

> 
> > > Note that skb_queue already has a lock so you'd just need to make
> > > that lock protect the flag/bit as well, overall the number of locks
> > > remains the same. Take the queue's lock, check the flag, use
> > > __skb_queue_tail(), release etc.
> >
> > This is a good optimization idea indeed, to use the priv->tx_skb skb
> > list's spin lock, instead of adding another lock.