Re: [dpdk-dev] [PATCH v1 01/12] ethdev: add ethdev item to flow API

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

Hi Andrew, Ori,

On 04/10/2021 13:47, Andrew Rybchenko wrote:
> On 10/4/21 3:00 AM, Ivan Malov wrote:
>> Hi Ori,
>>
>> On 04/10/2021 00:09, Ori Kam wrote:
>>> Hi Ivan,
>>>
>>>> -----Original Message-----
>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>> Subject: Re: [PATCH v1 01/12] ethdev: add ethdev item to flow API
>>>>
>>>> Hi Ori,
>>>>
>>>> On 03/10/2021 14:52, Ori Kam wrote:
>>>>> Hi Andrew and Ivan,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Sent: Friday, October 1, 2021 4:47 PM
>>>>>> Subject: [PATCH v1 01/12] ethdev: add ethdev item to flow API
>>>>>>
>>>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>>
>>>>>> For use with "transfer" flows. Supposed to match traffic transmitted
>>>>>> by the DPDK application via the specified ethdev, at e-switch level.
>>>>>>
>>>>>> Must not be combined with attributes "ingress" / "egress".
>>>>>>
>>>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> ---
>>>>>
>>>>> [Snip]
>>>>>
>>>>>>     /** Generate flow_action[] entry. */ diff --git
>>>>>> a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
>>>>>> 7b1ed7f110..880502098e 100644
>>>>>> --- a/lib/ethdev/rte_flow.h
>>>>>> +++ b/lib/ethdev/rte_flow.h
>>>>>> @@ -574,6 +574,15 @@ enum rte_flow_item_type {
>>>>>>          * @see struct rte_flow_item_conntrack.
>>>>>>          */
>>>>>>         RTE_FLOW_ITEM_TYPE_CONNTRACK,
>>>>>> +
>>>>>> +    /**
>>>>>> +     * [META]
>>>>>> +     *
>>>>>> +     * Matches traffic at e-switch going from (sent by) the given
>>>>>> ethdev.
>>>>>> +     *
>>>>>> +     * @see struct rte_flow_item_ethdev
>>>>>> +     */
>>>>>> +    RTE_FLOW_ITEM_TYPE_ETHDEV,
>>>>>>     };
>>>>>>
>>>>>>     /**
>>>>>> @@ -1799,6 +1808,24 @@ static const struct rte_flow_item_conntrack
>>>>>> rte_flow_item_conntrack_mask = {  };  #endif
>>>>>>
>>>>>> +/**
>>>>>> + * @warning
>>>>>> + * @b EXPERIMENTAL: this structure may change without prior notice
>>>>>> + *
>>>>>> + * Provides an ethdev ID for use with items which are as follows:
>>>>>> + * RTE_FLOW_ITEM_TYPE_ETHDEV.
>>>>>> + */
>>>>>> +struct rte_flow_item_ethdev {
>>>>>> +    uint16_t id; /**< Ethdev ID */
>>>>>
>>>>> True for all above uses,
>>>>> should this be renamed to port?
>>>>
>>>> I'd not rename it to "port". The very idea of this series is to
>>>> disambiguate
>>>> things. This structure is shared between primitives ETHDEV and
>>>> ESWITCH_PORT. If we go for "port", then in conjunction with ESWITCH_PORT
>>>> the structure name may trick readers into thinking that the ID in
>>>> question is
>>>> the own ID of the e-switch port itself. But in fact this ID is an
>>>> ethdev ID which
>>>> is associated with the e-switch port.
>>>>
>>>> Should you wish to elaborate on your concerns with regard to naming,
>>>> please
>>>> do so. I'm all ears.
>>>>
>>> Fully understand and agree that the idea is to clear the ambiguaty.
>>> My concern is that we don't use ethdev id, from application ethdev has
>>> only
>>> ports, so what is the id? (if we keep this, we should document that
>>> the id is the
>>> port)
>>> What about ETHDEV_PORT and ESWITCH_PORT?
>>
>> I understand that, technically, the only ports which the application can
>> really interface with are ethdevs. So, terms "ethdev" and "port" may
>> appear synonymous to the application - you are right on that. But, given
>> the fact that we have some primitives like PHY_PORT and the likes, which
>> also have "PORT" in their names, I'd rather go for "ethdev" as more
>> precise term.
>>
>> But let me assure you: I'm not saying that my opinion should prevail.
>> I'm giving more thoughts to this in the background. Maybe Andrew can
>> join this conversation as well.
> 
> As far as I can see ethdev API uses 'port_id' everywhere to
> refer to ethdev port by its number. So, I suggest
> 
> struct rte_flow_item_ethdev {
>      uint16_t port_id; /**< ethdev port ID */
> };
> 
> Basically I agree with Ori, that just "id" is a bit confusing
> even when it is a member of the _ethdev structure, but I'd
> prepend "port_"  a field name to sync with ethdev API which
> uses port_id. So, we have ethdev->port_id.

Ack

> 
> Andrew.
> 

-- 
Ivan M