[PATCH v3 net-next 0/3] Add support for encoding multi-attr to ynl

[PATCH v3 net-next 0/3] Add support for encoding multi-attr to ynl

[Alessandro Marcolini]

This patchset add the support for encoding multi-attr attributes, making
it possible to use ynl with qdisc which have this kind of attributes
(e.g: taprio, ets).

Patch 1 corrects two docstrings in nlspec.py
Patch 2 adds the multi-attr attribute to taprio entry
Patch 3 adds the support for encoding multi-attr

v1 --> v2:
- Use SearchAttrs instead of ChainMap

v2 --> v3:
- Handle multi-attr at every level, not only in nested attributes

Alessandro Marcolini (3):
  tools: ynl: correct typo and docstring
  doc: netlink: specs: tc: add multi-attr to tc-taprio-sched-entry
  tools: ynl: add support for encoding multi-attr

 Documentation/netlink/specs/tc.yaml | 1 +
 tools/net/ynl/lib/nlspec.py         | 7 +++----
 tools/net/ynl/lib/ynl.py            | 5 +++++
 3 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v3 net-next 1/3] tools: ynl: correct typo and docstring

[Alessandro Marcolini]

Correct typo in SpecAttr docstring. Changed SpecSubMessageFormat
docstring.

Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>
Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/nlspec.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 5d197a12ab8d..fbce52395b3b 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -144,7 +144,7 @@ class SpecEnumSet(SpecElement):
 
 
 class SpecAttr(SpecElement):
-    """ Single Netlink atttribute type
+    """ Single Netlink attribute type
 
     Represents a single attribute type within an attr space.
 
@@ -308,10 +308,9 @@ class SpecSubMessage(SpecElement):
 
 
 class SpecSubMessageFormat(SpecElement):
-    """ Netlink sub-message definition
+    """ Netlink sub-message format definition
 
-    Represents a set of sub-message formats for polymorphic nlattrs
-    that contain type-specific sub messages.
+    Represents a single format for a sub-message.
 
     Attributes:
         value         attribute value to match against type selector
-- 
2.43.0

[PATCH v3 net-next 2/3] doc: netlink: specs: tc: add multi-attr to tc-taprio-sched-entry

[Alessandro Marcolini]

Add multi-attr attribute to tc-taprio-sched-entry to specify multiple
entries.

Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>
Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/specs/tc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
index 4b21b00dbebe..324fa182cd14 100644
--- a/Documentation/netlink/specs/tc.yaml
+++ b/Documentation/netlink/specs/tc.yaml
@@ -3376,6 +3376,7 @@ attribute-sets:
         name: entry
         type: nest
         nested-attributes: tc-taprio-sched-entry
+        multi-attr: true
   -
     name: tc-taprio-sched-entry
     attributes:
-- 
2.43.0

[PATCH v3 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Alessandro Marcolini]

Multi-attr elements could not be encoded because of missing logic in the
ynl code. Enable encoding of these attributes by checking if the
attribute is a multi-attr and if the value to be processed is a list.

This has been tested both with the taprio and ets qdisc which contain
this kind of attributes.

Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>
---
 tools/net/ynl/lib/ynl.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 0f4193cc2e3b..d5779d023b10 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -444,6 +444,11 @@ class YnlFamily(SpecFamily):
         except KeyError:
             raise Exception(f"Space '{space}' has no attribute '{name}'")
         nl_type = attr.value
+        if attr.is_multi and isinstance(value, list):
+            attr_payload = b''
+            for subvalue in value:
+                attr_payload += self._add_attr(space, name, subvalue, search_attrs)
+            return attr_payload
         if attr["type"] == 'nest':
             nl_type |= Netlink.NLA_F_NESTED
             attr_payload = b''
-- 
2.43.0

Re: [PATCH v3 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Donald Hunter]

Alessandro Marcolini <alessandromarcolini99@gmail.com> writes:

> Multi-attr elements could not be encoded because of missing logic in the
> ynl code. Enable encoding of these attributes by checking if the
> attribute is a multi-attr and if the value to be processed is a list.
>
> This has been tested both with the taprio and ets qdisc which contain
> this kind of attributes.
>
> Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

> ---
>  tools/net/ynl/lib/ynl.py | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 0f4193cc2e3b..d5779d023b10 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -444,6 +444,11 @@ class YnlFamily(SpecFamily):
>          except KeyError:
>              raise Exception(f"Space '{space}' has no attribute '{name}'")
>          nl_type = attr.value
> +        if attr.is_multi and isinstance(value, list):
> +            attr_payload = b''
> +            for subvalue in value:
> +                attr_payload += self._add_attr(space, name, subvalue, search_attrs)
> +            return attr_payload
>          if attr["type"] == 'nest':
>              nl_type |= Netlink.NLA_F_NESTED
>              attr_payload = b''

Re: [PATCH v3 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Jakub Kicinski]

On Fri,  2 Feb 2024 15:00:05 +0100 Alessandro Marcolini wrote:
>          nl_type = attr.value
> +        if attr.is_multi and isinstance(value, list):
> +            attr_payload = b''
> +            for subvalue in value:
> +                attr_payload += self._add_attr(space, name, subvalue, search_attrs)
> +            return attr_payload
>          if attr["type"] == 'nest':

nit: would you mind adding an empty line before and after the new
block? It's logically separate from the other code, sort of an
alternative to the "actual" handling, as well as finding the attr set.

Also agreed on adding an example to the cover letter (either one or
both).

With that feel free to add:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

on all 3 patches in v4.

Re: [PATCH v3 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Alessandro Marcolini]

On 2/3/24 03:17, Jakub Kicinski wrote:
> On Fri,  2 Feb 2024 15:00:05 +0100 Alessandro Marcolini wrote:
>>          nl_type = attr.value
>> +        if attr.is_multi and isinstance(value, list):
>> +            attr_payload = b''
>> +            for subvalue in value:
>> +                attr_payload += self._add_attr(space, name, subvalue, search_attrs)
>> +            return attr_payload
>>          if attr["type"] == 'nest':
> nit: would you mind adding an empty line before and after the new
> block? It's logically separate from the other code, sort of an
> alternative to the "actual" handling, as well as finding the attr set.

This makes sense.

> Also agreed on adding an example to the cover letter (either one or
> both).
>
> With that feel free to add:
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
>
> on all 3 patches in v4.
Perfect, I'll post the v4 soon with both the examples. Thanks!

Re: [PATCH v3 net-next 0/3] Add support for encoding multi-attr to ynl

[Donald Hunter]

Alessandro Marcolini <alessandromarcolini99@gmail.com> writes:

> This patchset add the support for encoding multi-attr attributes, making
> it possible to use ynl with qdisc which have this kind of attributes
> (e.g: taprio, ets).

I liked the fact that your original cover letter included an example
ynl.py command. With this patchset plus the merged struct encoding
patch, you can add a taprio qdisc like this:

# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/tc.yaml \
  --do newqdisc --create --json '{
  "family":1, "ifindex":111, "handle":65536, "parent":4294967295, "info":0,
   "kind":"taprio",
   "stab":{
       "base": {
         "cell-log": 0,
         "size-log": 0,
         "cell-align": 0,
         "overhead": 31,
         "linklayer": 0,
         "mpu": 0,
         "mtu": 0,
         "tsize": 0
       }
   },
   "options":{
       "priomap": {
           "num-tc": 3,
           "prio-tc-map": "01010101010101010101010101010101",
           "hw": 0,
           "count": "0100010002000000000000000000000000000000000000000000000000000000",
           "offset": "0100020003000000000000000000000000000000000000000000000000000000"
       },
       "sched-clockid":11,
       "sched-entry-list": {"entry": [
           {"index":0, "cmd":0, "gate-mask":1, "interval":300000},
           {"index":1, "cmd":0, "gate-mask":2, "interval":300000},
           {"index":2, "cmd":0, "gate-mask":4, "interval":400000} ]
       },
       "sched-base-time":1528743495910289987, "flags": 1
   }
  }'

> Patch 1 corrects two docstrings in nlspec.py
> Patch 2 adds the multi-attr attribute to taprio entry
> Patch 3 adds the support for encoding multi-attr
>
> v1 --> v2:
> - Use SearchAttrs instead of ChainMap
>
> v2 --> v3:
> - Handle multi-attr at every level, not only in nested attributes
>
> Alessandro Marcolini (3):
>   tools: ynl: correct typo and docstring
>   doc: netlink: specs: tc: add multi-attr to tc-taprio-sched-entry
>   tools: ynl: add support for encoding multi-attr
>
>  Documentation/netlink/specs/tc.yaml | 1 +
>  tools/net/ynl/lib/nlspec.py         | 7 +++----
>  tools/net/ynl/lib/ynl.py            | 5 +++++
>  3 files changed, 9 insertions(+), 4 deletions(-)

Re: [PATCH v3 net-next 0/3] Add support for encoding multi-attr to ynl

[Alessandro Marcolini]

On 2/2/24 16:05, Donald Hunter wrote:
> I liked the fact that your original cover letter included an example
> ynl.py command. With this patchset plus the merged struct encoding
> patch, you can add a taprio qdisc like this:
>
> # ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/tc.yaml \
>   --do newqdisc --create --json '{
>   "family":1, "ifindex":111, "handle":65536, "parent":4294967295, "info":0,
>    "kind":"taprio",
>    "stab":{
>        "base": {
>          "cell-log": 0,
>          "size-log": 0,
>          "cell-align": 0,
>          "overhead": 31,
>          "linklayer": 0,
>          "mpu": 0,
>          "mtu": 0,
>          "tsize": 0
>        }
>    },
>    "options":{
>        "priomap": {
>            "num-tc": 3,
>            "prio-tc-map": "01010101010101010101010101010101",
>            "hw": 0,
>            "count": "0100010002000000000000000000000000000000000000000000000000000000",
>            "offset": "0100020003000000000000000000000000000000000000000000000000000000"
>        },
>        "sched-clockid":11,
>        "sched-entry-list": {"entry": [
>            {"index":0, "cmd":0, "gate-mask":1, "interval":300000},
>            {"index":1, "cmd":0, "gate-mask":2, "interval":300000},
>            {"index":2, "cmd":0, "gate-mask":4, "interval":400000} ]
>        },
>        "sched-base-time":1528743495910289987, "flags": 1
>    }
>   }'
>
Yes, exactly! I didn't repost the updated command to avoid being too verbose.
FWIW, I've also tested it with an ets qdisc. The command is:

# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/tc.yaml --do newqdisc --create \
--json '{
"family":1, "ifindex":4, "handle":65536, "parent":4294967295, "kind":"ets",
"options":{
    "nbands":6,
    "nstrict":3,
    "quanta":{
        "quanta-band": [3500, 3000, 2500]
    },
    "priomap":{
        "priomap-band":[0, 1, 1, 1, 2, 3, 4, 5]
    }
}
}'