[2/5] ethdev: add capability to keep shared objects on restart

  From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>

rte_flow_action_handle_create() did not mention what happens
with an indirect action when a device is stopped, possibly reconfigured,
and started again. It is natural for some indirect actions to be
persistent, like counters and meters; keeping others just saves
application time and complexity. However, not all PMDs can support it.
It is proposed to add a device capability to indicate if indirect actions
are kept across the above sequence or implicitly destroyed.

In the future, indirect actions may not be the only type of objects
shared between flow rules. The capability bit intends to cover all
possible types of such objects, hence its name.

It may happen that in the future a PMD acquires support for a type of
shared objects that it cannot keep across a restart. It is undesirable
to stop advertising the capability so that applications that don't use
objects of the problematic type can still take advantage of it.
This is why PMDs are allowed to keep only a subset of shared objects
provided that the vendor mandatorily documents it.

If the device is being reconfigured in a way that is incompatible with
an existing shared objects, PMD is required to report an error.
This is mandatory, because flow API does not supply users with
capabilities, so this is the only way for a user to learn that
configuration is invalid. For example, if queue count changes and RSS
indirect action specifies queues that are going away, the user must
update the action before removing the queues or remove the action and
all flow rules that were using it.

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
---
 doc/guides/prog_guide/rte_flow.rst | 12 ++++++++++++
 lib/ethdev/rte_ethdev.h            |  6 ++++++
 2 files changed, 18 insertions(+)
  

Hi Dmitry,

> -----Original Message-----
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: Tuesday, October 5, 2021 3:52 AM
> To: dev@dpdk.org
> Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>;
> NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>
> Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on restart
> 
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> 
> rte_flow_action_handle_create() did not mention what happens with an
> indirect action when a device is stopped, possibly reconfigured, and started
> again. It is natural for some indirect actions to be persistent, like counters and
> meters; keeping others just saves application time and complexity. However,
> not all PMDs can support it.
> It is proposed to add a device capability to indicate if indirect actions are kept
> across the above sequence or implicitly destroyed.
> 
> In the future, indirect actions may not be the only type of objects shared
> between flow rules. The capability bit intends to cover all possible types of such
> objects, hence its name.
> 
> It may happen that in the future a PMD acquires support for a type of shared
> objects that it cannot keep across a restart. It is undesirable to stop advertising
> the capability so that applications that don't use objects of the problematic type
> can still take advantage of it.
> This is why PMDs are allowed to keep only a subset of shared objects provided
> that the vendor mandatorily documents it.
> 
> If the device is being reconfigured in a way that is incompatible with an existing
> shared objects, PMD is required to report an error.
> This is mandatory, because flow API does not supply users with capabilities, so
> this is the only way for a user to learn that configuration is invalid. For
> example, if queue count changes and RSS indirect action specifies queues that
> are going away, the user must update the action before removing the queues
> or remove the action and all flow rules that were using it.
> 
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> ---

Acked-by: Ori Kam <orika@nvidia.com>
Thanks,
Ori
  

This thread continues discussions on previous versions
to keep everything in the thread with final patches:

[1]: http://inbox.dpdk.org/dev/d5673b58-5aa6-ca35-5b60-d938e56cfee1@oktetlabs.ru/
[2]: http://inbox.dpdk.org/dev/DM8PR12MB5400997CCEC9169AC5AE0C89D6EA9@DM8PR12MB5400.namprd12.prod.outlook.com/

Please see below.

> -----Original Message-----
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Sent: 5 октября 2021 г. 3:52
> To: dev@dpdk.org
> Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>; NBU-
> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on
> restart
> 
> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> 
> rte_flow_action_handle_create() did not mention what happens with an
> indirect action when a device is stopped, possibly reconfigured, and
> started again. It is natural for some indirect actions to be persistent,
> like counters and meters; keeping others just saves application time and
> complexity. However, not all PMDs can support it.
> It is proposed to add a device capability to indicate if indirect actions
> are kept across the above sequence or implicitly destroyed.
> 
> In the future, indirect actions may not be the only type of objects shared
> between flow rules. The capability bit intends to cover all possible types
> of such objects, hence its name.
> 
> It may happen that in the future a PMD acquires support for a type of
> shared objects that it cannot keep across a restart. It is undesirable to
> stop advertising the capability so that applications that don't use
> objects of the problematic type can still take advantage of it.
> This is why PMDs are allowed to keep only a subset of shared objects
> provided that the vendor mandatorily documents it.
> 
> If the device is being reconfigured in a way that is incompatible with an
> existing shared objects, PMD is required to report an error.
> This is mandatory, because flow API does not supply users with
> capabilities, so this is the only way for a user to learn that
> configuration is invalid. For example, if queue count changes and RSS
> indirect action specifies queues that are going away, the user must update
> the action before removing the queues or remove the action and all flow
> rules that were using it.
> 
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> ---
> [...]

Current pain point is that capability bits may be insufficient
and a programmatic way is desired to check which types of objects
can be kept across restart, instead of documenting the limitations.

I support one of previous Ori's suggestions and want to clarify it [1]:

Ori: "Another way is to assume that if the action was created before port start it will be kept after port stop."
Andrew: "It does not sound like a solution. May be I simply don't know
target usecase."

What Ori suggests (offline discussion summary): Suppose an application wants to check whether a shared object (indirect action) or a flow rule of a particular kind. It calls rte_flow_action_handle_create() or rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it means objects of this type can be kept across restart, 2) it's a normal object created that will work after the port is started. This is logical, because if the PMD can keep some kind of objects when the port is stopped, it is likely to be able to create them when the port is not started. It is subject to discussion if "object kind" means only "type" or "type + transfer bit" combination; for mlx5 PMD it doesn't matter. One minor drawback is that applications can only do the test when the port is stopped, but it seems likely that the test really needs to be done at startup anyway.

If this is acceptable:
1. Capability bits are not needed anymore.
2. ethdev patches can be accepted in RC1, present behavior is undefined anyway.
3. PMD patches will need update that can be done by RC2.
  

On 10/13/2021 9:32 AM, Dmitry Kozlyuk wrote:
> This thread continues discussions on previous versions
> to keep everything in the thread with final patches:
> 
> [1]: http://inbox.dpdk.org/dev/d5673b58-5aa6-ca35-5b60-d938e56cfee1@oktetlabs.ru/
> [2]: http://inbox.dpdk.org/dev/DM8PR12MB5400997CCEC9169AC5AE0C89D6EA9@DM8PR12MB5400.namprd12.prod.outlook.com/
> 
> Please see below.
> 
>> -----Original Message-----
>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>> Sent: 5 октября 2021 г. 3:52
>> To: dev@dpdk.org
>> Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>; NBU-
>> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> <ferruh.yigit@intel.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on
>> restart
>>
>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>
>> rte_flow_action_handle_create() did not mention what happens with an
>> indirect action when a device is stopped, possibly reconfigured, and
>> started again. It is natural for some indirect actions to be persistent,
>> like counters and meters; keeping others just saves application time and
>> complexity. However, not all PMDs can support it.
>> It is proposed to add a device capability to indicate if indirect actions
>> are kept across the above sequence or implicitly destroyed.
>>
>> In the future, indirect actions may not be the only type of objects shared
>> between flow rules. The capability bit intends to cover all possible types
>> of such objects, hence its name.
>>
>> It may happen that in the future a PMD acquires support for a type of
>> shared objects that it cannot keep across a restart. It is undesirable to
>> stop advertising the capability so that applications that don't use
>> objects of the problematic type can still take advantage of it.
>> This is why PMDs are allowed to keep only a subset of shared objects
>> provided that the vendor mandatorily documents it.
>>
>> If the device is being reconfigured in a way that is incompatible with an
>> existing shared objects, PMD is required to report an error.
>> This is mandatory, because flow API does not supply users with
>> capabilities, so this is the only way for a user to learn that
>> configuration is invalid. For example, if queue count changes and RSS
>> indirect action specifies queues that are going away, the user must update
>> the action before removing the queues or remove the action and all flow
>> rules that were using it.
>>
>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>> ---
>> [...]
> 
> Current pain point is that capability bits may be insufficient
> and a programmatic way is desired to check which types of objects
> can be kept across restart, instead of documenting the limitations.
> 
> I support one of previous Ori's suggestions and want to clarify it [1]:
> 
> Ori: "Another way is to assume that if the action was created before port start it will be kept after port stop."
> Andrew: "It does not sound like a solution. May be I simply don't know
> target usecase."
> 
> What Ori suggests (offline discussion summary): Suppose an application wants to check whether a shared object (indirect action) or a flow rule of a particular kind. It calls rte_flow_action_handle_create() or rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it means objects of this type can be kept across restart, 2) it's a normal object created that will work after the port is started. This is logical, because if the PMD can keep some kind of objects when the port is stopped, it is likely to be able to create them when the port is not started. It is subject to discussion if "object kind" means only "type" or "type + transfer bit" combination; for mlx5 PMD it doesn't matter. One minor drawback is that applications can only do the test when the port is stopped, but it seems likely that the test really needs to be done at startup anyway.
> 
> If this is acceptable:
> 1. Capability bits are not needed anymore.
> 2. ethdev patches can be accepted in RC1, present behavior is undefined anyway.
> 3. PMD patches will need update that can be done by RC2.
> 

Hi Dmitry,

Are you planning to update drivers yourself on -rc2?
Or do you mean PMD maintainers should update themselves, if so do they
know about it?

If the ethdev layer is updated in a way to impact the drivers, it should
be either:
- all drivers updated with a change
or
- give PMDs time to implement it on their own time, meanwhile they can report
their support status by a flag

We had multiple sample of second case in the past but it is harder for
this case.

For this case what about having three states:
- FLOW_RULE_KEEP
- FLOW_RULE_DESTROY
- FLOW_RULE_UNKNOWN

And set 'FLOW_RULE_UNKNOWN' for all drivers, to simulate current status,
until driver is updated.
  

> -----Original Message-----
> From: Dmitry Kozlyuk
> Sent: 13 октября 2021 г. 11:33
> To: dev@dpdk.org; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ori
> Kam <orika@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>
> Subject: RE: [PATCH 2/5] ethdev: add capability to keep shared objects on
> restart
> 
> This thread continues discussions on previous versions to keep everything
> in the thread with final patches:
> 
> [1]: http://inbox.dpdk.org/dev/d5673b58-5aa6-ca35-5b60-
> d938e56cfee1@oktetlabs.ru/
> [2]:
> http://inbox.dpdk.org/dev/DM8PR12MB5400997CCEC9169AC5AE0C89D6EA9@DM8PR12MB
> 5400.namprd12.prod.outlook.com/
> 
> Please see below.
> 
> > -----Original Message-----
> > From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > Sent: 5 октября 2021 г. 3:52
> > To: dev@dpdk.org
> > Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>;
> > NBU- Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <andrew.rybchenko@oktetlabs.ru>
> > Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on
> > restart
> >
> > From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >
> > rte_flow_action_handle_create() did not mention what happens with an
> > indirect action when a device is stopped, possibly reconfigured, and
> > started again. It is natural for some indirect actions to be
> > persistent, like counters and meters; keeping others just saves
> > application time and complexity. However, not all PMDs can support it.
> > It is proposed to add a device capability to indicate if indirect
> > actions are kept across the above sequence or implicitly destroyed.
> >
> > In the future, indirect actions may not be the only type of objects
> > shared between flow rules. The capability bit intends to cover all
> > possible types of such objects, hence its name.
> >
> > It may happen that in the future a PMD acquires support for a type of
> > shared objects that it cannot keep across a restart. It is undesirable
> > to stop advertising the capability so that applications that don't use
> > objects of the problematic type can still take advantage of it.
> > This is why PMDs are allowed to keep only a subset of shared objects
> > provided that the vendor mandatorily documents it.
> >
> > If the device is being reconfigured in a way that is incompatible with
> > an existing shared objects, PMD is required to report an error.
> > This is mandatory, because flow API does not supply users with
> > capabilities, so this is the only way for a user to learn that
> > configuration is invalid. For example, if queue count changes and RSS
> > indirect action specifies queues that are going away, the user must
> > update the action before removing the queues or remove the action and
> > all flow rules that were using it.
> >
> > Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > ---
> > [...]
> 
> Current pain point is that capability bits may be insufficient and a
> programmatic way is desired to check which types of objects can be kept
> across restart, instead of documenting the limitations.
> 
> I support one of previous Ori's suggestions and want to clarify it [1]:
> 
> Ori: "Another way is to assume that if the action was created before port
> start it will be kept after port stop."
> Andrew: "It does not sound like a solution. May be I simply don't know
> target usecase."
> 
> What Ori suggests (offline discussion summary): Suppose an application
> wants to check whether a shared object (indirect action) or a flow rule of
> a particular kind. It calls rte_flow_action_handle_create() or
> rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it means
> objects of this type can be kept across restart, 2) it's a normal object
> created that will work after the port is started. This is logical, because
> if the PMD can keep some kind of objects when the port is stopped, it is
> likely to be able to create them when the port is not started. It is
> subject to discussion if "object kind" means only "type" or "type +
> transfer bit" combination; for mlx5 PMD it doesn't matter. One minor
> drawback is that applications can only do the test when the port is
> stopped, but it seems likely that the test really needs to be done at
> startup anyway.
> 
> If this is acceptable:
> 1. Capability bits are not needed anymore.
> 2. ethdev patches can be accepted in RC1, present behavior is undefined
> anyway.
> 3. PMD patches will need update that can be done by RC2.

Andrew, what do you think?
If you agree, do we need to include transfer bit into "kind"?
I'd like to conclude before RC1 and can update the docs quickly.

I've seen the proposition to advertise capability
to create flow rules before device start as a flag.
I don't think it conflicts with Ori's suggestion
because the flag doesn't imply that _any_ rule can be created,
neither does it say about indirect actions.
On the other hand, if PMD can create a flow object (rule, etc.)
when the device is not started, it is logical to assume that
after the device is stopped it can move existing flow objects
to the same state as when the device was not started, then restore
when it is started again.
  

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: 14 октября 2021 г. 16:47
> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
> Darawsheh <rasland@nvidia.com>
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Qi Zhang
> <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects on
> restart
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/13/2021 9:32 AM, Dmitry Kozlyuk wrote:
> > This thread continues discussions on previous versions to keep
> > everything in the thread with final patches:
> >
> > [1]:
> > http://inbox.dpdk.org/dev/d5673b58-5aa6-ca35-5b60-d938e56cfee1@oktetla
> > bs.ru/
> > [2]:
> > http://inbox.dpdk.org/dev/DM8PR12MB5400997CCEC9169AC5AE0C89D6EA9@DM8PR
> > 12MB5400.namprd12.prod.outlook.com/
> >
> > Please see below.
> >
> >> -----Original Message-----
> >> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >> Sent: 5 октября 2021 г. 3:52
> >> To: dev@dpdk.org
> >> Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>;
> >> NBU- Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >> <ferruh.yigit@intel.com>; Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru>
> >> Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on
> >> restart
> >>
> >> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >>
> >> rte_flow_action_handle_create() did not mention what happens with an
> >> indirect action when a device is stopped, possibly reconfigured, and
> >> started again. It is natural for some indirect actions to be
> >> persistent, like counters and meters; keeping others just saves
> >> application time and complexity. However, not all PMDs can support it.
> >> It is proposed to add a device capability to indicate if indirect
> >> actions are kept across the above sequence or implicitly destroyed.
> >>
> >> In the future, indirect actions may not be the only type of objects
> >> shared between flow rules. The capability bit intends to cover all
> >> possible types of such objects, hence its name.
> >>
> >> It may happen that in the future a PMD acquires support for a type of
> >> shared objects that it cannot keep across a restart. It is
> >> undesirable to stop advertising the capability so that applications
> >> that don't use objects of the problematic type can still take advantage
> of it.
> >> This is why PMDs are allowed to keep only a subset of shared objects
> >> provided that the vendor mandatorily documents it.
> >>
> >> If the device is being reconfigured in a way that is incompatible
> >> with an existing shared objects, PMD is required to report an error.
> >> This is mandatory, because flow API does not supply users with
> >> capabilities, so this is the only way for a user to learn that
> >> configuration is invalid. For example, if queue count changes and RSS
> >> indirect action specifies queues that are going away, the user must
> >> update the action before removing the queues or remove the action and
> >> all flow rules that were using it.
> >>
> >> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >> ---
> >> [...]
> >
> > Current pain point is that capability bits may be insufficient and a
> > programmatic way is desired to check which types of objects can be
> > kept across restart, instead of documenting the limitations.
> >
> > I support one of previous Ori's suggestions and want to clarify it [1]:
> >
> > Ori: "Another way is to assume that if the action was created before
> port start it will be kept after port stop."
> > Andrew: "It does not sound like a solution. May be I simply don't know
> > target usecase."
> >
> > What Ori suggests (offline discussion summary): Suppose an application
> wants to check whether a shared object (indirect action) or a flow rule of
> a particular kind. It calls rte_flow_action_handle_create() or
> rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it means
> objects of this type can be kept across restart, 2) it's a normal object
> created that will work after the port is started. This is logical, because
> if the PMD can keep some kind of objects when the port is stopped, it is
> likely to be able to create them when the port is not started. It is
> subject to discussion if "object kind" means only "type" or "type +
> transfer bit" combination; for mlx5 PMD it doesn't matter. One minor
> drawback is that applications can only do the test when the port is
> stopped, but it seems likely that the test really needs to be done at
> startup anyway.
> >
> > If this is acceptable:
> > 1. Capability bits are not needed anymore.
> > 2. ethdev patches can be accepted in RC1, present behavior is undefined
> anyway.
> > 3. PMD patches will need update that can be done by RC2.
> >
> 
> Hi Dmitry,
> 
> Are you planning to update drivers yourself on -rc2?
> Or do you mean PMD maintainers should update themselves, if so do they
> know about it?
> 
> If the ethdev layer is updated in a way to impact the drivers, it should
> be either:
> - all drivers updated with a change
> or
> - give PMDs time to implement it on their own time, meanwhile they can
> report their support status by a flag
> 
> We had multiple sample of second case in the past but it is harder for
> this case.
> 
> For this case what about having three states:
> - FLOW_RULE_KEEP
> - FLOW_RULE_DESTROY
> - FLOW_RULE_UNKNOWN
> 
> And set 'FLOW_RULE_UNKNOWN' for all drivers, to simulate current status,
> until driver is updated.

Hi Ferruh,

Indirect actions are only implemented by mlx5 PMD,
the patches will be in RC2.
If we don't use the flag as per the latest suggestion,
nothing needs to be done for other PMDs.
Flag can as well be kept with the following semantics:
0 => indirect actions are flushed on device stop
1 => at least some indirect actions are kept,
     application should check types it's interested in

Introducing UNKNOWN state seems wrong to me.
What should an application do when it is reported?
Now there's just no way to learn how the PMD behaves,
but if it provides a response, it can't be "I don't know what I do".

Here's what I understood from the code, assuming there are no bugs
Like allowing to stop the port and keep dangling flow handles:

bnxt        flush
bonding     depends
cnxk        can't figure out
cxgbe       keep
dpaa2       keep
e1000       keep
enic        flush
failsafe    depends
hinic       flush
hns3        keep
i40e        keep
iavf        keep
ice         keep
igc         keep
ipn3ke      keep
ixgbe       keep
mlx4        keep
mlx5        flush
mvpp2       keep
octeontx2   can't figure out
qede        keep
sfc         flush
softnic     flush
tap         keep
txgbe       keep

Currently one flag would be sufficient to describe PMD behavior:
they either keep or flush the flow rules.
If there are indeed no exceptions, which maintainers should confirm,
I can add flag reporting myself.
  

> Introducing UNKNOWN state seems wrong to me.
> What should an application do when it is reported?
> Now there's just no way to learn how the PMD behaves, but if it provides a
> response, it can't be "I don't know what I do".
> 
> Here's what I understood from the code, assuming there are no bugs Like
> allowing to stop the port and keep dangling flow handles:
> 
> bnxt        flush
> [...]

N.B.: This part is about flow rules. For indirect actions it is clear.
  

On 10/14/21 5:14 PM, Dmitry Kozlyuk wrote:
> 
> 
>> -----Original Message-----
>> From: Dmitry Kozlyuk
>> Sent: 13 октября 2021 г. 11:33
>> To: dev@dpdk.org; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Ori
>> Kam <orika@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>
>> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> <ferruh.yigit@intel.com>
>> Subject: RE: [PATCH 2/5] ethdev: add capability to keep shared objects on
>> restart
>>
>> This thread continues discussions on previous versions to keep everything
>> in the thread with final patches:
>>
>> [1]: http://inbox.dpdk.org/dev/d5673b58-5aa6-ca35-5b60-
>> d938e56cfee1@oktetlabs.ru/
>> [2]:
>> http://inbox.dpdk.org/dev/DM8PR12MB5400997CCEC9169AC5AE0C89D6EA9@DM8PR12MB
>> 5400.namprd12.prod.outlook.com/
>>
>> Please see below.
>>
>>> -----Original Message-----
>>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>> Sent: 5 октября 2021 г. 3:52
>>> To: dev@dpdk.org
>>> Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>;
>>> NBU- Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>> <ferruh.yigit@intel.com>; Andrew Rybchenko
>>> <andrew.rybchenko@oktetlabs.ru>
>>> Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on
>>> restart
>>>
>>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>
>>> rte_flow_action_handle_create() did not mention what happens with an
>>> indirect action when a device is stopped, possibly reconfigured, and
>>> started again. It is natural for some indirect actions to be
>>> persistent, like counters and meters; keeping others just saves
>>> application time and complexity. However, not all PMDs can support it.
>>> It is proposed to add a device capability to indicate if indirect
>>> actions are kept across the above sequence or implicitly destroyed.
>>>
>>> In the future, indirect actions may not be the only type of objects
>>> shared between flow rules. The capability bit intends to cover all
>>> possible types of such objects, hence its name.
>>>
>>> It may happen that in the future a PMD acquires support for a type of
>>> shared objects that it cannot keep across a restart. It is undesirable
>>> to stop advertising the capability so that applications that don't use
>>> objects of the problematic type can still take advantage of it.
>>> This is why PMDs are allowed to keep only a subset of shared objects
>>> provided that the vendor mandatorily documents it.
>>>
>>> If the device is being reconfigured in a way that is incompatible with
>>> an existing shared objects, PMD is required to report an error.
>>> This is mandatory, because flow API does not supply users with
>>> capabilities, so this is the only way for a user to learn that
>>> configuration is invalid. For example, if queue count changes and RSS
>>> indirect action specifies queues that are going away, the user must
>>> update the action before removing the queues or remove the action and
>>> all flow rules that were using it.
>>>
>>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>> ---
>>> [...]
>>
>> Current pain point is that capability bits may be insufficient and a
>> programmatic way is desired to check which types of objects can be kept
>> across restart, instead of documenting the limitations.
>>
>> I support one of previous Ori's suggestions and want to clarify it [1]:
>>
>> Ori: "Another way is to assume that if the action was created before port
>> start it will be kept after port stop."
>> Andrew: "It does not sound like a solution. May be I simply don't know
>> target usecase."
>>
>> What Ori suggests (offline discussion summary): Suppose an application
>> wants to check whether a shared object (indirect action) or a flow rule of
>> a particular kind. It calls rte_flow_action_handle_create() or
>> rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it means
>> objects of this type can be kept across restart, 2) it's a normal object
>> created that will work after the port is started. This is logical, because
>> if the PMD can keep some kind of objects when the port is stopped, it is
>> likely to be able to create them when the port is not started. It is
>> subject to discussion if "object kind" means only "type" or "type +
>> transfer bit" combination; for mlx5 PMD it doesn't matter. One minor
>> drawback is that applications can only do the test when the port is
>> stopped, but it seems likely that the test really needs to be done at
>> startup anyway.
>>
>> If this is acceptable:
>> 1. Capability bits are not needed anymore.
>> 2. ethdev patches can be accepted in RC1, present behavior is undefined
>> anyway.
>> 3. PMD patches will need update that can be done by RC2.
> 
> Andrew, what do you think?
> If you agree, do we need to include transfer bit into "kind"?
> I'd like to conclude before RC1 and can update the docs quickly.
> 
> I've seen the proposition to advertise capability
> to create flow rules before device start as a flag.
> I don't think it conflicts with Ori's suggestion
> because the flag doesn't imply that _any_ rule can be created,
> neither does it say about indirect actions.
> On the other hand, if PMD can create a flow object (rule, etc.)
> when the device is not started, it is logical to assume that
> after the device is stopped it can move existing flow objects
> to the same state as when the device was not started, then restore
> when it is started again.
> 

Dmitry, thanks for the explanations. Ori's idea makes sense to
me now. The problem is to document it properly. We must define
rules to check it. Which bits in the check request matter and
how application should make a choice of rule to try. Which
status code should be returned by the PMD to clearly say that
addition in started state is not supported and, therefore,
preserving across restart is not supported. Must the device be
configured before an attempt to check it? Should transfer and
non-transfer rules/indirect actions be checked separately?

Andrew.
  

> [...]
> >>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >>>
> >>> rte_flow_action_handle_create() did not mention what happens with an
> >>> indirect action when a device is stopped, possibly reconfigured, and
> >>> started again. It is natural for some indirect actions to be
> >>> persistent, like counters and meters; keeping others just saves
> >>> application time and complexity. However, not all PMDs can support it.
> >>> It is proposed to add a device capability to indicate if indirect
> >>> actions are kept across the above sequence or implicitly destroyed.
> >>>
> >>> In the future, indirect actions may not be the only type of objects
> >>> shared between flow rules. The capability bit intends to cover all
> >>> possible types of such objects, hence its name.
> >>>
> >>> It may happen that in the future a PMD acquires support for a type
> >>> of shared objects that it cannot keep across a restart. It is
> >>> undesirable to stop advertising the capability so that applications
> >>> that don't use objects of the problematic type can still take
> advantage of it.
> >>> This is why PMDs are allowed to keep only a subset of shared objects
> >>> provided that the vendor mandatorily documents it.
> >>>
> >>> If the device is being reconfigured in a way that is incompatible
> >>> with an existing shared objects, PMD is required to report an error.
> >>> This is mandatory, because flow API does not supply users with
> >>> capabilities, so this is the only way for a user to learn that
> >>> configuration is invalid. For example, if queue count changes and
> >>> RSS indirect action specifies queues that are going away, the user
> >>> must update the action before removing the queues or remove the
> >>> action and all flow rules that were using it.
> >>>
> >>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> >>> ---
> >>> [...]
> >>
> >> Current pain point is that capability bits may be insufficient and a
> >> programmatic way is desired to check which types of objects can be
> >> kept across restart, instead of documenting the limitations.
> >>
> >> I support one of previous Ori's suggestions and want to clarify it [1]:
> >>
> >> Ori: "Another way is to assume that if the action was created before
> >> port start it will be kept after port stop."
> >> Andrew: "It does not sound like a solution. May be I simply don't
> >> know target usecase."
> >>
> >> What Ori suggests (offline discussion summary): Suppose an
> >> application wants to check whether a shared object (indirect action)
> >> or a flow rule of a particular kind. It calls
> >> rte_flow_action_handle_create() or
> >> rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it
> >> means objects of this type can be kept across restart, 2) it's a
> >> normal object created that will work after the port is started. This
> >> is logical, because if the PMD can keep some kind of objects when the
> >> port is stopped, it is likely to be able to create them when the port
> >> is not started. It is subject to discussion if "object kind" means
> >> only "type" or "type + transfer bit" combination; for mlx5 PMD it
> >> doesn't matter. One minor drawback is that applications can only do
> >> the test when the port is stopped, but it seems likely that the test
> >> really needs to be done at startup anyway.
> >>
> >> If this is acceptable:
> >> 1. Capability bits are not needed anymore.
> >> 2. ethdev patches can be accepted in RC1, present behavior is
> >> undefined anyway.
> >> 3. PMD patches will need update that can be done by RC2.
> >
> > Andrew, what do you think?
> > If you agree, do we need to include transfer bit into "kind"?
> > I'd like to conclude before RC1 and can update the docs quickly.
> >
> > I've seen the proposition to advertise capability to create flow rules
> > before device start as a flag.
> > I don't think it conflicts with Ori's suggestion because the flag
> > doesn't imply that _any_ rule can be created, neither does it say
> > about indirect actions.
> > On the other hand, if PMD can create a flow object (rule, etc.) when
> > the device is not started, it is logical to assume that after the
> > device is stopped it can move existing flow objects to the same state
> > as when the device was not started, then restore when it is started
> > again.
>
> Dmitry, thanks for the explanations. Ori's idea makes sense to me now. The
> problem is to document it properly. We must define rules to check it.
> Which bits in the check request matter and how application should make a
> choice of rule to try.

This is a generalization of the last question about the transfer bit.
I call the bits that matter a "kind". As I see it:

rule kind = seq. of item types + seq. of action types
indirect action kind = action type

As Ori mentioned, for mlx5 PMD transfer bit doesn't affect object persistence.
If you or other PMD maintainers think it may be relevant, no problem,
because PMDs like mlx5 will just ignore it when checking. Then it will be:

rule kind = seq. of item types + seq. of action types + attr. transfer bit
indirect action kind = action type + indirect action conf. transfer bit

Including the transfer bit seems to be a safe choice from DPDK point of view,
but obviously it can force applications to do up to twice as many checks.

The application needs to construct a valid flow configuration
that is (1) valid and (2) has the kind the application is interested in.
It is worth noting that these checks are not about resource consumption,
i.e. it is sufficient to test an indirect RSS with one queue
to be confident that indirect RSS with any number of queues are preserved.

> Which status code should be returned by the PMD to
> clearly say that addition in started state is not supported and,
> therefore, preserving across restart is not supported.

I suggest a new DPDK-specific value in rte_errno.h.

> Must the device be configured before an attempt to check it?

Yes, because flow objects created by these checks are as good as any others
and AFAIK no PMD supports rte_flow calls before configuration is done.

> Should transfer and non-transfer rules/indirect actions be checked separately?

See above.
  

On 10/15/21 12:04 PM, Dmitry Kozlyuk wrote:
>> [...]
>>>>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>>>
>>>>> rte_flow_action_handle_create() did not mention what happens with an
>>>>> indirect action when a device is stopped, possibly reconfigured, and
>>>>> started again. It is natural for some indirect actions to be
>>>>> persistent, like counters and meters; keeping others just saves
>>>>> application time and complexity. However, not all PMDs can support it.
>>>>> It is proposed to add a device capability to indicate if indirect
>>>>> actions are kept across the above sequence or implicitly destroyed.
>>>>>
>>>>> In the future, indirect actions may not be the only type of objects
>>>>> shared between flow rules. The capability bit intends to cover all
>>>>> possible types of such objects, hence its name.
>>>>>
>>>>> It may happen that in the future a PMD acquires support for a type
>>>>> of shared objects that it cannot keep across a restart. It is
>>>>> undesirable to stop advertising the capability so that applications
>>>>> that don't use objects of the problematic type can still take
>> advantage of it.
>>>>> This is why PMDs are allowed to keep only a subset of shared objects
>>>>> provided that the vendor mandatorily documents it.
>>>>>
>>>>> If the device is being reconfigured in a way that is incompatible
>>>>> with an existing shared objects, PMD is required to report an error.
>>>>> This is mandatory, because flow API does not supply users with
>>>>> capabilities, so this is the only way for a user to learn that
>>>>> configuration is invalid. For example, if queue count changes and
>>>>> RSS indirect action specifies queues that are going away, the user
>>>>> must update the action before removing the queues or remove the
>>>>> action and all flow rules that were using it.
>>>>>
>>>>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>>> ---
>>>>> [...]
>>>>
>>>> Current pain point is that capability bits may be insufficient and a
>>>> programmatic way is desired to check which types of objects can be
>>>> kept across restart, instead of documenting the limitations.
>>>>
>>>> I support one of previous Ori's suggestions and want to clarify it [1]:
>>>>
>>>> Ori: "Another way is to assume that if the action was created before
>>>> port start it will be kept after port stop."
>>>> Andrew: "It does not sound like a solution. May be I simply don't
>>>> know target usecase."
>>>>
>>>> What Ori suggests (offline discussion summary): Suppose an
>>>> application wants to check whether a shared object (indirect action)
>>>> or a flow rule of a particular kind. It calls
>>>> rte_flow_action_handle_create() or
>>>> rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it
>>>> means objects of this type can be kept across restart, 2) it's a
>>>> normal object created that will work after the port is started. This
>>>> is logical, because if the PMD can keep some kind of objects when the
>>>> port is stopped, it is likely to be able to create them when the port
>>>> is not started. It is subject to discussion if "object kind" means
>>>> only "type" or "type + transfer bit" combination; for mlx5 PMD it
>>>> doesn't matter. One minor drawback is that applications can only do
>>>> the test when the port is stopped, but it seems likely that the test
>>>> really needs to be done at startup anyway.
>>>>
>>>> If this is acceptable:
>>>> 1. Capability bits are not needed anymore.
>>>> 2. ethdev patches can be accepted in RC1, present behavior is
>>>> undefined anyway.
>>>> 3. PMD patches will need update that can be done by RC2.
>>>
>>> Andrew, what do you think?
>>> If you agree, do we need to include transfer bit into "kind"?
>>> I'd like to conclude before RC1 and can update the docs quickly.
>>>
>>> I've seen the proposition to advertise capability to create flow rules
>>> before device start as a flag.
>>> I don't think it conflicts with Ori's suggestion because the flag
>>> doesn't imply that _any_ rule can be created, neither does it say
>>> about indirect actions.
>>> On the other hand, if PMD can create a flow object (rule, etc.) when
>>> the device is not started, it is logical to assume that after the
>>> device is stopped it can move existing flow objects to the same state
>>> as when the device was not started, then restore when it is started
>>> again.
>>
>> Dmitry, thanks for the explanations. Ori's idea makes sense to me now. The
>> problem is to document it properly. We must define rules to check it.
>> Which bits in the check request matter and how application should make a
>> choice of rule to try.
> 
> This is a generalization of the last question about the transfer bit.
> I call the bits that matter a "kind". As I see it:
> 
> rule kind = seq. of item types + seq. of action types
> indirect action kind = action type
> 
> As Ori mentioned, for mlx5 PMD transfer bit doesn't affect object persistence.
> If you or other PMD maintainers think it may be relevant, no problem,
> because PMDs like mlx5 will just ignore it when checking. Then it will be:
> 
> rule kind = seq. of item types + seq. of action types + attr. transfer bit
> indirect action kind = action type + indirect action conf. transfer bit
> 
> Including the transfer bit seems to be a safe choice from DPDK point of view,
> but obviously it can force applications to do up to twice as many checks.
> 
> The application needs to construct a valid flow configuration
> that is (1) valid and (2) has the kind the application is interested in.
> It is worth noting that these checks are not about resource consumption,
> i.e. it is sufficient to test an indirect RSS with one queue
> to be confident that indirect RSS with any number of queues are preserved.
> 
>> Which status code should be returned by the PMD to
>> clearly say that addition in started state is not supported and,
>> therefore, preserving across restart is not supported.
> 
> I suggest a new DPDK-specific value in rte_errno.h.
> 
>> Must the device be configured before an attempt to check it?
> 
> Yes, because flow objects created by these checks are as good as any others
> and AFAIK no PMD supports rte_flow calls before configuration is done.
> 
>> Should transfer and non-transfer rules/indirect actions be checked separately?
> 
> See above.
> 

Please, try to put it into the patch for documentation
and I'll review the result. All my above questions
should be answered in the documentation.
  

On 10/14/2021 10:45 PM, Dmitry Kozlyuk wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: 14 октября 2021 г. 16:47
>> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
>> Darawsheh <rasland@nvidia.com>
>> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Qi Zhang
>> <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
>> <maxime.coquelin@redhat.com>
>> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects on
>> restart
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/13/2021 9:32 AM, Dmitry Kozlyuk wrote:
>>> This thread continues discussions on previous versions to keep
>>> everything in the thread with final patches:
>>>
>>> [1]:
>>> http://inbox.dpdk.org/dev/d5673b58-5aa6-ca35-5b60-d938e56cfee1@oktetla
>>> bs.ru/
>>> [2]:
>>> http://inbox.dpdk.org/dev/DM8PR12MB5400997CCEC9169AC5AE0C89D6EA9@DM8PR
>>> 12MB5400.namprd12.prod.outlook.com/
>>>
>>> Please see below.
>>>
>>>> -----Original Message-----
>>>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>> Sent: 5 октября 2021 г. 3:52
>>>> To: dev@dpdk.org
>>>> Cc: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ori Kam <orika@nvidia.com>;
>>>> NBU- Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>>> <ferruh.yigit@intel.com>; Andrew Rybchenko
>>>> <andrew.rybchenko@oktetlabs.ru>
>>>> Subject: [PATCH 2/5] ethdev: add capability to keep shared objects on
>>>> restart
>>>>
>>>> From: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>>
>>>> rte_flow_action_handle_create() did not mention what happens with an
>>>> indirect action when a device is stopped, possibly reconfigured, and
>>>> started again. It is natural for some indirect actions to be
>>>> persistent, like counters and meters; keeping others just saves
>>>> application time and complexity. However, not all PMDs can support it.
>>>> It is proposed to add a device capability to indicate if indirect
>>>> actions are kept across the above sequence or implicitly destroyed.
>>>>
>>>> In the future, indirect actions may not be the only type of objects
>>>> shared between flow rules. The capability bit intends to cover all
>>>> possible types of such objects, hence its name.
>>>>
>>>> It may happen that in the future a PMD acquires support for a type of
>>>> shared objects that it cannot keep across a restart. It is
>>>> undesirable to stop advertising the capability so that applications
>>>> that don't use objects of the problematic type can still take advantage
>> of it.
>>>> This is why PMDs are allowed to keep only a subset of shared objects
>>>> provided that the vendor mandatorily documents it.
>>>>
>>>> If the device is being reconfigured in a way that is incompatible
>>>> with an existing shared objects, PMD is required to report an error.
>>>> This is mandatory, because flow API does not supply users with
>>>> capabilities, so this is the only way for a user to learn that
>>>> configuration is invalid. For example, if queue count changes and RSS
>>>> indirect action specifies queues that are going away, the user must
>>>> update the action before removing the queues or remove the action and
>>>> all flow rules that were using it.
>>>>
>>>> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>> ---
>>>> [...]
>>>
>>> Current pain point is that capability bits may be insufficient and a
>>> programmatic way is desired to check which types of objects can be
>>> kept across restart, instead of documenting the limitations.
>>>
>>> I support one of previous Ori's suggestions and want to clarify it [1]:
>>>
>>> Ori: "Another way is to assume that if the action was created before
>> port start it will be kept after port stop."
>>> Andrew: "It does not sound like a solution. May be I simply don't know
>>> target usecase."
>>>
>>> What Ori suggests (offline discussion summary): Suppose an application
>> wants to check whether a shared object (indirect action) or a flow rule of
>> a particular kind. It calls rte_flow_action_handle_create() or
>> rte_flow_create() before rte_eth_dev_start(). If it succeeds, 1) it means
>> objects of this type can be kept across restart, 2) it's a normal object
>> created that will work after the port is started. This is logical, because
>> if the PMD can keep some kind of objects when the port is stopped, it is
>> likely to be able to create them when the port is not started. It is
>> subject to discussion if "object kind" means only "type" or "type +
>> transfer bit" combination; for mlx5 PMD it doesn't matter. One minor
>> drawback is that applications can only do the test when the port is
>> stopped, but it seems likely that the test really needs to be done at
>> startup anyway.
>>>
>>> If this is acceptable:
>>> 1. Capability bits are not needed anymore.
>>> 2. ethdev patches can be accepted in RC1, present behavior is undefined
>> anyway.
>>> 3. PMD patches will need update that can be done by RC2.
>>>
>>
>> Hi Dmitry,
>>
>> Are you planning to update drivers yourself on -rc2?
>> Or do you mean PMD maintainers should update themselves, if so do they
>> know about it?
>>
>> If the ethdev layer is updated in a way to impact the drivers, it should
>> be either:
>> - all drivers updated with a change
>> or
>> - give PMDs time to implement it on their own time, meanwhile they can
>> report their support status by a flag
>>
>> We had multiple sample of second case in the past but it is harder for
>> this case.
>>
>> For this case what about having three states:
>> - FLOW_RULE_KEEP
>> - FLOW_RULE_DESTROY
>> - FLOW_RULE_UNKNOWN
>>
>> And set 'FLOW_RULE_UNKNOWN' for all drivers, to simulate current status,
>> until driver is updated.
> 
> Hi Ferruh,
> 
> Indirect actions are only implemented by mlx5 PMD,
> the patches will be in RC2.
> If we don't use the flag as per the latest suggestion,
> nothing needs to be done for other PMDs.
> Flag can as well be kept with the following semantics:
> 0 => indirect actions are flushed on device stop
> 1 => at least some indirect actions are kept,
>       application should check types it's interested in
> 

My concerns is related to the 'flow rules', not indirect actions,
the patch mentions capability is for both of them.

> Introducing UNKNOWN state seems wrong to me.
> What should an application do when it is reported?
> Now there's just no way to learn how the PMD behaves,
> but if it provides a response, it can't be "I don't know what I do".
> 

I agree 'unknown' state is not ideal, but my intentions is prevent
drivers that not implemented this new feature report wrong capability.

Without capability, application already doesn't know how underlying
PMD behaves, so this is by default 'unknown' state.
I suggest keeping that state until driver explicitly updates its state
to the correct value.

But having below list is good, if you will update all drivers than
no need to have the 'unknown' state, but updating drivers may require
driver maintainers ack which can take some time.

Can you please clarify what is you plan according PMDs, will you update
them all, or will you only update mlx5 in -rc2?
And what is the exact plan for the -rc2 that you mention?

> Here's what I understood from the code, assuming there are no bugs
> Like allowing to stop the port and keep dangling flow handles:
> 
> bnxt        flush
> bonding     depends
> cnxk        can't figure out
> cxgbe       keep
> dpaa2       keep
> e1000       keep
> enic        flush
> failsafe    depends
> hinic       flush
> hns3        keep
> i40e        keep
> iavf        keep
> ice         keep
> igc         keep
> ipn3ke      keep
> ixgbe       keep
> mlx4        keep
> mlx5        flush
> mvpp2       keep
> octeontx2   can't figure out
> qede        keep
> sfc         flush
> softnic     flush
> tap         keep
> txgbe       keep
> 
> Currently one flag would be sufficient to describe PMD behavior:
> they either keep or flush the flow rules.
> If there are indeed no exceptions, which maintainers should confirm,
> I can add flag reporting myself.
>
  

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> [...]
> > Introducing UNKNOWN state seems wrong to me.
> > What should an application do when it is reported?
> > Now there's just no way to learn how the PMD behaves,
> > but if it provides a response, it can't be "I don't know what I do".
> >
> 
> I agree 'unknown' state is not ideal, but my intentions is prevent
> drivers that not implemented this new feature report wrong capability.
> 
> Without capability, application already doesn't know how underlying
> PMD behaves, so this is by default 'unknown' state.
> I suggest keeping that state until driver explicitly updates its state
> to the correct value.

My concern is that when all the drivers are changed to report a proper
capability, UNKNOWN remains in the API meaning "there's a bug in DPDK".

Instead of UNKNOWN response we can declare that rte_flow_flush()
must be called unless the application wants to keep the rules
and has made sure it's possible, or the behavior is undefined.
(Can be viewed as "UNKNOWN by default", but is simpler.)
This way neither UNKNOWN state is needed,
nor the bit saying the flow rules are flushed.
Here is why, let's consider KEEP and FLUSH combinations:

(1) FLUSH=0, KEEP=0 is equivalent to UNKNOWN, i.e. the application
                    must explicitly flush the rules itself
                    in order to get deterministic behavior.
(2) FLUSH=1, KEEP=0 means PMD flushes all rules on the device stop.
(3) FLUSH=0, KEEP=1 means PMD can keep at least some rules,
                    exact support must be checked with rte_flow_create()
                    when the device is stopped.
(4) FLUSH=1, KEEP=1 is forbidden.

If the application doesn't need the PMD to keep flow rules,
it can as well flush them always before the device stop
regardless of whether the driver does it automatically or not.
It's even simpler and probably as efficient. Testpmd does this.
If the application wants to take advantage of rule-keeping ability,
it just tests the KEEP bit. If it is unset that's the previous case,
application should call rte_flow_flush() before the device stop to be sure.
Otherwise, the application can test capability to keep flow rule kinds
it is interested in (see my reply to Andrew).

Result: no changes to PMDs are _immediately_ needed when such behavior
is documented. They can start advertising it whenever they like,
it's not even an RC2 task. Currently applications that relied on certain
behavior are non-portable anyway.

> But having below list is good, if you will update all drivers than
> no need to have the 'unknown' state, but updating drivers may require
> driver maintainers ack which can take some time.

If you agree with what I suggest above, there will be no urgency.
The list can be used to notify maintainers that they can enhance
their PMD user experience whenever they like.

> Can you please clarify what is you plan according PMDs, will you update
> them all, or will you only update mlx5 in -rc2?
> And what is the exact plan for the -rc2 that you mention?

mlx5 PMD will be updated with the patches from this series.
Regarding indirect actions: no other PMD needs an update.
Regarding flow rules: if the above suggestion is accepted,
no PMDs need to be updated urgently.
  

On 10/15/2021 1:35 PM, Dmitry Kozlyuk wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> [...]
>>> Introducing UNKNOWN state seems wrong to me.
>>> What should an application do when it is reported?
>>> Now there's just no way to learn how the PMD behaves,
>>> but if it provides a response, it can't be "I don't know what I do".
>>>
>>
>> I agree 'unknown' state is not ideal, but my intentions is prevent
>> drivers that not implemented this new feature report wrong capability.
>>
>> Without capability, application already doesn't know how underlying
>> PMD behaves, so this is by default 'unknown' state.
>> I suggest keeping that state until driver explicitly updates its state
>> to the correct value.
> 
> My concern is that when all the drivers are changed to report a proper
> capability, UNKNOWN remains in the API meaning "there's a bug in DPDK".
> 

When all drivers are changed, of course we can remove the 'unknown' flag.

> Instead of UNKNOWN response we can declare that rte_flow_flush()
> must be called unless the application wants to keep the rules
> and has made sure it's possible, or the behavior is undefined.
> (Can be viewed as "UNKNOWN by default", but is simpler.)
> This way neither UNKNOWN state is needed,
> nor the bit saying the flow rules are flushed.
> Here is why, let's consider KEEP and FLUSH combinations:
> 
> (1) FLUSH=0, KEEP=0 is equivalent to UNKNOWN, i.e. the application
>                      must explicitly flush the rules itself
>                      in order to get deterministic behavior.
> (2) FLUSH=1, KEEP=0 means PMD flushes all rules on the device stop.
> (3) FLUSH=0, KEEP=1 means PMD can keep at least some rules,
>                      exact support must be checked with rte_flow_create()
>                      when the device is stopped.
> (4) FLUSH=1, KEEP=1 is forbidden.
> 

What is 'FLUSH' here? Are you proposing a new capability?

> If the application doesn't need the PMD to keep flow rules,
> it can as well flush them always before the device stop
> regardless of whether the driver does it automatically or not.
> It's even simpler and probably as efficient. Testpmd does this.
> If the application wants to take advantage of rule-keeping ability,
> it just tests the KEEP bit. If it is unset that's the previous case,
> application should call rte_flow_flush() before the device stop to be sure.
> Otherwise, the application can test capability to keep flow rule kinds
> it is interested in (see my reply to Andrew).
> 

Overall this is an optimization, application can workaround without this
capability.

If driver doesn't set KEEP capability, it is not clear what does it
mean, driver doesn't keep rules or driver is not updated yet.
I suggest to update comment to clarify the meaning of the missing KEEP
flag.

And unless we have two explicit status flags application can never be
sure that driver doesn't keep rules after stop. I am don't know if
application wants to know this.

Other concern is how PMD maintainers will know that there is something
to update here, I am sure many driver maintainers won't even be aware of
this, your patch doesn't even cc them. Your approach feels like you are
thinking only single PMD and ignore rest.

My intention was to have a way to follow drivers that is not updated,
by marking them with UNKNOWN flag. But this also doesn't work with new
drivers, they may forget setting capability.


What about following:
1) Clarify KEEP flag meaning:
having KEEP: flow rules are kept after stop
missing KEEP: unknown behavior

2) Mark all PMDs with useless flag:
dev_capa &= ~KEEP
Maintainer can remove or update this later, and we can easily track it.



> Result: no changes to PMDs are _immediately_ needed when such behavior
> is documented. They can start advertising it whenever they like,
> it's not even an RC2 task. Currently applications that relied on certain
> behavior are non-portable anyway.
> 
>> But having below list is good, if you will update all drivers than
>> no need to have the 'unknown' state, but updating drivers may require
>> driver maintainers ack which can take some time.
> 
> If you agree with what I suggest above, there will be no urgency.
> The list can be used to notify maintainers that they can enhance
> their PMD user experience whenever they like.
> 
>> Can you please clarify what is you plan according PMDs, will you update
>> them all, or will you only update mlx5 in -rc2?
>> And what is the exact plan for the -rc2 that you mention?
> 
> mlx5 PMD will be updated with the patches from this series.
> Regarding indirect actions: no other PMD needs an update.
> Regarding flow rules: if the above suggestion is accepted,
> no PMDs need to be updated urgently.
>
  

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: 15 октября 2021 г. 19:27
> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
> Darawsheh <rasland@nvidia.com>
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Qi Zhang
> <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects on
> restart
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/15/2021 1:35 PM, Dmitry Kozlyuk wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> [...]
> >>> Introducing UNKNOWN state seems wrong to me.
> >>> What should an application do when it is reported?
> >>> Now there's just no way to learn how the PMD behaves,
> >>> but if it provides a response, it can't be "I don't know what I do".
> >>>
> >>
> >> I agree 'unknown' state is not ideal, but my intentions is prevent
> >> drivers that not implemented this new feature report wrong capability.
> >>
> >> Without capability, application already doesn't know how underlying
> >> PMD behaves, so this is by default 'unknown' state.
> >> I suggest keeping that state until driver explicitly updates its state
> >> to the correct value.
> >
> > My concern is that when all the drivers are changed to report a proper
> > capability, UNKNOWN remains in the API meaning "there's a bug in DPDK".
> >
> 
> When all drivers are changed, of course we can remove the 'unknown' flag.
> 
> > Instead of UNKNOWN response we can declare that rte_flow_flush()
> > must be called unless the application wants to keep the rules
> > and has made sure it's possible, or the behavior is undefined.
> > (Can be viewed as "UNKNOWN by default", but is simpler.)
> > This way neither UNKNOWN state is needed,
> > nor the bit saying the flow rules are flushed.
> > Here is why, let's consider KEEP and FLUSH combinations:
> >
> > (1) FLUSH=0, KEEP=0 is equivalent to UNKNOWN, i.e. the application
> >                      must explicitly flush the rules itself
> >                      in order to get deterministic behavior.
> > (2) FLUSH=1, KEEP=0 means PMD flushes all rules on the device stop.
> > (3) FLUSH=0, KEEP=1 means PMD can keep at least some rules,
> >                      exact support must be checked with
> rte_flow_create()
> >                      when the device is stopped.
> > (4) FLUSH=1, KEEP=1 is forbidden.
> >
> 
> What is 'FLUSH' here? Are you proposing a new capability?
> 
> > If the application doesn't need the PMD to keep flow rules,
> > it can as well flush them always before the device stop
> > regardless of whether the driver does it automatically or not.
> > It's even simpler and probably as efficient. Testpmd does this.
> > If the application wants to take advantage of rule-keeping ability,
> > it just tests the KEEP bit. If it is unset that's the previous case,
> > application should call rte_flow_flush() before the device stop to be
> sure.
> > Otherwise, the application can test capability to keep flow rule kinds
> > it is interested in (see my reply to Andrew).
> >
> 
> Overall this is an optimization, application can workaround without this
> capability.
> 
> If driver doesn't set KEEP capability, it is not clear what does it
> mean, driver doesn't keep rules or driver is not updated yet.
> I suggest to update comment to clarify the meaning of the missing KEEP
> flag.
> 
> And unless we have two explicit status flags application can never be
> sure that driver doesn't keep rules after stop. I am don't know if
> application wants to know this.
> 
> Other concern is how PMD maintainers will know that there is something
> to update here, I am sure many driver maintainers won't even be aware of
> this, your patch doesn't even cc them. Your approach feels like you are
> thinking only single PMD and ignore rest.
> 
> My intention was to have a way to follow drivers that is not updated,
> by marking them with UNKNOWN flag. But this also doesn't work with new
> drivers, they may forget setting capability.
> 
> 
> What about following:
> 1) Clarify KEEP flag meaning:
> having KEEP: flow rules are kept after stop
> missing KEEP: unknown behavior
> 
> 2) Mark all PMDs with useless flag:
> dev_capa &= ~KEEP
> Maintainer can remove or update this later, and we can easily track it.

Item 1) is almost what I did in v2. The difference (or clarification) is that
if the bit is set, it doesn't mean that all rules are kept.
It allows the PMD to not support keeping some kinds of rules.
Please see the doc update about how the kind is defined
and how the application can test what is unsupported.

This complication is needed so that if a PMD cannot keep some exotic kind of rules,
it is not forced to remove the capability completely,
blocking optimizations even if the application doesn't use problematic rule kinds.
It makes the capability future-proof.

The second flag (FLUSH) would not be of much help.
Consider it is not set, but the PMD can keep some kinds of rules.
The application still needs to test all the kinds it needs.
But it needs to do the same if the KEEP bit is set.
Only if it is set the application can skip the tests and rte_flow_flush(),
but these optimizations are small compared to keeping the rules itself.

Item 2) needs not to be done, because the absence of the bit is *the* useless value:
it means the unspecified same behavior as it is presently.
It is worth noting that currently any application that relies on the PMD
to keep or flush the rules is non-portable, because PMD is allowed to do anything.
To get a reliable behavior application must explicitly clear the rules.

Regarding you concern about maintainers forgetting to update PMDs,
I think there are better task-tracking tools then constants in the code
(the authors of golang's context.TODO may disagree :)
  

On 10/16/2021 9:32 PM, Dmitry Kozlyuk wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: 15 октября 2021 г. 19:27
>> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
>> Darawsheh <rasland@nvidia.com>
>> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Qi Zhang
>> <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
>> <maxime.coquelin@redhat.com>
>> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects on
>> restart
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/15/2021 1:35 PM, Dmitry Kozlyuk wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> [...]
>>>>> Introducing UNKNOWN state seems wrong to me.
>>>>> What should an application do when it is reported?
>>>>> Now there's just no way to learn how the PMD behaves,
>>>>> but if it provides a response, it can't be "I don't know what I do".
>>>>>
>>>>
>>>> I agree 'unknown' state is not ideal, but my intentions is prevent
>>>> drivers that not implemented this new feature report wrong capability.
>>>>
>>>> Without capability, application already doesn't know how underlying
>>>> PMD behaves, so this is by default 'unknown' state.
>>>> I suggest keeping that state until driver explicitly updates its state
>>>> to the correct value.
>>>
>>> My concern is that when all the drivers are changed to report a proper
>>> capability, UNKNOWN remains in the API meaning "there's a bug in DPDK".
>>>
>>
>> When all drivers are changed, of course we can remove the 'unknown' flag.
>>
>>> Instead of UNKNOWN response we can declare that rte_flow_flush()
>>> must be called unless the application wants to keep the rules
>>> and has made sure it's possible, or the behavior is undefined.
>>> (Can be viewed as "UNKNOWN by default", but is simpler.)
>>> This way neither UNKNOWN state is needed,
>>> nor the bit saying the flow rules are flushed.
>>> Here is why, let's consider KEEP and FLUSH combinations:
>>>
>>> (1) FLUSH=0, KEEP=0 is equivalent to UNKNOWN, i.e. the application
>>>                       must explicitly flush the rules itself
>>>                       in order to get deterministic behavior.
>>> (2) FLUSH=1, KEEP=0 means PMD flushes all rules on the device stop.
>>> (3) FLUSH=0, KEEP=1 means PMD can keep at least some rules,
>>>                       exact support must be checked with
>> rte_flow_create()
>>>                       when the device is stopped.
>>> (4) FLUSH=1, KEEP=1 is forbidden.
>>>
>>
>> What is 'FLUSH' here? Are you proposing a new capability?
>>
>>> If the application doesn't need the PMD to keep flow rules,
>>> it can as well flush them always before the device stop
>>> regardless of whether the driver does it automatically or not.
>>> It's even simpler and probably as efficient. Testpmd does this.
>>> If the application wants to take advantage of rule-keeping ability,
>>> it just tests the KEEP bit. If it is unset that's the previous case,
>>> application should call rte_flow_flush() before the device stop to be
>> sure.
>>> Otherwise, the application can test capability to keep flow rule kinds
>>> it is interested in (see my reply to Andrew).
>>>
>>
>> Overall this is an optimization, application can workaround without this
>> capability.
>>
>> If driver doesn't set KEEP capability, it is not clear what does it
>> mean, driver doesn't keep rules or driver is not updated yet.
>> I suggest to update comment to clarify the meaning of the missing KEEP
>> flag.
>>
>> And unless we have two explicit status flags application can never be
>> sure that driver doesn't keep rules after stop. I am don't know if
>> application wants to know this.
>>
>> Other concern is how PMD maintainers will know that there is something
>> to update here, I am sure many driver maintainers won't even be aware of
>> this, your patch doesn't even cc them. Your approach feels like you are
>> thinking only single PMD and ignore rest.
>>
>> My intention was to have a way to follow drivers that is not updated,
>> by marking them with UNKNOWN flag. But this also doesn't work with new
>> drivers, they may forget setting capability.
>>
>>
>> What about following:
>> 1) Clarify KEEP flag meaning:
>> having KEEP: flow rules are kept after stop
>> missing KEEP: unknown behavior
>>
>> 2) Mark all PMDs with useless flag:
>> dev_capa &= ~KEEP
>> Maintainer can remove or update this later, and we can easily track it.
> 
> Item 1) is almost what I did in v2. The difference (or clarification) is that
> if the bit is set, it doesn't mean that all rules are kept.
> It allows the PMD to not support keeping some kinds of rules.
> Please see the doc update about how the kind is defined
> and how the application can test what is unsupported.
> 
> This complication is needed so that if a PMD cannot keep some exotic kind of rules,
> it is not forced to remove the capability completely,
> blocking optimizations even if the application doesn't use problematic rule kinds.
> It makes the capability future-proof.
> 
> The second flag (FLUSH) would not be of much help.
> Consider it is not set, but the PMD can keep some kinds of rules.
> The application still needs to test all the kinds it needs.
> But it needs to do the same if the KEEP bit is set.
> Only if it is set the application can skip the tests and rte_flow_flush(),
> but these optimizations are small compared to keeping the rules itself.
> 
> Item 2) needs not to be done, because the absence of the bit is *the* useless value:
> it means the unspecified same behavior as it is presently.
> It is worth noting that currently any application that relies on the PMD
> to keep or flush the rules is non-portable, because PMD is allowed to do anything.
> To get a reliable behavior application must explicitly clear the rules.
> 
> Regarding you concern about maintainers forgetting to update PMDs,
> I think there are better task-tracking tools then constants in the code
> (the authors of golang's context.TODO may disagree :)
> 

Hi Dmitry,

This is a valid concern, and adding code to the drivers proved that it works.

There are multiple authors updating the ethdev layer and expecting PMD
maintainers will do required changes. For your case you are updating the PMD
you concern but how other PMD maintainers will even be aware that there is
something to change in their PMD?
By your change you are putting some responsibility to other maintainers,
without even cc'ing them. And it is for sure they are not reading all emails
in the mail list, they can't.

Task-tracking is an option, it the past I tried to upstream some todo doc
for PMDs. But I can see the additional maintenance cost to trace features
from a central point, comparing the distributing it to PMDS (adding code
to PMDs).

I think best method is whoever doing the ethdev layer do the relevant change
in the PMDs, but has the obvious problem that not able to know enough about
the PMDs to update them.

We have used the following option, and it worked in the past:
- When an ethdev feature require change in PMDs, ehtdev supports both new
   and old method
- PMDs set a flag by default to request old method, so there is no update
   in the PMD default behavior
- When PMD does the required changes, it removes the flag
- This lets me (or other maintainer), to trace the update status and ping
   relevant maintainers
- When all PMDs updated, ethdev support for old method removed
- This method allows PMD maintainers do the change on their own time
  

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: 18 октября 2021 г. 11:42
> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
> Darawsheh <rasland@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>
> Cc: Qi Zhang <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects on
> restart
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/16/2021 9:32 PM, Dmitry Kozlyuk wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: 15 октября 2021 г. 19:27
> >> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew
> Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
> >> Darawsheh <rasland@nvidia.com>
> >> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Qi Zhang
> >> <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
> >> <maxime.coquelin@redhat.com>
> >> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects
> on
> >> restart
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> On 10/15/2021 1:35 PM, Dmitry Kozlyuk wrote:
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> [...]
> >>>>> Introducing UNKNOWN state seems wrong to me.
> >>>>> What should an application do when it is reported?
> >>>>> Now there's just no way to learn how the PMD behaves,
> >>>>> but if it provides a response, it can't be "I don't know what I do".
> >>>>>
> >>>>
> >>>> I agree 'unknown' state is not ideal, but my intentions is prevent
> >>>> drivers that not implemented this new feature report wrong
> capability.
> >>>>
> >>>> Without capability, application already doesn't know how underlying
> >>>> PMD behaves, so this is by default 'unknown' state.
> >>>> I suggest keeping that state until driver explicitly updates its
> state
> >>>> to the correct value.
> >>>
> >>> My concern is that when all the drivers are changed to report a proper
> >>> capability, UNKNOWN remains in the API meaning "there's a bug in
> DPDK".
> >>>
> >>
> >> When all drivers are changed, of course we can remove the 'unknown'
> flag.
> >>
> >>> Instead of UNKNOWN response we can declare that rte_flow_flush()
> >>> must be called unless the application wants to keep the rules
> >>> and has made sure it's possible, or the behavior is undefined.
> >>> (Can be viewed as "UNKNOWN by default", but is simpler.)
> >>> This way neither UNKNOWN state is needed,
> >>> nor the bit saying the flow rules are flushed.
> >>> Here is why, let's consider KEEP and FLUSH combinations:
> >>>
> >>> (1) FLUSH=0, KEEP=0 is equivalent to UNKNOWN, i.e. the application
> >>>                       must explicitly flush the rules itself
> >>>                       in order to get deterministic behavior.
> >>> (2) FLUSH=1, KEEP=0 means PMD flushes all rules on the device stop.
> >>> (3) FLUSH=0, KEEP=1 means PMD can keep at least some rules,
> >>>                       exact support must be checked with
> >> rte_flow_create()
> >>>                       when the device is stopped.
> >>> (4) FLUSH=1, KEEP=1 is forbidden.
> >>>
> >>
> >> What is 'FLUSH' here? Are you proposing a new capability?
> >>
> >>> If the application doesn't need the PMD to keep flow rules,
> >>> it can as well flush them always before the device stop
> >>> regardless of whether the driver does it automatically or not.
> >>> It's even simpler and probably as efficient. Testpmd does this.
> >>> If the application wants to take advantage of rule-keeping ability,
> >>> it just tests the KEEP bit. If it is unset that's the previous case,
> >>> application should call rte_flow_flush() before the device stop to be
> >> sure.
> >>> Otherwise, the application can test capability to keep flow rule kinds
> >>> it is interested in (see my reply to Andrew).
> >>>
> >>
> >> Overall this is an optimization, application can workaround without
> this
> >> capability.
> >>
> >> If driver doesn't set KEEP capability, it is not clear what does it
> >> mean, driver doesn't keep rules or driver is not updated yet.
> >> I suggest to update comment to clarify the meaning of the missing KEEP
> >> flag.
> >>
> >> And unless we have two explicit status flags application can never be
> >> sure that driver doesn't keep rules after stop. I am don't know if
> >> application wants to know this.
> >>
> >> Other concern is how PMD maintainers will know that there is something
> >> to update here, I am sure many driver maintainers won't even be aware
> of
> >> this, your patch doesn't even cc them. Your approach feels like you are
> >> thinking only single PMD and ignore rest.
> >>
> >> My intention was to have a way to follow drivers that is not updated,
> >> by marking them with UNKNOWN flag. But this also doesn't work with new
> >> drivers, they may forget setting capability.
> >>
> >>
> >> What about following:
> >> 1) Clarify KEEP flag meaning:
> >> having KEEP: flow rules are kept after stop
> >> missing KEEP: unknown behavior
> >>
> >> 2) Mark all PMDs with useless flag:
> >> dev_capa &= ~KEEP
> >> Maintainer can remove or update this later, and we can easily track it.
> >
> > Item 1) is almost what I did in v2. The difference (or clarification) is
> that
> > if the bit is set, it doesn't mean that all rules are kept.
> > It allows the PMD to not support keeping some kinds of rules.
> > Please see the doc update about how the kind is defined
> > and how the application can test what is unsupported.
> >
> > This complication is needed so that if a PMD cannot keep some exotic
> kind of rules,
> > it is not forced to remove the capability completely,
> > blocking optimizations even if the application doesn't use problematic
> rule kinds.
> > It makes the capability future-proof.
> >
> > The second flag (FLUSH) would not be of much help.
> > Consider it is not set, but the PMD can keep some kinds of rules.
> > The application still needs to test all the kinds it needs.
> > But it needs to do the same if the KEEP bit is set.
> > Only if it is set the application can skip the tests and
> rte_flow_flush(),
> > but these optimizations are small compared to keeping the rules itself.
> >
> > Item 2) needs not to be done, because the absence of the bit is *the*
> useless value:
> > it means the unspecified same behavior as it is presently.
> > It is worth noting that currently any application that relies on the PMD
> > to keep or flush the rules is non-portable, because PMD is allowed to do
> anything.
> > To get a reliable behavior application must explicitly clear the rules.
> >
> > Regarding you concern about maintainers forgetting to update PMDs,
> > I think there are better task-tracking tools then constants in the code
> > (the authors of golang's context.TODO may disagree :)
> >
> 
> Hi Dmitry,
> 
> This is a valid concern, and adding code to the drivers proved that it
> works.
> 
> There are multiple authors updating the ethdev layer and expecting PMD
> maintainers will do required changes. For your case you are updating the
> PMD
> you concern but how other PMD maintainers will even be aware that there is
> something to change in their PMD?
> By your change you are putting some responsibility to other maintainers,
> without even cc'ing them. And it is for sure they are not reading all
> emails
> in the mail list, they can't.
> 
> Task-tracking is an option, it the past I tried to upstream some todo doc
> for PMDs. But I can see the additional maintenance cost to trace features
> from a central point, comparing the distributing it to PMDS (adding code
> to PMDs).
> 
> I think best method is whoever doing the ethdev layer do the relevant
> change
> in the PMDs, but has the obvious problem that not able to know enough
> about
> the PMDs to update them.
> 
> We have used the following option, and it worked in the past:
> - When an ethdev feature require change in PMDs, ehtdev supports both new
>    and old method
> - PMDs set a flag by default to request old method, so there is no update
>    in the PMD default behavior
> - When PMD does the required changes, it removes the flag
> - This lets me (or other maintainer), to trace the update status and ping
>    relevant maintainers
> - When all PMDs updated, ethdev support for old method removed
> - This method allows PMD maintainers do the change on their own time

Hi Ferruh,

Thanks for sharing the experience.
You suggest updating PMDs with an explicit reset of this bit,
despite that it will be zero anyway, to attract maintainers' attention.
From user's perspective it will be all the same: KEEP bit reset,
not a special value saying the PMD is not updated
that we would need to deprecate and remove later.
If this understanding is correct, then for sure I can add a patch
updating the relevant PMDs.
  

On 10/18/2021 12:13 PM, Dmitry Kozlyuk wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: 18 октября 2021 г. 11:42
>> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
>> Darawsheh <rasland@nvidia.com>; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>
>> Cc: Qi Zhang <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
>> <maxime.coquelin@redhat.com>
>> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects on
>> restart
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 10/16/2021 9:32 PM, Dmitry Kozlyuk wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: 15 октября 2021 г. 19:27
>>>> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org; Andrew
>> Rybchenko
>>>> <andrew.rybchenko@oktetlabs.ru>; Ori Kam <orika@nvidia.com>; Raslan
>>>> Darawsheh <rasland@nvidia.com>
>>>> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Qi Zhang
>>>> <qi.z.zhang@intel.com>; jerinj@marvell.com; Maxime Coquelin
>>>> <maxime.coquelin@redhat.com>
>>>> Subject: Re: [PATCH 2/5] ethdev: add capability to keep shared objects
>> on
>>>> restart
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 10/15/2021 1:35 PM, Dmitry Kozlyuk wrote:
>>>>>> -----Original Message-----
>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>> [...]
>>>>>>> Introducing UNKNOWN state seems wrong to me.
>>>>>>> What should an application do when it is reported?
>>>>>>> Now there's just no way to learn how the PMD behaves,
>>>>>>> but if it provides a response, it can't be "I don't know what I do".
>>>>>>>
>>>>>>
>>>>>> I agree 'unknown' state is not ideal, but my intentions is prevent
>>>>>> drivers that not implemented this new feature report wrong
>> capability.
>>>>>>
>>>>>> Without capability, application already doesn't know how underlying
>>>>>> PMD behaves, so this is by default 'unknown' state.
>>>>>> I suggest keeping that state until driver explicitly updates its
>> state
>>>>>> to the correct value.
>>>>>
>>>>> My concern is that when all the drivers are changed to report a proper
>>>>> capability, UNKNOWN remains in the API meaning "there's a bug in
>> DPDK".
>>>>>
>>>>
>>>> When all drivers are changed, of course we can remove the 'unknown'
>> flag.
>>>>
>>>>> Instead of UNKNOWN response we can declare that rte_flow_flush()
>>>>> must be called unless the application wants to keep the rules
>>>>> and has made sure it's possible, or the behavior is undefined.
>>>>> (Can be viewed as "UNKNOWN by default", but is simpler.)
>>>>> This way neither UNKNOWN state is needed,
>>>>> nor the bit saying the flow rules are flushed.
>>>>> Here is why, let's consider KEEP and FLUSH combinations:
>>>>>
>>>>> (1) FLUSH=0, KEEP=0 is equivalent to UNKNOWN, i.e. the application
>>>>>                        must explicitly flush the rules itself
>>>>>                        in order to get deterministic behavior.
>>>>> (2) FLUSH=1, KEEP=0 means PMD flushes all rules on the device stop.
>>>>> (3) FLUSH=0, KEEP=1 means PMD can keep at least some rules,
>>>>>                        exact support must be checked with
>>>> rte_flow_create()
>>>>>                        when the device is stopped.
>>>>> (4) FLUSH=1, KEEP=1 is forbidden.
>>>>>
>>>>
>>>> What is 'FLUSH' here? Are you proposing a new capability?
>>>>
>>>>> If the application doesn't need the PMD to keep flow rules,
>>>>> it can as well flush them always before the device stop
>>>>> regardless of whether the driver does it automatically or not.
>>>>> It's even simpler and probably as efficient. Testpmd does this.
>>>>> If the application wants to take advantage of rule-keeping ability,
>>>>> it just tests the KEEP bit. If it is unset that's the previous case,
>>>>> application should call rte_flow_flush() before the device stop to be
>>>> sure.
>>>>> Otherwise, the application can test capability to keep flow rule kinds
>>>>> it is interested in (see my reply to Andrew).
>>>>>
>>>>
>>>> Overall this is an optimization, application can workaround without
>> this
>>>> capability.
>>>>
>>>> If driver doesn't set KEEP capability, it is not clear what does it
>>>> mean, driver doesn't keep rules or driver is not updated yet.
>>>> I suggest to update comment to clarify the meaning of the missing KEEP
>>>> flag.
>>>>
>>>> And unless we have two explicit status flags application can never be
>>>> sure that driver doesn't keep rules after stop. I am don't know if
>>>> application wants to know this.
>>>>
>>>> Other concern is how PMD maintainers will know that there is something
>>>> to update here, I am sure many driver maintainers won't even be aware
>> of
>>>> this, your patch doesn't even cc them. Your approach feels like you are
>>>> thinking only single PMD and ignore rest.
>>>>
>>>> My intention was to have a way to follow drivers that is not updated,
>>>> by marking them with UNKNOWN flag. But this also doesn't work with new
>>>> drivers, they may forget setting capability.
>>>>
>>>>
>>>> What about following:
>>>> 1) Clarify KEEP flag meaning:
>>>> having KEEP: flow rules are kept after stop
>>>> missing KEEP: unknown behavior
>>>>
>>>> 2) Mark all PMDs with useless flag:
>>>> dev_capa &= ~KEEP
>>>> Maintainer can remove or update this later, and we can easily track it.
>>>
>>> Item 1) is almost what I did in v2. The difference (or clarification) is
>> that
>>> if the bit is set, it doesn't mean that all rules are kept.
>>> It allows the PMD to not support keeping some kinds of rules.
>>> Please see the doc update about how the kind is defined
>>> and how the application can test what is unsupported.
>>>
>>> This complication is needed so that if a PMD cannot keep some exotic
>> kind of rules,
>>> it is not forced to remove the capability completely,
>>> blocking optimizations even if the application doesn't use problematic
>> rule kinds.
>>> It makes the capability future-proof.
>>>
>>> The second flag (FLUSH) would not be of much help.
>>> Consider it is not set, but the PMD can keep some kinds of rules.
>>> The application still needs to test all the kinds it needs.
>>> But it needs to do the same if the KEEP bit is set.
>>> Only if it is set the application can skip the tests and
>> rte_flow_flush(),
>>> but these optimizations are small compared to keeping the rules itself.
>>>
>>> Item 2) needs not to be done, because the absence of the bit is *the*
>> useless value:
>>> it means the unspecified same behavior as it is presently.
>>> It is worth noting that currently any application that relies on the PMD
>>> to keep or flush the rules is non-portable, because PMD is allowed to do
>> anything.
>>> To get a reliable behavior application must explicitly clear the rules.
>>>
>>> Regarding you concern about maintainers forgetting to update PMDs,
>>> I think there are better task-tracking tools then constants in the code
>>> (the authors of golang's context.TODO may disagree :)
>>>
>>
>> Hi Dmitry,
>>
>> This is a valid concern, and adding code to the drivers proved that it
>> works.
>>
>> There are multiple authors updating the ethdev layer and expecting PMD
>> maintainers will do required changes. For your case you are updating the
>> PMD
>> you concern but how other PMD maintainers will even be aware that there is
>> something to change in their PMD?
>> By your change you are putting some responsibility to other maintainers,
>> without even cc'ing them. And it is for sure they are not reading all
>> emails
>> in the mail list, they can't.
>>
>> Task-tracking is an option, it the past I tried to upstream some todo doc
>> for PMDs. But I can see the additional maintenance cost to trace features
>> from a central point, comparing the distributing it to PMDS (adding code
>> to PMDs).
>>
>> I think best method is whoever doing the ethdev layer do the relevant
>> change
>> in the PMDs, but has the obvious problem that not able to know enough
>> about
>> the PMDs to update them.
>>
>> We have used the following option, and it worked in the past:
>> - When an ethdev feature require change in PMDs, ehtdev supports both new
>>     and old method
>> - PMDs set a flag by default to request old method, so there is no update
>>     in the PMD default behavior
>> - When PMD does the required changes, it removes the flag
>> - This lets me (or other maintainer), to trace the update status and ping
>>     relevant maintainers
>> - When all PMDs updated, ethdev support for old method removed
>> - This method allows PMD maintainers do the change on their own time
> 
> Hi Ferruh,
> 
> Thanks for sharing the experience.
> You suggest updating PMDs with an explicit reset of this bit,
> despite that it will be zero anyway, to attract maintainers' attention.

ack, but please with a brief comment to clarify intention.

>  From user's perspective it will be all the same: KEEP bit reset,
> not a special value saying the PMD is not updated
> that we would need to deprecate and remove later.

ack, only it needs to be clear for application that PMD not advertising
KEEP flag means behavior is undefined, it does NOT mean PMD flush rules.
Which you already said updated like this in v2, but I am just stressing it.

> If this understanding is correct, then for sure I can add a patch
> updating the relevant PMDs.
>