From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 56589E5D for ; Mon, 11 Dec 2017 13:51:16 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2017 04:51:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,391,1508828400"; d="scan'208";a="1623145" Received: from rnicolau-mobl.ger.corp.intel.com (HELO [10.237.221.73]) ([10.237.221.73]) by fmsmga008.fm.intel.com with ESMTP; 11 Dec 2017 04:51:15 -0800 To: Shahaf Shuler , "dev@dpdk.org" References: <20171123121941.144335-1-shahafs@mellanox.com> <20171123121941.144335-6-shahafs@mellanox.com> <1f393cb6-1765-5711-0e60-384da087bbf3@intel.com> From: Radu Nicolau Message-ID: Date: Mon, 11 Dec 2017 12:51:14 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH 15/39] examples/ipsec-secgw: convert to new ethdev offloads API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Dec 2017 12:51:17 -0000 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 >>> --- >>> 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,