[dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec

Radu Nicolau radu.nicolau at intel.com
Tue Oct 10 18:10:05 CEST 2017


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 <konstantin.ananyev at intel.com>; Akhil Goyal <akhil.goyal at nxp.com>; dev at dpdk.org
>> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com;
>> borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net; sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com;
>> Mcnamara, John <john.mcnamara at intel.com>; olivier.matz at 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 at dpdk.org] On Behalf Of Akhil Goyal
>>>> Sent: Tuesday, October 3, 2017 2:14 PM
>>>> To: dev at dpdk.org
>>>> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>;
>> hemant.agrawal at nxp.com;
>>>> Nicolau, Radu <radu.nicolau at intel.com>; borisp at mellanox.com; aviadye at mellanox.com; thomas at monjalon.net;
>>>> sandeep.malik at nxp.com; jerin.jacob at caviumnetworks.com; Mcnamara, John <john.mcnamara at intel.com>; olivier.matz at 6wind.com
>>>> Subject: [dpdk-dev] [PATCH v2 10/12] net/ixgbe: enable inline ipsec
>>>>
>>>> From: Radu Nicolau <radu.nicolau at intel.com>
>>>>
>>>> Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
>>>> Signed-off-by: Declan Doherty <declan.doherty at intel.com>
>>>> ---
>>>> <snip>
>>>>
>>>>    	eth_dev->dev_ops = &ixgbe_eth_dev_ops;
>>>> +#ifdef RTE_LIBRTE_IXGBE_IPSEC
>>>> +	rte_security_register(&eth_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.
>
>>>> <snip>
>>>> +#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.
>>>> <snip>
>>>>    }
>>>>
>>>> @@ -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.
>



More information about the dev mailing list