DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
@ 2021-03-31  8:53 Leyi Rong
  2021-04-01 12:27 ` Zhang, Qi Z
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Leyi Rong @ 2021-03-31  8:53 UTC (permalink / raw)
  To: qi.z.zhang, beilei.xing; +Cc: dev, Leyi Rong

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;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-03-31  8:53 [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path Leyi Rong
@ 2021-04-01 12:27 ` Zhang, Qi Z
  2021-04-01 12:33 ` David Marchand
  2021-04-07 16:39 ` Ferruh Yigit
  2 siblings, 0 replies; 11+ messages in thread
From: Zhang, Qi Z @ 2021-04-01 12:27 UTC (permalink / raw)
  To: Rong, Leyi, Xing, Beilei; +Cc: dev



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-03-31  8:53 [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path Leyi Rong
  2021-04-01 12:27 ` Zhang, Qi Z
@ 2021-04-01 12:33 ` David Marchand
  2021-04-01 13:19   ` Ananyev, Konstantin
  2021-04-07 16:39 ` Ferruh Yigit
  2 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2021-04-01 12:33 UTC (permalink / raw)
  To: Leyi Rong; +Cc: Qi Zhang, Beilei Xing, dev

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?



-- 
David Marchand


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-01 12:33 ` David Marchand
@ 2021-04-01 13:19   ` Ananyev, Konstantin
  0 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2021-04-01 13:19 UTC (permalink / raw)
  To: David Marchand, Rong, Leyi; +Cc: Zhang, Qi Z, Xing, Beilei, dev



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-03-31  8:53 [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path Leyi Rong
  2021-04-01 12:27 ` Zhang, Qi Z
  2021-04-01 12:33 ` David Marchand
@ 2021-04-07 16:39 ` Ferruh Yigit
  2021-04-08  8:29   ` Rong, Leyi
  2021-04-08  8:39   ` David Marchand
  2 siblings, 2 replies; 11+ messages in thread
From: Ferruh Yigit @ 2021-04-07 16:39 UTC (permalink / raw)
  To: Leyi Rong, qi.z.zhang, beilei.xing; +Cc: dev

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-07 16:39 ` Ferruh Yigit
@ 2021-04-08  8:29   ` Rong, Leyi
  2021-04-08 10:32     ` Ananyev, Konstantin
  2021-04-08  8:39   ` David Marchand
  1 sibling, 1 reply; 11+ messages in thread
From: Rong, Leyi @ 2021-04-08  8:29 UTC (permalink / raw)
  To: Yigit, Ferruh, Zhang, Qi Z, Xing, Beilei, Ananyev, Konstantin; +Cc: dev


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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-07 16:39 ` Ferruh Yigit
  2021-04-08  8:29   ` Rong, Leyi
@ 2021-04-08  8:39   ` David Marchand
  2021-04-08  8:49     ` Rong, Leyi
  1 sibling, 1 reply; 11+ messages in thread
From: David Marchand @ 2021-04-08  8:39 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Leyi Rong, Qi Zhang, Beilei Xing, dev

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-08  8:39   ` David Marchand
@ 2021-04-08  8:49     ` Rong, Leyi
  0 siblings, 0 replies; 11+ messages in thread
From: Rong, Leyi @ 2021-04-08  8:49 UTC (permalink / raw)
  To: David Marchand, Yigit, Ferruh; +Cc: Zhang, Qi Z, Xing, Beilei, dev


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-08  8:29   ` Rong, Leyi
@ 2021-04-08 10:32     ` Ananyev, Konstantin
  2021-04-09  7:19       ` Rong, Leyi
  0 siblings, 1 reply; 11+ messages in thread
From: Ananyev, Konstantin @ 2021-04-08 10:32 UTC (permalink / raw)
  To: Rong, Leyi, Yigit, Ferruh, Zhang, Qi Z, Xing, Beilei; +Cc: dev


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


 
 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-08 10:32     ` Ananyev, Konstantin
@ 2021-04-09  7:19       ` Rong, Leyi
  2021-04-09 10:40         ` Ananyev, Konstantin
  0 siblings, 1 reply; 11+ messages in thread
From: Rong, Leyi @ 2021-04-09  7:19 UTC (permalink / raw)
  To: Ananyev, Konstantin, Yigit, Ferruh, Zhang, Qi Z, Xing, Beilei; +Cc: dev


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
  2021-04-09  7:19       ` Rong, Leyi
@ 2021-04-09 10:40         ` Ananyev, Konstantin
  0 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2021-04-09 10:40 UTC (permalink / raw)
  To: Rong, Leyi, Yigit, Ferruh, Zhang, Qi Z, Xing, Beilei; +Cc: dev



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-04-09 10:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-31  8:53 [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path Leyi Rong
2021-04-01 12:27 ` Zhang, Qi Z
2021-04-01 12:33 ` David Marchand
2021-04-01 13:19   ` Ananyev, Konstantin
2021-04-07 16:39 ` Ferruh Yigit
2021-04-08  8:29   ` Rong, Leyi
2021-04-08 10:32     ` Ananyev, Konstantin
2021-04-09  7:19       ` Rong, Leyi
2021-04-09 10:40         ` Ananyev, Konstantin
2021-04-08  8:39   ` David Marchand
2021-04-08  8:49     ` Rong, Leyi

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git