Message-ID: <20201204122613.542c2362@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Date:   Fri, 4 Dec 2020 12:26:13 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Eran Ben Elisha <eranbe@...dia.com>,
        Tariq Toukan <tariqt@...dia.com>,
        Richard Cochran <richardcochran@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support

On Fri, 04 Dec 2020 11:33:26 -0800 Saeed Mahameed wrote:
> On Thu, 2020-12-03 at 18:29 -0800, Jakub Kicinski wrote:
> > On Wed, 2 Dec 2020 20:21:01 -0800 Saeed Mahameed wrote:  
> > > Add TX PTP port object support for better TX timestamping accuracy.
> > > Currently, driver supports CQE based TX port timestamp. Device
> > > also offers TX port timestamp, which has less jitter and better
> > > reflects the actual time of a packet's transmit.  
> > 
> > How much better is it?
> > 
> > Is the new implementation is standard compliant or just a "better
> > guess"?
> 
> It is not a guess for sure, the closer to the output port you take the
> stamp the more accurate you get, this is why we need the HW timestamp
> in first place, i don't have the exact number though, but we target to
> be compliant with G.8273.2 class C, (30 nsec), and this code allow
> Linux systems to be deployed in the 5G telco edge. Where this standard
> is needed.

I see. IIRC there was also an IEEE standard which specified the exact
time stamping point (i.e. SFD crosses layer X). If it's class C that
answers the question, I think.

> > > Define new driver layout called ptpsq, on which driver will create
> > > SQs that will support TX port timestamp for their transmitted
> > > packets.
> > > Driver to identify PTP TX skbs and steer them to these dedicated
> > > SQs
> > > as part of the select queue ndo.
> > > 
> > > Driver to hold ptpsq per TC and report them at
> > > netif_set_real_num_tx_queues().
> > > 
> > > Add support for all needed functionality in order to xmit and poll
> > > completions received via ptpsq.
> > > 
> > > Add ptpsq to the TX reporter recover, diagnose and dump methods.
> > > 
> > > Creation of ptpsqs is disabled by default, and can be enabled via
> > > tx_port_ts private flag.  
> > 
> > This flag is pretty bad user experience.
> 
> Yeah, nothing i  could do about this, there is a large memory foot
> print i want to avoid, and we don't want to complicate PTP ctrl API of
> the HW operating mode, so until we improve the HW, we prefer to keep
> this feature as a private flag.
> 
> > > This patch steer all timestamp related packets to a ptpsq, but it
> > > does not open the port timestamp support for it. The support will
> > > be added in the following patch.  
> > 
> > Overall I'm a little shocked by this, let me sleep on it :)
> > 
> > More info on the trade offs and considerations which led to the
> > implementation would be useful.  
> 
> To get the Improved accuracy we need a special type of SQs attached to
> special HW objects that will provide more accurate stamping.
> 
> Trade-offs are :
> 
> options 1) convert ALL regular txqs (SQs) to work in this port stamping
> mode.
> 
> Pros: no need for any special mode in driver, no additional memory,
> other than the new HW objects we create for the special stamping.
> 
> Cons: significant performance hit for non PTP traffic, (the hw stamps
> all packets in the slow but more accurate mode)

Just to be clear (Alexei brought this up when I mentioned these
patches) - the requirement for the separate queues is because the time
stamp enable is a queue property, not a per WQE / frame thing? I
couldn't find this in the code - could you point me to where it's set?

> option 2) route PTP traffic to a special SQs per ring, this SQ will be
> PTP port accurate, Normal traffic will continue through regular SQs
> 
> Pros: Regular non PTP traffic not affected.
> Cons: High memory footprint for creating special SQs
> 
> 
> So we prefer (2) + private flag to avoid the performance hit and the
> redundant memory usage out of the box.

Option 3 - have only one special PTP queue in the system. PTP traffic
is rather low rate, queue per core doesn't seem necessary.


Since you said the PTP queues are slower / higher overhead - are you not
concerned that QUIC traffic will get mis-directed to them? People like
hardware time stamps for all sort of measurements these days. Plus,
since UDP doesn't itself set ooo those applications may be surprised to
see increased out-of-order rate.

Why not use the PTP classification helpers we already have?

Powered by blists - more mailing lists