Re: [dpdk-dev] [PATCH v3 1/2] ethdev: introduce transfer attribute to shared action conf

[Ivan Malov <Ivan.Malov@oktetlabs.ru>]

Hi Ferruh,

On 02/11/2020 21:54, Ferruh Yigit wrote:
> On 11/2/2020 11:43 AM, Ivan Malov wrote:
>> In a flow rule, attribute "transfer" means operation level
>> at which both traffic is matched and actions are conducted.
>>
>> Add the very same attribute to shared action configuration.
>> If a driver needs to prepare HW resources in two different
>> ways, depending on the operation level, in order to set up
>> an action, then this new attribute will indicate the level.
>> Also, when handling a flow rule insertion, the driver will
>> be able to turn down a shared action if its level is unfit.
>>
>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Acked-by: Ori Kam <orika@nvidia.com>
>> ---
>>   lib/librte_ethdev/rte_flow.h | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
>> index a8eac4deb..8b970ba0b 100644
>> --- a/lib/librte_ethdev/rte_flow.h
>> +++ b/lib/librte_ethdev/rte_flow.h
>> @@ -3487,6 +3487,14 @@ struct rte_flow_shared_action_conf {
>>       /**< Action valid for rules applied to ingress traffic. */
>>       uint32_t egress:1;
>>       /**< Action valid for rules applied to egress traffic. */
>> +
>> +    /**
>> +     * When set to 1, indicates that the action is valid for
>> +     * transfer traffic; otherwise, for non-transfer traffic.
>> +     *
>> +     * See struct rte_flow_attr.
>> +     */
>> +    uint32_t transfer:1;
> 
> Is this require any documentation update?
> 
> Also cc'ed Andrey, as he is author of the shared action feature, @Andrey 
> can you please review this update?

Many-many thanks to you for reviewing the patch. And thanks for inviting 
@Andrey. I should've done that from the very beginning.

What's for documentation update, = I did take a look at 
"doc/guides/prog_guide/rte_flow.rst" after Ori had suggested to do so. 
As far as I can learn from the file, the convention is to describe the 
action structure itself, i.e. not any auxiliary structures.

For example, direct input to action SHARED is "struct 
rte_flow_shared_action". And it's already described by an empty table 
("table:: SHARED") in the doc file.

On the other hand, documentation of "struct rte_flow_shared_action_conf" 
belongs in a place where the API rte_flow_shared_action_create() is 
documented. This API is only mentioned by 
"doc/guides/prog_guide/rte_flow.rst", and it looks like it's not 
documented anywhere but in the header file itself 
("lib/librte_ethdev/rte_flow.h").

So, it looks like this particular patch does not need to provide an 
update to documentation other that the existing comment before the field 
in the structure.

-- 
Ivan M