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 66D091B2F6 for ; Tue, 10 Oct 2017 18:10:14 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Oct 2017 09:10:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,505,1503385200"; d="scan'208";a="1229225648" Received: from rnicolau-mobl.ger.corp.intel.com (HELO [10.237.221.79]) ([10.237.221.79]) by fmsmga002.fm.intel.com with ESMTP; 10 Oct 2017 09:10:05 -0700 To: "Ananyev, Konstantin" , Akhil Goyal , "dev@dpdk.org" Cc: "Doherty, Declan" , "De Lara Guarch, Pablo" , "hemant.agrawal@nxp.com" , "borisp@mellanox.com" , "aviadye@mellanox.com" , "thomas@monjalon.net" , "sandeep.malik@nxp.com" , "jerin.jacob@caviumnetworks.com" , "Mcnamara, John" , "olivier.matz@6wind.com" References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20171003131413.23846-1-akhil.goyal@nxp.com> <20171003131413.23846-11-akhil.goyal@nxp.com> <2601191342CEEE43887BDE71AB9772585FAA4EE0@IRSMSX103.ger.corp.intel.com> <1377fcac-61f1-c545-b354-52343a903349@intel.com> <2601191342CEEE43887BDE71AB9772585FAA590E@IRSMSX103.ger.corp.intel.com> From: Radu Nicolau Message-ID: Date: Tue, 10 Oct 2017 17:10:05 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB9772585FAA590E@IRSMSX103.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec 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: Tue, 10 Oct 2017 16:10:16 -0000 Hi, Next iteration will have the macro removed and the register write then polling macro reworked and moved to ixgbe_osdep.h Regards, Radu On 10/6/2017 7:33 PM, Ananyev, Konstantin wrote: > >> -----Original Message----- >> From: Nicolau, Radu >> Sent: Friday, October 6, 2017 10:18 AM >> To: Ananyev, Konstantin ; Akhil Goyal ; dev@dpdk.org >> Cc: Doherty, Declan ; De Lara Guarch, Pablo ; hemant.agrawal@nxp.com; >> borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net; sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; >> Mcnamara, John ; olivier.matz@6wind.com >> Subject: Re: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec >> >> Thanks for reviewing! >> >> Some comments inline. >> >> >> On 10/5/2017 6:55 PM, Ananyev, Konstantin wrote: >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal >>>> Sent: Tuesday, October 3, 2017 2:14 PM >>>> To: dev@dpdk.org >>>> Cc: Doherty, Declan ; De Lara Guarch, Pablo ; >> hemant.agrawal@nxp.com; >>>> Nicolau, Radu ; borisp@mellanox.com; aviadye@mellanox.com; thomas@monjalon.net; >>>> sandeep.malik@nxp.com; jerin.jacob@caviumnetworks.com; Mcnamara, John ; olivier.matz@6wind.com >>>> Subject: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec >>>> >>>> From: Radu Nicolau >>>> >>>> Signed-off-by: Radu Nicolau >>>> Signed-off-by: Declan Doherty >>>> --- >>>> >>>> >>>> eth_dev->dev_ops = &ixgbe_eth_dev_ops; >>>> +#ifdef RTE_LIBRTE_IXGBE_IPSEC >>>> + rte_security_register(ð_dev->data->sec_id, >>>> + (void *)eth_dev, &ixgbe_security_ops); >>>> +#endif /* RTE_LIBRTE_IXGBE_IPSEC */ >>> I still wonder do we really need new config macro and >>> Ifdef it through all ixgbe code? >>> Can we have it just always on? >>> If the RX/TX performance suffers a lot we can have a special >>> RX/TX functions for ipsec enabled case. >> I only put it there in case there is a performance degradation, but I'm >> fairly certain that there is none. >> So if you think that's best I will remove it, but just in case that >> there is a performance degradation for non-ipsec traffic it will provide >> a quick way to turn the feature off. > My position is let's remove the macro in any way. > If the testing will show no performance degradation - let's have ipsec > non-ipsec path together. > It the testing will show noticeable perfoamce degradation - let's > Have a special rx/tx function for ipsec enabled. > In that case users will still have an option to use ipsec if needed, > and can switch it on/off at runtime. > >>>> >>>> +#include "base/ixgbe_type.h" >>>> +#include "base/ixgbe_api.h" >>>> +#include "ixgbe_ethdev.h" >>>> +#include "ixgbe_ipsec.h" >>>> + >>>> + >>>> +#define IXGBE_WAIT_RW(__reg, __rw) \ >>>> +{ \ >>>> + int cnt = 100; \ >>>> + IXGBE_WRITE_REG(hw, (__reg), reg); \ >>>> + while (((IXGBE_READ_REG(hw, (__reg))) & (__rw)) && (cnt--)) \ >>>> + rte_delay_us(1); \ >>>> +} >>> Looks usefull. >>> Probably worth to add cnt as a parameter and put the macro (or even better inline func) >>> Inside base/ixgbe_osdep.h. >> First let me explain why I've put it there: in the datasheet it is >> stated that after the software requests a write the hw will perform the >> write and afterwards clear the write bit (7.12.9.2.1). > I think I understand what are you doing here. > You write to the HW register and then poll on that register till HW indicate that the operation completed. > In fact that's not the only place where you have to do same thing. > Let say at dev_rxtx_start(): > rxdctl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(rxq->reg_idx)); > rxdctl |= IXGBE_RXDCTL_ENABLE; > IXGBE_WRITE_REG(hw, IXGBE_RXDCTL(rxq->reg_idx), rxdctl); > > /* Wait until RX Enable ready */ > poll_ms = RTE_IXGBE_REGISTER_POLL_WAIT_10_MS; > do { > rte_delay_ms(1); > rxdctl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(rxq->reg_idx)); > } while (--poll_ms && !(rxdctl & IXGBE_RXDCTL_ENABLE)); > if (!poll_ms) > PMD_INIT_LOG(ERR, "Could not enable Rx Queue %d", > rx_queue_id); > So my thought was that common macro(inline function will be useful here. > Though I think we have to get rid of implicit parameters. > Konstantin > >> My understanding is that I need to wait for the write bit to clear until >> I request another subsequent write (and there are multiple writes into >> multiple tables in succession for setting up the Rx SA). >> I added the cnt because I wasn't comfortable with a potentially endless >> loop... >> So if you think that this will be useful in other places I will move it >> as you say. >>>> >>>> } >>>> >>>> @@ -4981,6 +5024,22 @@ ixgbe_dev_rxtx_start(struct rte_eth_dev *dev) >>>> dev->data->dev_conf.lpbk_mode == IXGBE_LPBK_82599_TX_RX) >>>> ixgbe_setup_loopback_link_82599(hw); >>> As I can see from the datasheet LRO and IPsec are mutually exclusive, >>> plus IPsec requires hw crc strip enabled. >>> I think you need add extra checks regarding that in ixgbe_dev_rx_init() or so. >>> Another thing - probably need to update ixgbe_set_tx_function() to >>> select full-featured TX func when txmode.enable_sec is on. >> I will look into it. >