.../ethernet/mellanox/mlx5/counters.rst | 11 +++ .../net/ethernet/mellanox/mlx5/core/en/ptp.c | 1 + .../ethernet/mellanox/mlx5/core/en_ethtool.c | 9 +++ .../ethernet/mellanox/mlx5/core/en_stats.c | 71 +++++++++++++++++++ .../ethernet/mellanox/mlx5/core/en_stats.h | 4 ++ .../net/ethernet/mellanox/mlx5/core/en_tx.c | 6 +- include/linux/ethtool.h | 28 ++++++++ include/uapi/linux/ethtool.h | 20 ++++++ include/uapi/linux/ethtool_netlink.h | 17 +++++ net/ethtool/netlink.h | 3 +- net/ethtool/stats.c | 53 ++++++++++++-- net/ethtool/strset.c | 5 ++ tools/net/ynl/ethtool.py | 9 ++- 13 files changed, 227 insertions(+), 10 deletions(-)
The goal of this patch series is to introduce a common set of ethtool statistics
for hardware timestamping that a driver implementer can hook into. The
statistics counters added are based on what I believe are common
patterns/behaviors found across various hardware timestamping implementations
seen in the kernel tree today. The mlx5 family of devices is used as the PoC for
this patch series. Other vendors are more than welcome to chim in on this
series.
Statistics can be queried from either the DMA or PHY layers. I think this
concept of layer-based statistics selection and the general timestamping layer
selection work Kory Maincent is working on can be converged together [1].
[1] https://lore.kernel.org/netdev/20240216-feature_ptp_netnext-v8-0-510f42f444fb@bootlin.com/
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
Rahul Rameshbabu (6):
ethtool: add interface to read Tx hardware timestamping statistics
net/mlx5e: Introduce lost_cqe statistic counter for PTP Tx port
timestamping CQ
net/mlx5e: Introduce timestamps statistic counter for Tx DMA layer
net/mlx5e: Implement ethtool hardware timestamping statistics
tools: ynl: ethtool.py: Make tool invokable from any CWD
tools: ynl: ethtool.py: Add ts ethtool statistics group
.../ethernet/mellanox/mlx5/counters.rst | 11 +++
.../net/ethernet/mellanox/mlx5/core/en/ptp.c | 1 +
.../ethernet/mellanox/mlx5/core/en_ethtool.c | 9 +++
.../ethernet/mellanox/mlx5/core/en_stats.c | 71 +++++++++++++++++++
.../ethernet/mellanox/mlx5/core/en_stats.h | 4 ++
.../net/ethernet/mellanox/mlx5/core/en_tx.c | 6 +-
include/linux/ethtool.h | 28 ++++++++
include/uapi/linux/ethtool.h | 20 ++++++
include/uapi/linux/ethtool_netlink.h | 17 +++++
net/ethtool/netlink.h | 3 +-
net/ethtool/stats.c | 53 ++++++++++++--
net/ethtool/strset.c | 5 ++
tools/net/ynl/ethtool.py | 9 ++-
13 files changed, 227 insertions(+), 10 deletions(-)
--
2.42.0
On 2/23/2024 11:24 AM, Rahul Rameshbabu wrote: > The goal of this patch series is to introduce a common set of ethtool statistics > for hardware timestamping that a driver implementer can hook into. The > statistics counters added are based on what I believe are common > patterns/behaviors found across various hardware timestamping implementations > seen in the kernel tree today. The mlx5 family of devices is used as the PoC for > this patch series. Other vendors are more than welcome to chim in on this > series. > > Statistics can be queried from either the DMA or PHY layers. I think this > concept of layer-based statistics selection and the general timestamping layer > selection work Kory Maincent is working on can be converged together [1]. > > [1] https://lore.kernel.org/netdev/20240216-feature_ptp_netnext-v8-0-510f42f444fb@bootlin.com/ > Makes sense! I like this direction, I had meant to work on this for the Intel parts, but got sidetracked by other tasks. I look forward to seeing what you've done here.
On Fri, 23 Feb, 2024 13:00:10 -0800 Jacob Keller <jacob.e.keller@intel.com> wrote: > On 2/23/2024 11:24 AM, Rahul Rameshbabu wrote: >> The goal of this patch series is to introduce a common set of ethtool statistics >> for hardware timestamping that a driver implementer can hook into. The >> statistics counters added are based on what I believe are common >> patterns/behaviors found across various hardware timestamping implementations >> seen in the kernel tree today. The mlx5 family of devices is used as the PoC for >> this patch series. Other vendors are more than welcome to chim in on this >> series. >> >> Statistics can be queried from either the DMA or PHY layers. I think this >> concept of layer-based statistics selection and the general timestamping layer >> selection work Kory Maincent is working on can be converged together [1]. >> >> [1] https://lore.kernel.org/netdev/20240216-feature_ptp_netnext-v8-0-510f42f444fb@bootlin.com/ >> > > Makes sense! I like this direction, I had meant to work on this for the > Intel parts, but got sidetracked by other tasks. I still have a back burner task for a cyclecounter API change you suggested a while back.... Hoping to get to that after getting some large features out of the way in the next two months. -- Thanks, Rahul Rameshbabu
On 2/23/2024 1:00 PM, Jacob Keller wrote: > > > On 2/23/2024 11:24 AM, Rahul Rameshbabu wrote: >> The goal of this patch series is to introduce a common set of ethtool statistics >> for hardware timestamping that a driver implementer can hook into. The >> statistics counters added are based on what I believe are common >> patterns/behaviors found across various hardware timestamping implementations >> seen in the kernel tree today. The mlx5 family of devices is used as the PoC for >> this patch series. Other vendors are more than welcome to chim in on this >> series. >> >> Statistics can be queried from either the DMA or PHY layers. I think this >> concept of layer-based statistics selection and the general timestamping layer >> selection work Kory Maincent is working on can be converged together [1]. >> >> [1] https://lore.kernel.org/netdev/20240216-feature_ptp_netnext-v8-0-510f42f444fb@bootlin.com/ >> > > Makes sense! I like this direction, I had meant to work on this for the > Intel parts, but got sidetracked by other tasks. I look forward to > seeing what you've done here. > I'm fairly happy with the series overall, and the entire thing read pretty straight forward. I'd be happy to follow up with patches to convert the ice driver to report these statistics as well.
The goal of this patch series is to introduce a common set of ethtool statistics
for hardware timestamping that a driver implementer can hook into. The
statistics counters added are based on what I believe are common
patterns/behaviors found across various hardware timestamping implementations
seen in the kernel tree today. The mlx5 family of devices is used as the PoC for
this patch series. Other vendors are more than welcome to chim in on this
series.
Changes since v1:
- Dropped the late statistics counter since that was not general enough
- Dropped the layer selection attribute for timestamping statistics
- Take Jakub's suggestion and converted to ETHTOOL_FLAG_STATS
- Provided a working interface to query these new statistics from
tools/net/ynl/ethtool.py
Link: https://lore.kernel.org/netdev/20240223192658.45893-1-rrameshbabu@nvidia.com/
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
Rahul Rameshbabu (6):
ethtool: add interface to read Tx hardware timestamping statistics
net/mlx5e: Introduce lost_cqe statistic counter for PTP Tx port
timestamping CQ
net/mlx5e: Introduce timestamps statistic counter for Tx DMA layer
net/mlx5e: Implement ethtool hardware timestamping statistics
tools: ynl: ethtool.py: Make tool invokable from any CWD
tools: ynl: ethtool.py: Output timestamping statistics from tsinfo-get
operation
Documentation/netlink/specs/ethtool.yaml | 20 +++++++
.../ethernet/mellanox/mlx5/counters.rst | 11 ++++
.../net/ethernet/mellanox/mlx5/core/en/ptp.c | 1 +
.../ethernet/mellanox/mlx5/core/en_ethtool.c | 9 ++++
.../ethernet/mellanox/mlx5/core/en_stats.c | 48 +++++++++++++++++
.../ethernet/mellanox/mlx5/core/en_stats.h | 4 ++
.../net/ethernet/mellanox/mlx5/core/en_tx.c | 6 ++-
include/linux/ethtool.h | 21 ++++++++
include/uapi/linux/ethtool_netlink.h | 15 ++++++
net/ethtool/tsinfo.c | 52 ++++++++++++++++++-
tools/net/ynl/ethtool.py | 19 +++++--
11 files changed, 200 insertions(+), 6 deletions(-)
--
2.42.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(-)
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;
}
--
2.42.0On 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...
>
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
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 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
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).
> -----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 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 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
Track the number of times a CQE was expected to not be delivered on PTP Tx
port timestamping CQ. A CQE is expected to not be delivered if a certain
amount of time passes since the corresponding CQE containing the DMA
timestamp information has arrived. Increment the late_cqe counter when such
a CQE does manage to be delivered to the CQ.
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
.../device_drivers/ethernet/mellanox/mlx5/counters.rst | 6 ++++++
drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c | 1 +
drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 1 +
drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 1 +
4 files changed, 9 insertions(+)
diff --git a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
index f69ee1ebee01..5464cd9e2694 100644
--- a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
+++ b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
@@ -702,6 +702,12 @@ the software port.
the device typically ensures not posting the CQE.
- Error
+ * - `ptp_cq[i]_lost_cqe`
+ - Number of times a CQE is expected to not be delivered on the PTP
+ timestamping CQE by the device due to a time delta elapsing. If such a
+ CQE is somehow delivered, `ptp_cq[i]_late_cqe` is incremented.
+ - Error
+
.. [#ring_global] The corresponding ring and global counters do not share the
same name (i.e. do not follow the common naming scheme).
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
index fd4ef6431142..1dd4bf7f7dbe 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c
@@ -169,6 +169,7 @@ static void mlx5e_ptpsq_mark_ts_cqes_undelivered(struct mlx5e_ptpsq *ptpsq,
WARN_ON_ONCE(!pos->inuse);
pos->inuse = false;
list_del(&pos->entry);
+ ptpsq->cq_stats->lost_cqe++;
}
spin_unlock(&cqe_list->tracker_list_lock);
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index 4b96ad657145..7e63d7c88894 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -2158,6 +2158,7 @@ static const struct counter_desc ptp_cq_stats_desc[] = {
{ MLX5E_DECLARE_PTP_CQ_STAT(struct mlx5e_ptp_cq_stats, abort) },
{ MLX5E_DECLARE_PTP_CQ_STAT(struct mlx5e_ptp_cq_stats, abort_abs_diff_ns) },
{ MLX5E_DECLARE_PTP_CQ_STAT(struct mlx5e_ptp_cq_stats, late_cqe) },
+ { MLX5E_DECLARE_PTP_CQ_STAT(struct mlx5e_ptp_cq_stats, lost_cqe) },
};
static const struct counter_desc ptp_rq_stats_desc[] = {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 12b3607afecd..03f6265d3ed5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -461,6 +461,7 @@ struct mlx5e_ptp_cq_stats {
u64 abort;
u64 abort_abs_diff_ns;
u64 late_cqe;
+ u64 lost_cqe;
};
struct mlx5e_rep_stats {
--
2.42.0Count number of transmitted packets that were hardware timestamped at the
device DMA layer.
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
.../device_drivers/ethernet/mellanox/mlx5/counters.rst | 5 +++++
drivers/net/ethernet/mellanox/mlx5/core/en_stats.c | 2 ++
drivers/net/ethernet/mellanox/mlx5/core/en_stats.h | 1 +
drivers/net/ethernet/mellanox/mlx5/core/en_tx.c | 6 ++++--
4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
index 5464cd9e2694..fed821ef9b09 100644
--- a/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
+++ b/Documentation/networking/device_drivers/ethernet/mellanox/mlx5/counters.rst
@@ -300,6 +300,11 @@ the software port.
in the beginning of the queue. This is a normal condition.
- Informative
+ * - `tx[i]_timestamps`
+ - Transmitted packets that were hardware timestamped at the device's DMA
+ layer.
+ - Informative
+
* - `tx[i]_added_vlan_packets`
- The number of packets sent where vlan tag insertion was offloaded to the
hardware.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index 7e63d7c88894..bc31196d348a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -2046,6 +2046,7 @@ static const struct counter_desc sq_stats_desc[] = {
{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, csum_partial_inner) },
{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, added_vlan_packets) },
{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, nop) },
+ { MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, timestamps) },
{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, mpwqe_blks) },
{ MLX5E_DECLARE_TX_STAT(struct mlx5e_sq_stats, mpwqe_pkts) },
#ifdef CONFIG_MLX5_EN_TLS
@@ -2198,6 +2199,7 @@ static const struct counter_desc qos_sq_stats_desc[] = {
{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, csum_partial_inner) },
{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, added_vlan_packets) },
{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, nop) },
+ { MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, timestamps) },
{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, mpwqe_blks) },
{ MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, mpwqe_pkts) },
#ifdef CONFIG_MLX5_EN_TLS
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 03f6265d3ed5..3c634c5fd420 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -429,6 +429,7 @@ struct mlx5e_sq_stats {
u64 stopped;
u64 dropped;
u64 recover;
+ u64 timestamps;
/* dirtied @completion */
u64 cqes ____cacheline_aligned_in_smp;
u64 wake;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 5c166d9d2dca..5acba323246e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -748,11 +748,13 @@ static void mlx5e_consume_skb(struct mlx5e_txqsq *sq, struct sk_buff *skb,
u64 ts = get_cqe_ts(cqe);
hwts.hwtstamp = mlx5e_cqe_ts_to_ns(sq->ptp_cyc2time, sq->clock, ts);
- if (sq->ptpsq)
+ if (sq->ptpsq) {
mlx5e_skb_cb_hwtstamp_handler(skb, MLX5E_SKB_CB_CQE_HWTSTAMP,
hwts.hwtstamp, sq->ptpsq->cq_stats);
- else
+ } else {
skb_tstamp_tx(skb, &hwts);
+ sq->stats->timestamps++;
+ }
}
napi_consume_skb(skb, napi_budget);
--
2.42.0Feed driver statistics counters related to hardware timestamping to
standardized ethtool hardware timestamping statistics group.
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
.../ethernet/mellanox/mlx5/core/en_ethtool.c | 9 ++++
.../ethernet/mellanox/mlx5/core/en_stats.c | 45 +++++++++++++++++++
.../ethernet/mellanox/mlx5/core/en_stats.h | 2 +
3 files changed, 56 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index cc51ce16df14..d3b77054c30a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -2381,6 +2381,14 @@ static void mlx5e_get_rmon_stats(struct net_device *netdev,
mlx5e_stats_rmon_get(priv, rmon_stats, ranges);
}
+static void mlx5e_get_ts_stats(struct net_device *netdev,
+ struct ethtool_ts_stats *ts_stats)
+{
+ struct mlx5e_priv *priv = netdev_priv(netdev);
+
+ mlx5e_stats_ts_get(priv, ts_stats);
+}
+
const struct ethtool_ops mlx5e_ethtool_ops = {
.cap_rss_ctx_supported = true,
.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
@@ -2430,5 +2438,6 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
.get_eth_mac_stats = mlx5e_get_eth_mac_stats,
.get_eth_ctrl_stats = mlx5e_get_eth_ctrl_stats,
.get_rmon_stats = mlx5e_get_rmon_stats,
+ .get_ts_stats = mlx5e_get_ts_stats,
.get_link_ext_stats = mlx5e_get_link_ext_stats
};
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index bc31196d348a..465c1423528f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -1155,6 +1155,51 @@ void mlx5e_stats_rmon_get(struct mlx5e_priv *priv,
*ranges = mlx5e_rmon_ranges;
}
+void mlx5e_stats_ts_get(struct mlx5e_priv *priv,
+ struct ethtool_ts_stats *ts_stats)
+{
+ int i, j;
+
+ mutex_lock(&priv->state_lock);
+
+ if (priv->tx_ptp_opened) {
+ struct mlx5e_ptp *ptp = priv->channels.ptp;
+
+ ts_stats->pkts = 0;
+ ts_stats->err = 0;
+ ts_stats->lost = 0;
+
+ /* Aggregate stats across all TCs */
+ for (i = 0; i < ptp->num_tc; i++) {
+ struct mlx5e_ptp_cq_stats *stats =
+ ptp->ptpsq[i].cq_stats;
+
+ ts_stats->pkts += stats->cqe;
+ ts_stats->err += stats->abort + stats->err_cqe +
+ stats->late_cqe;
+ ts_stats->lost += stats->lost_cqe;
+ }
+ } else {
+ /* DMA layer will always successfully timestamp packets. Other
+ * counters do not make sense for this layer.
+ */
+ ts_stats->pkts = 0;
+
+ /* Aggregate stats across all SQs */
+ for (j = 0; j < priv->channels.num; j++) {
+ struct mlx5e_channel *c = priv->channels.c[j];
+
+ for (i = 0; i < c->num_tc; i++) {
+ struct mlx5e_sq_stats *stats = c->sq[i].stats;
+
+ ts_stats->pkts += stats->timestamps;
+ }
+ }
+ }
+
+ mutex_unlock(&priv->state_lock);
+}
+
#define PPORT_PHY_STATISTICAL_OFF(c) \
MLX5_BYTE_OFF(ppcnt_reg, \
counter_set.phys_layer_statistical_cntrs.c##_high)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
index 3c634c5fd420..7b3e6cf1229a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
@@ -126,6 +126,8 @@ void mlx5e_stats_eth_ctrl_get(struct mlx5e_priv *priv,
void mlx5e_stats_rmon_get(struct mlx5e_priv *priv,
struct ethtool_rmon_stats *rmon,
const struct ethtool_rmon_hist_range **ranges);
+void mlx5e_stats_ts_get(struct mlx5e_priv *priv,
+ struct ethtool_ts_stats *ts_stats);
void mlx5e_get_link_ext_stats(struct net_device *dev,
struct ethtool_link_ext_stats *stats);
--
2.42.0ethtool.py depends on yml files in a specific location of the linux kernel
tree. Using relative lookup for those files means that ethtool.py would
need to be run under tools/net/ynl/. Lookup needed yml files without
depending on the current working directory that ethtool.py is invoked from.
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
tools/net/ynl/ethtool.py | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/net/ynl/ethtool.py b/tools/net/ynl/ethtool.py
index 6c9f7e31250c..44ba3ba58ed9 100755
--- a/tools/net/ynl/ethtool.py
+++ b/tools/net/ynl/ethtool.py
@@ -6,6 +6,7 @@ import json
import pprint
import sys
import re
+import os
from lib import YnlFamily
@@ -152,8 +153,11 @@ def main():
global args
args = parser.parse_args()
- spec = '../../../Documentation/netlink/specs/ethtool.yaml'
- schema = '../../../Documentation/netlink/genetlink-legacy.yaml'
+ script_abs_dir = os.path.dirname(os.path.abspath(sys.argv[0]))
+ spec = os.path.join(script_abs_dir,
+ '../../../Documentation/netlink/specs/ethtool.yaml')
+ schema = os.path.join(script_abs_dir,
+ '../../../Documentation/netlink/genetlink-legacy.yaml')
ynl = YnlFamily(spec, schema)
--
2.42.0On Sat, 9 Mar 2024 00:44:39 -0800 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote: > ethtool.py depends on yml files in a specific location of the linux kernel > tree. Using relative lookup for those files means that ethtool.py would > need to be run under tools/net/ynl/. Lookup needed yml files without > depending on the current working directory that ethtool.py is invoked from. > > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> You should send this patch standalone as it has no relevance to the series subject. -- Köry Maincent, Bootlin Embedded Linux and kernel engineering https://bootlin.com
Print the nested stats attribute containing timestamping statistics when
the --show-time-stamping flag is used.
[root@binary-eater-vm-01 linux-ethtool-ts]# ./tools/net/ynl/ethtool.py --show-time-stamping mlx5_1
Time stamping parameters for mlx5_1:
Capabilities:
hardware-transmit
hardware-receive
hardware-raw-clock
PTP Hardware Clock: 0
Hardware Transmit Timestamp Modes:
off
on
Hardware Receive Filter Modes:
none
all
Statistics:
tx-pkts: 8
tx-lost: 0
tx-err: 0
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
tools/net/ynl/ethtool.py | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/net/ynl/ethtool.py b/tools/net/ynl/ethtool.py
index 44ba3ba58ed9..193399e7fbd1 100755
--- a/tools/net/ynl/ethtool.py
+++ b/tools/net/ynl/ethtool.py
@@ -324,7 +324,13 @@ def main():
return
if args.show_time_stamping:
- tsinfo = dumpit(ynl, args, 'tsinfo-get')
+ req = {
+ 'header': {
+ 'flags': 1 << 2,
+ },
+ }
+
+ tsinfo = dumpit(ynl, args, 'tsinfo-get', req)
print(f'Time stamping parameters for {args.device}:')
@@ -338,6 +344,9 @@ def main():
print('Hardware Receive Filter Modes:')
[print(f'\t{v}') for v in bits_to_dict(tsinfo['rx-filters'])]
+
+ print('Statistics:')
+ [print(f'\t{k}: {v}') for k, v in tsinfo['stats'].items()]
return
print(f'Settings for {args.device}:')
--
2.42.0On Sat, 9 Mar 2024 00:44:40 -0800 Rahul Rameshbabu wrote:
> + req = {
> + 'header': {
> + 'flags': 1 << 2,
> + },
> + }
You should be able to use the name of the flag instead of the raw value.
Jiri added that recently, IIRC.
On Tue, 12 Mar, 2024 16:55:44 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> On Sat, 9 Mar 2024 00:44:40 -0800 Rahul Rameshbabu wrote:
>> + req = {
>> + 'header': {
>> + 'flags': 1 << 2,
>> + },
>> + }
>
> You should be able to use the name of the flag instead of the raw value.
> Jiri added that recently, IIRC.
I think this is for 'flag' type attributes. Not for the "header" flags
for the ethtool request, so I believe this cannot be done here, since
the header flags are a u32 type, not a flag type.
https://lore.kernel.org/netdev/20240222134351.224704-2-jiri@resnulli.us/
-
name: header
attributes:
-
name: dev-index
type: u32
-
name: dev-name
type: string
-
name: flags
type: u32
vs
-
name: bitset-bit
attributes:
-
name: index
type: u32
-
name: name
type: string
-
name: value
type: flag
So I believe Jiri's change applies for the latter, not the former (could
be wrong here).
--
Thanks,
Rahul Rameshbabu
On Wed, 13 Mar 2024 17:22:50 -0700 Rahul Rameshbabu wrote:
> I think this is for 'flag' type attributes. Not for the "header" flags
> for the ethtool request, so I believe this cannot be done here, since
> the header flags are a u32 type, not a flag type.
>
> https://lore.kernel.org/netdev/20240222134351.224704-2-jiri@resnulli.us/
>
> -
> name: header
> attributes:
> -
> name: dev-index
> type: u32
> -
> name: dev-name
> type: string
> -
> name: flags
> type: u32
>
> vs
>
> -
> name: bitset-bit
> attributes:
> -
> name: index
> type: u32
> -
> name: name
> type: string
> -
> name: value
> type: flag
>
> So I believe Jiri's change applies for the latter, not the former (could
> be wrong here).
Ah, we're missing the enum definition and linking :S
I mean:
diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 197208f419dc..e1626c94d93b 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -16,6 +16,10 @@ doc: Partial family for Ethtool Netlink.
name: stringset
type: enum
entries: []
+ -
+ name: header-flags
+ type: flags
+ entries: [ compact-bitset, omit-reply, stats ]
attribute-sets:
-
@@ -30,6 +34,7 @@ doc: Partial family for Ethtool Netlink.
-
name: flags
type: u32
+ enum: header-flags
-
name: bitset-bit
See if that works and feel free to post it with my suggested-by
On Wed, 13 Mar 2024 17:47:07 -0700 Jakub Kicinski wrote: > + - > + name: header-flags > + type: flags > + entries: [ compact-bitset, omit-reply, stats ] Ah. Throw in an empty: enum-name: into this, or change the uAPI so that an enum called ethtool_header_flags actually exists :) Otherwise make -C tools/net/ynl will break
On Thu, 14 Mar, 2024 13:04:36 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Wed, 13 Mar 2024 17:47:07 -0700 Jakub Kicinski wrote: >> + - >> + name: header-flags >> + type: flags >> + entries: [ compact-bitset, omit-reply, stats ] > > Ah. Throw in an empty: > > enum-name: > > into this, or change the uAPI so that an enum called > ethtool_header_flags actually exists :) > > Otherwise make -C tools/net/ynl will break Already handled this in my patches by making the enum for ethtool_header_flags. Thanks for double checking though. -- Rahul Rameshbabu
On Wed, 13 Mar, 2024 17:47:07 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
>
> Ah, we're missing the enum definition and linking :S
>
> I mean:
>
> diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
> index 197208f419dc..e1626c94d93b 100644
> --- a/Documentation/netlink/specs/ethtool.yaml
> +++ b/Documentation/netlink/specs/ethtool.yaml
> @@ -16,6 +16,10 @@ doc: Partial family for Ethtool Netlink.
> name: stringset
> type: enum
> entries: []
> + -
> + name: header-flags
> + type: flags
> + entries: [ compact-bitset, omit-reply, stats ]
I am running into some strange issues with this even after regenerating
ynl generated/ by running make under tools/net/ynl/.
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 824, in _op
msg += self._add_attr(op.attr_set.name, name, value, search_attrs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 459, in _add_attr
attr_payload += self._add_attr(attr['nested-attributes'],
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/root/linux-ethtool-ts/tools/net/ynl/lib/ynl.py", line 481, in _add_attr
attr_payload = format.pack(int(value))
^^^^^^^^^^
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'dict'
What's your expectation for how the request structure would look like? I
have tried the following.
if args.show_time_stamping:
req = {
'header': {
'flags': 'stats',
},
}
if args.show_time_stamping:
req = {
'header': {
'flags': {
'stats': True,
},
},
}
I tried looking through the lib/ynl.py code, but I did not understand
how the 'flags' type was specifically handled.
>
> attribute-sets:
> -
> @@ -30,6 +34,7 @@ doc: Partial family for Ethtool Netlink.
> -
> name: flags
> type: u32
> + enum: header-flags
>
> -
> name: bitset-bit
>
> See if that works and feel free to post it with my suggested-by
--
Thanks,
Rahul Rameshbabu
On Wed, 13 Mar 2024 23:07:22 -0700 Rahul Rameshbabu wrote:
> What's your expectation for how the request structure would look like? I
> have tried the following.
>
> if args.show_time_stamping:
> req = {
> 'header': {
> 'flags': 'stats',
> },
> }
>
> if args.show_time_stamping:
> req = {
> 'header': {
> 'flags': {
> 'stats': True,
> },
> },
> }
>
> I tried looking through the lib/ynl.py code, but I did not understand
> how the 'flags' type was specifically handled.
Rebased on latest net-next?
I used this:
./tools/net/ynl/cli.py \
--spec Documentation/netlink/specs/ethtool.yaml \
--do fec-get --json '{"header":{"dev-index": 2, "flags": "stats"}}'
On Thu, 14 Mar, 2024 11:05:53 -0700 Jakub Kicinski <kuba@kernel.org> wrote:
> On Wed, 13 Mar 2024 23:07:22 -0700 Rahul Rameshbabu wrote:
>> What's your expectation for how the request structure would look like? I
>> have tried the following.
>>
>> if args.show_time_stamping:
>> req = {
>> 'header': {
>> 'flags': 'stats',
>> },
>> }
>>
>> if args.show_time_stamping:
>> req = {
>> 'header': {
>> 'flags': {
>> 'stats': True,
>> },
>> },
>> }
>>
>> I tried looking through the lib/ynl.py code, but I did not understand
>> how the 'flags' type was specifically handled.
>
> Rebased on latest net-next?
Thanks, I was using our internal branch that mixes net-next with some of
our mlx5 changes for verification purposes, and it was not rebased to
latest net-next. Your suggestion works as expected with latest net-next
and will be integrated into the series (with the suggested-by trailer
added).
>
> I used this:
>
> ./tools/net/ynl/cli.py \
> --spec Documentation/netlink/specs/ethtool.yaml \
> --do fec-get --json '{"header":{"dev-index": 2, "flags": "stats"}}'
--
Thanks,
Rahul Rameshbabu
© 2016 - 2026 Red Hat, Inc.