Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support

From: Saeed Mahameed <saeed@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org, Eran Ben Elisha <eranbe@nvidia.com>,
	Tariq Toukan <tariqt@nvidia.com>
Subject: Re: [net-next V2 08/15] net/mlx5e: Add TX PTP port object support
Date: Fri, 04 Dec 2020 11:33:26 -0800	[thread overview]
Message-ID: <b761c676af87a4a82e3ea4f6f5aff3d1159c63e7.camel@kernel.org> (raw)
In-Reply-To: <20201203182908.1d25ea3f@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>

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.

> > 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)

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.