| Message ID | 20240309084440.299358-2-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool HW timestamping statistics | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Sat, 9 Mar 2024 00:44:35 -0800 Rahul Rameshbabu wrote: > Multiple network devices that support hardware timestamping appear to have > common behavior with regards to timestamp handling. Implement common Tx > hardware timestamping statistics in a tx_stats struct_group. Common Rx > hardware timestamping statistics can subsequently be implemented in a > rx_stats struct_group for ethtool_ts_stats. > Documentation/netlink/specs/ethtool.yaml | 20 +++++++++ > include/linux/ethtool.h | 21 ++++++++++ > include/uapi/linux/ethtool_netlink.h | 15 +++++++ > net/ethtool/tsinfo.c | 52 +++++++++++++++++++++++- > 4 files changed, 107 insertions(+), 1 deletion(-) Feels like we should mention the new stats somehow in Documentation/networking/ethtool-netlink.rst > diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml > index 197208f419dc..f99b003c78c0 100644 > --- a/Documentation/netlink/specs/ethtool.yaml > +++ b/Documentation/netlink/specs/ethtool.yaml > @@ -559,6 +559,21 @@ attribute-sets: > - > name: tx-lpi-timer > type: u32 > + - > + name: ts-stat > + attributes: > + - > + name: pad > + type: pad You can remove the pad entry, and... > + - > + name: tx-pkts > + type: u64 ...use the uint type for the stats > + - > + name: tx-lost > + type: u64 > + - > + name: tx-err > + type: u64 > - > name: tsinfo > attributes: > +/** > + * struct ethtool_ts_stats - HW timestamping statistics > + * @tx_stats: struct group for TX HW timestamping > + * @pkts: Number of packets successfully timestamped by the queried > + * layer. > + * @lost: Number of packet timestamps that failed to get applied on a > + * packet by the queried layer. > + * @err: Number of timestamping errors that occurred on the queried > + * layer. the kdocs for @lost and @err are not very clear > + * @get_ts_stats: Query the device hardware timestamping statistics. Let's copy & paste the "Drivers must not zero" text in here? People seem to miss that requirement anyway, but at least we'll have something to point at in review. > +enum { > + ETHTOOL_A_TS_STAT_UNSPEC, > + ETHTOOL_A_TS_STAT_PAD, > + > + ETHTOOL_A_TS_STAT_TX_PKT, /* array, u64 */ > + ETHTOOL_A_TS_STAT_TX_LOST, /* array, u64 */ > + ETHTOOL_A_TS_STAT_TX_ERR, /* array, u64 */ I don't think these are arrays. > + > + /* add new constants above here */ > + __ETHTOOL_A_TS_STAT_CNT, > + ETHTOOL_A_TS_STAT_MAX = (__ETHTOOL_A_TS_STAT_CNT - 1) > + > +};
On Tue, 12 Mar, 2024 16:53:46 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Sat, 9 Mar 2024 00:44:35 -0800 Rahul Rameshbabu wrote: >> Multiple network devices that support hardware timestamping appear to have >> common behavior with regards to timestamp handling. Implement common Tx >> hardware timestamping statistics in a tx_stats struct_group. Common Rx >> hardware timestamping statistics can subsequently be implemented in a >> rx_stats struct_group for ethtool_ts_stats. > >> Documentation/netlink/specs/ethtool.yaml | 20 +++++++++ >> include/linux/ethtool.h | 21 ++++++++++ >> include/uapi/linux/ethtool_netlink.h | 15 +++++++ >> net/ethtool/tsinfo.c | 52 +++++++++++++++++++++++- >> 4 files changed, 107 insertions(+), 1 deletion(-) > > Feels like we should mention the new stats somehow in > Documentation/networking/ethtool-netlink.rst > >> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml >> index 197208f419dc..f99b003c78c0 100644 >> --- a/Documentation/netlink/specs/ethtool.yaml >> +++ b/Documentation/netlink/specs/ethtool.yaml >> @@ -559,6 +559,21 @@ attribute-sets: >> - >> name: tx-lpi-timer >> type: u32 >> + - >> + name: ts-stat >> + attributes: >> + - >> + name: pad >> + type: pad > > You can remove the pad entry, and... > >> + - >> + name: tx-pkts >> + type: u64 > > ...use the uint type for the stats > >> + - >> + name: tx-lost >> + type: u64 >> + - >> + name: tx-err >> + type: u64 >> - >> name: tsinfo >> attributes: > >> +/** >> + * struct ethtool_ts_stats - HW timestamping statistics >> + * @tx_stats: struct group for TX HW timestamping >> + * @pkts: Number of packets successfully timestamped by the queried >> + * layer. >> + * @lost: Number of packet timestamps that failed to get applied on a >> + * packet by the queried layer. >> + * @err: Number of timestamping errors that occurred on the queried >> + * layer. > > the kdocs for @lost and @err are not very clear Makes sense given that these are stale and should have been changed between my v1 and v2. Here is my new attempt at this. /** * struct ethtool_ts_stats - HW timestamping statistics * @tx_stats: struct group for TX HW timestamping * @pkts: Number of packets successfully timestamped by the hardware. * @lost: Number of hardware timestamping requests where the timestamping * information from the hardware never arrived for submission with * the skb. * @err: Number of arbitrary timestamp generation error events that the * hardware encountered. */ > >> + * @get_ts_stats: Query the device hardware timestamping statistics. > > Let's copy & paste the "Drivers must not zero" text in here? > People seem to miss that requirement anyway, but at least we'll > have something to point at in review. > >> +enum { >> + ETHTOOL_A_TS_STAT_UNSPEC, >> + ETHTOOL_A_TS_STAT_PAD, >> + >> + ETHTOOL_A_TS_STAT_TX_PKT, /* array, u64 */ >> + ETHTOOL_A_TS_STAT_TX_LOST, /* array, u64 */ >> + ETHTOOL_A_TS_STAT_TX_ERR, /* array, u64 */ > > I don't think these are arrays. Sorry, copy-paste mistake from FEC stats.... > >> + >> + /* add new constants above here */ >> + __ETHTOOL_A_TS_STAT_CNT, >> + ETHTOOL_A_TS_STAT_MAX = (__ETHTOOL_A_TS_STAT_CNT - 1) >> + >> +}; Agreed with all the comments here. Have accounted for them in my patches for my next submission (aiming for non-RFC once the net-next window is open again). -- Thanks, Rahul Rameshbabu
On Wed, 13 Mar 2024 17:26:11 -0700 Rahul Rameshbabu wrote: > Makes sense given that these are stale and should have been changed > between my v1 and v2. Here is my new attempt at this. > > /** > * struct ethtool_ts_stats - HW timestamping statistics > * @tx_stats: struct group for TX HW timestamping > * @pkts: Number of packets successfully timestamped by the hardware. > * @lost: Number of hardware timestamping requests where the timestamping > * information from the hardware never arrived for submission with > * the skb. Should we give some guidance to drivers which "ignore" time stamping requests if they used up all the "slots"? Even if just temporary until they are fixed? Maybe we can add after all the fields something like: For drivers which ignore further timestamping requests when there are too many in flight, the ignored requests are currently not counted by any of the statistics. Adjust as needed, I basing this on the vague memory that this was the conclusion in the last discussion :) > * @err: Number of arbitrary timestamp generation error events that the > * hardware encountered. > */ Just to be crystal clear let's also call out that @lost is not included in @err.
On Wed, 13 Mar, 2024 17:41:07 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Wed, 13 Mar 2024 17:26:11 -0700 Rahul Rameshbabu wrote: >> Makes sense given that these are stale and should have been changed >> between my v1 and v2. Here is my new attempt at this. >> >> /** >> * struct ethtool_ts_stats - HW timestamping statistics >> * @tx_stats: struct group for TX HW timestamping >> * @pkts: Number of packets successfully timestamped by the hardware. >> * @lost: Number of hardware timestamping requests where the timestamping >> * information from the hardware never arrived for submission with >> * the skb. > > Should we give some guidance to drivers which "ignore" time stamping > requests if they used up all the "slots"? Even if just temporary until > they are fixed? Maybe we can add after all the fields something like: > > For drivers which ignore further timestamping requests when there are > too many in flight, the ignored requests are currently not counted by > any of the statistics. I was actually thinking it would be better to merge them into the error counter temporarily. Reason being is that in the case Intel notices that their slots are full, they just drop traffic from my understanding today. If the error counters increment in that situation, it helps with the debug to a degree. EBUSY is an error in general. > > Adjust as needed, I basing this on the vague memory that this was > the conclusion in the last discussion :) > >> * @err: Number of arbitrary timestamp generation error events that the >> * hardware encountered. >> */ > > Just to be crystal clear let's also call out that @lost is not included > in @err. Ack. -- Thanks, Rahul Rameshbabu
On Wed, 13 Mar 2024 17:50:39 -0700 Rahul Rameshbabu wrote: > > Should we give some guidance to drivers which "ignore" time stamping > > requests if they used up all the "slots"? Even if just temporary until > > they are fixed? Maybe we can add after all the fields something like: > > > > For drivers which ignore further timestamping requests when there are > > too many in flight, the ignored requests are currently not counted by > > any of the statistics. > > I was actually thinking it would be better to merge them into the error > counter temporarily. Reason being is that in the case Intel notices that > their slots are full, they just drop traffic from my understanding > today. If the error counters increment in that situation, it helps with > the debug to a degree. EBUSY is an error in general. That works, too, let's recommend it (FWIW no preference whether in the entry for @err or somewhere separately in the kdoc).
On Wed, 13 Mar, 2024 18:40:17 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Wed, 13 Mar 2024 17:50:39 -0700 Rahul Rameshbabu wrote: >> > Should we give some guidance to drivers which "ignore" time stamping >> > requests if they used up all the "slots"? Even if just temporary until >> > they are fixed? Maybe we can add after all the fields something like: >> > >> > For drivers which ignore further timestamping requests when there are >> > too many in flight, the ignored requests are currently not counted by >> > any of the statistics. >> >> I was actually thinking it would be better to merge them into the error >> counter temporarily. Reason being is that in the case Intel notices that >> their slots are full, they just drop traffic from my understanding >> today. If the error counters increment in that situation, it helps with >> the debug to a degree. EBUSY is an error in general. > > That works, too, let's recommend it (FWIW no preference whether > in the entry for @err or somewhere separately in the kdoc). /** * struct ethtool_ts_stats - HW timestamping statistics * @tx_stats: struct group for TX HW timestamping * @pkts: Number of packets successfully timestamped by the hardware. * @lost: Number of hardware timestamping requests where the timestamping * information from the hardware never arrived for submission with * the skb. * @err: Number of arbitrary timestamp generation error events that the * hardware encountered, exclusive of @lost statistics. Cases such * as resource exhaustion, unavailability, firmware errors, and * detected illogical timestamp values not submitted with the skb * are inclusive to this counter. */ Here is my current draft for the error counter documentation. -- Thanks, Rahul Rameshbabu
On Tue, 12 Mar, 2024 16:53:46 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Sat, 9 Mar 2024 00:44:35 -0800 Rahul Rameshbabu wrote: >> Multiple network devices that support hardware timestamping appear to have >> common behavior with regards to timestamp handling. Implement common Tx >> hardware timestamping statistics in a tx_stats struct_group. Common Rx >> hardware timestamping statistics can subsequently be implemented in a >> rx_stats struct_group for ethtool_ts_stats. > >> Documentation/netlink/specs/ethtool.yaml | 20 +++++++++ >> include/linux/ethtool.h | 21 ++++++++++ >> include/uapi/linux/ethtool_netlink.h | 15 +++++++ >> net/ethtool/tsinfo.c | 52 +++++++++++++++++++++++- >> 4 files changed, 107 insertions(+), 1 deletion(-) > > Feels like we should mention the new stats somehow in > Documentation/networking/ethtool-netlink.rst > >> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml >> index 197208f419dc..f99b003c78c0 100644 >> --- a/Documentation/netlink/specs/ethtool.yaml >> +++ b/Documentation/netlink/specs/ethtool.yaml >> @@ -559,6 +559,21 @@ attribute-sets: >> - >> name: tx-lpi-timer >> type: u32 >> + - >> + name: ts-stat >> + attributes: >> + - >> + name: pad >> + type: pad > > You can remove the pad entry, and... > You need the pad to match with ETHTOOL_A_TS_STAT_PAD (which similar to other ethtool stats currently defined). Otherwise, you run into the following.... mm-stat and fec-stat are good examples. [root@binary-eater-vm-01 linux-ethtool-ts]# ./tools/net/ynl/ethtool.py --show-time-stamping mlx5_1 Traceback (most recent call last): File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 598, in _decode attr_spec = attr_space.attrs_by_val[attr.type] ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^ KeyError: 4 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/root/linux-ethtool-ts/./tools/net/ynl/ethtool.py", line 437, in <module> main() File "/root/linux-ethtool-ts/./tools/net/ynl/ethtool.py", line 333, in main tsinfo = dumpit(ynl, args, 'tsinfo-get', req) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/root/linux-ethtool-ts/./tools/net/ynl/ethtool.py", line 91, in dumpit reply = ynl.dump(op_name, { 'header': {} } | extra) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 873, in dump return self._op(method, vals, [], dump=True) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 858, in _op rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 607, in _decode subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], search_attrs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 601, in _decode raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'") Exception: Space 'ts-stat' has no attribute with value '4' >> +enum { >> + ETHTOOL_A_TS_STAT_UNSPEC, >> + ETHTOOL_A_TS_STAT_PAD, >> + >> + ETHTOOL_A_TS_STAT_TX_PKT, /* array, u64 */ >> + ETHTOOL_A_TS_STAT_TX_LOST, /* array, u64 */ >> + ETHTOOL_A_TS_STAT_TX_ERR, /* array, u64 */ > > I don't think these are arrays. > >> + >> + /* add new constants above here */ >> + __ETHTOOL_A_TS_STAT_CNT, >> + ETHTOOL_A_TS_STAT_MAX = (__ETHTOOL_A_TS_STAT_CNT - 1) >> + >> +}; -- Thanks, Rahul Rameshbabu
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Wednesday, March 13, 2024 6:40 PM > To: Rahul Rameshbabu <rrameshbabu@nvidia.com> > Cc: Zaki, Ahmed <ahmed.zaki@intel.com>; Lobakin, Aleksander > <aleksander.lobakin@intel.com>; alexandre.torgue@foss.st.com; > andrew@lunn.ch; corbet@lwn.net; davem@davemloft.net; dtatulea@nvidia.com; > edumazet@google.com; gal@nvidia.com; hkallweit1@gmail.com; Keller, Jacob E > <jacob.e.keller@intel.com>; jiri@resnulli.us; joabreu@synopsys.com; > justinstitt@google.com; kory.maincent@bootlin.com; leon@kernel.org; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; liuhangbin@gmail.com; > maxime.chevallier@bootlin.com; netdev@vger.kernel.org; pabeni@redhat.com; > Greenwalt, Paul <paul.greenwalt@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; rdunlap@infradead.org; > richardcochran@gmail.com; saeed@kernel.org; tariqt@nvidia.com; > vadim.fedorenko@linux.dev; vladimir.oltean@nxp.com; Drewek, Wojciech > <wojciech.drewek@intel.com> > Subject: Re: [PATCH RFC v2 1/6] ethtool: add interface to read Tx hardware > timestamping statistics > > On Wed, 13 Mar 2024 17:50:39 -0700 Rahul Rameshbabu wrote: > > > Should we give some guidance to drivers which "ignore" time stamping > > > requests if they used up all the "slots"? Even if just temporary until > > > they are fixed? Maybe we can add after all the fields something like: > > > > > > For drivers which ignore further timestamping requests when there are > > > too many in flight, the ignored requests are currently not counted by > > > any of the statistics. > > > > I was actually thinking it would be better to merge them into the error > > counter temporarily. Reason being is that in the case Intel notices that > > their slots are full, they just drop traffic from my understanding > > today. If the error counters increment in that situation, it helps with > > the debug to a degree. EBUSY is an error in general. > > That works, too, let's recommend it (FWIW no preference whether > in the entry for @err or somewhere separately in the kdoc). We don't drop traffic, we send the packets just fine.. We just never report a timestamp for them, since we don't program the hardware to capture that timestamp.
On Thu, 14 Mar 2024 10:01:49 -0700 Rahul Rameshbabu wrote: > You need the pad to match with ETHTOOL_A_TS_STAT_PAD (which similar to > other ethtool stats currently defined). Otherwise, you run into the > following.... mm-stat and fec-stat are good examples. I don't understand. Are you sure you changef the kernel to use uint, rebuilt and there is no ETHTOOL_A_TS_STAT_PAD anywhere, anymore?
On Thu, 14 Mar, 2024 10:59:43 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 14 Mar 2024 10:01:49 -0700 Rahul Rameshbabu wrote: >> You need the pad to match with ETHTOOL_A_TS_STAT_PAD (which similar to >> other ethtool stats currently defined). Otherwise, you run into the >> following.... mm-stat and fec-stat are good examples. > > I don't understand. > Are you sure you changef the kernel to use uint, rebuilt and > there is no ETHTOOL_A_TS_STAT_PAD anywhere, anymore? Sorry, I was not as clear as I could have been with my last reply. I did leave ETHTOOL_A_TS_STAT_PAD in when I tested (intentionally). I was trying to mimic other ethtool stats implementations, but you are saying that in general there is no need for this padding (which I agree with) and I can remove that unnecessary offset. It'll be different from the existing stats, but I am ok with that. -- Thanks, Rahul Rameshbabu
On Thu, 14 Mar, 2024 17:50:10 +0000 "Keller, Jacob E" <jacob.e.keller@intel.com> wrote: >> -----Original Message----- >> From: Jakub Kicinski <kuba@kernel.org> >> Sent: Wednesday, March 13, 2024 6:40 PM >> To: Rahul Rameshbabu <rrameshbabu@nvidia.com> >> Cc: Zaki, Ahmed <ahmed.zaki@intel.com>; Lobakin, Aleksander >> <aleksander.lobakin@intel.com>; alexandre.torgue@foss.st.com; >> andrew@lunn.ch; corbet@lwn.net; davem@davemloft.net; dtatulea@nvidia.com; >> edumazet@google.com; gal@nvidia.com; hkallweit1@gmail.com; Keller, Jacob E >> <jacob.e.keller@intel.com>; jiri@resnulli.us; joabreu@synopsys.com; >> justinstitt@google.com; kory.maincent@bootlin.com; leon@kernel.org; linux- >> doc@vger.kernel.org; linux-kernel@vger.kernel.org; liuhangbin@gmail.com; >> maxime.chevallier@bootlin.com; netdev@vger.kernel.org; pabeni@redhat.com; >> Greenwalt, Paul <paul.greenwalt@intel.com>; Kitszel, Przemyslaw >> <przemyslaw.kitszel@intel.com>; rdunlap@infradead.org; >> richardcochran@gmail.com; saeed@kernel.org; tariqt@nvidia.com; >> vadim.fedorenko@linux.dev; vladimir.oltean@nxp.com; Drewek, Wojciech >> <wojciech.drewek@intel.com> >> Subject: Re: [PATCH RFC v2 1/6] ethtool: add interface to read Tx hardware >> timestamping statistics >> >> On Wed, 13 Mar 2024 17:50:39 -0700 Rahul Rameshbabu wrote: >> > > Should we give some guidance to drivers which "ignore" time stamping >> > > requests if they used up all the "slots"? Even if just temporary until >> > > they are fixed? Maybe we can add after all the fields something like: >> > > >> > > For drivers which ignore further timestamping requests when there are >> > > too many in flight, the ignored requests are currently not counted by >> > > any of the statistics. >> > >> > I was actually thinking it would be better to merge them into the error >> > counter temporarily. Reason being is that in the case Intel notices that >> > their slots are full, they just drop traffic from my understanding >> > today. If the error counters increment in that situation, it helps with >> > the debug to a degree. EBUSY is an error in general. >> >> That works, too, let's recommend it (FWIW no preference whether >> in the entry for @err or somewhere separately in the kdoc). > > We don't drop traffic, we send the packets just fine.. We just never report a > timestamp for them, since we don't program the hardware to capture that > timestamp. My actual kdoc comments are better now, but I should have been better with the language I used here in the email. In my head, I was thinking more about the case of not submitting HW timestamp information when sending out the packet rather than dropping the packet entirely (I would say that is still a timestamping error case). -- Thanks, Rahul Rameshbabu
On Thu, 14 Mar 2024 11:43:07 -0700 Rahul Rameshbabu wrote: > > I don't understand. > > Are you sure you changef the kernel to use uint, rebuilt and > > there is no ETHTOOL_A_TS_STAT_PAD anywhere, anymore? > > Sorry, I was not as clear as I could have been with my last reply. I did > leave ETHTOOL_A_TS_STAT_PAD in when I tested (intentionally). I was > trying to mimic other ethtool stats implementations, but you are saying > that in general there is no need for this padding (which I agree with) > and I can remove that unnecessary offset. It'll be different from the > existing stats, but I am ok with that. Yes, the small divergence is fine - uint is pretty recent addition.
On Thu, 14 Mar, 2024 12:06:47 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 14 Mar 2024 11:43:07 -0700 Rahul Rameshbabu wrote: >> > I don't understand. >> > Are you sure you changef the kernel to use uint, rebuilt and >> > there is no ETHTOOL_A_TS_STAT_PAD anywhere, anymore? >> >> Sorry, I was not as clear as I could have been with my last reply. I did >> leave ETHTOOL_A_TS_STAT_PAD in when I tested (intentionally). I was >> trying to mimic other ethtool stats implementations, but you are saying >> that in general there is no need for this padding (which I agree with) >> and I can remove that unnecessary offset. It'll be different from the >> existing stats, but I am ok with that. > > Yes, the small divergence is fine - uint is pretty recent addition. Yes, the uint suggestion is great since we no longer need to depend on the padding. Thanks for the feedback. Accounted for in my patches. -- Thanks, Rahul Rameshbabu
diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml index 197208f419dc..f99b003c78c0 100644 --- a/Documentation/netlink/specs/ethtool.yaml +++ b/Documentation/netlink/specs/ethtool.yaml @@ -559,6 +559,21 @@ attribute-sets: - name: tx-lpi-timer type: u32 + - + name: ts-stat + attributes: + - + name: pad + type: pad + - + name: tx-pkts + type: u64 + - + name: tx-lost + type: u64 + - + name: tx-err + type: u64 - name: tsinfo attributes: @@ -581,6 +596,10 @@ attribute-sets: - name: phc-index type: u32 + - + name: stats + type: nest + nested-attributes: ts-stat - name: cable-result attributes: @@ -1388,6 +1407,7 @@ operations: - tx-types - rx-filters - phc-index + - stats dump: *tsinfo-get-op - name: cable-test-act diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index b90c33607594..a1704938a6fb 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -483,6 +483,24 @@ struct ethtool_rmon_stats { ); }; +/** + * struct ethtool_ts_stats - HW timestamping statistics + * @tx_stats: struct group for TX HW timestamping + * @pkts: Number of packets successfully timestamped by the queried + * layer. + * @lost: Number of packet timestamps that failed to get applied on a + * packet by the queried layer. + * @err: Number of timestamping errors that occurred on the queried + * layer. + */ +struct ethtool_ts_stats { + struct_group(tx_stats, + u64 pkts; + u64 lost; + u64 err; + ); +}; + #define ETH_MODULE_EEPROM_PAGE_LEN 128 #define ETH_MODULE_MAX_I2C_ADDRESS 0x7f @@ -759,6 +777,7 @@ struct ethtool_rxfh_param { * It may be called with RCU, or rtnl or reference on the device. * Drivers supporting transmit time stamps in software should set this to * ethtool_op_get_ts_info(). + * @get_ts_stats: Query the device hardware timestamping statistics. * @get_module_info: Get the size and type of the eeprom contained within * a plug-in module. * @get_module_eeprom: Get the eeprom information from the plug-in module @@ -901,6 +920,8 @@ struct ethtool_ops { struct ethtool_dump *, void *); int (*set_dump)(struct net_device *, struct ethtool_dump *); int (*get_ts_info)(struct net_device *, struct ethtool_ts_info *); + void (*get_ts_stats)(struct net_device *dev, + struct ethtool_ts_stats *ts_stats); int (*get_module_info)(struct net_device *, struct ethtool_modinfo *); int (*get_module_eeprom)(struct net_device *, diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h index 3f89074aa06c..046a78d9421d 100644 --- a/include/uapi/linux/ethtool_netlink.h +++ b/include/uapi/linux/ethtool_netlink.h @@ -478,12 +478,27 @@ enum { ETHTOOL_A_TSINFO_TX_TYPES, /* bitset */ ETHTOOL_A_TSINFO_RX_FILTERS, /* bitset */ ETHTOOL_A_TSINFO_PHC_INDEX, /* u32 */ + ETHTOOL_A_TSINFO_STATS, /* nest - _A_TSINFO_STAT */ /* add new constants above here */ __ETHTOOL_A_TSINFO_CNT, ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1) }; +enum { + ETHTOOL_A_TS_STAT_UNSPEC, + ETHTOOL_A_TS_STAT_PAD, + + ETHTOOL_A_TS_STAT_TX_PKT, /* array, u64 */ + ETHTOOL_A_TS_STAT_TX_LOST, /* array, u64 */ + ETHTOOL_A_TS_STAT_TX_ERR, /* array, u64 */ + + /* add new constants above here */ + __ETHTOOL_A_TS_STAT_CNT, + ETHTOOL_A_TS_STAT_MAX = (__ETHTOOL_A_TS_STAT_CNT - 1) + +}; + /* PHC VCLOCKS */ enum { diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c index 9daed0aab162..0d1370ded122 100644 --- a/net/ethtool/tsinfo.c +++ b/net/ethtool/tsinfo.c @@ -13,14 +13,18 @@ struct tsinfo_req_info { struct tsinfo_reply_data { struct ethnl_reply_data base; struct ethtool_ts_info ts_info; + struct ethtool_ts_stats stats; }; #define TSINFO_REPDATA(__reply_base) \ container_of(__reply_base, struct tsinfo_reply_data, base) +#define ETHTOOL_TS_STAT_CNT \ + (__ETHTOOL_A_TS_STAT_CNT - (ETHTOOL_A_TS_STAT_PAD + 1)) + const struct nla_policy ethnl_tsinfo_get_policy[] = { [ETHTOOL_A_TSINFO_HEADER] = - NLA_POLICY_NESTED(ethnl_header_policy), + NLA_POLICY_NESTED(ethnl_header_policy_stats), }; static int tsinfo_prepare_data(const struct ethnl_req_info *req_base, @@ -34,6 +38,12 @@ static int tsinfo_prepare_data(const struct ethnl_req_info *req_base, ret = ethnl_ops_begin(dev); if (ret < 0) return ret; + if (req_base->flags & ETHTOOL_FLAG_STATS && + dev->ethtool_ops->get_ts_stats) { + ethtool_stats_init((u64 *)&data->stats, + sizeof(data->stats) / sizeof(u64)); + dev->ethtool_ops->get_ts_stats(dev, &data->stats); + } ret = __ethtool_get_ts_info(dev, &data->ts_info); ethnl_ops_complete(dev); @@ -79,10 +89,47 @@ static int tsinfo_reply_size(const struct ethnl_req_info *req_base, } if (ts_info->phc_index >= 0) len += nla_total_size(sizeof(u32)); /* _TSINFO_PHC_INDEX */ + if (req_base->flags & ETHTOOL_FLAG_STATS) + len += nla_total_size(0) + /* _TSINFO_STATS */ + nla_total_size_64bit(sizeof(u64)) * ETHTOOL_TS_STAT_CNT; return len; } +static int tsinfo_put_stat(struct sk_buff *skb, u64 val, u16 attrtype) +{ + if (val == ETHTOOL_STAT_NOT_SET) + return 0; + if (nla_put_u64_64bit(skb, attrtype, val, ETHTOOL_A_TS_STAT_PAD)) + return -EMSGSIZE; + return 0; +} + +static int tsinfo_put_stats(struct sk_buff *skb, + const struct ethtool_ts_stats *stats) +{ + struct nlattr *nest; + + nest = nla_nest_start(skb, ETHTOOL_A_TSINFO_STATS); + if (!nest) + return -EMSGSIZE; + + if (tsinfo_put_stat(skb, stats->tx_stats.pkts, + ETHTOOL_A_TS_STAT_TX_PKT) || + tsinfo_put_stat(skb, stats->tx_stats.lost, + ETHTOOL_A_TS_STAT_TX_LOST) || + tsinfo_put_stat(skb, stats->tx_stats.err, + ETHTOOL_A_TS_STAT_TX_ERR)) + goto err_cancel; + + nla_nest_end(skb, nest); + return 0; + +err_cancel: + nla_nest_cancel(skb, nest); + return -EMSGSIZE; +} + static int tsinfo_fill_reply(struct sk_buff *skb, const struct ethnl_req_info *req_base, const struct ethnl_reply_data *reply_base) @@ -119,6 +166,9 @@ static int tsinfo_fill_reply(struct sk_buff *skb, if (ts_info->phc_index >= 0 && nla_put_u32(skb, ETHTOOL_A_TSINFO_PHC_INDEX, ts_info->phc_index)) return -EMSGSIZE; + if (req_base->flags & ETHTOOL_FLAG_STATS && + tsinfo_put_stats(skb, &data->stats)) + return -EMSGSIZE; return 0; }
Multiple network devices that support hardware timestamping appear to have common behavior with regards to timestamp handling. Implement common Tx hardware timestamping statistics in a tx_stats struct_group. Common Rx hardware timestamping statistics can subsequently be implemented in a rx_stats struct_group for ethtool_ts_stats. Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> --- Documentation/netlink/specs/ethtool.yaml | 20 +++++++++ include/linux/ethtool.h | 21 ++++++++++ include/uapi/linux/ethtool_netlink.h | 15 +++++++ net/ethtool/tsinfo.c | 52 +++++++++++++++++++++++- 4 files changed, 107 insertions(+), 1 deletion(-)