| Message ID | 20240402205223.137565-2-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool HW timestamping statistics | expand |
On Tue, 2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote: > +/** > + * 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. > + */ > +struct ethtool_ts_stats { > + struct_group(tx_stats, Doesn't seem like the group should be documented: include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats' description in 'ethtool_ts_stats'
On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote: >> +/** >> + * 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. >> + */ >> +struct ethtool_ts_stats { >> + struct_group(tx_stats, > > Doesn't seem like the group should be documented: > > include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats' description in 'ethtool_ts_stats' Was looking into why our internal verification did not catch this. We run W=1 with clang, but looks like the warning does not get triggered unless explicitly run with scripts/kernel-doc. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments I have debugged using strace that the way the kernel doc checking works when W=1 is set is that the matching source file that is being compiled is passed to scripts/kernel-doc, so include files are missed from the doc check. I think this is worth adding to the kernel documentation. Anyway, I will send out a v2 with this fixed but wait for potentially more feedback on v1. -- Thanks, Rahul Rameshbabu
> -----Original Message----- > From: Rahul Rameshbabu <rrameshbabu@nvidia.com> > Sent: Tuesday, April 2, 2024 10:15 PM > To: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > doc@vger.kernel.org; Zaki, Ahmed <ahmed.zaki@intel.com>; Lobakin, Aleksander > <aleksander.lobakin@intel.com>; alexandre.torgue@foss.st.com; > andrew@lunn.ch; cjubran@nvidia.com; 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; liuhangbin@gmail.com; > maxime.chevallier@bootlin.com; 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 net-next v1 1/6] ethtool: add interface to read Tx hardware > timestamping statistics > > On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote: > >> +/** > >> + * 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. > >> + */ > >> +struct ethtool_ts_stats { > >> + struct_group(tx_stats, > > > > Doesn't seem like the group should be documented: > > > > include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats' > description in 'ethtool_ts_stats' > > Was looking into why our internal verification did not catch this. We > run W=1 with clang, but looks like the warning does not get triggered > unless explicitly run with scripts/kernel-doc. > > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to- > format-kernel-doc-comments > > I have debugged using strace that the way the kernel doc checking works > when W=1 is set is that the matching source file that is being compiled > is passed to scripts/kernel-doc, so include files are missed from the > doc check. I think this is worth adding to the kernel documentation. > It would be great if the W=1 setup could figure out the include files and send those to kernel-doc too, but I'm not sure if this is possible and if so how difficult it would be to implement it. A lot of headers produce warnings because a lot fewer people manually run kernel-doc on the entire source. > Anyway, I will send out a v2 with this fixed but wait for potentially > more feedback on v1. > > -- > Thanks, > > Rahul Rameshbabu
On Wed, 03 Apr, 2024 18:44:52 +0000 "Keller, Jacob E" <jacob.e.keller@intel.com> wrote: >> On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@kernel.org> wrote: >> > On Tue, 2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote: >> >> +/** >> >> + * 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. >> >> + */ >> >> +struct ethtool_ts_stats { >> >> + struct_group(tx_stats, >> > >> > Doesn't seem like the group should be documented: >> > >> > include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats' >> description in 'ethtool_ts_stats' >> >> Was looking into why our internal verification did not catch this. We >> run W=1 with clang, but looks like the warning does not get triggered >> unless explicitly run with scripts/kernel-doc. >> >> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to- >> format-kernel-doc-comments >> >> I have debugged using strace that the way the kernel doc checking works >> when W=1 is set is that the matching source file that is being compiled >> is passed to scripts/kernel-doc, so include files are missed from the >> doc check. I think this is worth adding to the kernel documentation. >> > > It would be great if the W=1 setup could figure out the include files and send > those to kernel-doc too, but I'm not sure if this is possible and if so how > difficult it would be to implement it. A lot of headers produce warnings because > a lot fewer people manually run kernel-doc on the entire source. > I took a look into this, and the one naive solution I had in mind was a checkdocs target for the kernel where you use gcc -MM to deduce all the includes, create a unique list, and then run scripts/kernel-doc against the list of include files. That said, I do think this is excessive compared to having a checkpatch logic that runs scripts/kernel-doc on the parts that change in your commit before and after the patch is applied. Kudos to the netdev CI for having this. https://github.com/linux-netdev/nipa/blob/main/tests/patch/kdoc/kdoc.sh
diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml index 197208f419dc..f5aa1e7f3383 100644 --- a/Documentation/netlink/specs/ethtool.yaml +++ b/Documentation/netlink/specs/ethtool.yaml @@ -559,6 +559,18 @@ attribute-sets: - name: tx-lpi-timer type: u32 + - + name: ts-stat + attributes: + - + name: tx-pkts + type: uint + - + name: tx-lost + type: uint + - + name: tx-err + type: uint - name: tsinfo attributes: @@ -581,6 +593,10 @@ attribute-sets: - name: phc-index type: u32 + - + name: stats + type: nest + nested-attributes: ts-stat - name: cable-result attributes: @@ -1388,6 +1404,7 @@ operations: - tx-types - rx-filters - phc-index + - stats dump: *tsinfo-get-op - name: cable-test-act diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index d583d9abf2f8..08d330b0f50f 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -1237,12 +1237,21 @@ Kernel response contents: ``ETHTOOL_A_TSINFO_TX_TYPES`` bitset supported Tx types ``ETHTOOL_A_TSINFO_RX_FILTERS`` bitset supported Rx filters ``ETHTOOL_A_TSINFO_PHC_INDEX`` u32 PTP hw clock index + ``ETHTOOL_A_TSINFO_STATS`` nested HW timestamping statistics ===================================== ====== ========================== ``ETHTOOL_A_TSINFO_PHC_INDEX`` is absent if there is no associated PHC (there is no special value for this case). The bitset attributes are omitted if they would be empty (no bit set). +Additional hardware timestamping statistics response contents: + + ===================================== ====== =================================== + ``ETHTOOL_A_TS_STAT_TX_PKTS`` u64 Packets with Tx HW timestamps + ``ETHTOOL_A_TS_STAT_TX_LOST`` u64 Tx HW timestamp not arrived count + ``ETHTOOL_A_TS_STAT_TX_ERR`` u64 HW error request Tx timestamp count + ===================================== ====== =================================== + CABLE_TEST ========== diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index 9901e563f706..4b5ca7628700 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -480,6 +480,27 @@ 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 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. + */ +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 @@ -755,7 +776,10 @@ struct ethtool_rxfh_param { * @get_ts_info: Get the time stamping and PTP hardware clock capabilities. * 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(). + * ethtool_op_get_ts_info(). Drivers must not zero statistics which they + * don't report. The stats structure is initialized to ETHTOOL_STAT_NOT_SET + * indicating driver does not report statistics. + * @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 @@ -898,6 +922,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..8dfd714c2308 100644 --- a/include/uapi/linux/ethtool_netlink.h +++ b/include/uapi/linux/ethtool_netlink.h @@ -478,12 +478,26 @@ 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_TX_PKTS, /* u64 */ + ETHTOOL_A_TS_STAT_TX_LOST, /* u64 */ + ETHTOOL_A_TS_STAT_TX_ERR, /* 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..be2755c8d8fd 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_UNSPEC + 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_uint(skb, attrtype, val)) + 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_PKTS) || + 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; }