| Message ID | cover.1729786087.git.petrm@nvidia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: Shift responsibility for FDB notifications to drivers | expand |
On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: > Besides this approach, we considered just passing a boolean back from the > driver, which would indicate whether the notification was done. But the > approach presented here seems cleaner. Oops, I missed the v2, same question: What about adding a bit to the ops struct to indicate that the driver will generate the notification? Seems smaller in terms of LoC and shifts the responsibility of doing extra work towards more complex users. https://lore.kernel.org/all/20241029121619.1a710601@kernel.org/
Jakub Kicinski <kuba@kernel.org> writes: > On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: >> Besides this approach, we considered just passing a boolean back from the >> driver, which would indicate whether the notification was done. But the >> approach presented here seems cleaner. > > Oops, I missed the v2, same question: > > What about adding a bit to the ops struct to indicate that > the driver will generate the notification? Seems smaller in > terms of LoC and shifts the responsibility of doing extra > work towards more complex users. > > https://lore.kernel.org/all/20241029121619.1a710601@kernel.org/ Sorry for only responding now, I was out of office last week. The reason I went with outright responsibility shift is that the alternatives are more complex. For the flag in particular, first there's no place to set the flag currently, we'd need a field in struct net_device_ops. But mainly, then you have a code that needs to corrently handle both states of the flag, and new-style drivers need to remember to set the flag, which is done in a different place from the fdb_add/del themselves. It might be fewer LOCs, but it's a harder to understand system. Responsibility shift is easy. "Thou shalt notify." Done, easy to understand, easy to document. When cut'n'pasting, you won't miss it. Let me know what you think.
On Mon, 4 Nov 2024 12:43:11 +0100 Petr Machata wrote: > > On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: > >> Besides this approach, we considered just passing a boolean back from the > >> driver, which would indicate whether the notification was done. But the > >> approach presented here seems cleaner. > > > > Oops, I missed the v2, same question: > > > > What about adding a bit to the ops struct to indicate that > > the driver will generate the notification? Seems smaller in > > terms of LoC and shifts the responsibility of doing extra > > work towards more complex users. > > > > https://lore.kernel.org/all/20241029121619.1a710601@kernel.org/ > > Sorry for only responding now, I was out of office last week. > > The reason I went with outright responsibility shift is that the > alternatives are more complex. > > For the flag in particular, first there's no place to set the flag > currently, we'd need a field in struct net_device_ops. But mainly, then > you have a code that needs to corrently handle both states of the flag, > and new-style drivers need to remember to set the flag, which is done in > a different place from the fdb_add/del themselves. It might be fewer > LOCs, but it's a harder to understand system. > > Responsibility shift is easy. "Thou shalt notify." Done, easy to > understand, easy to document. When cut'n'pasting, you won't miss it. Makes sense for real proto drivers, but we also need to touch 4 Ethernet drivers. While we can trust proto drivers to do the right thing, HW driver devs on average are average. And I can't think of another case where driver would send netlink notifications directly. > Let me know what you think. Mild preference towards keeping the expectations from HW drivers as low as possible. But I don't feel strongly. Let me revive the series in PW so it is top of the list for Paolo tomorrow.. :)
On 11/4/24 12:43, Petr Machata wrote: > Jakub Kicinski <kuba@kernel.org> writes: >> On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: >>> Besides this approach, we considered just passing a boolean back from the >>> driver, which would indicate whether the notification was done. But the >>> approach presented here seems cleaner. >> >> Oops, I missed the v2, same question: >> >> What about adding a bit to the ops struct to indicate that >> the driver will generate the notification? Seems smaller in >> terms of LoC and shifts the responsibility of doing extra >> work towards more complex users. >> >> https://lore.kernel.org/all/20241029121619.1a710601@kernel.org/ > > Sorry for only responding now, I was out of office last week. > > The reason I went with outright responsibility shift is that the > alternatives are more complex. > > For the flag in particular, first there's no place to set the flag > currently, we'd need a field in struct net_device_ops. But mainly, then > you have a code that needs to corrently handle both states of the flag, > and new-style drivers need to remember to set the flag, which is done in > a different place from the fdb_add/del themselves. It might be fewer > LOCs, but it's a harder to understand system. > > Responsibility shift is easy. "Thou shalt notify." Done, easy to > understand, easy to document. When cut'n'pasting, you won't miss it. > > Let me know what you think. I think that keeping as much action/responsibilities as possible in the core code is in general a better option - at very least to avoid duplicate code. I don't think that the C&P is a very good argument, as I would argue against C&P without understanding of the underlying code. Still I agree that keeping all the relevant info together is better, and a separate flag would be not so straight-forward. What about using the return value of fbd_add/fdb_del to tell the core that the driver did the notification? a positive value means 'already notified', a negative one error, zero 'please notify. Cheers, Paolo
Paolo Abeni <pabeni@redhat.com> writes: > On 11/4/24 12:43, Petr Machata wrote: >> Jakub Kicinski <kuba@kernel.org> writes: >>> On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: >>>> Besides this approach, we considered just passing a boolean back from the >>>> driver, which would indicate whether the notification was done. But the >>>> approach presented here seems cleaner. >>> >>> Oops, I missed the v2, same question: >>> >>> What about adding a bit to the ops struct to indicate that >>> the driver will generate the notification? Seems smaller in >>> terms of LoC and shifts the responsibility of doing extra >>> work towards more complex users. >>> >>> https://lore.kernel.org/all/20241029121619.1a710601@kernel.org/ >> >> Sorry for only responding now, I was out of office last week. >> >> The reason I went with outright responsibility shift is that the >> alternatives are more complex. >> >> For the flag in particular, first there's no place to set the flag >> currently, we'd need a field in struct net_device_ops. But mainly, then >> you have a code that needs to corrently handle both states of the flag, >> and new-style drivers need to remember to set the flag, which is done in >> a different place from the fdb_add/del themselves. It might be fewer >> LOCs, but it's a harder to understand system. >> >> Responsibility shift is easy. "Thou shalt notify." Done, easy to >> understand, easy to document. When cut'n'pasting, you won't miss it. >> >> Let me know what you think. > > I think that keeping as much action/responsibilities as possible in the > core code is in general a better option - at very least to avoid > duplicate code. > > I don't think that the C&P is a very good argument, as I would argue > against C&P without understanding of the underlying code. Still I agree > that keeping all the relevant info together is better, and a separate > flag would be not so straight-forward. > > What about using the return value of fbd_add/fdb_del to tell the core > that the driver did the notification? a positive value means 'already > notified', a negative one error, zero 'please notify. That would work. How about passing an explicit bool* argument for the callee to set? I'm suspicious of these one-off errno protocols. Most of the time the return value is an errno, these aberrations feel easy to miss. We decided against a dedicated argument originally, because it's not very pretty, but if the callback itself should somehow carry the please-notify interface (and I think it should), an argument is a more explicit and obvious way to do it.
On 11/5/24 10:45, Petr Machata wrote: > Paolo Abeni <pabeni@redhat.com> writes: >> On 11/4/24 12:43, Petr Machata wrote: >>> Jakub Kicinski <kuba@kernel.org> writes: >>>> On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: >>>>> Besides this approach, we considered just passing a boolean back from the >>>>> driver, which would indicate whether the notification was done. But the >>>>> approach presented here seems cleaner. >>>> >>>> Oops, I missed the v2, same question: >>>> >>>> What about adding a bit to the ops struct to indicate that >>>> the driver will generate the notification? Seems smaller in >>>> terms of LoC and shifts the responsibility of doing extra >>>> work towards more complex users. >>>> >>>> https://lore.kernel.org/all/20241029121619.1a710601@kernel.org/ >>> >>> Sorry for only responding now, I was out of office last week. >>> >>> The reason I went with outright responsibility shift is that the >>> alternatives are more complex. >>> >>> For the flag in particular, first there's no place to set the flag >>> currently, we'd need a field in struct net_device_ops. But mainly, then >>> you have a code that needs to corrently handle both states of the flag, >>> and new-style drivers need to remember to set the flag, which is done in >>> a different place from the fdb_add/del themselves. It might be fewer >>> LOCs, but it's a harder to understand system. >>> >>> Responsibility shift is easy. "Thou shalt notify." Done, easy to >>> understand, easy to document. When cut'n'pasting, you won't miss it. >>> >>> Let me know what you think. >> >> I think that keeping as much action/responsibilities as possible in the >> core code is in general a better option - at very least to avoid >> duplicate code. >> >> I don't think that the C&P is a very good argument, as I would argue >> against C&P without understanding of the underlying code. Still I agree >> that keeping all the relevant info together is better, and a separate >> flag would be not so straight-forward. >> >> What about using the return value of fbd_add/fdb_del to tell the core >> that the driver did the notification? a positive value means 'already >> notified', a negative one error, zero 'please notify. > > That would work. > > How about passing an explicit bool* argument for the callee to set? I'm > suspicious of these one-off errno protocols. Most of the time the return > value is an errno, these aberrations feel easy to miss. I would be ok with that - a large arguments list should not be something concerning for the control path. Just to be clear: the caller init the bool to false, only the callees doing the notification set it, right? Thanks! Paolo
Paolo Abeni <pabeni@redhat.com> writes: > On 11/5/24 10:45, Petr Machata wrote: >> Paolo Abeni <pabeni@redhat.com> writes: >>> On 11/4/24 12:43, Petr Machata wrote: >>>> Jakub Kicinski <kuba@kernel.org> writes: >>>>> On Thu, 24 Oct 2024 18:57:35 +0200 Petr Machata wrote: >>>>>> Besides this approach, we considered just passing a boolean back from the >>>>>> driver, which would indicate whether the notification was done. But the >>>>>> approach presented here seems cleaner. >>>>> >>>>> Oops, I missed the v2, same question: >>>>> >>>>> What about adding a bit to the ops struct to indicate that >>>>> the driver will generate the notification? Seems smaller in >>>>> terms of LoC and shifts the responsibility of doing extra >>>>> work towards more complex users. >> >> How about passing an explicit bool* argument for the callee to set? I'm >> suspicious of these one-off errno protocols. Most of the time the return >> value is an errno, these aberrations feel easy to miss. > > I would be ok with that - a large arguments list should not be something > concerning for the control path. Just to be clear: the caller init the > bool to false, only the callees doing the notification set it, right? Yes. OK, I'll do it like that.
Currently when FDB entries are added to or deleted from a VXLAN netdevice, the VXLAN driver emits one notification, including the VXLAN-specific attributes. The core however always sends a notification as well, a generic one. Thus two notifications are unnecessarily sent for these operations. A similar situation comes up with bridge driver, which also emits notifications on its own. # ip link add name vx type vxlan id 1000 dstport 4789 # bridge monitor fdb & [1] 1981693 # bridge fdb add de:ad:be:ef:13:37 dev vx self dst 192.0.2.1 de:ad:be:ef:13:37 dev vx dst 192.0.2.1 self permanent de:ad:be:ef:13:37 dev vx self permanent In order to prevent this duplicity, shift the responsibility to send the notification always to the drivers. Only where the default FDB add / del operations are used does the core emit notifications. If fdb_add and fdb_del are overridden, the driver should do that instead. To facilitate upholding this new responsibility, export rtnl_fdb_notify() for drivers to use. Besides this approach, we considered just passing a boolean back from the driver, which would indicate whether the notification was done. But the approach presented here seems cleaner. Patches #1 to #3 are concerned with the above. In the remaining patches, #4 to #8, add a selftest. This takes place across several patches. Many of the helpers we would like to use for the test are in forwarding/lib.sh, whereas net/ is a more suitable place for the test, so the libraries need to be massaged a bit first. v2: - Patches #2, #3: - Fix qlcnic build Petr Machata (8): net: rtnetlink: Publish rtnl_fdb_notify() ndo_fdb_add: Shift responsibility for notifying to drivers ndo_fdb_del: Shift responsibility for notifying to drivers selftests: net: lib: Move logging from forwarding/lib.sh here selftests: net: lib: Move tests_run from forwarding/lib.sh here selftests: net: lib: Move checks from forwarding/lib.sh here selftests: net: lib: Add kill_process selftests: net: fdb_notify: Add a test for FDB notifications drivers/net/ethernet/intel/i40e/i40e_main.c | 3 + drivers/net/ethernet/intel/ice/ice_main.c | 6 + drivers/net/ethernet/mscc/ocelot_net.c | 16 +- .../net/ethernet/qlogic/qlcnic/qlcnic_main.c | 12 +- drivers/net/macvlan.c | 6 + include/linux/netdevice.h | 5 + include/linux/rtnetlink.h | 2 + net/core/rtnetlink.c | 24 +- .../drivers/net/mlxsw/devlink_trap.sh | 2 +- .../net/mlxsw/devlink_trap_l3_drops.sh | 4 +- .../net/mlxsw/devlink_trap_l3_exceptions.sh | 12 +- .../net/mlxsw/devlink_trap_tunnel_ipip.sh | 4 +- .../net/mlxsw/devlink_trap_tunnel_ipip6.sh | 4 +- .../net/mlxsw/devlink_trap_tunnel_vxlan.sh | 4 +- .../mlxsw/devlink_trap_tunnel_vxlan_ipv6.sh | 4 +- .../selftests/drivers/net/mlxsw/tc_sample.sh | 4 +- .../net/netdevsim/fib_notifications.sh | 6 +- tools/testing/selftests/net/Makefile | 2 +- .../selftests/net/drop_monitor_tests.sh | 2 +- tools/testing/selftests/net/fdb_notify.sh | 95 ++++++++ tools/testing/selftests/net/fib_tests.sh | 8 +- .../selftests/net/forwarding/devlink_lib.sh | 2 +- tools/testing/selftests/net/forwarding/lib.sh | 199 +--------------- .../selftests/net/forwarding/tc_police.sh | 8 +- tools/testing/selftests/net/lib.sh | 223 ++++++++++++++++++ 25 files changed, 411 insertions(+), 246 deletions(-) create mode 100755 tools/testing/selftests/net/fdb_notify.sh