diff mbox series

net/i40e: add Tx preparation for vector data path

Message ID 20210331085345.6290-1-leyi.rong@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/i40e: add Tx preparation for vector data path |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot success travis build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS

Commit Message

Leyi Rong March 31, 2021, 8:53 a.m. UTC 
  Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
data path selection, as the sanity check is needed ideally.

Signed-off-by: Leyi Rong <leyi.rong@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Qi Zhang April 1, 2021, 12:27 p.m. UTC | #1 
> -----Original Message-----
> From: Rong, Leyi <leyi.rong@intel.com>
> Sent: Wednesday, March 31, 2021 4:54 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Rong, Leyi <leyi.rong@intel.com>
> Subject: [PATCH] net/i40e: add Tx preparation for vector data path
> 
> Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
> data path selection, as the sanity check is needed ideally.
> 
> Signed-off-by: Leyi Rong <leyi.rong@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
David Marchand April 1, 2021, 12:33 p.m. UTC | #2 
On Wed, Mar 31, 2021 at 11:17 AM Leyi Rong <leyi.rong@intel.com> wrote:
>
> Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
> data path selection, as the sanity check is needed ideally.
>
> Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 61cb204be2..b3d7765e3b 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>                         PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
>                         dev->tx_pkt_burst = i40e_xmit_pkts_simple;
>                 }
> -               dev->tx_pkt_prepare = NULL;
> +               dev->tx_pkt_prepare = i40e_prep_pkts;
>         } else {
>                 PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
>                 dev->tx_pkt_burst = i40e_xmit_pkts;

I find this a bit surprising to set this op at two different locations.
Why not simply leave the value set in i40e_ethdev.c?
Ananyev, Konstantin April 1, 2021, 1:19 p.m. UTC | #3 
> 
> On Wed, Mar 31, 2021 at 11:17 AM Leyi Rong <leyi.rong@intel.com> wrote:
> >
> > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
> > data path selection, as the sanity check is needed ideally.
> >
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > index 61cb204be2..b3d7765e3b 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> >                         PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> >                         dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> >                 }
> > -               dev->tx_pkt_prepare = NULL;
> > +               dev->tx_pkt_prepare = i40e_prep_pkts;
> >         } else {
> >                 PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> >                 dev->tx_pkt_burst = i40e_xmit_pkts;
> 
> I find this a bit surprising to set this op at two different locations.
> Why not simply leave the value set in i40e_ethdev.c?
> 

Actually I wonder should we set dev->tx_pkt_prepare to  point to i40e_prep_pkts()
for simple TX path?
As I remember, simple TX path doesn't support neither multi-segment packets, 
neither TX offloads. 
While i40e_prep_pkts() assumes that selected tx_burst() supports all that.
If you don't want to keep dev->tx_pkt_prepare==NULL for simple TX path,
why not create  a new one: i40e_simple_prep_pkts() or so?

Konstantin
Ferruh Yigit April 7, 2021, 4:39 p.m. UTC | #4 
On 3/31/2021 9:53 AM, Leyi Rong wrote:
> Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
> data path selection, as the sanity check is needed ideally.
> 
> Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> ---
>   drivers/net/i40e/i40e_rxtx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 61cb204be2..b3d7765e3b 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>   			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
>   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
>   		}
> -		dev->tx_pkt_prepare = NULL;
> +		dev->tx_pkt_prepare = i40e_prep_pkts;
>   	} else {
>   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
>   		dev->tx_pkt_burst = i40e_xmit_pkts;
> 

It seems prepare function is doing some sanity checks before handing packets to 
the HW.
So with this change all Tx paths calls the same Tx prepare function, if so why 
not set the function pointer outside of the if block, instead of setting it in 
both legs of the if/else? This clarifies that Tx prepare used always.
Leyi Rong April 8, 2021, 8:29 a.m. UTC | #5 
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, April 8, 2021 12:40 AM
> To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data
> path
> 
> On 3/31/2021 9:53 AM, Leyi Rong wrote:
> > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and
> > simple data path selection, as the sanity check is needed ideally.
> >
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > ---
> >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> >   			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> >   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> >   		}
> > -		dev->tx_pkt_prepare = NULL;
> > +		dev->tx_pkt_prepare = i40e_prep_pkts;
> >   	} else {
> >   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> >   		dev->tx_pkt_burst = i40e_xmit_pkts;
> >
> 
> It seems prepare function is doing some sanity checks before handing packets to
> the HW.
> So with this change all Tx paths calls the same Tx prepare function, if so why not
> set the function pointer outside of the if block, instead of setting it in both legs
> of the if/else? This clarifies that Tx prepare used always.

Hi Ferruh,

Yes, it make sense.

Hi Konstantin,

Would that be something wrong if the prepare function goes for simple Tx function although it does not support the offload feature yet?
David Marchand April 8, 2021, 8:39 a.m. UTC | #6 
On Wed, Apr 7, 2021 at 6:40 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 3/31/2021 9:53 AM, Leyi Rong wrote:
> > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and simple
> > data path selection, as the sanity check is needed ideally.
> >
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > ---
> >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > index 61cb204be2..b3d7765e3b 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> >                       PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> >                       dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> >               }
> > -             dev->tx_pkt_prepare = NULL;
> > +             dev->tx_pkt_prepare = i40e_prep_pkts;
> >       } else {
> >               PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> >               dev->tx_pkt_burst = i40e_xmit_pkts;
> >
>
> It seems prepare function is doing some sanity checks before handing packets to
> the HW.
> So with this change all Tx paths calls the same Tx prepare function, if so why
> not set the function pointer outside of the if block, instead of setting it in
> both legs of the if/else? This clarifies that Tx prepare used always.

As I said earlier, what is the point of setting it in ethdev.c too?
It would be more consistent to have this in a single location.

drivers/net/i40e/i40e_ethdev.c: dev->tx_pkt_burst = i40e_xmit_pkts;
drivers/net/i40e/i40e_ethdev.c: dev->tx_pkt_prepare = i40e_prep_pkts;
drivers/net/i40e/i40e_ethdev_vf.c:      eth_dev->tx_pkt_burst = &i40e_xmit_pkts;
drivers/net/i40e/i40e_rxtx.c:   if (dev->tx_pkt_burst ==
i40e_xmit_pkts_vec_avx512) {
drivers/net/i40e/i40e_rxtx.c:   if (dev->tx_pkt_burst ==
i40e_xmit_pkts_vec_avx2 ||
drivers/net/i40e/i40e_rxtx.c:                   dev->tx_pkt_burst ==
i40e_xmit_pkts_vec) {
drivers/net/i40e/i40e_rxtx.c:
dev->tx_pkt_burst = i40e_xmit_pkts_vec_avx512;
drivers/net/i40e/i40e_rxtx.c:
dev->tx_pkt_burst = use_avx2 ?
drivers/net/i40e/i40e_rxtx.c:                   dev->tx_pkt_burst =
i40e_xmit_pkts_simple;
drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_prepare = NULL;
drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_burst = i40e_xmit_pkts;
drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_prepare = i40e_prep_pkts;
drivers/net/i40e/i40e_rxtx.c:   eth_tx_burst_t pkt_burst = dev->tx_pkt_burst;
drivers/net/i40e/i40e_vf_representor.c: ethdev->tx_pkt_burst =
i40e_vf_representor_tx_burst;
Leyi Rong April 8, 2021, 8:49 a.m. UTC | #7 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, April 8, 2021 4:40 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>; dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data
> path
> 
> On Wed, Apr 7, 2021 at 6:40 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >
> > On 3/31/2021 9:53 AM, Leyi Rong wrote:
> > > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and
> > > simple data path selection, as the sanity check is needed ideally.
> > >
> > > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > > ---
> > >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> > >                       PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> > >                       dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> > >               }
> > > -             dev->tx_pkt_prepare = NULL;
> > > +             dev->tx_pkt_prepare = i40e_prep_pkts;
> > >       } else {
> > >               PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> > >               dev->tx_pkt_burst = i40e_xmit_pkts;
> > >
> >
> > It seems prepare function is doing some sanity checks before handing
> > packets to the HW.
> > So with this change all Tx paths calls the same Tx prepare function,
> > if so why not set the function pointer outside of the if block,
> > instead of setting it in both legs of the if/else? This clarifies that Tx prepare
> used always.
> 
> As I said earlier, what is the point of setting it in ethdev.c too?
> It would be more consistent to have this in a single location.
> 
> drivers/net/i40e/i40e_ethdev.c: dev->tx_pkt_burst = i40e_xmit_pkts;
> drivers/net/i40e/i40e_ethdev.c: dev->tx_pkt_prepare = i40e_prep_pkts;
> drivers/net/i40e/i40e_ethdev_vf.c:      eth_dev->tx_pkt_burst = &i40e_xmit_pkts;
> drivers/net/i40e/i40e_rxtx.c:   if (dev->tx_pkt_burst ==
> i40e_xmit_pkts_vec_avx512) {
> drivers/net/i40e/i40e_rxtx.c:   if (dev->tx_pkt_burst ==
> i40e_xmit_pkts_vec_avx2 ||
> drivers/net/i40e/i40e_rxtx.c:                   dev->tx_pkt_burst ==
> i40e_xmit_pkts_vec) {
> drivers/net/i40e/i40e_rxtx.c:
> dev->tx_pkt_burst = i40e_xmit_pkts_vec_avx512;
> drivers/net/i40e/i40e_rxtx.c:
> dev->tx_pkt_burst = use_avx2 ?
> drivers/net/i40e/i40e_rxtx.c:                   dev->tx_pkt_burst =
> i40e_xmit_pkts_simple;
> drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_prepare = NULL;
> drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_burst = i40e_xmit_pkts;
> drivers/net/i40e/i40e_rxtx.c:           dev->tx_pkt_prepare = i40e_prep_pkts;
> drivers/net/i40e/i40e_rxtx.c:   eth_tx_burst_t pkt_burst = dev->tx_pkt_burst;
> drivers/net/i40e/i40e_vf_representor.c: ethdev->tx_pkt_burst =
> i40e_vf_representor_tx_burst;
> 
> 
> --
> David Marchand

Hi David,

Yes, if agreed to set the same prepare function for all Tx path, can leave the setting in one place.
Ananyev, Konstantin April 8, 2021, 10:32 a.m. UTC | #8 
> 
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: Thursday, April 8, 2021 12:40 AM
> > To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Xing,
> > Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data
> > path
> >
> > On 3/31/2021 9:53 AM, Leyi Rong wrote:
> > > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and
> > > simple data path selection, as the sanity check is needed ideally.
> > >
> > > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > > ---
> > >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> > >   			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> > >   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> > >   		}
> > > -		dev->tx_pkt_prepare = NULL;
> > > +		dev->tx_pkt_prepare = i40e_prep_pkts;
> > >   	} else {
> > >   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> > >   		dev->tx_pkt_burst = i40e_xmit_pkts;
> > >
> >
> > It seems prepare function is doing some sanity checks before handing packets to
> > the HW.
> > So with this change all Tx paths calls the same Tx prepare function, if so why not
> > set the function pointer outside of the if block, instead of setting it in both legs
> > of the if/else? This clarifies that Tx prepare used always.
> 
> Hi Ferruh,
> 
> Yes, it make sense.
> 
> Hi Konstantin,

Hi Leyi,

> 
> Would that be something wrong if the prepare function goes for simple Tx function although it does not support the offload feature yet?
> 

Current situation:
For simple TX path we set dev->tx_pkt_prepare = NULL.
That makes rte_eth_tx_prepare() a stub that does nothing and always returns: "All packets are good".
That is unsafe off-course, and if upper layer will pass a packet that is not supported,
then it can lead to various bad things: bad cksum, corrupted packets, TX hang, etc.
But at least it keeps simple TX path fast.
With that patch:
For simple TX path we set dev->tx_pkt_prepare = i40e_prep_pkts.
Now on TX path we invoke extra function that does a lot of checks, but it still unsafe:
as i40e_prep_pkts() assumes that  full-featured TX function is in place (multi-segs are allowed, etc.).
So our simple TX path became slower, but still is unsafe.
I think that if we want to introduce tx_prepare() for simple TX path,
then the proper way - create a new function for it (i40e_simple_prep_pkts() or so).
It will be aware that simple TX path is in place and more restrictions should be met:
check that nb_segs==1 and no TX offloads (except FAST_FREE?) are enabled,
plus usual checks for min and max pkt_len.

Konstantin
Leyi Rong April 9, 2021, 7:19 a.m. UTC | #9 
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, April 8, 2021 6:33 PM
> To: Rong, Leyi <leyi.rong@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data
> path
> 
> 
> >
> > > -----Original Message-----
> > > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > > Sent: Thursday, April 8, 2021 12:40 AM
> > > To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for
> > > vector data path
> > >
> > > On 3/31/2021 9:53 AM, Leyi Rong wrote:
> > > > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and
> > > > simple data path selection, as the sanity check is needed ideally.
> > > >
> > > > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > > > ---
> > > >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> > > >   			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> > > >   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> > > >   		}
> > > > -		dev->tx_pkt_prepare = NULL;
> > > > +		dev->tx_pkt_prepare = i40e_prep_pkts;
> > > >   	} else {
> > > >   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> > > >   		dev->tx_pkt_burst = i40e_xmit_pkts;
> > > >
> > >
> > > It seems prepare function is doing some sanity checks before handing
> > > packets to the HW.
> > > So with this change all Tx paths calls the same Tx prepare function,
> > > if so why not set the function pointer outside of the if block,
> > > instead of setting it in both legs of the if/else? This clarifies that Tx prepare
> used always.
> >
> > Hi Ferruh,
> >
> > Yes, it make sense.
> >
> > Hi Konstantin,
> 
> Hi Leyi,
> 
> >
> > Would that be something wrong if the prepare function goes for simple Tx
> function although it does not support the offload feature yet?
> >
> 
> Current situation:
> For simple TX path we set dev->tx_pkt_prepare = NULL.
> That makes rte_eth_tx_prepare() a stub that does nothing and always returns:
> "All packets are good".
> That is unsafe off-course, and if upper layer will pass a packet that is not
> supported, then it can lead to various bad things: bad cksum, corrupted packets,
> TX hang, etc.
> But at least it keeps simple TX path fast.
> With that patch:
> For simple TX path we set dev->tx_pkt_prepare = i40e_prep_pkts.
> Now on TX path we invoke extra function that does a lot of checks, but it still
> unsafe:
> as i40e_prep_pkts() assumes that  full-featured TX function is in place (multi-segs
> are allowed, etc.).
> So our simple TX path became slower, but still is unsafe.
> I think that if we want to introduce tx_prepare() for simple TX path, then the
> proper way - create a new function for it (i40e_simple_prep_pkts() or so).
> It will be aware that simple TX path is in place and more restrictions should be
> met:
> check that nb_segs==1 and no TX offloads (except FAST_FREE?) are enabled,
> plus usual checks for min and max pkt_len.
> 
> Konstantin
> 

Hi Konstantin,

Thanks for the explanation, I know the current full-featured prepare function will cost more CPU cycle, but not sure how to say is still unsafe?
Why I set the simple Tx prepare function to the current i40e_prep_pkts() is we may support more offload features like current full-featured Tx for vector path(which is included in simple Tx currently), if so, the current tx prepare function can be re-used.
Ananyev, Konstantin April 9, 2021, 10:40 a.m. UTC | #10 
> > > > > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector and
> > > > > simple data path selection, as the sanity check is needed ideally.
> > > > >
> > > > > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > > > > ---
> > > > >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b 100644
> > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> > > > >   			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
> > > > >   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> > > > >   		}
> > > > > -		dev->tx_pkt_prepare = NULL;
> > > > > +		dev->tx_pkt_prepare = i40e_prep_pkts;
> > > > >   	} else {
> > > > >   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> > > > >   		dev->tx_pkt_burst = i40e_xmit_pkts;
> > > > >
> > > >
> > > > It seems prepare function is doing some sanity checks before handing
> > > > packets to the HW.
> > > > So with this change all Tx paths calls the same Tx prepare function,
> > > > if so why not set the function pointer outside of the if block,
> > > > instead of setting it in both legs of the if/else? This clarifies that Tx prepare
> > used always.
> > >
> > > Hi Ferruh,
> > >
> > > Yes, it make sense.
> > >
> > > Hi Konstantin,
> >
> > Hi Leyi,
> >
> > >
> > > Would that be something wrong if the prepare function goes for simple Tx
> > function although it does not support the offload feature yet?
> > >
> >
> > Current situation:
> > For simple TX path we set dev->tx_pkt_prepare = NULL.
> > That makes rte_eth_tx_prepare() a stub that does nothing and always returns:
> > "All packets are good".
> > That is unsafe off-course, and if upper layer will pass a packet that is not
> > supported, then it can lead to various bad things: bad cksum, corrupted packets,
> > TX hang, etc.
> > But at least it keeps simple TX path fast.
> > With that patch:
> > For simple TX path we set dev->tx_pkt_prepare = i40e_prep_pkts.
> > Now on TX path we invoke extra function that does a lot of checks, but it still
> > unsafe:
> > as i40e_prep_pkts() assumes that  full-featured TX function is in place (multi-segs
> > are allowed, etc.).
> > So our simple TX path became slower, but still is unsafe.
> > I think that if we want to introduce tx_prepare() for simple TX path, then the
> > proper way - create a new function for it (i40e_simple_prep_pkts() or so).
> > It will be aware that simple TX path is in place and more restrictions should be
> > met:
> > check that nb_segs==1 and no TX offloads (except FAST_FREE?) are enabled,
> > plus usual checks for min and max pkt_len.
> >
> > Konstantin
> >


Hi Leyi,
 
> Hi Konstantin,
> 
> Thanks for the explanation, I know the current full-featured prepare function will cost more CPU cycle, but not sure how to say is still
> unsafe?

Let say user will do:

mb = create_and_fill_multi_seg_pkt(...);
n =  rte_eth_tx_prepare(p, q, &mb, 1);
if (n == 1)
  n = rte_eth_tx_burst(p, q, &mb, 1);
else
  rte_pktmbuf_free(mb);

if dev->tx_pkt_prepare == i40e_prep_pkts and dev->tx_pkt_burs == i40e_xmit_pkts_simple,
then this code will TX the packet, even though it shouldn't in theory.

> Why I set the simple Tx prepare function to the current i40e_prep_pkts() is we may support more offload features like current full-featured
> Tx for vector path(which is included in simple Tx currently), if so, the current tx prepare function can be re-used.

AFAIK, for i40e current simple (and vector) TX path doesn't support all offloads that are supported by full-featured path
To be more specific: mulit-seg packets, TCP_CKSUM, TCP_SEG, etc.
Am I missing something obvious here?

Konstantin
Leyi Rong April 13, 2021, 10:23 a.m. UTC | #11 
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Friday, April 9, 2021 6:40 PM
> To: Rong, Leyi <leyi.rong@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data
> path
> 
> 
> 
> > > > > > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector
> > > > > > and simple data path selection, as the sanity check is needed ideally.
> > > > > >
> > > > > > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > > > > > ---
> > > > > >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b
> > > > > > 100644
> > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev
> *dev)
> > > > > >   			PMD_INIT_LOG(DEBUG, "Simple tx finally be
> used.");
> > > > > >   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> > > > > >   		}
> > > > > > -		dev->tx_pkt_prepare = NULL;
> > > > > > +		dev->tx_pkt_prepare = i40e_prep_pkts;
> > > > > >   	} else {
> > > > > >   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> > > > > >   		dev->tx_pkt_burst = i40e_xmit_pkts;
> > > > > >
> > > > >
> > > > > It seems prepare function is doing some sanity checks before
> > > > > handing packets to the HW.
> > > > > So with this change all Tx paths calls the same Tx prepare
> > > > > function, if so why not set the function pointer outside of the
> > > > > if block, instead of setting it in both legs of the if/else?
> > > > > This clarifies that Tx prepare
> > > used always.
> > > >
> > > > Hi Ferruh,
> > > >
> > > > Yes, it make sense.
> > > >
> > > > Hi Konstantin,
> > >
> > > Hi Leyi,
> > >
> > > >
> > > > Would that be something wrong if the prepare function goes for
> > > > simple Tx
> > > function although it does not support the offload feature yet?
> > > >
> > >
> > > Current situation:
> > > For simple TX path we set dev->tx_pkt_prepare = NULL.
> > > That makes rte_eth_tx_prepare() a stub that does nothing and always
> returns:
> > > "All packets are good".
> > > That is unsafe off-course, and if upper layer will pass a packet
> > > that is not supported, then it can lead to various bad things: bad
> > > cksum, corrupted packets, TX hang, etc.
> > > But at least it keeps simple TX path fast.
> > > With that patch:
> > > For simple TX path we set dev->tx_pkt_prepare = i40e_prep_pkts.
> > > Now on TX path we invoke extra function that does a lot of checks,
> > > but it still
> > > unsafe:
> > > as i40e_prep_pkts() assumes that  full-featured TX function is in
> > > place (multi-segs are allowed, etc.).
> > > So our simple TX path became slower, but still is unsafe.
> > > I think that if we want to introduce tx_prepare() for simple TX
> > > path, then the proper way - create a new function for it
> (i40e_simple_prep_pkts() or so).
> > > It will be aware that simple TX path is in place and more
> > > restrictions should be
> > > met:
> > > check that nb_segs==1 and no TX offloads (except FAST_FREE?) are
> > > enabled, plus usual checks for min and max pkt_len.
> > >
> > > Konstantin
> > >
> 
> 
> Hi Leyi,
> 
> > Hi Konstantin,
> >
> > Thanks for the explanation, I know the current full-featured prepare
> > function will cost more CPU cycle, but not sure how to say is still unsafe?
> 
> Let say user will do:
> 
> mb = create_and_fill_multi_seg_pkt(...);
> n =  rte_eth_tx_prepare(p, q, &mb, 1);
> if (n == 1)
>   n = rte_eth_tx_burst(p, q, &mb, 1);
> else
>   rte_pktmbuf_free(mb);
> 
> if dev->tx_pkt_prepare == i40e_prep_pkts and dev->tx_pkt_burs ==
> i40e_xmit_pkts_simple, then this code will TX the packet, even though it
> shouldn't in theory.
> 

Hi Konstantin,

Yes, it make sense for the current situation.

> > Why I set the simple Tx prepare function to the current
> > i40e_prep_pkts() is we may support more offload features like current full-
> featured Tx for vector path(which is included in simple Tx currently), if so, the
> current tx prepare function can be re-used.
> 
> AFAIK, for i40e current simple (and vector) TX path doesn't support all offloads
> that are supported by full-featured path To be more specific: mulit-seg packets,
> TCP_CKSUM, TCP_SEG, etc.
> Am I missing something obvious here?
> 
> Konstantin

We're intending to support more offload features into vector path gradually, ice/iavf PMD will support Tx checksum offload in AVX512 path in the 2105 release, also will try to support more in the future if possible.
Ananyev, Konstantin April 13, 2021, 10:41 a.m. UTC | #12 
> >
> > > > > > > Fill up dev->tx_pkt_prepare to i40e_pkt_prepare when on vector
> > > > > > > and simple data path selection, as the sanity check is needed ideally.
> > > > > > >
> > > > > > > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > > > > > > ---
> > > > > > >   drivers/net/i40e/i40e_rxtx.c | 2 +-
> > > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > b/drivers/net/i40e/i40e_rxtx.c index 61cb204be2..b3d7765e3b
> > > > > > > 100644
> > > > > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > > > > @@ -3412,7 +3412,7 @@ i40e_set_tx_function(struct rte_eth_dev
> > *dev)
> > > > > > >   			PMD_INIT_LOG(DEBUG, "Simple tx finally be
> > used.");
> > > > > > >   			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
> > > > > > >   		}
> > > > > > > -		dev->tx_pkt_prepare = NULL;
> > > > > > > +		dev->tx_pkt_prepare = i40e_prep_pkts;
> > > > > > >   	} else {
> > > > > > >   		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> > > > > > >   		dev->tx_pkt_burst = i40e_xmit_pkts;
> > > > > > >
> > > > > >
> > > > > > It seems prepare function is doing some sanity checks before
> > > > > > handing packets to the HW.
> > > > > > So with this change all Tx paths calls the same Tx prepare
> > > > > > function, if so why not set the function pointer outside of the
> > > > > > if block, instead of setting it in both legs of the if/else?
> > > > > > This clarifies that Tx prepare
> > > > used always.
> > > > >
> > > > > Hi Ferruh,
> > > > >
> > > > > Yes, it make sense.
> > > > >
> > > > > Hi Konstantin,
> > > >
> > > > Hi Leyi,
> > > >
> > > > >
> > > > > Would that be something wrong if the prepare function goes for
> > > > > simple Tx
> > > > function although it does not support the offload feature yet?
> > > > >
> > > >
> > > > Current situation:
> > > > For simple TX path we set dev->tx_pkt_prepare = NULL.
> > > > That makes rte_eth_tx_prepare() a stub that does nothing and always
> > returns:
> > > > "All packets are good".
> > > > That is unsafe off-course, and if upper layer will pass a packet
> > > > that is not supported, then it can lead to various bad things: bad
> > > > cksum, corrupted packets, TX hang, etc.
> > > > But at least it keeps simple TX path fast.
> > > > With that patch:
> > > > For simple TX path we set dev->tx_pkt_prepare = i40e_prep_pkts.
> > > > Now on TX path we invoke extra function that does a lot of checks,
> > > > but it still
> > > > unsafe:
> > > > as i40e_prep_pkts() assumes that  full-featured TX function is in
> > > > place (multi-segs are allowed, etc.).
> > > > So our simple TX path became slower, but still is unsafe.
> > > > I think that if we want to introduce tx_prepare() for simple TX
> > > > path, then the proper way - create a new function for it
> > (i40e_simple_prep_pkts() or so).
> > > > It will be aware that simple TX path is in place and more
> > > > restrictions should be
> > > > met:
> > > > check that nb_segs==1 and no TX offloads (except FAST_FREE?) are
> > > > enabled, plus usual checks for min and max pkt_len.
> > > >
> > > > Konstantin
> > > >
> >
> >
> > Hi Leyi,
> >
> > > Hi Konstantin,
> > >
> > > Thanks for the explanation, I know the current full-featured prepare
> > > function will cost more CPU cycle, but not sure how to say is still unsafe?
> >
> > Let say user will do:
> >
> > mb = create_and_fill_multi_seg_pkt(...);
> > n =  rte_eth_tx_prepare(p, q, &mb, 1);
> > if (n == 1)
> >   n = rte_eth_tx_burst(p, q, &mb, 1);
> > else
> >   rte_pktmbuf_free(mb);
> >
> > if dev->tx_pkt_prepare == i40e_prep_pkts and dev->tx_pkt_burs ==
> > i40e_xmit_pkts_simple, then this code will TX the packet, even though it
> > shouldn't in theory.
> >
> 
> Hi Konstantin,
> 
> Yes, it make sense for the current situation.

Hmm, could you elaborate a bit: what exactly makes sense?

> 
> > > Why I set the simple Tx prepare function to the current
> > > i40e_prep_pkts() is we may support more offload features like current full-
> > featured Tx for vector path(which is included in simple Tx currently), if so, the
> > current tx prepare function can be re-used.
> >
> > AFAIK, for i40e current simple (and vector) TX path doesn't support all offloads
> > that are supported by full-featured path To be more specific: mulit-seg packets,
> > TCP_CKSUM, TCP_SEG, etc.
> > Am I missing something obvious here?
> >
> > Konstantin
> 
> We're intending to support more offload features into vector path gradually, ice/iavf PMD will support Tx checksum offload in AVX512 path
> in the 2105 release, also will try to support more in the future if possible.

Well, when i40e vector TX will support these offloads, then it will make sense to
use the same prepare function for it. 
But right now, I think it is not the case.
So I still think create a separate one is the best approach for now.
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 61cb204be2..b3d7765e3b 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -3412,7 +3412,7 @@  i40e_set_tx_function(struct rte_eth_dev *dev)
 			PMD_INIT_LOG(DEBUG, "Simple tx finally be used.");
 			dev->tx_pkt_burst = i40e_xmit_pkts_simple;
 		}
-		dev->tx_pkt_prepare = NULL;
+		dev->tx_pkt_prepare = i40e_prep_pkts;
 	} else {
 		PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
 		dev->tx_pkt_burst = i40e_xmit_pkts;