zebra: add dplane helpers provide interface speed

[maxime-leroy]

Add dplane ctx helpers to carry interface speed and skip ethtool polling when the dataplane provides a value. Avoid scheduling speed update timers in this case.

[mjstapp]

A couple of questions: This appears to make the dplane-sourced link speed info one-shot, only really available along with a complete set of interface attributes. Would it make more sense to make the speed path more distinct, with a dedicated path, to allow it to be more dynamic? Or would it make more sense for the existing "query for the speed periodically" approach to be adapted to use the dplane instead of inline system calls? In that way, the kernel info could be checked as it is now, but a different dplane plugin could respond to that query event itself.

[maxime-leroy]

A couple of questions: This appears to make the dplane-sourced link speed info one-shot, only really available along with a complete set of interface attributes. Would it make more sense to make the speed path more distinct, with a dedicated path, to allow it to be more dynamic? Or would it make more sense for the existing "query for the speed periodically" approach to be adapted to use the dplane instead of inline system calls? In that way, the kernel info could be checked as it is now, but a different dplane plugin could respond to that query event itself.

Speed is always included in Grout’s interface notifications, updated when the interface goes up or down. In practice we receive all attributes needed for DPLANE_OP_INTF_INSTALL/DELETE, including speed.

Netlink is different: it does not carry speed for RTM_NEWLINK notfication, so FRR must call ethtool/ioctl—and often retry—to obtain a stable value.

Both of your proposals align with the kernel model where RTM_NEWLINK lacks speed. Your first option also removes the ioctl from the zebra main thread by doing it on the dplane thread, which is a better design overall.

This patch is the minimal change to consume Grout-provided speed. It's why this patch is a draft.

Anyway, introducing a dedicated API like DPLANE_OP_INTF_SPEED_UPDATE would require:

• netlink_link_change to start a timer to query speed on link-up, similar to what zebra_if_dplane_ifp_handling does today.
• zebra_if_dplane_ifp_handling currently sets speed for a new interface via kernel_get_speed. This cannot do anymore. We would need a default intial speed link (i.e. 0 ?) until DPLANE_OP_INTF_SPEED_UPDATE arrives.

For Grout, this would mean sending DPLANE_OP_INTF_UPDATE (to set link up for example) plus DPLANE_OP_INTF_UPDATE_SPEED (to set speed link). It’s not optimal, but acceptable if we want a unified API used by kernel and Grout.

[mjstapp]

I'm not trying to get into a "linux vs grout" struggle. I just want the dplane api to be something reasonably neutral, so if it's going to change, I'd rather it change in ways that could be somewhat general.

Assuming that there's never a speed available at creation seems limiting, as you say. But assuming that there's never a need to query is also limiting. I don't think you have to force "grout" to change to accomodate that - I'd just like there be a path to moving the existing work into the dataplane - and making it available to plugins to decide what to do (if anything).

A couple of questions: This appears to make the dplane-sourced link speed info one-shot, only really available along with a complete set of interface attributes. Would it make more sense to make the speed path more distinct, with a dedicated path, to allow it to be more dynamic? Or would it make more sense for the existing "query for the speed periodically" approach to be adapted to use the dplane instead of inline system calls? In that way, the kernel info could be checked as it is now, but a different dplane plugin could respond to that query event itself.

Speed is always included in Grout’s interface notifications, updated when the interface goes up or down. In practice we receive all attributes needed for DPLANE_OP_INTF_INSTALL/DELETE, including speed.

Netlink is different: it does not carry speed for RTM_NEWLINK notfication, so FRR must call ethtool/ioctl—and often retry—to obtain a stable value.

Both of your proposals align with the kernel model where RTM_NEWLINK lacks speed. Your first option also removes the ioctl from the zebra main thread by doing it on the dplane thread, which is a better design overall.

This patch is the minimal change to consume Grout-provided speed. It's why this patch is a draft.

Anyway, introducing a dedicated API like DPLANE_OP_INTF_SPEED_UPDATE would require:

* netlink_link_change to start a timer to query speed on link-up, similar to what zebra_if_dplane_ifp_handling does today.

* zebra_if_dplane_ifp_handling currently sets speed for a new interface via kernel_get_speed. This cannot do anymore. We would need a default intial speed link (i.e. 0 ?) until DPLANE_OP_INTF_SPEED_UPDATE arrives.

For Grout, this would mean sending DPLANE_OP_INTF_UPDATE (to set link up for example) plus DPLANE_OP_INTF_UPDATE_SPEED (to set speed link). It’s not optimal, but acceptable if we want a unified API used by kernel and Grout.

[donaldsharp]

This code change WILL break bonds/lags under linux. It is common for a bond/LAG to be slow coming up while FRR is already started and the interface speed will change.

This is the specific scenario I put this code in for:

a) System start b) Interface comes up with Speed X ( say we are bonding 10 interfaces and only 3 of the 10 interfaces have been bonded ) c) FRR starts receives speed X d) Interface adds a new interface to the bond, speed becomes Y e) Interface adds a final interface to the bond, speed becomes Z

With this code change you are proposing Zebra will now no longer be able to handle this

What you need to do is either have a grout version of kernel_get_speed ( that returns the interface speed again so it looks like nothing has changed ), or you need to add a dplane query for getting the speed that returns the new speed when asked for it.

[maxime-leroy]

This code change WILL break bonds/lags under linux. It is common for a bond/LAG to be slow coming up while FRR is already started and the interface speed will change.

This is the specific scenario I put this code in for:

a) System start b) Interface comes up with Speed X ( say we are bonding 10 interfaces and only 3 of the 10 interfaces have been bonded ) c) FRR starts receives speed X d) Interface adds a new interface to the bond, speed becomes Y e) Interface adds a final interface to the bond, speed becomes Z

With this code change you are proposing Zebra will now no longer be able to handle this

What you need to do is either have a grout version of kernel_get_speed ( that returns the interface speed again so it looks like nothing has changed ), or you need to add a dplane query for getting the speed that returns the new speed when asked for it.

First, this PR is a draft; it is not intended to be merged. Second, this commit only adds APIs to set link speed via DPLANE_OP_INTF_INSTALL/DELETE. These APIs are not used by the zebra/kernel path. The existing timer still collects link speed by calling kernel_get_speed, so behavior is unchanged and bonds/LAGs should continue to work as before.

Last point: I think it would be better to run the ioctl on the zebra dplane thread rather than the main thread—unless I’m missing something?

We could add DPLANE_OP_INTF_UPDATE_SPEED to obtain speed from the dplane and update it on the main thread (as Mjtsapp suggested). That should handle bond interfaces properly.

How the dplane obtains link speed (polling via ioctl or by notification) is an implementation detail that should remain in the dplane backend.

[maxime-leroy]

Still work in progress, just back from holiday, didn't have time to test it (yet).

[maxime-leroy]

hmm, no, I think maybe some of the discussion wasn't clear?

the way the dplane works is by sending events down from zebra towards the dataplane(s), and sending results and other events "up" towards zebra from the dataplane(s).

it'd be fine to include a field for interface speed, and apply that if it's present

the interface update timer could emit a dplane event for processing. the kernel dplane would make the appropriate system call. some other dataplane plugin might just ignore the event. if the current notion of the speed were in the event, and if that was unchanged, the kernel plugin could also avoid making any change.

a dplane event with a speed update would be processed in the zebra main pthread context, which is the only pthread allowed to touch the zebra structs.

I don't think it's worth adding a header file just to rename a couple of status codes - is there a real benefit to changing those labels?

Thanks for the feedback. Just updated in consequence. It's still WIP, see notes section in last commit.

[maxime-leroy]

For dplane_intf_speed, should we reuse dplane_intf_update_internal(ifp, DPLANE_OP_INTF_SPEED) or build own ctx dplane ? In this case, should we have own dplane stats (i.e. .dg_intfs_in/errors) ? thanks

[maxime-leroy]

This new api is used in a PR on grout side: https://github.com/DPDK/grout/pull/292/commits/9e24a6261366b23a3bad494813f1735770a35c2a

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[maxime-leroy]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

This PR has also conflicts with #19804 . Since PR19804 is a fix, I’ll wait for it to be merged before resolving the conflicts here.

In the meantime, I’d appreciate any feedback from @donaldsharp and @mjstapp.

[maxime-leroy]

The PR has been updated to resolve conflicts. It's ready for review.