Re: [PATCH v3 net-next 3/3] net: ena: Add PHC documentation

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Tue, 05 Nov, 2024 16:52:11 +0000 "Arinzon, David" <darinzon@amazon.com> wrote:
>> >>> +=================
>> >> ======================================================
>> >>> +**phc_cnt**         Number of successful retrieved timestamps (below
>> >> expire timeout).
>> >>> +**phc_exp**         Number of expired retrieved timestamps (above
>> >> expire timeout).
>> >>> +**phc_skp**         Number of skipped get time attempts (during block
>> >> period).
>> >>> +**phc_err**         Number of failed get time attempts (entering into
>> block
>> >> state).
>> >>> +=================
>> >> ======================================================
>> >>
>> >> I seem to recall we had an unpleasant conversation about using
>> >> standard stats recently. Please tell me where you looked to check if
>> >> Linux has standard stats for packet timestamping. We need to add the
>> right info there.
>> >> --
>> >> pw-bot: cr
>> >
>> > Hi Jakub,
>> >
>> > Just wanted to clarify that this feature and the associated
>> > documentation are specifically intended for reading a HW timestamp, not
>> for TX/RX packet timestamping.
>> > We reviewed similar drivers that support HW timestamping via
>> > `gettime64` and `gettimex64` APIs, and we couldn't identify any that
>> capture or report statistics related to reading a HW timestamp.
>> > Let us know if further details would be helpful.
>> 
>> David, did you consider Rahul's recent timestamping stats API?
>> 0e9c127729be ("ethtool: add interface to read Tx hardware timestamping
>> statistics")
>
> Hi Gal,
>
> We've looked into the `get_ts_stats` ethtool hook, and it refers to TX HW packet timestamping
> and not HW timestamp which is retrieved through `gettime64` and `gettimex64`.

Hi folks,

I think everyone might be on the same page now, but I wanted to provide
some clarifications just in case.

The use case that the TX HW timestamping statistics covers


    ┌─────────────────────────────────────┐
    │                                     │
    │                                     │
    │                              ┌──────┤
    │           NIC hw             │      │             Packets out
    │                              │  PF  ├────────────────────────────────────────────────►
    │                              │      │
    │                              └───┬──┤
    │                                  │  │
    │                                  │  │
    │                                  │  │
    │                             ┌────┘  │
    │                             │       │
    └─────────────────────────────┼───────┘
                                  │Hw timestamp information per packet
    ┌─────────────────────────────┼───────┐
    │                             │       │
    │                       ┌─────▼────┐  │
    │                       │  cmsg    │  │
    │                       │          │  │
    │                       │  queue   │  │
    │                       │          │  │
    │                       └──────────┘  │
    │                                     │
    │                                     │
    │         Linux Kernel Stack          │
    │                                     │
    │                                     │
    │                                     │
    └─────────────────────────────────────┘

We are collecting statistics on every packet being sent out the wire.
The use case being described here


    ┌──────────────────────────────────┐
    │                                  │
    │                                  │
    │                          ┌───────┤
    │            NIC hw        │       │
    │                          │   PF  │
    │    ┌───────────────────┐ │       │
    │    │                   │ └───────┤
    │    │        PHC        │         │
    │    │                   │         │
    │    │                   │         │
    │    │ (just a clock dev)│         │
    │    └─────────┬─────────┘         │
    │              │                   │
    └──────────────┼───────────────────┘
                   │ Query device's clock for current time
    ┌──────────────┼───────────────────┐      ┌─────────────────────────────────┐
    │              │                   │      │                                 │
    │              │                   │      │                                 │
    │   ┌──────────┼─────────────┐     │      │                                 │
    │   │          │             │     │      │    ┌───────────────────────┐    │
    │   │          │             │     │      │    │                       │    │
    │   │          ▼          ───┼─────┼──────┼────┼──────────►            │    │
    │   │                        │     │      │    │                       │    │
    │   │ .gettimex64 callback   │     │      │    │  clock_gettime syscall│    │
    │   └────────────────────────┘     │      │    └───────────────────────┘    │
    │                                  │      │                                 │
    │        Linux Kernel Stack        │      │            Userspace            │
    │                                  │      │                                 │
    └──────────────────────────────────┘      └─────────────────────────────────┘

The model above is about getting the time from the NIC hardware in the
userspace application (which has not involvement with TX/RX traffic).

I do think the phc-to-host related statistics are on the niche side of
things. The following drivers are error free in their gettimex64 paths.

* AMD pesando/ionic
* Broadcom Tigon3
* Intel ixgbe
* Intel igc
* NVIDIA mlxsw
* NVIDIA mlx5_core

The above drivers would definitely not benefit from having "phc
(nic)"-to-host related statistics being presented here. I am more in
favor of making these statistics specific to amazon's ENA driver since I
think most drivers do not have a complex . Also, what value is there in
the count of phc-to-host successful/failed operations versus just
keeping track of the errors in userspace for whoever is calling
clock_gettime. I am somewhat ok with these counters, but I honestly
cannot imagine any practical use to this especially since they are not
related to anything over-the-wire. So the errors in userspace would be
enough of an indicator of whether there is excessive utilization of the
requests and the counters seem redundant to that (at least to me). Feel
free to share how you feel these counters would be helpful beyond
handling the return codes through clock_gettime. I might just be missing
something.

Hope this helps.

-- 
Thanks,

Rahul Rameshbabu