[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(ð_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