| Message ID | 20240309084440.299358-7-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | RFC |
| 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: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 <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
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
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}:')
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(-)