LinuxLists.cc - [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Mon, 27 Jul 2020 14:02:21 +0300 Moshe Shemesh wrote:
> Add devlink reload level to allow the user to request a specific reload
> level. The level parameter is optional, if not specified then driver's
> default reload level is used (backward compatible).

Please don't leave space for driver-specific behavior. The OS is
supposed to abstract device differences away.

Previously the purpose of reload was to activate new devlink params
(with driverinit cmode), now you want the ability to activate new
firmware. Let users specify their intent and their constraints.

> Reload levels supported are:
> driver: driver entities re-instantiation only.
> fw_reset: firmware reset and driver entities re-instantiation.
> fw_live_patch: firmware live patching only.

I'm concerned live_patch is not first - it's the lowest impact (since
it's live). Please make sure you clearly specify the expected behavior
for the new API.

The notion of multi-host is key for live patching, so it has to be
mentioned.

> Signed-off-by: Moshe Shemesh <[email protected]>

2020-07-28 13:59:08

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

Tue, Jul 28, 2020 at 02:58:02AM CEST, [email protected] wrote:
>On Mon, 27 Jul 2020 14:02:21 +0300 Moshe Shemesh wrote:
>> Add devlink reload level to allow the user to request a specific reload
>> level. The level parameter is optional, if not specified then driver's
>> default reload level is used (backward compatible).
>
>Please don't leave space for driver-specific behavior. The OS is
>supposed to abstract device differences away.

But this is needed to maintain the existing behaviour which is different
for different drivers.

>
>Previously the purpose of reload was to activate new devlink params
>(with driverinit cmode), now you want the ability to activate new
>firmware. Let users specify their intent and their constraints.
>
>> Reload levels supported are:
>> driver: driver entities re-instantiation only.
>> fw_reset: firmware reset and driver entities re-instantiation.
>> fw_live_patch: firmware live patching only.
>
>I'm concerned live_patch is not first - it's the lowest impact (since
>it's live). Please make sure you clearly specify the expected behavior
>for the new API.
>
>The notion of multi-host is key for live patching, so it has to be
>mentioned.
>
>> Signed-off-by: Moshe Shemesh <[email protected]>

2020-07-28 16:49:03

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 7/28/2020 6:58 AM, Jiri Pirko wrote:
> Tue, Jul 28, 2020 at 02:58:02AM CEST, [email protected] wrote:
>> On Mon, 27 Jul 2020 14:02:21 +0300 Moshe Shemesh wrote:
>>> Add devlink reload level to allow the user to request a specific reload
>>> level. The level parameter is optional, if not specified then driver's
>>> default reload level is used (backward compatible).
>>
>> Please don't leave space for driver-specific behavior. The OS is
>> supposed to abstract device differences away.
>
> But this is needed to maintain the existing behaviour which is different
> for different drivers.
>

Which drivers behave differently here?

2020-07-28 20:03:58

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Tue, 28 Jul 2020 09:47:00 -0700 Jacob Keller wrote:
> On 7/28/2020 6:58 AM, Jiri Pirko wrote:
> > But this is needed to maintain the existing behaviour which is different
> > for different drivers.
>
> Which drivers behave differently here?

I think Jiri refers to mlxsw vs mlx5.

mlxsw loads firmware on probe, by default at least. So reloading the
driver implies a FW reset. NIC drivers OTOH don't generally load FW
so they didn't reset FW.

Now since we're redefining the API from "do a reload so that driverinit
params are applied" (or "so that all netdevs get spawned in a new
netns") to "do a reset of depth X" we have to change the paradigm.

What I was trying to suggest is that we should not have to re-define
the API like this.

From user perspective what's important is what the reset achieves (and
perhaps how destructive it is). We can define the reset levels as:

$ devlink dev reload pci/0000:82:00.0 net-ns-respawn
$ devlink dev reload pci/0000:82:00.0 driver-param-init
$ devlink dev reload pci/0000:82:00.0 fw-activate

combining should be possible when user wants multiple things to happen:

$ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init

Then we have the use case of a "live reset" which is slightly
under-defined right now IMHO, but we can extend it as:

$ devlink dev reload pci/0000:82:00.0 fw-activate --live

We can also add the "reset level" specifier - for the cases where
device is misbehaving:

$ devlink dev reload pci/0000:82:00.0 level [driver|fw|hardware]

But I don't think that we can go from the current reload command
cleanly to just a level reset. The driver-specific default is a bad
smell which indicates we're changing semantics from what user wants
to what the reset depth is. Our semantics with the patch as it stands
are in fact:
- if you want to load new params or change netns, don't pass the level
- the "driver default" workaround dictates the right reset level for
param init;
- if you want to activate new firmware - select the reset level you'd
like from the reset level options.

2020-07-28 20:05:20

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
> On Tue, 28 Jul 2020 09:47:00 -0700 Jacob Keller wrote:
>> On 7/28/2020 6:58 AM, Jiri Pirko wrote:
>>> But this is needed to maintain the existing behaviour which is different
>>> for different drivers.
>>
>> Which drivers behave differently here?
>
> I think Jiri refers to mlxsw vs mlx5.
>
> mlxsw loads firmware on probe, by default at least. So reloading the
> driver implies a FW reset. NIC drivers OTOH don't generally load FW
> so they didn't reset FW.
>

Ok.

> Now since we're redefining the API from "do a reload so that driverinit
> params are applied" (or "so that all netdevs get spawned in a new
> netns") to "do a reset of depth X" we have to change the paradigm.
>
> What I was trying to suggest is that we should not have to re-define
> the API like this.

Ok.

>
> From user perspective what's important is what the reset achieves (and
> perhaps how destructive it is). We can define the reset levels as:
>
> $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
> $ devlink dev reload pci/0000:82:00.0 driver-param-init
> $ devlink dev reload pci/0000:82:00.0 fw-activate
>
> combining should be possible when user wants multiple things to happen:
>
> $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
>

Where today "driver-param-init" is the default behavior. But didn't we
just say that mlxsw also does the equivalent of fw-activate?

>
> Then we have the use case of a "live reset" which is slightly
> under-defined right now IMHO, but we can extend it as:
>
> $ devlink dev reload pci/0000:82:00.0 fw-activate --live
>

Yea, I think live fw patching things aren't quite as defined yet.

>
> We can also add the "reset level" specifier - for the cases where
> device is misbehaving:
>
> $ devlink dev reload pci/0000:82:00.0 level [driver|fw|hardware]
>

I guess I don't quite see how level fits in? This is orthogonal to the
other settings?

>
> But I don't think that we can go from the current reload command
> cleanly to just a level reset. The driver-specific default is a bad
> smell which indicates we're changing semantics from what user wants
> to what the reset depth is. Our semantics with the patch as it stands
> are in fact:
> - if you want to load new params or change netns, don't pass the level
> - the "driver default" workaround dictates the right reset level for
> param init;
> - if you want to activate new firmware - select the reset level you'd
> like from the reset level options.
>

I think I agree, having the "what gets reloaded" as a separate vector
makes sense and avoids confusion. For example for ice hardware,
"fw-activate" really does mean "Do an EMP reset" rather than just a
"device reset" which could be interpreted differently. ice can do
function level reset, or core device reset also. Neither of those two
resets activates firmware.

Additionally the current function load process in ice doesn't support
driver-init at all. That's something I'd like to see happen but is quite
a significant refactor for how the driver loads. We need to refactor
everything out so that devlink is created early on and factor out
load/unload into handlers that can be called by the devlink reload. As I
have primarily been focused on flash update I sort of left that for the
future because it was a huge task to solve.

2020-07-28 20:07:24

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Tue, 28 Jul 2020 12:18:30 -0700 Jacob Keller wrote:
> On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
> > From user perspective what's important is what the reset achieves (and
> > perhaps how destructive it is). We can define the reset levels as:
> >
> > $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
> > $ devlink dev reload pci/0000:82:00.0 driver-param-init
> > $ devlink dev reload pci/0000:82:00.0 fw-activate
> >
> > combining should be possible when user wants multiple things to happen:
> >
> > $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
>
> Where today "driver-param-init" is the default behavior. But didn't we
> just say that mlxsw also does the equivalent of fw-activate?

Actually the default should probably be the combination of
driver-param-init and net-ns-respawn.

My expectations would be that the driver must perform the lowest reset
level possible that satisfies the requested functional change.
IOW driver may do more, in fact it should be acceptable for the driver
to always for a full HW reset (unless --live or other constraint is
specified).

> > We can also add the "reset level" specifier - for the cases where
> > device is misbehaving:
> >
> > $ devlink dev reload pci/0000:82:00.0 level [driver|fw|hardware]
>
> I guess I don't quite see how level fits in? This is orthogonal to the
> other settings?

Yup, it is, it's already orthogonal to what reload does today, hence the
need for the "driver default" hack.

> > But I don't think that we can go from the current reload command
> > cleanly to just a level reset. The driver-specific default is a bad
> > smell which indicates we're changing semantics from what user wants
> > to what the reset depth is. Our semantics with the patch as it stands
> > are in fact:
> > - if you want to load new params or change netns, don't pass the level
> > - the "driver default" workaround dictates the right reset level for
> > param init;
> > - if you want to activate new firmware - select the reset level you'd
> > like from the reset level options.
> >
>
> I think I agree, having the "what gets reloaded" as a separate vector
> makes sense and avoids confusion. For example for ice hardware,
> "fw-activate" really does mean "Do an EMP reset" rather than just a
> "device reset" which could be interpreted differently. ice can do
> function level reset, or core device reset also. Neither of those two
> resets activates firmware.
>
> Additionally the current function load process in ice doesn't support
> driver-init at all. That's something I'd like to see happen but is quite
> a significant refactor for how the driver loads. We need to refactor
> everything out so that devlink is created early on and factor out
> load/unload into handlers that can be called by the devlink reload. As I
> have primarily been focused on flash update I sort of left that for the
> future because it was a huge task to solve.

Cool! That was what I was concerned about, but I didn't know any
existing driver already has the problem. "FW reset" is not nearly
a clear enough operation. We'd end up with drivers differing and
users having to refer to vendor documentation to find out which
"reset level" maps to what.

I think the components in ethtool-reset try to address the same
problem, and they have the notion of per-port, and per-device.
In the modern world we lack the per-host notion, but that's still
strictly clearer than the limited API proposed here.

2020-07-29 14:57:37

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 7/28/2020 11:06 PM, Jakub Kicinski wrote:
> On Tue, 28 Jul 2020 12:18:30 -0700 Jacob Keller wrote:
>> On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
>>> From user perspective what's important is what the reset achieves (and
>>> perhaps how destructive it is). We can define the reset levels as:
>>>
>>> $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
>>> $ devlink dev reload pci/0000:82:00.0 driver-param-init
>>> $ devlink dev reload pci/0000:82:00.0 fw-activate
>>>
>>> combining should be possible when user wants multiple things to happen:
>>>
>>> $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
>> Where today "driver-param-init" is the default behavior. But didn't we
>> just say that mlxsw also does the equivalent of fw-activate?
> Actually the default should probably be the combination of
> driver-param-init and net-ns-respawn.

What about the support of these combinations, one device needs to reset
fw to apply the param init,

while another device can apply param-init without fw reset, but has to
reload the driver for fw-reset.

So the support per driver will be a matrix of combinations ?

> My expectations would be that the driver must perform the lowest reset
> level possible that satisfies the requested functional change.
> IOW driver may do more, in fact it should be acceptable for the driver
> to always for a full HW reset (unless --live or other constraint is
> specified).

OK, but some combinations may still not be valid for specific driver
even if it tries lowest level possible.

>>> We can also add the "reset level" specifier - for the cases where
>>> device is misbehaving:
>>>
>>> $ devlink dev reload pci/0000:82:00.0 level [driver|fw|hardware]
>> I guess I don't quite see how level fits in? This is orthogonal to the
>> other settings?
> Yup, it is, it's already orthogonal to what reload does today, hence the
> need for the "driver default" hack.
>
>>> But I don't think that we can go from the current reload command
>>> cleanly to just a level reset. The driver-specific default is a bad
>>> smell which indicates we're changing semantics from what user wants
>>> to what the reset depth is. Our semantics with the patch as it stands
>>> are in fact:
>>> - if you want to load new params or change netns, don't pass the level
>>> - the "driver default" workaround dictates the right reset level for
>>> param init;
>>> - if you want to activate new firmware - select the reset level you'd
>>> like from the reset level options.
>>>
>> I think I agree, having the "what gets reloaded" as a separate vector
>> makes sense and avoids confusion. For example for ice hardware,
>> "fw-activate" really does mean "Do an EMP reset" rather than just a
>> "device reset" which could be interpreted differently. ice can do
>> function level reset, or core device reset also. Neither of those two
>> resets activates firmware.
>>
>> Additionally the current function load process in ice doesn't support
>> driver-init at all. That's something I'd like to see happen but is quite
>> a significant refactor for how the driver loads. We need to refactor
>> everything out so that devlink is created early on and factor out
>> load/unload into handlers that can be called by the devlink reload. As I
>> have primarily been focused on flash update I sort of left that for the
>> future because it was a huge task to solve.
> Cool! That was what I was concerned about, but I didn't know any
> existing driver already has the problem. "FW reset" is not nearly
> a clear enough operation. We'd end up with drivers differing and
> users having to refer to vendor documentation to find out which
> "reset level" maps to what.
>
> I think the components in ethtool-reset try to address the same
> problem, and they have the notion of per-port, and per-device.
> In the modern world we lack the per-host notion, but that's still
> strictly clearer than the limited API proposed here.

2020-07-29 21:08:55

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Wed, 29 Jul 2020 17:54:08 +0300 Moshe Shemesh wrote:
> On 7/28/2020 11:06 PM, Jakub Kicinski wrote:
> > On Tue, 28 Jul 2020 12:18:30 -0700 Jacob Keller wrote:
> >> On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
> >>> From user perspective what's important is what the reset achieves (and
> >>> perhaps how destructive it is). We can define the reset levels as:
> >>>
> >>> $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
> >>> $ devlink dev reload pci/0000:82:00.0 driver-param-init
> >>> $ devlink dev reload pci/0000:82:00.0 fw-activate
> >>>
> >>> combining should be possible when user wants multiple things to happen:
> >>>
> >>> $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
> >> Where today "driver-param-init" is the default behavior. But didn't we
> >> just say that mlxsw also does the equivalent of fw-activate?
> > Actually the default should probably be the combination of
> > driver-param-init and net-ns-respawn.
>
> What about the support of these combinations, one device needs to reset
> fw to apply the param init, while another device can apply param-init
> without fw reset, but has to reload the driver for fw-reset.
>
> So the support per driver will be a matrix of combinations ?

Note that there is no driver reload in my examples, driver reload is
likely not user's goal. Whatever the driver needs to reset to satisfy
the goal is fair game IMO.

It's already the case that some drivers reset FW for param init and some
don't and nobody is complaining.

We should treat constraints separate (in this set we have the live
activation which is a constraint on the reload operation).

> > My expectations would be that the driver must perform the lowest
> > reset level possible that satisfies the requested functional change.
> > IOW driver may do more, in fact it should be acceptable for the
> > driver to always for a full HW reset (unless --live or other
> > constraint is specified).
>
> OK, but some combinations may still not be valid for specific driver
> even if it tries lowest level possible.

Can you give an example?

2020-07-30 12:33:05

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 7/30/2020 12:07 AM, Jakub Kicinski wrote:
> On Wed, 29 Jul 2020 17:54:08 +0300 Moshe Shemesh wrote:
>> On 7/28/2020 11:06 PM, Jakub Kicinski wrote:
>>> On Tue, 28 Jul 2020 12:18:30 -0700 Jacob Keller wrote:
>>>> On 7/28/2020 11:44 AM, Jakub Kicinski wrote:
>>>>> From user perspective what's important is what the reset achieves (and
>>>>> perhaps how destructive it is). We can define the reset levels as:
>>>>>
>>>>> $ devlink dev reload pci/0000:82:00.0 net-ns-respawn
>>>>> $ devlink dev reload pci/0000:82:00.0 driver-param-init
>>>>> $ devlink dev reload pci/0000:82:00.0 fw-activate
>>>>>
>>>>> combining should be possible when user wants multiple things to happen:
>>>>>
>>>>> $ devlink dev reload pci/0000:82:00.0 fw-activate driver-param-init
>>>> Where today "driver-param-init" is the default behavior. But didn't we
>>>> just say that mlxsw also does the equivalent of fw-activate?
>>> Actually the default should probably be the combination of
>>> driver-param-init and net-ns-respawn.
>> What about the support of these combinations, one device needs to reset
>> fw to apply the param init, while another device can apply param-init
>> without fw reset, but has to reload the driver for fw-reset.
>>
>> So the support per driver will be a matrix of combinations ?
> Note that there is no driver reload in my examples, driver reload is
> likely not user's goal. Whatever the driver needs to reset to satisfy
> the goal is fair game IMO.

Actually, driver-param-init (cmode driverinit) implicit driver
re-initialization.

> It's already the case that some drivers reset FW for param init and some
> don't and nobody is complaining.

Right, driver may need more than driver re-initialization for
driver-param-init, but I think that driver re-initialization is the
minimum for driver-param-init.

> We should treat constraints separate (in this set we have the live
> activation which is a constraint on the reload operation).
>
>>> My expectations would be that the driver must perform the lowest
>>> reset level possible that satisfies the requested functional change.
>>> IOW driver may do more, in fact it should be acceptable for the
>>> driver to always for a full HW reset (unless --live or other
>>> constraint is specified).
>> OK, but some combinations may still not be valid for specific driver
>> even if it tries lowest level possible.
> Can you give an example?

For example take the combination of fw-live-patch and param-init.

The fw-live-patch needs no re-initialization, while the param-init
requires driver re-initialization.

So the only way to do that is to the one command after the other, not
really combining.

Other combination, as fw-atcivate and param-init may not be valid for a
specific driver as it doesn't support one of them and so can't even run
one after the other.

2020-07-30 23:12:03

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Thu, 30 Jul 2020 15:30:45 +0300 Moshe Shemesh wrote:
> >>> My expectations would be that the driver must perform the lowest
> >>> reset level possible that satisfies the requested functional change.
> >>> IOW driver may do more, in fact it should be acceptable for the
> >>> driver to always for a full HW reset (unless --live or other
> >>> constraint is specified).
> >> OK, but some combinations may still not be valid for specific driver
> >> even if it tries lowest level possible.
> > Can you give an example?
>
> For example take the combination of fw-live-patch and param-init.
>
> The fw-live-patch needs no re-initialization, while the param-init
> requires driver re-initialization.
>
> So the only way to do that is to the one command after the other, not
> really combining.

You need to read my responses more carefully. I don't have
fw-live-patch in my proposal. The operation is fw-activate,
--live is independent and an constraint, not an operation.

2020-08-01 21:34:15

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 7/31/2020 2:11 AM, Jakub Kicinski wrote:
> On Thu, 30 Jul 2020 15:30:45 +0300 Moshe Shemesh wrote:
>>>>> My expectations would be that the driver must perform the lowest
>>>>> reset level possible that satisfies the requested functional change.
>>>>> IOW driver may do more, in fact it should be acceptable for the
>>>>> driver to always for a full HW reset (unless --live or other
>>>>> constraint is specified).
>>>> OK, but some combinations may still not be valid for specific driver
>>>> even if it tries lowest level possible.
>>> Can you give an example?
>> For example take the combination of fw-live-patch and param-init.
>>
>> The fw-live-patch needs no re-initialization, while the param-init
>> requires driver re-initialization.
>>
>> So the only way to do that is to the one command after the other, not
>> really combining.
> You need to read my responses more carefully. I don't have
> fw-live-patch in my proposal. The operation is fw-activate,
> --live is independent and an constraint, not an operation.

OK, I probably didn't get the whole picture right.

I am not sure I got it yet, please review if that's the uAPI that you
mean to:

devlink dev reload [ net-ns-respawn { PID | NAME | ID } ] [
driver-param-init ] [ fw-activate [ --live] ]

Also, I recall that before devlink param was added the devlink reload
was used for devlink resources.

I am not sure it is still used for devlink resources as I don't see it
in the code of devlink reload.

But if it is we probably should add it as another operation.

Jiri, please comment on that.

2020-08-03 14:17:54

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

Sat, Aug 01, 2020 at 11:32:25PM CEST, [email protected] wrote:
>
>On 7/31/2020 2:11 AM, Jakub Kicinski wrote:
>> On Thu, 30 Jul 2020 15:30:45 +0300 Moshe Shemesh wrote:
>> > > > > My expectations would be that the driver must perform the lowest
>> > > > > reset level possible that satisfies the requested functional change.
>> > > > > IOW driver may do more, in fact it should be acceptable for the
>> > > > > driver to always for a full HW reset (unless --live or other
>> > > > > constraint is specified).
>> > > > OK, but some combinations may still not be valid for specific driver
>> > > > even if it tries lowest level possible.
>> > > Can you give an example?
>> > For example take the combination of fw-live-patch and param-init.
>> >
>> > The fw-live-patch needs no re-initialization, while the param-init
>> > requires driver re-initialization.
>> >
>> > So the only way to do that is to the one command after the other, not
>> > really combining.
>> You need to read my responses more carefully. I don't have
>> fw-live-patch in my proposal. The operation is fw-activate,
>> --live is independent and an constraint, not an operation.
>
>
>OK, I probably didn't get the whole picture right.
>
>I am not sure I got it yet, please review if that's the uAPI that you mean
>to:
>
>devlink dev reload [ net-ns-respawn { PID | NAME | ID } ] [ driver-param-init
>] [ fw-activate [ --live] ]

Jakub, why do you prefer to have another extra level-specific option
"live"? I think it is clear to have it as a separate level. The behaviour
of the operation is quite different.

>
>
>Also, I recall that before devlink param was added the devlink reload was
>used for devlink resources.

Yes. That was the primary usecase. That is also why mlxsw does fw reset,
because the fw reset is needed in order to pass resources configuration.

So I don't think that the name should be "driver-param-init" as it is
not specific to params.

>
>I am not sure it is still used for devlink resources as I don't see it in the
>code of devlink reload.
>
>But if it is we probably should add it as another operation.
>
>Jiri, please comment on that.
>

2020-08-03 20:59:35

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Mon, 3 Aug 2020 16:14:42 +0200 Jiri Pirko wrote:
> >devlink dev reload [ net-ns-respawn { PID | NAME | ID } ] [ driver-param-init
> >] [ fw-activate [ --live] ]
>
> Jakub, why do you prefer to have another extra level-specific option
> "live"? I think it is clear to have it as a separate level. The behaviour
> of the operation is quite different.

I was trying to avoid having to provide a Cartesian product of
operation and system disruption level, if any other action can
be done "live" at some point.

But no strong feelings about that one.

Really, as long as there is no driver-specific defaults (or as
little driver-specific anything as possible) and user actions
are clearly expressed (fw-reset does not necessarily imply
fw-activation) - the API will be fine IMO.

2020-08-04 10:07:03

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

Mon, Aug 03, 2020 at 10:57:03PM CEST, [email protected] wrote:
>On Mon, 3 Aug 2020 16:14:42 +0200 Jiri Pirko wrote:
>> >devlink dev reload [ net-ns-respawn { PID | NAME | ID } ] [ driver-param-init
>> >] [ fw-activate [ --live] ]
>>
>> Jakub, why do you prefer to have another extra level-specific option
>> "live"? I think it is clear to have it as a separate level. The behaviour
>> of the operation is quite different.
>
>I was trying to avoid having to provide a Cartesian product of
>operation and system disruption level, if any other action can
>be done "live" at some point.
>
>But no strong feelings about that one.
>
>Really, as long as there is no driver-specific defaults (or as
>little driver-specific anything as possible) and user actions
>are clearly expressed (fw-reset does not necessarily imply
>fw-activation) - the API will be fine IMO.

Clear actions, that is what I'm fine with.

But not sure how you think we can achieve no driver-specific defaults.
We have them already :/ I don't think we can easily remove them and not
break user expectations.

2020-08-04 20:43:05

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Tue, 4 Aug 2020 12:04:18 +0200 Jiri Pirko wrote:
> Mon, Aug 03, 2020 at 10:57:03PM CEST, [email protected] wrote:
> >I was trying to avoid having to provide a Cartesian product of
> >operation and system disruption level, if any other action can
> >be done "live" at some point.
> >
> >But no strong feelings about that one.
> >
> >Really, as long as there is no driver-specific defaults (or as
> >little driver-specific anything as possible) and user actions
> >are clearly expressed (fw-reset does not necessarily imply
> >fw-activation) - the API will be fine IMO.
>
> Clear actions, that is what I'm fine with.
>
> But not sure how you think we can achieve no driver-specific defaults.
> We have them already :/ I don't think we can easily remove them and not
> break user expectations.

AFAIU the per-driver default is needed because we went too low
level with what the action constitutes. We need maintain the higher
level actions.

The user clearly did not care if FW was reset during devlink reload
before this set, so what has changed? The objective user has is to
activate their config / FW / move to different net ns.

Reloading the driver or resetting FW is a low level detail which
achieves different things for different implementations. So it's
not a suitable abstraction -> IOW we need the driver default.

The work flow for the user is:

0. download fw to /lib/firmware
1. devlink flash $dev $fw
2. if live activation is enabled
yes - devlink reload $dev $live-activate
no - report machine has to be drained for reboot

fw-reset can't be $live-activate, because as Jake said fw-reset does
not activate the new image for Intel. So will we end up per-driver
defaults in the kernel space, and user space maintaining a mapping from
a driver to what a "level" of reset implies.

I hope this makes things crystal clear. Please explain what problems
you're seeing and extensions you're expecting. A list of user scenarios
you foresee would be v. useful.

2020-08-05 19:57:42

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

Tue, Aug 04, 2020 at 10:39:46PM CEST, [email protected] wrote:
>On Tue, 4 Aug 2020 12:04:18 +0200 Jiri Pirko wrote:
>> Mon, Aug 03, 2020 at 10:57:03PM CEST, [email protected] wrote:
>> >I was trying to avoid having to provide a Cartesian product of
>> >operation and system disruption level, if any other action can
>> >be done "live" at some point.
>> >
>> >But no strong feelings about that one.
>> >
>> >Really, as long as there is no driver-specific defaults (or as
>> >little driver-specific anything as possible) and user actions
>> >are clearly expressed (fw-reset does not necessarily imply
>> >fw-activation) - the API will be fine IMO.
>>
>> Clear actions, that is what I'm fine with.
>>
>> But not sure how you think we can achieve no driver-specific defaults.
>> We have them already :/ I don't think we can easily remove them and not
>> break user expectations.
>
>AFAIU the per-driver default is needed because we went too low
>level with what the action constitutes. We need maintain the higher
>level actions.
>
>The user clearly did not care if FW was reset during devlink reload
>before this set, so what has changed? The objective user has is to

Well for mlxsw, the user is used to this flow:
devlink dev flash - flash new fw
devlink dev reload - new fw is activated and reset and driver instances
are re-created.

>activate their config / FW / move to different net ns.
>
>Reloading the driver or resetting FW is a low level detail which
>achieves different things for different implementations. So it's
>not a suitable abstraction -> IOW we need the driver default.

I'm confused. So you think we need the driver default?

>
>
>The work flow for the user is:
>
>0. download fw to /lib/firmware
>1. devlink flash $dev $fw
>2. if live activation is enabled
> yes - devlink reload $dev $live-activate
> no - report machine has to be drained for reboot
>
>fw-reset can't be $live-activate, because as Jake said fw-reset does
>not activate the new image for Intel. So will we end up per-driver
>defaults in the kernel space, and user space maintaining a mapping from

Well, that is what what is Moshe's proposal. Per-driver kernel default..
I'm not sure what we are arguing about then :/

>a driver to what a "level" of reset implies.
>
>I hope this makes things crystal clear. Please explain what problems
>you're seeing and extensions you're expecting. A list of user scenarios
>you foresee would be v. useful.

2020-08-06 18:43:33

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Wed, 5 Aug 2020 13:02:58 +0200 Jiri Pirko wrote:
> Tue, Aug 04, 2020 at 10:39:46PM CEST, [email protected] wrote:
> >AFAIU the per-driver default is needed because we went too low
> >level with what the action constitutes. We need maintain the higher
> >level actions.
> >
> >The user clearly did not care if FW was reset during devlink reload
> >before this set, so what has changed? The objective user has is to
>
> Well for mlxsw, the user is used to this flow:
> devlink dev flash - flash new fw
> devlink dev reload - new fw is activated and reset and driver instances
> are re-created.

Ugh, if the current behavior already implies fw-activation for some
drivers then the default has to probably be "do all the things" :S

> >activate their config / FW / move to different net ns.
> >
> >Reloading the driver or resetting FW is a low level detail which
> >achieves different things for different implementations. So it's
> >not a suitable abstraction -> IOW we need the driver default.
>
> I'm confused. So you think we need the driver default?

No, I'm talking about the state of this patch set. _In this patchset_
we need a driver default because of the unsuitable abstraction.

Better design would not require it.

> >The work flow for the user is:
> >
> >0. download fw to /lib/firmware
> >1. devlink flash $dev $fw
> >2. if live activation is enabled
> > yes - devlink reload $dev $live-activate
> > no - report machine has to be drained for reboot
> >
> >fw-reset can't be $live-activate, because as Jake said fw-reset does
> >not activate the new image for Intel. So will we end up per-driver
> >defaults in the kernel space, and user space maintaining a mapping from
>
> Well, that is what what is Moshe's proposal. Per-driver kernel default..
> I'm not sure what we are arguing about then :/

The fact that if I do a pure "driver reload" it will active new
firmware for mlxsw but not for mlx5. In this patchset for mlx5 I need
driver reload fw-reset. And for Intel there is no suitable option.

> >a driver to what a "level" of reset implies.
> >
> >I hope this makes things crystal clear. Please explain what problems
> >you're seeing and extensions you're expecting. A list of user scenarios
> >you foresee would be v. useful.

2020-08-06 22:57:44

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 8/6/2020 11:25 AM, Jakub Kicinski wrote:
> On Wed, 5 Aug 2020 13:02:58 +0200 Jiri Pirko wrote:
>> Tue, Aug 04, 2020 at 10:39:46PM CEST, [email protected] wrote:
>>> AFAIU the per-driver default is needed because we went too low
>>> level with what the action constitutes. We need maintain the higher
>>> level actions.
>>>
>>> The user clearly did not care if FW was reset during devlink reload
>>> before this set, so what has changed? The objective user has is to
>>
>> Well for mlxsw, the user is used to this flow:
>> devlink dev flash - flash new fw
>> devlink dev reload - new fw is activated and reset and driver instances
>> are re-created.
>
> Ugh, if the current behavior already implies fw-activation for some
> drivers then the default has to probably be "do all the things" :S
>
>>> activate their config / FW / move to different net ns.
>>>
>>> Reloading the driver or resetting FW is a low level detail which
>>> achieves different things for different implementations. So it's
>>> not a suitable abstraction -> IOW we need the driver default.
>>
>> I'm confused. So you think we need the driver default?
>
> No, I'm talking about the state of this patch set. _In this patchset_
> we need a driver default because of the unsuitable abstraction.
>
> Better design would not require it.
>
>>> The work flow for the user is:
>>>
>>> 0. download fw to /lib/firmware
>>> 1. devlink flash $dev $fw
>>> 2. if live activation is enabled
>>> yes - devlink reload $dev $live-activate
>>> no - report machine has to be drained for reboot
>>>
>>> fw-reset can't be $live-activate, because as Jake said fw-reset does
>>> not activate the new image for Intel. So will we end up per-driver
>>> defaults in the kernel space, and user space maintaining a mapping from
>>
>> Well, that is what what is Moshe's proposal. Per-driver kernel default..
>> I'm not sure what we are arguing about then :/
>
> The fact that if I do a pure "driver reload" it will active new
> firmware for mlxsw but not for mlx5. In this patchset for mlx5 I need
> driver reload fw-reset. And for Intel there is no suitable option.
>

I want to clarify here, at least for ice: we *do* have a reset that can
activate firmware, but we have various level of reset which not all of
them do a fw activation. We have several levels of firmware reset,
including a "PF" reset that only resets data associated with that
function, a CORE reset which resets all functions, and then an EMP reset
which will activate the new firmware. For all of these resets, affected
PFs are notified over their firmware admin message queue. However, there
isn't a notion of negotiating beyond a message indicating what type of
reset is occurring.

I mostly wanted to clarify that "fw-reset" as a name doesn't necessarily
imply firmware activation. (hence separating fw-activate vs fw-reset).

Thanks,
Jake

>>> a driver to what a "level" of reset implies.
>>>
>>> I hope this makes things crystal clear. Please explain what problems
>>> you're seeing and extensions you're expecting. A list of user scenarios
>>> you foresee would be v. useful.

2020-08-09 13:22:35

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 8/6/2020 9:25 PM, Jakub Kicinski wrote:
> On Wed, 5 Aug 2020 13:02:58 +0200 Jiri Pirko wrote:
>> Tue, Aug 04, 2020 at 10:39:46PM CEST, [email protected] wrote:
>>> AFAIU the per-driver default is needed because we went too low
>>> level with what the action constitutes. We need maintain the higher
>>> level actions.
>>>
>>> The user clearly did not care if FW was reset during devlink reload
>>> before this set, so what has changed? The objective user has is to
>> Well for mlxsw, the user is used to this flow:
>> devlink dev flash - flash new fw
>> devlink dev reload - new fw is activated and reset and driver instances
>> are re-created.
> Ugh, if the current behavior already implies fw-activation for some
> drivers then the default has to probably be "do all the things" :S

Okay, so devlink reload default for mlx5 will include also fw-activate
to align with mlxsw default.

Meaning drivers that supports fw-activate will add it to the default.

The flow of devlink reload default on mlx5 will be:

If there is FW image pending and live patch is suitable to apply, do
live patch and driver re-initialization.

If there is FW image pending but live patch doesn't fit do fw-reset and
driver-initialization.

If no FW image pending just do driver-initialization.

I still think I should on top of that add the level option to be
selected by the user if he prefers a specific action, so the uAPI would be:

devlink dev reload [ netns { PID | NAME | ID } ] [ level { fw-live-patch
| driver-reinit |fw-activate } ]

But I am still missing something: fw-activate implies that it will
activate a new FW image stored on flash, pending activation. What if the
user wants to reset and reload the FW if no new FW pending ? Should we
add --force option to fw-activate level ?

>>> activate their config / FW / move to different net ns.
>>>
>>> Reloading the driver or resetting FW is a low level detail which
>>> achieves different things for different implementations. So it's
>>> not a suitable abstraction -> IOW we need the driver default.
>> I'm confused. So you think we need the driver default?
> No, I'm talking about the state of this patch set. _In this patchset_
> we need a driver default because of the unsuitable abstraction.
>
> Better design would not require it.
>
>>> The work flow for the user is:
>>>
>>> 0. download fw to /lib/firmware
>>> 1. devlink flash $dev $fw
>>> 2. if live activation is enabled
>>> yes - devlink reload $dev $live-activate
>>> no - report machine has to be drained for reboot
>>>
>>> fw-reset can't be $live-activate, because as Jake said fw-reset does
>>> not activate the new image for Intel. So will we end up per-driver
>>> defaults in the kernel space, and user space maintaining a mapping from
>> Well, that is what what is Moshe's proposal. Per-driver kernel default..
>> I'm not sure what we are arguing about then :/
> The fact that if I do a pure "driver reload" it will active new
> firmware for mlxsw but not for mlx5. In this patchset for mlx5 I need
> driver reload fw-reset. And for Intel there is no suitable option.
>
>>> a driver to what a "level" of reset implies.
>>>
>>> I hope this makes things crystal clear. Please explain what problems
>>> you're seeing and extensions you're expecting. A list of user scenarios
>>> you foresee would be v. useful.

2020-08-10 16:54:21

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Sun, 9 Aug 2020 16:21:29 +0300 Moshe Shemesh wrote:
> Okay, so devlink reload default for mlx5 will include also fw-activate
> to align with mlxsw default.
>
> Meaning drivers that supports fw-activate will add it to the default.

No per-driver default.

Maybe the difference between mlxsw and mlx5 can be simply explained by
the fact that mlxsw loads firmware from /lib/firmware on every probe
(more or less).

It's only natural for a driver which loads FW from disk to load it on
driver reload.

> The flow of devlink reload default on mlx5 will be:
>
> If there is FW image pending and live patch is suitable to apply, do
> live patch and driver re-initialization.
>
> If there is FW image pending but live patch doesn't fit do fw-reset and
> driver-initialization.
>
> If no FW image pending just do driver-initialization.

This sounds too complicated. Don't try to guess what the user wants.

> I still think I should on top of that add the level option to be
> selected by the user if he prefers a specific action, so the uAPI would be:
>
> devlink dev reload [ netns { PID | NAME | ID } ] [ level { fw-live-patch
> | driver-reinit |fw-activate } ]

I'm all for the level/action.

> But I am still missing something: fw-activate implies that it will
> activate a new FW image stored on flash, pending activation. What if the
> user wants to reset and reload the FW if no new FW pending ? Should we
> add --force option to fw-activate level ?

Since reload does not check today if anything changed - i.e. if reload
is actually needed, neither should fw-activate, IMO. I'd expect the
"--force behavior" to be the default.

2020-08-10 17:10:22

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On 8/10/2020 9:53 AM, Jakub Kicinski wrote:
> On Sun, 9 Aug 2020 16:21:29 +0300 Moshe Shemesh wrote:
>> Okay, so devlink reload default for mlx5 will include also fw-activate
>> to align with mlxsw default.
>>
>> Meaning drivers that supports fw-activate will add it to the default.
>
> No per-driver default.
>
> Maybe the difference between mlxsw and mlx5 can be simply explained by
> the fact that mlxsw loads firmware from /lib/firmware on every probe
> (more or less).
>
> It's only natural for a driver which loads FW from disk to load it on
> driver reload.
>

This seems reasonable to me as long as the drivers document this
behavior in their devlink/<driver>.rst. We shouldn't change existing
behavior. One could argue that this difference in behavior amounts to a
"driver default"... but I agree that we shouldn't enshrine that in the
interface.

>> The flow of devlink reload default on mlx5 will be:
>>
>> If there is FW image pending and live patch is suitable to apply, do
>> live patch and driver re-initialization.
>>
>> If there is FW image pending but live patch doesn't fit do fw-reset and
>> driver-initialization.
>>
>> If no FW image pending just do driver-initialization.
>
> This sounds too complicated. Don't try to guess what the user wants.
> >> I still think I should on top of that add the level option to be
>> selected by the user if he prefers a specific action, so the uAPI would be:
>>
>> devlink dev reload [ netns { PID | NAME | ID } ] [ level { fw-live-patch
>> | driver-reinit |fw-activate } ]
>
> I'm all for the level/action.
>

Yep, same here.

>> But I am still missing something: fw-activate implies that it will
>> activate a new FW image stored on flash, pending activation. What if the
>> user wants to reset and reload the FW if no new FW pending ? Should we
>> add --force option to fw-activate level ?
>
> Since reload does not check today if anything changed - i.e. if reload
> is actually needed, neither should fw-activate, IMO. I'd expect the
> "--force behavior" to be the default.
>

Yep. What about if there is HW/FW that can't initiate the fw-activate
reset unless there is a pending update? I think ice firmware might
respond to the "please reset/activate" command with a specific status
code indicating that no update was pending.

I think the simplest solution is to just interpret this as a success.
Alternatively we could report a specific error to inform user that no
activation took place?

2020-08-10 18:18:29

[permalink] [raw]

Subject: Re: [PATCH net-next RFC 01/13] devlink: Add reload level option to devlink reload command

On Mon, 10 Aug 2020 10:09:20 -0700 Jacob Keller wrote:
> >> But I am still missing something: fw-activate implies that it will
> >> activate a new FW image stored on flash, pending activation. What if the
> >> user wants to reset and reload the FW if no new FW pending ? Should we
> >> add --force option to fw-activate level ?
> >
> > Since reload does not check today if anything changed - i.e. if reload
> > is actually needed, neither should fw-activate, IMO. I'd expect the
> > "--force behavior" to be the default.
> >
>
> Yep. What about if there is HW/FW that can't initiate the fw-activate
> reset unless there is a pending update? I think ice firmware might
> respond to the "please reset/activate" command with a specific status
> code indicating that no update was pending.
>
> I think the simplest solution is to just interpret this as a success.
> Alternatively we could report a specific error to inform user that no
> activation took place?

I'd do EOPNOTSUPP + extack.

2020-08-11 05:47:50