| Message ID | 20231129101159.99197-1-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/net/ynl: Add dynamic selector for options attrs | expand |
On Wed, 29 Nov 2023 10:11:53 +0000 Donald Hunter wrote: > This patchset adds a dynamic selector mechanism to YNL for kind-specific > options attributes. I am sending this as an RFC solicit feedback on a > couple of issues before I complete the patchset. Exciting stuff! > I started adding this feature for the rt_link spec which is monomorphic, > i.e. the kind-specific 'data' attribute is always a nest. The selector > looked like this: > > - > name: data > type: dynamic > selector: > attribute: kind > list: > - > value: bridge > nested-attributes: linkinfo-bridge-attrs > - > value: erspan > nested-attributes: linkinfo-gre-attrs It's kinda moot given your discovery below :(, but FWIW this is very close to what I've been thinking. After some pondering I thought it'd be better to structure it just a bit differently: - name: data type: poly-nest selector: kind # which attr carries the key that's it for the attr, and then in attr-set I'd add a "key": - name: linkinfo-bridge-attrs poly-key: bridge putting the key on the attr set is worse if we ever need to "key" the same attr set with different selectors, but it makes the attr definition a lot smaller. And in practice I didn't expect us to ever need keying into one attr set with different selectors. If we did - we could complicate it later, but start simple. > Then I started working on tc and found that the 'options' attribute is > poymorphic. It is typically either a C struct or a nest. So I extended the > dynamic selector to include a 'type' field and type-specific sub-fields: > > - > name: options > type: dynamic > selector: > attribute: kind > list: > - > value: bfifo > type: binary > struct: tc-fifo-qopt > - > value: cake > type: nest > nested-attributes: tc-cake-attrs > - > value: cbs > type: nest > nested-attributes: tc-cbs-attrs > > Then I encountered 'netem' which has a nest with a C struct header. I > realised that maybe my mental model had been wrong and that all cases > could be supported by a nest type with an optional fixed-header followed > by zero or more nlattrs. > > - > value: netem > type: nest > fixed-header: tc-netem-qopt > nested-attributes: tc-netem-attrs > > Perhaps it is attribute-sets in general that should have an optional > fixed-header, which would also work for fixed-headers at the start of > genetlink messages. I originally added fixed-header support to > operations for genetlink, but fixed headers on attribute sets would work > for all these cases. > > I now see a few possible ways forward and would like feedback on the > preferred approach: > > 1. Simplify the current patchset to implement fixed-header & nest > support in the dynamic selector. This would leave existing > fixed-header support for messages unchanged. We could drop the 'type' > field. > > - > value: netem > fixed-header: tc-netem-qopt > nested-attributes: tc-netem-attrs > > 2. Keep the 'type' field and support for the 'binary' type which is > useful for specifying nests with unknown attribute spaces. An > alternative would be to default to 'binary' behaviour if there is no > selector entry. > > 3. Refactor the existing fixed-header support to be an optional part of > all attribute sets instead of just messages (in legacy and raw specs) > and dynamic attribute nests (in raw specs). > > attribute-sets: > - > name: tc-netem-attrs > fixed-header: tc-netem-qopt > attributes: Reading this makes me feel like netem wants to be a "sub-message"? It has a fixed header followed by attrs, that's quite message-like. Something along the lines of 1 makes most sense to me, but can we put the "selector ladder" out-of-line? I'm worried that the attr definition will get crazy long. attribute-sets: - name: outside-attrs attributes: ... - name: kind type: string - name: options type: sub-message sub-type: inside-msg # reuse sub-type or new property? selector: kind ... - name: inside-attrs: attributes: ... sub-messages: list: - name: inside-msg formats: # not a great name?.. - value: some-value fixed-header: struct-name - value: other-value fixed-header: struct-name-two nested-attributes: inside-attrs - value: another-one nested-attributes: inside-attrs - name: different-inside-msg ... operations: ... At least that's what comes to my mind after reading the problem description. Does it make sense?
Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 29 Nov 2023 10:11:53 +0000 Donald Hunter wrote: >> This patchset adds a dynamic selector mechanism to YNL for kind-specific >> options attributes. I am sending this as an RFC solicit feedback on a >> couple of issues before I complete the patchset. > > Exciting stuff! > >> I started adding this feature for the rt_link spec which is monomorphic, >> i.e. the kind-specific 'data' attribute is always a nest. The selector >> looked like this: >> >> - >> name: data >> type: dynamic >> selector: >> attribute: kind >> list: >> - >> value: bridge >> nested-attributes: linkinfo-bridge-attrs >> - >> value: erspan >> nested-attributes: linkinfo-gre-attrs > > It's kinda moot given your discovery below :(, but FWIW this is very > close to what I've been thinking. > > After some pondering I thought it'd be better to structure it just > a bit differently: > > - > name: data > type: poly-nest > selector: kind # which attr carries the key > > that's it for the attr, and then in attr-set I'd add a "key": > > - > name: linkinfo-bridge-attrs > poly-key: bridge > > putting the key on the attr set is worse if we ever need to "key" > the same attr set with different selectors, but it makes the attr > definition a lot smaller. And in practice I didn't expect us > to ever need keying into one attr set with different selectors. > If we did - we could complicate it later, but start simple. rt_link shares attribute-sets between different kinds of link so I think that rules out putting the key on the attribute-set. I think we may also see reuse across stats attribute sets in tc. FWIW I initially considered avoiding a selector list by using a template to generate the attribute set name, but that broke pretty quickly. >> ... >> I now see a few possible ways forward and would like feedback on the >> preferred approach: >> >> 1. Simplify the current patchset to implement fixed-header & nest >> support in the dynamic selector. This would leave existing >> fixed-header support for messages unchanged. We could drop the 'type' >> field. >> >> - >> value: netem >> fixed-header: tc-netem-qopt >> nested-attributes: tc-netem-attrs >> >> 2. Keep the 'type' field and support for the 'binary' type which is >> useful for specifying nests with unknown attribute spaces. An >> alternative would be to default to 'binary' behaviour if there is no >> selector entry. >> >> 3. Refactor the existing fixed-header support to be an optional part of >> all attribute sets instead of just messages (in legacy and raw specs) >> and dynamic attribute nests (in raw specs). >> >> attribute-sets: >> - >> name: tc-netem-attrs >> fixed-header: tc-netem-qopt >> attributes: > > Reading this makes me feel like netem wants to be a "sub-message"? > It has a fixed header followed by attrs, that's quite message-like. Yeah, I guess we could call it sub-message because it's not a pure nest and the different name makes it an explicitly netlink-raw concept. > Something along the lines of 1 makes most sense to me, but can we > put the "selector ladder" out-of-line? I'm worried that the attr > definition will get crazy long. It seems reasonable to pull the selector list out of line because they do get big, e.g. over 100 lines for tc "options". My preference is 1, probably including a fallback to "binary" if there is no selector match. > attribute-sets: > - > name: outside-attrs > attributes: > ... > - > name: kind > type: string > - > name: options > type: sub-message > sub-type: inside-msg # reuse sub-type or new property? > selector: kind > ... > - > name: inside-attrs: > attributes: > ... > > sub-messages: > list: > - > name: inside-msg > formats: # not a great name?.. > - > value: some-value > fixed-header: struct-name > - > value: other-value > fixed-header: struct-name-two > nested-attributes: inside-attrs > - > value: another-one > nested-attributes: inside-attrs > - > name: different-inside-msg > ... > > operations: > ... > > At least that's what comes to my mind after reading the problem > description. Does it make sense? I think that once you have broken out to a sub-message, they're no longer "nested-attributes" and we should maybe reuse "attribute-set". I don't think we can reuse "sub-type" because the schema for it is the set of netlink type names, not a free string. Maybe we add "sub-message" instead? So how about this: attribute-sets: - name: outside-attrs attributes: ... - name: kind type: string - name: options type: sub-message sub-message: inside-msg selector: kind ... - name: inside-attrs: attributes: ... sub-messages: - name: inside-msg formats: - value: some-value fixed-header: struct-name - value: other-value fixed-header: struct-name-two attribute-set: inside-attrs - value: another-one attribute-set: inside-attrs - name: different-inside-msg ... operations: ... I cannot think of a better name than "formats" so happy to go with that. Did you want an explicit "list:" in the yaml schema? Thanks, Donald.
On Wed, 29 Nov 2023 16:58:57 +0000 Donald Hunter wrote: > rt_link shares attribute-sets between different kinds of link so I think > that rules out putting the key on the attribute-set. I think we may also > see reuse across stats attribute sets in tc. > > FWIW I initially considered avoiding a selector list by using a template > to generate the attribute set name, but that broke pretty quickly. Ah :( > It seems reasonable to pull the selector list out of line because > they do get big, e.g. over 100 lines for tc "options". > > My preference is 1, probably including a fallback to "binary" if there > is no selector match. Are there any "nests" that need a real binary type? An actual byte array? Or are these all structs? If the latter then fixed-header covers it. > I think that once you have broken out to a sub-message, they're no > longer "nested-attributes" and we should maybe reuse "attribute-set". Good point. > I don't think we can reuse "sub-type" because the schema for it is the > set of netlink type names, not a free string. Maybe we add "sub-message" > instead? Sounds good. > So how about this: > > attribute-sets: > - > name: outside-attrs > attributes: > ... > - > name: kind > type: string > - > name: options > type: sub-message > sub-message: inside-msg > selector: kind > ... > - > name: inside-attrs: > attributes: > ... > > sub-messages: > - > name: inside-msg > formats: > - > value: some-value > fixed-header: struct-name > - > value: other-value > fixed-header: struct-name-two > attribute-set: inside-attrs > - > value: another-one > attribute-set: inside-attrs > - > name: different-inside-msg > ... > > operations: > ... LG! > I cannot think of a better name than "formats" so happy to go with that. Or maybe "variants" ? > Did you want an explicit "list:" in the yaml schema? You mean instead of the "formats" or in addition somewhere? Under sub-messages? The "formats" is basically a "list", just feels less artificial to call it something else than "list". No strong preference, tho. If you mean under "sub-messages" - I can't think of any extra property we may want to put there. So going directly to entries seems fine.
Jakub Kicinski <kuba@kernel.org> writes: > >> Did you want an explicit "list:" in the yaml schema? > > You mean instead of the "formats" or in addition somewhere? > Under sub-messages? > > The "formats" is basically a "list", just feels less artificial > to call it something else than "list". No strong preference, tho. > > If you mean under "sub-messages" - I can't think of any extra property > we may want to put there. So going directly to entries seems fine. Sorry, I wasn't clear - yes, under sub-messages. Thanks!
This patchset adds a dynamic selector mechanism to YNL for kind-specific options attributes. I am sending this as an RFC solicit feedback on a couple of issues before I complete the patchset. I started adding this feature for the rt_link spec which is monomorphic, i.e. the kind-specific 'data' attribute is always a nest. The selector looked like this: - name: data type: dynamic selector: attribute: kind list: - value: bridge nested-attributes: linkinfo-bridge-attrs - value: erspan nested-attributes: linkinfo-gre-attrs Then I started working on tc and found that the 'options' attribute is poymorphic. It is typically either a C struct or a nest. So I extended the dynamic selector to include a 'type' field and type-specific sub-fields: - name: options type: dynamic selector: attribute: kind list: - value: bfifo type: binary struct: tc-fifo-qopt - value: cake type: nest nested-attributes: tc-cake-attrs - value: cbs type: nest nested-attributes: tc-cbs-attrs Then I encountered 'netem' which has a nest with a C struct header. I realised that maybe my mental model had been wrong and that all cases could be supported by a nest type with an optional fixed-header followed by zero or more nlattrs. - value: netem type: nest fixed-header: tc-netem-qopt nested-attributes: tc-netem-attrs Perhaps it is attribute-sets in general that should have an optional fixed-header, which would also work for fixed-headers at the start of genetlink messages. I originally added fixed-header support to operations for genetlink, but fixed headers on attribute sets would work for all these cases. I now see a few possible ways forward and would like feedback on the preferred approach: 1. Simplify the current patchset to implement fixed-header & nest support in the dynamic selector. This would leave existing fixed-header support for messages unchanged. We could drop the 'type' field. - value: netem fixed-header: tc-netem-qopt nested-attributes: tc-netem-attrs 2. Keep the 'type' field and support for the 'binary' type which is useful for specifying nests with unknown attribute spaces. An alternative would be to default to 'binary' behaviour if there is no selector entry. 3. Refactor the existing fixed-header support to be an optional part of all attribute sets instead of just messages (in legacy and raw specs) and dynamic attribute nests (in raw specs). attribute-sets: - name: tc-netem-attrs fixed-header: tc-netem-qopt attributes: ... Thoughts? Patch 1 adds missing scalars to the netlink-raw schema Patch 2 and 3 add dynamic nest support to the schema and ynl Patches 4 and 5 contain specs that use dynamic nests Patch 6 adds support for nests with a fixed-header Donald Hunter (6): doc/netlink: Add bitfield32, s8, s16 to the netlink-raw schema doc/netlink: Add a nest selector to netlink-raw schema tools/net/ynl: Add dynamic attribute decoding to ynl doc/netlink/specs: add dynamic nest selector for rt_link data doc/netlink/specs: Add a spec for tc tools/net/ynl: Add optional fixed-header to dynamic nests Documentation/netlink/netlink-raw.yaml | 43 +- Documentation/netlink/specs/rt_link.yaml | 278 ++- Documentation/netlink/specs/tc.yaml | 2074 ++++++++++++++++++++++ tools/net/ynl/lib/nlspec.py | 27 + tools/net/ynl/lib/ynl.py | 54 +- 5 files changed, 2462 insertions(+), 14 deletions(-) create mode 100644 Documentation/netlink/specs/tc.yaml