Re: [dpdk-dev] [PATCH 2/5] ethdev: add capability to keep shared objects on restart

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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.