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

[Ferruh Yigit]

On 11/3/2020 2:10 PM, Ivan Malov wrote:
> 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 at oktetlabs.ru>
>>> Acked-by: Ori Kam <orika at 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.
> 

I missed that Ori requested same before, and I agree on your documentation 
analysis above, there is not clear location to put this other than doxygen 
comment, so OK to proceed as it is.