Re: [PATCH v3] doc: announce ABI change for struct rte_eth_conf

[Ivan Boule <ivan.boule@6wind.com>]

Hi Jijiang,

See my comments inline below prefixewd with IB>
Ivan

On 12/18/2015 03:00 AM, Liu, Jijiang wrote:
> Hi Boule,
>
>> -----Original Message-----
>> From: Ivan Boule [mailto:ivan.boule@6wind.com]
>> Sent: Tuesday, December 15, 2015 4:50 PM
>> To: Liu, Jijiang
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3] doc: announce ABI change for struct
>> rte_eth_conf
>>
>> On 12/14/2015 08:48 AM, Jijiang Liu wrote:
>>> In current codes, tunnel configuration information is not stored in a device
>> configuration, and it will get nothing if application want to retrieve tunnel
>> config, so I think it is necessary to add rte_eth_dev_tunnel_configure()
>> function is to do the configurations including flow and classification
>> information and store it in a device configuration.
>>>
>>> And tunneling packet encapsulation operation will benifit from the change.
>>>
>>> There are more descriptions for the ABI changes below,
>>>
>>> The struct 'rte_eth_tunnel_conf' is a new, its defination like below,
>>> struct rte_eth_tunnel_conf {
>>>          uint16_t tx_queue;
>>>          uint16_t filter_type;
>>>          struct rte_eth_tunnel_flow flow_tnl; };
>>>
>>> The ABI change announcement of struct 'rte_eth_tunnel_flow' have
>> already sent out, refer to [1].
>>>
>>> [1]http://dpdk.org/ml/archives/dev/2015-December/029837.html.
>>>
>>> The change of struct 'rte_eth_conf' like below, but it have not finalized yet.
>>> struct rte_eth_conf {
>>> 	...
>>> 	uint32_t dcb_capability_en;
>>> 	struct rte_fdir_conf fdir_conf; /**< FDIR configuration. */
>>> 	struct rte_intr_conf intr_conf; /**< Interrupt mode configuration. */
>>> 	struct rte_eth_tunnel_conf
>> *tunnel_conf[RTE_MAX_QUEUES_PER_PORT];
>>> 	/**< Tunnel configuration. */
>>> };
>>>
>>> v2 change:
>>>     Add more description for the change.
>>>
>>> v3 change:
>>>     Change ABI announcement description.
>>>
>>> Signed-off-by: Jijiang Liu <jijiang.liu@intel.com> ---cmdline.c
>>>    doc/guides/rel_notes/deprecation.rst |    6 ++++++
>>>    1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>> b/doc/guides/rel_notes/deprecation.rst
>>> index 5c458f2..9dbe89e 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -23,3 +23,9 @@ Deprecation Notices
>>>    * ABI changes are planned for struct rte_eth_tunnel_flow in order to
>> extend new fileds to support
>>>      tunneling packet configuration in unified tunneling APIs. The release 2.2
>> does not contain these ABI
>>>      changes, but release 2.3 will, and no backwards compatibility is planned.
>>> +
>>> +* ABI changes are planned for the struct rte_eth_conf in order to add
>>> +'tunnel_conf' variable
>>> +  in the struct to store tunnel configuration when using new API
>>> +rte_eth_dev_tunnel_configure
>>> +  (uint8_t port_id, uint16_t rx_queue, struct rte_eth_tunnel_conf *
>>> +tunnel_conf) to configure
>>> +  tunnel flow and classification information. The release 2.2 does
>>> +not contain these ABI change,
>>> +  but release 2.3 will, and no backward compatibility is planned.
>>>
>> Hi Jijiang,
>>
>> Can you provide a real use case - I mean an example of a real network
>> application - that really needs to save tunnel configurations in a data
>> structure associated with a NIC port?
>
> I'm trying to provide a tunneling packet solution in DPDK, which would accelerate de/encapsulation operation of tunneling packet.
IB> I was asking for an example of an application that needs to SAVE in 
the DPDK structure associated with a port a tunnel configuration that it 
applies to that port.
Where does that saved tunnel configuration will participate to the 
acceleration of decap/encap ops?

>
> It was described at [1],
> [1] http://dpdk.org/ml/archives/dev/2015-December/030283.html
>
>
> Let me provide more details on this, these data structure definition have not fully finalized yet, just for your reference.
> We are talking about why tunnel configuration need to be stored.
IB? yes :-)

>
> For NIC A RX process,
> VM 0--->VTEP A---> VXLAN network--->VTEP B---NIC A (Rx queue 1 with info [1] )--->SW decapsulation--->vSwitch--->VM 0
>
> For NIC A TX process,
> VM 0<---VTEP A<---VXLAN network<---VTEP B<---NIC A (TX queue 1)<---SW Encapsulation with info[2]<---vSwitch<---VM 0
>
> The[2] information  will be got by retrieving the tunnel configuration, if the tunnel configuration is not stored  in 'rt_eth_conf', and how to get it?

IB> it is assumed that the encapsulation acceleration relies on having 
this operation done in hardware. Am I wrong?
If I am right, then can you tell me which PMD function accesses the 
saved tunnel configuration?

>
> Of course, the tunnel configuration is also stored in Application, does it make sense?
IB> No. Why store it twice? Are you considering that memory if available 
for free?

>
> [1] outr src ip(192.168.10.1) + outer dst ip(10.239.129.11) + outer src port(1000) + outer dst port(2000) + tunnel id(100)
> [2] outer src ip(10.239.129.11) + outer dst ip(192.168.10.1) + outer src port(2000) + outr dst port(1000) + tunnel id(100)
>
>>
>> Firstly, if the only usage is to enable applications to retrieve tunnel
>> configurations, then you are simply growing the size of the per-port structure
>> with tunnel configurations, and imposing it to every DPDK application.
>> You impose it to those applications that don't care about tunneling, but also
>> to those applications which do care, but which prefer to have their own
>> representation of ports where they store everything they need to.
>> If the tunnel configuration is also used for other purposes, then it must be
>> precisely described what happens with the saved tunnel configuration when
>> the application changes the state of a port.
>> This is the case for instance when the application reconfigures the number of
>> RX queues of a port.
>> Who is responsible for checking that some tunnels won't be matched
>> anymore?
>> Who is responsible for dropping/invalidating the saved tunnel configuration,
>> if such operations must be performed?
>> This list is likely to be not exhaustive, of course.
>
> About above these question, it is related to design, I will send RFC patch out for review.
IB> Do you mean that I will find EXPLICIT answers to those questions in 
your RFC patch? If so, why not supply them inline ?

>
>>
>> More globally, all side-effects of saving the tunnel configuration must be
>> considered and addressed in a coherent way and in an easy-to-use approach.
>>
>> By the way, as far as I know, the Linux kernel does not [need to] save tunnel
>> data or ARP entries behind "netdevice" structures.
>
> It is not related ARP entries, I'm talking about tunnel flow.
IB> Really? I did not notice :-)
IB> More seriously, what I meant is that it is a good programming 
practice for an application to have its private representation of 
low-level objects (a DPDK port here) it uses, and to maintain into it 
whatever informations it need to.
I referred to ARP entries as an example of such informations that can be 
associated with a port, and thus that might also be saved
in a data structure of the DPDK port. Just to outline that there is no 
end to such approach...

>
>> PS : in the "rte_eth_tunnel_conf" data structure, I think that the first field
>> should be named "rx_queue" instead of "tx_queue".
>
> No, 'rx_queue' id can be as index of tunnel_conf[RTE_MAX_QUEUES_PER_PORT];
IB> then, what does the tx_index refer to in in the 
"rte_eth_tunnel_conf" data structure, and how is it used, and by which 
DPDK code ?
Please, note that by design, the default DPDK rule for the usage of TX 
queues consists in having DPDK applications assigning each TX queue of a 
port to a different [paquet processing] core, so that each core can 
safely transmit paquets through a port in a lockless fashion.
Can you guarantee that your tunneling spec. still comply with this rule?

Regards,
Ivan

-- 
Ivan Boule
6WIND Development Engineer