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

[Eran Ben Elisha <eranbe@nvidia.com>]

On 12/7/2020 10:37 AM, Saeed Mahameed wrote:
> On Sun, 2020-12-06 at 09:08 -0800, Richard Cochran wrote:
>> On Sun, Dec 06, 2020 at 03:37:47PM +0200, Eran Ben Elisha wrote:
>>> Adding new enum to the ioctl means we have add
>>> (HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY for example) all the way -
>>> drivers,
>>> kernel ptp, user space ptp, ethtool.
>>>
> 
> Not exactly,
> 1) the flag name should be HWTSTAMP_TX_PTP_EVENTS, similar to what we
> already have in RX, which will mean:
> HW stamp all PTP events, don't care about the rest.
> 
> 2) no need to add it to drivers from the get go, only drivers who are
> interested may implement it, and i am sure there are tons who would
> like to have this flag if their hw timestamping implementation is slow
> ! other drivers will just keep doing what they are doing, timestamp all
> traffic even if user requested this flag, again exactly like many other
> drivers do for RX flags (hwtstamp_rx_filters).
> 
>>> My concerns are:
>>> 1. Timestamp applications (like ptp4l or similar) will have to add
>>> support
>>> for configuring the driver to use HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY
>>> if
>>> supported via ioctl prior to packets transmit. From application
>>> point of
>>> view, the dual-modes (HWTSTAMP_TX_ON_TIME_CRITICAL_ONLY ,
>>> HWTSTAMP_TX_ON)
>>> support is redundant, as it offers nothing new.
>>
>> Well said.
>>
> 
> disagree, it is not a dual mode, just allow the user to have better
> granularity for what hw stamps, exactly like what we have in rx.
> 
> we are not adding any new mechanism.
> 
>>> 2. Other vendors will have to support it as well, when not sure
>>> what is the
>>> expectation from them if they cannot improve accuracy between them.
>>
>> If there were multiple different devices out there with this kind of
>> implementation (different levels of accuracy with increasing run time
>> performance cost), then we could consider such a flag.  However, to
>> my
>> knowledge, this feature is unique to your device.
>>
> 
> I agree, but i never meant to have a flag that indicate two different
> levels of accuracy, that would be a very wild mistake for sure!
> 
> The new flag will be about selecting granularity of what gets a hw
> stamp and what doesn't, aligning with the RX filter API.
> 
>>> This feature is just an internal enhancement, and as such it should
>>> be added
>>> only as a vendor private configuration flag. We are not offering
>>> here about
>>> any standard for others to follow.
>>
>> +1
>>
> 
> Our driver feature is and internal enhancement yes, but the suggested
> flag is very far from indicating any internal enhancement, is actually
> an enhancement to the current API, and is a very simple extension with
> wide range of improvements to all layers.
> 
> Our driver can optimize accuracy when this flag is set, other drivers
> might be happy to implement it since they already have a slow hw and
> this flag would allow them to run better TCP/UDP performance while
> still performing ptp hw stamping, some admins/apps will use it to avoid
> stamping all traffic on tx, win win win.
> 
> 
Seems interesting. I can form such V2 patches soon.