Date:   Thu, 11 Aug 2022 09:18:03 -0700
From:   sdf@...gle.com
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, jacob.e.keller@...el.com, vadfed@...com,
        johannes@...solutions.net, jiri@...nulli.us, dsahern@...nel.org,
        stephen@...workplumber.org, fw@...len.de, linux-doc@...r.kernel.org
Subject: Re: [RFC net-next 4/4] ynl: add a sample user for ethtool

On 08/10, Jakub Kicinski wrote:
> A sample schema describing ethtool channels and a script showing
> that it works. The script is called like this:

>   ./tools/net/ynl/samples/ethtool.py \
>      --spec Documentation/netlink/bindings/ethtool.yaml \
>      --device eth0

> I have schemas for genetlink, FOU and the proposed DPLL subsystem,
> to validate that the concept has wide applicability, but ethtool
> feels like the best demo of the 4.

Looks super promising! I'm going to comment mostly here because it's easier
to talk about the sample schema definition.

> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   Documentation/netlink/bindings/ethtool.yaml | 115 ++++++++++++++++++++
>   tools/net/ynl/samples/ethtool.py            |  30 +++++
>   2 files changed, 145 insertions(+)
>   create mode 100644 Documentation/netlink/bindings/ethtool.yaml
>   create mode 100755 tools/net/ynl/samples/ethtool.py

> diff --git a/Documentation/netlink/bindings/ethtool.yaml  
> b/Documentation/netlink/bindings/ethtool.yaml
> new file mode 100644
> index 000000000000..b4540d60b4b3
> --- /dev/null
> +++ b/Documentation/netlink/bindings/ethtool.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +
> +name: ethtool
> +
> +description: |
> +  Ethernet device configuration interface.
> +

[..]

> +attr-cnt-suffix: CNT

Is it a hack to make the generated header fit into existing
implementation? Should we #define ETHTOOL_XXX_CNT ETHTOOL_XXX in
the implementation instead? (or s/ETHTOOL_XXX_CNT/ETHTOOL_XXX/ the
source itself?)

> +attribute-spaces:

Are you open to bike shedding? :-) I like how ethtool_netlink.h calls
these 'message types'.

> +  -
> +    name: header
> +    name-prefix: ETHTOOL_A_HEADER_

Any issue with name-prefix+name-suffix being non-greppable? Have you tried
something like this instead:

- name: ETHTOOL_A_HEADER # this is fake, for ynl reference only
   attributes:
     - name: ETHTOOL_A_HEADER_DEV_INDEX
       val:
       type:
     - name ETHTOOL_A_HEADER_DEV_NAME
       ..

It seems a bit easier to map the spec into what it's going to produce.
For example, it took me a while to translate 'channels_get' below into
ETHTOOL_MSG_CHANNELS_GET.

Or is it too much ETHTOOL_A_HEADER_?

> +    attributes:
> +      -
> +        name: dev_index
> +        val: 1
> +        type: u32
> +      -
> +        name: dev_name
> +        type: nul-string

[..]

> +        len: ALTIFNAMSIZ - 1

Not sure how strict you want to be here. ALTIFNAMSIZ is defined
somewhere else it seems? (IOW, do we want to have implicit dependencies
on external/uapi headers?)

> +      -
> +        name: flags
> +        type: u32
> +  -
> +    name: channels
> +    name-prefix: ETHTOOL_A_CHANNELS_
> +    attributes:
> +      -
> +        name: header
> +        val: 1
> +        type: nest
> +        nested-attributes: header
> +      -
> +        name: rx_max
> +        type: u32
> +      -
> +        name: tx_max
> +        type: u32
> +      -
> +        name: other_max
> +        type: u32
> +      -
> +        name: combined_max
> +        type: u32
> +      -
> +        name: rx_count
> +        type: u32
> +      -
> +        name: tx_count
> +        type: u32
> +      -
> +        name: other_count
> +        type: u32
> +      -
> +        name: combined_count
> +        type: u32
> +
> +headers:
> +  user: linux/if.h
> +  uapi: linux/ethtool_netlink.h
> +
> +operations:
> +  name-prefix: ETHTOOL_MSG_
> +  async-prefix: ETHTOOL_MSG_
> +  list:
> +    -
> +      name: channels_get
> +      val: 17
> +      description: Get current and max supported number of channels.
> +      attribute-space: channels
> +      do:
> +        request:
> +          attributes:
> +            - header
> +        reply: &channel_reply
> +          attributes:
> +            - header
> +            - rx_max
> +            - tx_max
> +            - other_max
> +            - combined_max
> +            - rx_count
> +            - tx_count
> +            - other_count
> +            - combined_count
> +      dump:
> +        reply: *channel_reply
> +
> +    -
> +      name: channels_ntf
> +      description: Notification for device changing its number of  
> channels.
> +      notify: channels_get
> +      mcgrp: monitor
> +
> +    -
> +      name: channels_set
> +      description: Set number of channels.
> +      attribute-space: channels
> +      do:
> +        request:
> +          attributes:

[..]

> +            - header
> +            - rx_count
> +            - tx_count
> +            - other_count
> +            - combined_count

My netlink is super rusty, might be worth mentioning in the spec: these
are possible attributes, but are all of them required?

You also mention the validation part in the cover letter, do you plan
add some of these policy properties to the spec or everything is
there already? (I'm assuming we care about the types which we have above and
optional/required attribute indication?)

> +
> +mcast-groups:
> +  name-prefix: ETHTOOL_MCGRP_
> +  name-suffix: _NAME
> +  list:
> +    -
> +      name: monitor
> diff --git a/tools/net/ynl/samples/ethtool.py  
> b/tools/net/ynl/samples/ethtool.py
> new file mode 100755
> index 000000000000..63c8e29f8e5d
> --- /dev/null
> +++ b/tools/net/ynl/samples/ethtool.py
> @@ -0,0 +1,30 @@
> +#!/usr/bin/env python
> +# SPDX-License-Identifier: BSD-3-Clause
> +
> +import argparse
> +
> +from ynl import YnlFamily
> +
> +
> +def main():
> +    parser = argparse.ArgumentParser(description='YNL ethtool sample')
> +    parser.add_argument('--spec', dest='spec', type=str, required=True)
> +    parser.add_argument('--schema', dest='schema', type=str)
> +    parser.add_argument('--device', dest='dev_name', type=str)
> +    parser.add_argument('--ifindex', dest='ifindex', type=str)
> +    args = parser.parse_args()
> +
> +    ynl = YnlFamily(args.spec)
> +
> +    if args.dev_name:
> +        channels = ynl.channels_get({'header': {'dev_name':  
> args.dev_name}})
> +    elif args.ifindex:
> +        channels = ynl.channels_get({'header': {'dev_index':  
> args.ifindex}})
> +    else:
> +        return
> +    print('Netlink responded with:')
> +    print(channels)
> +
> +
> +if __name__ == "__main__":
> +    main()
> --
> 2.37.1

Powered by blists - more mailing lists