[dpdk-dev] [PATCH 15/39] examples/ipsec-secgw: convert to new ethdev offloads API

[Radu Nicolau]

On 12/11/2017 12:33 PM, Shahaf Shuler wrote:
> Hi Radu,
>
> Monday, December 11, 2017 1:48 PM, Radu Nicolau :
>> Hi,
>>
>> Comment inline
>>
>>
>> On 11/23/2017 12:19 PM, Shahaf Shuler wrote:
>>> Ethdev offloads API has changed since:
>>>
>>> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") commit
>>> cba7f53b717d ("ethdev: introduce Tx queue offloads API")
>>>
>>> This commit support the new API.
>>>
>>> Signed-off-by: Shahaf Shuler <shahafs at mellanox.com>
>>> ---
>>>    examples/ipsec-secgw/ipsec-secgw.c | 27
>> +++++++++++++++++++++++++--
>>>    1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c
>>> b/examples/ipsec-secgw/ipsec-secgw.c
>>> index c98454a90..6e538a1ab 100644
>>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>>> @@ -217,6 +217,8 @@ static struct rte_eth_conf port_conf = {
>>>    	},
>>>    	.txmode = {
>>>    		.mq_mode = ETH_MQ_TX_NONE,
>>> +		.offloads = (DEV_TX_OFFLOAD_IPV4_CKSUM |
>>> +			     DEV_TX_OFFLOAD_MULTI_SEGS),
>>>    	},
>>>    };
>>>
>>> @@ -1394,6 +1396,22 @@ port_init(uint16_t portid)
>>>    	if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_SECURITY)
>>>    		port_conf.txmode.offloads |= DEV_TX_OFFLOAD_SECURITY;
>>>
>>> +	if ((dev_info.rx_offload_capa & port_conf.rxmode.offloads) !=
>>> +	    port_conf.rxmode.offloads) {
>>> +		printf("Some Rx offloads are not supported "
>>> +		       "by port %d: requested 0x%lx supported 0x%lx\n",
>>> +		       portid, port_conf.rxmode.offloads,
>>> +		       dev_info.rx_offload_capa);
>>> +		port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>>> +	}
>>> +	if ((dev_info.tx_offload_capa & port_conf.txmode.offloads) !=
>>> +	     port_conf.txmode.offloads) {
>>> +		printf("Some Tx offloads are not supported "
>>> +		       "by port %d: requested 0x%lx supported 0x%lx\n",
>>> +		       portid, port_conf.txmode.offloads,
>>> +		       dev_info.tx_offload_capa);
>>> +		port_conf.txmode.offloads &= dev_info.tx_offload_capa;
>>> +	}
>> I don't think that clearing the offload flags that are not advertised in the
>> capabilities is a good approach, although it may be the right one.
>>   From what I can see there are more PMDs that don't fully populate the
>> offload capabilities, but actually check for them in the configure/start
>> function. One of them is ixgbe, which needs CRC strip enabled when IPSec is
>> enabled, and will fail to start otherwise. So although it supports CRC strip it
>> does not set the flag in the capabilities, but checks it in the start function.
> Why ixgbe don't expose the CRC cap then? It seems wrong behavior to expect the application to set it without any cap reported.
It is bad behavior but from what I can see most, if not all, PMDs don't 
expose CRC strip (or jumbo frames) while still supporting it.
>
>> I would propose to just print a warning if a requested offload is not set in the
>> capabilities, but let the pmd start fail if it is not really supported.
>
> I think I agree, however not from the reason you mentioned.
> It is bad to mask the un-supported offloads because the application relies on them to be set successfully. The application will not run successfully if the IPV4 checksum is not actually set (for example).
>
> On v2 I will print just the warn.
>
>>>    	ret = rte_eth_dev_configure(portid, nb_rx_queue, nb_tx_queue,
>>>    			&port_conf);
>>>    	if (ret < 0)
>>> @@ -1420,7 +1438,8 @@ port_init(uint16_t portid)
>>>    		printf("Setup txq=%u,%d,%d\n", lcore_id, tx_queueid,
>> socket_id);
>>>    		txconf = &dev_info.default_txconf;
>>> -		txconf->txq_flags = 0;
>>> +		txconf->txq_flags = ETH_TXQ_FLAGS_IGNORE;
>>> +		txconf->offloads = port_conf.txmode.offloads;
>>>
>>>    		ret = rte_eth_tx_queue_setup(portid, tx_queueid, nb_txd,
>>>    				socket_id, txconf);
>>> @@ -1434,6 +1453,8 @@ port_init(uint16_t portid)
>>>
>>>    		/* init RX queues */
>>>    		for (queue = 0; queue < qconf->nb_rx_queue; ++queue) {
>>> +			struct rte_eth_rxconf rxq_conf;
>>> +
>>>    			if (portid != qconf->rx_queue_list[queue].port_id)
>>>    				continue;
>>>
>>> @@ -1442,8 +1463,10 @@ port_init(uint16_t portid)
>>>    			printf("Setup rxq=%d,%d,%d\n", portid, rx_queueid,
>>>    					socket_id);
>>>
>>> +			rxq_conf = dev_info.default_rxconf;
>>> +			rxq_conf.offloads = port_conf.rxmode.offloads;
>>>    			ret = rte_eth_rx_queue_setup(portid, rx_queueid,
>>> -					nb_rxd,	socket_id, NULL,
>>> +					nb_rxd,	socket_id,
>> &rxq_conf,
>>>    					socket_ctx[socket_id].mbuf_pool);
>>>    			if (ret < 0)
>>>    				rte_exit(EXIT_FAILURE,