Re: [PATCH v4] ethdev: add special flags when creating async transfer table

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

On 11/8/22 16:29, Thomas Monjalon wrote:
> 08/11/2022 12:47, Andrew Rybchenko:
>> On 11/8/22 14:39, Andrew Rybchenko wrote:
>>> On 11/4/22 13:44, Rongwei Liu wrote:
>>>> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
>>>> index 8858b56428..1eab12796f 100644
>>>> --- a/lib/ethdev/rte_flow.h
>>>> +++ b/lib/ethdev/rte_flow.h
>>>> @@ -5186,6 +5186,34 @@ rte_flow_actions_template_destroy(uint16_t
>>>> port_id,
>>>>     */
>>>>    struct rte_flow_template_table;
>>>> +/**
>>>> + * @warning
>>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>>> + *
>>>> + * Special optional flags for template table attribute.
>>>> + * Each bit stands for a table specialization
>>>> + * offering a potential optimization at PMD layer.
>>>> + * PMD can ignore the unsupported bits silently.
>>>> + */
>>>> +enum rte_flow_template_table_specialize {
>>>> +    /**
>>>> +     * Specialize table for transfer flows which come only from wire.
>>>> +     * It allows PMD not to allocate resources for non-wire
>>>> originated traffic.
>>>> +     * This bit is not a matching criteria, just an optimization hint.
>>>> +     * Flow rules which match non-wire originated traffic will be missed
>>>> +     * if the hint is supported.
>>
>> Sorry, but if so, the hint changes behavior.
> 
> Yes the hint may change behaviour.
> 
>> Let's consider a rule which matches both VF originating and
>> wire originating traffic. Will the rule be missed (ignored)
>> regardless if the hint is supported or not?
> 
> If the hint RTE_FLOW_TRANSFER_WIRE_ORIG is used,
> the PMD may assume the table won't be used for traffic
> which is not coming from wire ports.
> As a consequence, the table may be implemented on the path
> of wire traffic only.
> In this case, the traffic coming from virtual ports
> won't be affected by this table.
> To answer the question, a rule matching both virtual and wire traffic
> will be applied in a table affecting only wire traffic,
> so it will still apply (not completely ignored).

If so, it is not a hint. It becomes matching criteria
which should be in pattern as we discussed.

> 
> If you really want to manage both types of traffic in this table,
> you must not use such hint.
> 
>> I.e. it will not apply to wire originated traffic as well.
>>
>>>> +     */
>>>> +    RTE_FLOW_TRANSFER_WIRE_ORIG = RTE_BIT32(0),
>>>> +    /**
>>>> +     * Specialize table for transfer flows which come only from vport
>>>> (e.g. VF, SF).
>>>> +     * It allows PMD not to allocate resources for non-vport
>>>> originated traffic.
>>>> +     * This bit is not a matching criteria, just an optimization hint.
>>>> +     * Flow rules which match non-vport originated traffic will be
>>>> missed
>>>> +     * if the hint is supported.
>>>> +     */
>>>> +    RTE_FLOW_TRANSFER_VPORT_ORIG = RTE_BIT32(1),
>>>> +};
>>>> +
>>>>    /**
>>>>     * @warning
>>>>     * @b EXPERIMENTAL: this API may change without prior notice.
>>>> @@ -5201,6 +5229,10 @@ struct rte_flow_template_table_attr {
>>>>         * Maximum number of flow rules that this table holds.
>>>>         */
>>>>        uint32_t nb_flows;
>>>> +    /**
>>>> +     * Optional hint flags for PMD optimization.
>>>> +     */
>>>> +    enum rte_flow_template_table_specialize specialize;
>>>
>>>
>>> IMHO it is not 100% correct to use enum for flag since
>>> RTE_FLOW_TRANSFER_WIRE_ORIG | RTE_FLOW_TRANSFER_VPORT_ORIG
>>> is not the enum member. uint32_t is a better option here since
>>> bits are defined as RTE_BIT32. enum should be mentioned in the
>>> description.
> 
> I agree, let's not use enum.
> Instead we can mention the prefix of the defines in the comments.
> 
>