Re: [PATCH net-next 0/5] Move devlink_register to be near devlink_reload_enable

On Tue, Aug 10, 2021 at 04:53:18PM -0700, Jakub Kicinski wrote:

On Tue, 10 Aug 2021 16:37:30 +0300 Leon Romanovsky wrote:

quoted

This series prepares code to remove devlink_reload_enable/_disable API
and in order to do, we move all devlink_register() calls to be right
before devlink_reload_enable().

The best place for such a call should be right before exiting from
the probe().

This is done because devlink_register() opens devlink netlink to the
users and gives them a venue to issue commands before initialization
is finished.

1. Some drivers were aware of such "functionality" and tried to protect
themselves with extra locks, state machines and devlink_reload_enable().
Let's assume that it worked for them, but I'm personally skeptical about
it.

2. Some drivers copied that pattern, but without locks and state
machines. That protected them from reload flows, but not from any _set_
routines.

3. And all other drivers simply didn't understand the implications of early
devlink_register() and can be seen as "broken".

What are those implications for drivers which don't implement reload?
Depending on which parts of devlink the drivers implement there may well
be nothing to worry about.

Plus devlink instances start out with reload disabled. Could you please
take a step back and explain why these changes are needed.

The problem is that devlink_register() adds new devlink instance to the
list of visible devlinks (devlink_list). It means that all devlink_*_dumpit()
will try to access devices during their initialization, before they are ready.

The more troublesome case is that devlink_list is iterated in the
devlink_get_from_attrs() and it is used in devlink_nl_pre_doit(). The
latter function will return to the caller that new devlink is valid and
such caller will be able to proceed to *_set_doit() functions.

Just as an example:
 * user sends netlink message
  * devlink_nl_cmd_eswitch_set_doit()
   * ops->eswitch_mode_set()
    * Are you sure that all drivers protected here?
      I remind that driver is in the middle of its probe().

Someone can argue that drivers and devlink are protected from anything
harmful with their global (devlink_mutex and devlink->lock) and internal
(device->lock, e.t.c.) locks. However it is impossible to prove for all
drivers and prone to errors.

Reload enable/disable gives false impression that the problem exists in
that flow only, which is not true.

devlink_reload_enable() is a duct tape because reload flows much easier
to hit.

quoted

In this series, we focus on items #1 and #2.

Please share your opinion if I should change ALL other drivers to make
sure that devlink_register() is the last command or leave them in an
as-is state.

Can you please share the output of devlink monitor and ip monitor link
before and after?  The modified drivers will not register ports before
they register the devlink instance itself.

Not really, they will register but won't be accessible from the user space.
The only difference is the location of "[dev,new] ..." notification.

[leonro@vm ~]$ sudo modprobe mlx5_core
[  105.575790] mlx5_core 0000:00:09.0: firmware version: 4.8.9999
[  105.576349] mlx5_core 0000:00:09.0: 0.000 Gb/s available PCIe bandwidth (8.0 GT/s PCIe x255 link)
[  105.686217] pps pps0: new PPS source ptp0
[  105.688144] mlx5_core 0000:00:09.0: E-Switch: Total vports 2, per vport: max uc(32768) max mc(32768)
[  105.717736] mlx5_core 0000:00:09.0: MLX5E: StrdRq(1) RqSz(8) StrdSz(2048) RxCqeCmprss(0)
[  106.957028] mlx5_core 0000:00:09.0 eth1: Link down
[  106.960379] mlx5_core 0000:00:09.0 eth1: Link up
[  106.967916] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
================================================================================================
Before:
[leonro@vm ~]$ sudo devlink monitor
[dev,new] pci/0000:00:09.0
[param,new] pci/0000:00:09.0: name flow_steering_mode type driver-specific
  values:
[param,new] pci/0000:00:09.0: name esw_port_metadata type driver-specific
  values:
[param,new] pci/0000:00:09.0: name enable_remote_dev_reset type generic
  values:
[param,new] pci/0000:00:09.0: name enable_roce type generic
  values:
    cmode driverinit value true
[param,new] pci/0000:00:09.0: name fdb_large_groups type driver-specific
  values:
    cmode driverinit value 15
[param,new] pci/0000:00:09.0: name flow_steering_mode type driver-specific
  values:
    cmode runtime value dmfs
[param,new] pci/0000:00:09.0: name enable_roce type generic
  values:
    cmode driverinit value true
[param,new] pci/0000:00:09.0: name fdb_large_groups type driver-specific
  values:
    cmode driverinit value 15
[param,new] pci/0000:00:09.0: name esw_port_metadata type driver-specific
  values:
    cmode runtime value true
[param,new] pci/0000:00:09.0: name enable_remote_dev_reset type generic
  values:
    cmode runtime value true
[trap-group,new] pci/0000:00:09.0: name l2_drops generic true
[trap,new] pci/0000:00:09.0: name ingress_vlan_filter type drop generic true action drop group l2_drops
[trap,new] pci/0000:00:09.0: name dmac_filter type drop generic true action drop group l2_drops
[port,new] pci/0000:00:09.0/131071: type notset flavour physical port 0 splittable false
[port,new] pci/0000:00:09.0/131071: type eth netdev eth1 flavour physical port 0 splittable false

[leonro@vm ~]$ sudo ip monitor
inet eth1 forwarding off rp_filter loose mc_forwarding off proxy_neigh off ignore_routes_with_linkdown off 
inet6 eth1 forwarding off mc_forwarding off proxy_neigh off ignore_routes_with_linkdown off 
4: eth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default 
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
4: eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state DOWN group default 
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
multicast ff00::/8 dev eth1 table local proto kernel metric 256 pref medium
fe80::/64 dev eth1 proto kernel metric 256 pref medium
4: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default 
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
4: eth1    inet6 fe80::5054:ff:fe12:3456/64 scope link 
       valid_lft forever preferred_lft forever
local fe80::5054:ff:fe12:3456 dev eth1 table local proto kernel metric 0 pref medium

===========================================================================================================
After:
[leonro@vm ~]$ sudo devlink monitor
[param,new] pci/0000:00:09.0: name flow_steering_mode type driver-specific
  values:
[param,new] pci/0000:00:09.0: name esw_port_metadata type driver-specific
  values:
[param,new] pci/0000:00:09.0: name enable_remote_dev_reset type generic
  values:
[param,new] pci/0000:00:09.0: name enable_roce type generic
  values:
    cmode driverinit value true
[param,new] pci/0000:00:09.0: name fdb_large_groups type driver-specific
  values:
    cmode driverinit value 15
[param,new] pci/0000:00:09.0: name flow_steering_mode type driver-specific
  values:
    cmode runtime value dmfs
[param,new] pci/0000:00:09.0: name enable_roce type generic
  values:
    cmode driverinit value true
[param,new] pci/0000:00:09.0: name fdb_large_groups type driver-specific
  values:
    cmode driverinit value 15
[param,new] pci/0000:00:09.0: name esw_port_metadata type driver-specific
  values:
    cmode runtime value true
[param,new] pci/0000:00:09.0: name enable_remote_dev_reset type generic
  values:
    cmode runtime value true
[trap-group,new] pci/0000:00:09.0: name l2_drops generic true
[trap,new] pci/0000:00:09.0: name ingress_vlan_filter type drop generic true action drop group l2_drops
[trap,new] pci/0000:00:09.0: name dmac_filter type drop generic true action drop group l2_drops
[dev,new] pci/0000:00:09.0
[port,new] pci/0000:00:09.0/131071: type notset flavour physical port 0 splittable false
[port,new] pci/0000:00:09.0/131071: type eth netdev eth1 flavour physical port 0 splittable false

[leonro@vm ~]$ sudo ip monitor
inet eth1 forwarding off rp_filter loose mc_forwarding off proxy_neigh off ignore_routes_with_linkdown off 
inet6 eth1 forwarding off mc_forwarding off proxy_neigh off ignore_routes_with_linkdown off 
4: eth1: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default 
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
4: eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state DOWN group default 
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
multicast ff00::/8 dev eth1 table local proto kernel metric 256 pref medium
fe80::/64 dev eth1 proto kernel metric 256 pref medium
4: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default 
    link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
4: eth1    inet6 fe80::5054:ff:fe12:3456/64 scope link 
       valid_lft forever preferred_lft forever
local fe80::5054:ff:fe12:3456 dev eth1 table local proto kernel metric 0 pref medium