Re: [PATCH net-next] netdevsim: Register and unregister devlink traps on probe/remove device

On Mon, Oct 25, 2021 at 01:56:17PM +0300, Leon Romanovsky wrote:

On Mon, Oct 25, 2021 at 11:08:00AM +0300, Ido Schimmel wrote:

quoted

On Mon, Oct 25, 2021 at 08:34:55AM +0300, Leon Romanovsky wrote:

quoted

On Sun, Oct 24, 2021 at 01:48:25PM +0300, Ido Schimmel wrote:

quoted

On Sun, Oct 24, 2021 at 12:54:52PM +0300, Leon Romanovsky wrote:

quoted

On Sun, Oct 24, 2021 at 12:05:12PM +0300, Ido Schimmel wrote:

quoted

On Sun, Oct 24, 2021 at 11:42:11AM +0300, Leon Romanovsky wrote:

quoted

From: Leon Romanovsky <leonro@nvidia.com>

Align netdevsim to be like all other physical devices that register and
unregister devlink traps during their probe and removal respectively.

No, this is incorrect. Out of the three drivers that support both reload
and traps, both netdevsim and mlxsw unregister the traps during reload.
Here is another report from syzkaller about mlxsw [1].

Sorry, I overlooked it.

quoted

Please revert both 22849b5ea595 ("devlink: Remove not-executed trap
policer notifications") and 8bbeed485823 ("devlink: Remove not-executed
trap group notifications").

However, before we rush and revert commit, can you please explain why
current behavior to reregister traps on reload is correct?

I think that you are not changing traps during reload, so traps before
reload will be the same as after reload, am I right?

During reload we tear down the entire driver and load it again. As part
of the reload_down() operation we tear down the various objects from
both devlink and the device (e.g., shared buffer, ports, traps, etc.).
As part of the reload_up() operation we issue a device reset and
register everything back.

This is an implementation which is arguably questionable and pinpoints
problem with devlink reload. It mixes different SW layers into one big
mess which I tried to untangle.

The devlink "feature" that driver reregisters itself again during execution
of other user-visible devlink command can't be right design.

quoted

While the list of objects doesn't change, their properties (e.g., shared
buffer size, trap action, policer rate) do change back to the default
after reload and we cannot go back on that as it's a user-visible
change.

I don't propose to go back, just prefer to see fixed mlxsw that
shouldn't touch already created and registered objects from net/core/devlink.c.

All reset-to-default should be performed internally to the driver
without any need to devlink_*_register() again, so we will be able to
clean rest devlink notifications.

So at least for the netdevsim, this change looks like the correct one,
while mlxsw should be fixed next.

No, it's not correct. After your patch, trap properties like action are
not set back to the default. Regardless of what you think is the "right
design", you cannot introduce such regressions.

Again, I'm not against fixing the regression, I'm trying to understand
why it is impossible to fix mlxsw and netdevsim to honor SW layering
properly.

devlink reload was implemented in 4.16 along with mlxsw support and from
day one it meant that devlink objects such as ports and shared buffer
were dynamically destroyed / created during reload. You cannot come
almost 4 years later, sprinkle assertions according to how you
implemented reload support and claim that the rest are wrong and need to
be fixed so that the assertions don't trigger.

Note that devlink reload is not the only command that can trigger the
destruction / creation of objects. devlink ports can also come and go
following split / unsplit commands.

quoted

Calling devlink_*_unregister() in reload_down() and devlink_*_register()
in reload_up() is not new. It is done for multiple objects (e.g., ports,
regions, shared buffer, etc). After your patch, netdevsim is still doing
it.

Yeah, it was introduced by same developers who did it in mlxsw, so no
wonders that same patterns exist in both drivers.

The point of netdevsim is to both provide a reference and allow
developers to test the code. If you bothered to run the tests under
tools/testing/selftests/drivers/net/netdevsim/ (or just instantiate /
reload netdevsim, really), these regressions [1][2] would have been
avoided. Instead, I need to ask you for the third time to revert the
changes.

[1] https://syzkaller.appspot.com/bug?id=c58973ab3345057753c9f629a88275e30ed2a370
[2] https://syzkaller.appspot.com/bug?extid=93d5accfaefceedf43c1

quoted

Again, please revert the two commits I mentioned. If you think they are
necessary, you can re-submit them in the future, after proper review and
testing of the affected code paths.

It was posted for review in the ML, no one objected.

So what?

Can you please explain why is it so important to touch devlink SW
objects, reallocate them again and again on every reload in mlxsw?

Because that's how reload was defined and implemented. A complete
reload. We are not changing the semantics 4 years later.