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

[Ananyev, Konstantin]

> -----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.