* [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
` (4 more replies)
0 siblings, 5 replies; 20+ 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] 20+ 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
` (3 subsequent siblings)
4 siblings, 0 replies; 20+ 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] 20+ 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 subsequent siblings)
4 siblings, 1 reply; 20+ 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] 20+ 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; 20+ 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] 20+ 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
2021-04-19 8:36 ` [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx " Leyi Rong
2021-04-20 5:29 ` [dpdk-dev] [PATCH v3] " Leyi Rong
4 siblings, 2 replies; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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; 20+ 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] 20+ 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
2021-04-13 10:23 ` Rong, Leyi
0 siblings, 1 reply; 20+ 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] 20+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
2021-04-09 10:40 ` Ananyev, Konstantin
@ 2021-04-13 10:23 ` Rong, Leyi
2021-04-13 10:41 ` Ananyev, Konstantin
0 siblings, 1 reply; 20+ messages in thread
From: Rong, Leyi @ 2021-04-13 10:23 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: 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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path
2021-04-13 10:23 ` Rong, Leyi
@ 2021-04-13 10:41 ` Ananyev, Konstantin
0 siblings, 0 replies; 20+ messages in thread
From: Ananyev, Konstantin @ 2021-04-13 10:41 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.
> >
>
> 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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx data path
2021-03-31 8:53 [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path Leyi Rong
` (2 preceding siblings ...)
2021-04-07 16:39 ` Ferruh Yigit
@ 2021-04-19 8:36 ` Leyi Rong
2021-04-19 9:36 ` Zhang, Qi Z
2021-04-19 10:59 ` Ananyev, Konstantin
2021-04-20 5:29 ` [dpdk-dev] [PATCH v3] " Leyi Rong
4 siblings, 2 replies; 20+ messages in thread
From: Leyi Rong @ 2021-04-19 8:36 UTC (permalink / raw)
To: qi.z.zhang, konstantin.ananyev; +Cc: dev, Leyi Rong
Introduce i40e_simple_prep_pkts() as the preparation function for
simple Tx data path, as it's for sanity check for simple Tx.
Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Signed-off-by: Leyi Rong <leyi.rong@intel.com>
---
drivers/net/i40e/i40e_rxtx.c | 42 +++++++++++++++++++++++++++++++++++-
drivers/net/i40e/i40e_rxtx.h | 2 ++
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 3c7686c3f4..282eb5924b 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1479,6 +1479,46 @@ i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
return nb_tx;
}
+/*********************************************************************
+ *
+ * TX simple prep functions
+ *
+ **********************************************************************/
+uint16_t
+i40e_simple_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ int i;
+ uint64_t ol_flags;
+ struct rte_mbuf *m;
+ struct i40e_tx_queue *txq = (struct i40e_tx_queue *)tx_queue;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+ ol_flags = m->ol_flags;
+
+ if (!(txq->offloads ==
+ (txq->offloads & DEV_TX_OFFLOAD_MBUF_FAST_FREE) &&
+ m->nb_segs == 1)) {
+ rte_errno = EINVAL;
+ return i;
+ }
+
+ if (ol_flags & I40E_TX_OFFLOAD_NOTSUP_MASK) {
+ rte_errno = ENOTSUP;
+ return i;
+ }
+
+ /* check the size of packet */
+ if (m->pkt_len < I40E_TX_MIN_PKT_LEN ||
+ m->pkt_len > I40E_FRAME_SIZE_MAX) {
+ rte_errno = EINVAL;
+ return i;
+ }
+ }
+ return i;
+}
+
/*********************************************************************
*
* TX prep functions
@@ -3412,7 +3452,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_simple_prep_pkts;
} else {
PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
dev->tx_pkt_burst = i40e_xmit_pkts;
diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
index 3447a58c82..5ccf5773e8 100644
--- a/drivers/net/i40e/i40e_rxtx.h
+++ b/drivers/net/i40e/i40e_rxtx.h
@@ -208,6 +208,8 @@ uint16_t i40e_recv_scattered_pkts(void *rx_queue,
uint16_t i40e_xmit_pkts(void *tx_queue,
struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t i40e_simple_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
uint16_t i40e_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
int i40e_tx_queue_init(struct i40e_tx_queue *txq);
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx data path
2021-04-19 8:36 ` [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx " Leyi Rong
@ 2021-04-19 9:36 ` Zhang, Qi Z
2021-04-19 10:59 ` Ananyev, Konstantin
1 sibling, 0 replies; 20+ messages in thread
From: Zhang, Qi Z @ 2021-04-19 9:36 UTC (permalink / raw)
To: Rong, Leyi, Ananyev, Konstantin; +Cc: dev
> -----Original Message-----
> From: Rong, Leyi <leyi.rong@intel.com>
> Sent: Monday, April 19, 2021 4:36 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Rong, Leyi <leyi.rong@intel.com>
> Subject: [PATCH v2] net/i40e: add Tx preparation for simple Tx data path
>
> Introduce i40e_simple_prep_pkts() as the preparation function for simple Tx
> data path, as it's for sanity check for simple Tx.
>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 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] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx data path
2021-04-19 8:36 ` [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx " Leyi Rong
2021-04-19 9:36 ` Zhang, Qi Z
@ 2021-04-19 10:59 ` Ananyev, Konstantin
2021-04-20 5:31 ` Rong, Leyi
1 sibling, 1 reply; 20+ messages in thread
From: Ananyev, Konstantin @ 2021-04-19 10:59 UTC (permalink / raw)
To: Rong, Leyi, Zhang, Qi Z; +Cc: dev
Hi Leyi,
> Introduce i40e_simple_prep_pkts() as the preparation function for
> simple Tx data path, as it's for sanity check for simple Tx.
>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> ---
> drivers/net/i40e/i40e_rxtx.c | 42 +++++++++++++++++++++++++++++++++++-
> drivers/net/i40e/i40e_rxtx.h | 2 ++
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 3c7686c3f4..282eb5924b 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1479,6 +1479,46 @@ i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> return nb_tx;
> }
>
> +/*********************************************************************
> + *
> + * TX simple prep functions
> + *
> + **********************************************************************/
> +uint16_t
> +i40e_simple_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
> + uint16_t nb_pkts)
> +{
> + int i;
> + uint64_t ol_flags;
> + struct rte_mbuf *m;
> + struct i40e_tx_queue *txq = (struct i40e_tx_queue *)tx_queue;
> +
> + for (i = 0; i < nb_pkts; i++) {
> + m = tx_pkts[i];
> + ol_flags = m->ol_flags;
> +
> + if (!(txq->offloads ==
> + (txq->offloads & DEV_TX_OFFLOAD_MBUF_FAST_FREE) &&
> + m->nb_segs == 1)) {
I think we don't need to check txq->offloads here,
they should be already checked at setup phase.
I suppose:
if (m->nb_segs != 1) {...}
should be enough here.
> + rte_errno = EINVAL;
> + return i;
> + }
> +
> + if (ol_flags & I40E_TX_OFFLOAD_NOTSUP_MASK) {
It would return OK for the packet with any supported by any i40e TX offload (full-featured path).
Probably we need here instead:
if (ol_flags & PKT_TX_OFFLOAD_MASK) {...}
> + rte_errno = ENOTSUP;
> + return i;
> + }
> +
> + /* check the size of packet */
> + if (m->pkt_len < I40E_TX_MIN_PKT_LEN ||
> + m->pkt_len > I40E_FRAME_SIZE_MAX) {
> + rte_errno = EINVAL;
> + return i;
> + }
> + }
> + return i;
> +}
> +
> /*********************************************************************
> *
> * TX prep functions
> @@ -3412,7 +3452,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_simple_prep_pkts;
> } else {
> PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> dev->tx_pkt_burst = i40e_xmit_pkts;
> diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
> index 3447a58c82..5ccf5773e8 100644
> --- a/drivers/net/i40e/i40e_rxtx.h
> +++ b/drivers/net/i40e/i40e_rxtx.h
> @@ -208,6 +208,8 @@ uint16_t i40e_recv_scattered_pkts(void *rx_queue,
> uint16_t i40e_xmit_pkts(void *tx_queue,
> struct rte_mbuf **tx_pkts,
> uint16_t nb_pkts);
> +uint16_t i40e_simple_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> + uint16_t nb_pkts);
> uint16_t i40e_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> uint16_t nb_pkts);
> int i40e_tx_queue_init(struct i40e_tx_queue *txq);
> --
> 2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [dpdk-dev] [PATCH v3] net/i40e: add Tx preparation for simple Tx data path
2021-03-31 8:53 [dpdk-dev] [PATCH] net/i40e: add Tx preparation for vector data path Leyi Rong
` (3 preceding siblings ...)
2021-04-19 8:36 ` [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx " Leyi Rong
@ 2021-04-20 5:29 ` Leyi Rong
2021-04-20 8:03 ` Ananyev, Konstantin
4 siblings, 1 reply; 20+ messages in thread
From: Leyi Rong @ 2021-04-20 5:29 UTC (permalink / raw)
To: qi.z.zhang, konstantin.ananyev; +Cc: dev, Leyi Rong
Introduce i40e_simple_prep_pkts() as the preparation function for
simple Tx data path, as it's for sanity check for simple Tx.
Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Signed-off-by: Leyi Rong <leyi.rong@intel.com>
---
drivers/net/i40e/i40e_rxtx.c | 39 +++++++++++++++++++++++++++++++++++-
drivers/net/i40e/i40e_rxtx.h | 2 ++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 3c7686c3f4..86a9eae370 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1479,6 +1479,43 @@ i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
return nb_tx;
}
+/*********************************************************************
+ *
+ * TX simple prep functions
+ *
+ **********************************************************************/
+uint16_t
+i40e_simple_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ int i;
+ uint64_t ol_flags;
+ struct rte_mbuf *m;
+
+ for (i = 0; i < nb_pkts; i++) {
+ m = tx_pkts[i];
+ ol_flags = m->ol_flags;
+
+ if (m->nb_segs != 1) {
+ rte_errno = EINVAL;
+ return i;
+ }
+
+ if (ol_flags & PKT_TX_OFFLOAD_MASK) {
+ rte_errno = ENOTSUP;
+ return i;
+ }
+
+ /* check the size of packet */
+ if (m->pkt_len < I40E_TX_MIN_PKT_LEN ||
+ m->pkt_len > I40E_FRAME_SIZE_MAX) {
+ rte_errno = EINVAL;
+ return i;
+ }
+ }
+ return i;
+}
+
/*********************************************************************
*
* TX prep functions
@@ -3412,7 +3449,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_simple_prep_pkts;
} else {
PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
dev->tx_pkt_burst = i40e_xmit_pkts;
diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
index 3447a58c82..5ccf5773e8 100644
--- a/drivers/net/i40e/i40e_rxtx.h
+++ b/drivers/net/i40e/i40e_rxtx.h
@@ -208,6 +208,8 @@ uint16_t i40e_recv_scattered_pkts(void *rx_queue,
uint16_t i40e_xmit_pkts(void *tx_queue,
struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
+uint16_t i40e_simple_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts);
uint16_t i40e_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts);
int i40e_tx_queue_init(struct i40e_tx_queue *txq);
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx data path
2021-04-19 10:59 ` Ananyev, Konstantin
@ 2021-04-20 5:31 ` Rong, Leyi
0 siblings, 0 replies; 20+ messages in thread
From: Rong, Leyi @ 2021-04-20 5:31 UTC (permalink / raw)
To: Ananyev, Konstantin, Zhang, Qi Z; +Cc: dev
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, April 19, 2021 6:59 PM
> To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] net/i40e: add Tx preparation for simple Tx data path
>
> Hi Leyi,
>
> > Introduce i40e_simple_prep_pkts() as the preparation function for
> > simple Tx data path, as it's for sanity check for simple Tx.
> >
> > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> > ---
> > drivers/net/i40e/i40e_rxtx.c | 42
> > +++++++++++++++++++++++++++++++++++-
> > drivers/net/i40e/i40e_rxtx.h | 2 ++
> > 2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index 3c7686c3f4..282eb5924b 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1479,6 +1479,46 @@ i40e_xmit_pkts_vec(void *tx_queue, struct
> rte_mbuf **tx_pkts,
> > return nb_tx;
> > }
> >
> >
> +/***************************************************************
> *****
> > +*
> > + *
> > + * TX simple prep functions
> > + *
> > +
> >
> +****************************************************************
> *****
> > +*/
> > +uint16_t
> > +i40e_simple_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf
> **tx_pkts,
> > + uint16_t nb_pkts)
> > +{
> > + int i;
> > + uint64_t ol_flags;
> > + struct rte_mbuf *m;
> > + struct i40e_tx_queue *txq = (struct i40e_tx_queue *)tx_queue;
> > +
> > + for (i = 0; i < nb_pkts; i++) {
> > + m = tx_pkts[i];
> > + ol_flags = m->ol_flags;
> > +
> > + if (!(txq->offloads ==
> > + (txq->offloads & DEV_TX_OFFLOAD_MBUF_FAST_FREE)
> &&
> > + m->nb_segs == 1)) {
>
> I think we don't need to check txq->offloads here, they should be already
> checked at setup phase.
> I suppose:
> if (m->nb_segs != 1) {...}
> should be enough here.
>
> > + rte_errno = EINVAL;
> > + return i;
> > + }
> > +
> > + if (ol_flags & I40E_TX_OFFLOAD_NOTSUP_MASK) {
>
> It would return OK for the packet with any supported by any i40e TX offload
> (full-featured path).
> Probably we need here instead:
> if (ol_flags & PKT_TX_OFFLOAD_MASK) {...}
>
> > + rte_errno = ENOTSUP;
> > + return i;
> > + }
Hi Konstantin,
Got your points, will be fixed in v3, thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/i40e: add Tx preparation for simple Tx data path
2021-04-20 5:29 ` [dpdk-dev] [PATCH v3] " Leyi Rong
@ 2021-04-20 8:03 ` Ananyev, Konstantin
2021-04-20 14:00 ` Zhang, Qi Z
0 siblings, 1 reply; 20+ messages in thread
From: Ananyev, Konstantin @ 2021-04-20 8:03 UTC (permalink / raw)
To: Rong, Leyi, Zhang, Qi Z; +Cc: dev
> Introduce i40e_simple_prep_pkts() as the preparation function for
> simple Tx data path, as it's for sanity check for simple Tx.
>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Leyi Rong <leyi.rong@intel.com>
> ---
> drivers/net/i40e/i40e_rxtx.c | 39 +++++++++++++++++++++++++++++++++++-
> drivers/net/i40e/i40e_rxtx.h | 2 ++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 3c7686c3f4..86a9eae370 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1479,6 +1479,43 @@ i40e_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> return nb_tx;
> }
>
> +/*********************************************************************
> + *
> + * TX simple prep functions
> + *
> + **********************************************************************/
> +uint16_t
> +i40e_simple_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
> + uint16_t nb_pkts)
> +{
> + int i;
> + uint64_t ol_flags;
> + struct rte_mbuf *m;
> +
> + for (i = 0; i < nb_pkts; i++) {
> + m = tx_pkts[i];
> + ol_flags = m->ol_flags;
> +
> + if (m->nb_segs != 1) {
> + rte_errno = EINVAL;
> + return i;
> + }
> +
> + if (ol_flags & PKT_TX_OFFLOAD_MASK) {
> + rte_errno = ENOTSUP;
> + return i;
> + }
> +
> + /* check the size of packet */
> + if (m->pkt_len < I40E_TX_MIN_PKT_LEN ||
> + m->pkt_len > I40E_FRAME_SIZE_MAX) {
> + rte_errno = EINVAL;
> + return i;
> + }
> + }
> + return i;
> +}
> +
> /*********************************************************************
> *
> * TX prep functions
> @@ -3412,7 +3449,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_simple_prep_pkts;
> } else {
> PMD_INIT_LOG(DEBUG, "Xmit tx finally be used.");
> dev->tx_pkt_burst = i40e_xmit_pkts;
> diff --git a/drivers/net/i40e/i40e_rxtx.h b/drivers/net/i40e/i40e_rxtx.h
> index 3447a58c82..5ccf5773e8 100644
> --- a/drivers/net/i40e/i40e_rxtx.h
> +++ b/drivers/net/i40e/i40e_rxtx.h
> @@ -208,6 +208,8 @@ uint16_t i40e_recv_scattered_pkts(void *rx_queue,
> uint16_t i40e_xmit_pkts(void *tx_queue,
> struct rte_mbuf **tx_pkts,
> uint16_t nb_pkts);
> +uint16_t i40e_simple_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> + uint16_t nb_pkts);
> uint16_t i40e_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> uint16_t nb_pkts);
> int i40e_tx_queue_init(struct i40e_tx_queue *txq);
> --
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/i40e: add Tx preparation for simple Tx data path
2021-04-20 8:03 ` Ananyev, Konstantin
@ 2021-04-20 14:00 ` Zhang, Qi Z
0 siblings, 0 replies; 20+ messages in thread
From: Zhang, Qi Z @ 2021-04-20 14:00 UTC (permalink / raw)
To: Ananyev, Konstantin, Rong, Leyi; +Cc: dev
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, April 20, 2021 4:03 PM
> To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v3] net/i40e: add Tx preparation for simple Tx data path
>
>
> > Introduce i40e_simple_prep_pkts() as the preparation function for
> > simple Tx data path, as it's for sanity check for simple Tx.
> >
> > Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Signed-off-by: Leyi Rong <leyi.rong@intel.com>
Applied to dpdk-next-net-intel.
Thanks
Qi
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-04-20 14:00 UTC | newest]
Thread overview: 20+ 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-13 10:23 ` Rong, Leyi
2021-04-13 10:41 ` Ananyev, Konstantin
2021-04-08 8:39 ` David Marchand
2021-04-08 8:49 ` Rong, Leyi
2021-04-19 8:36 ` [dpdk-dev] [PATCH v2] net/i40e: add Tx preparation for simple Tx " Leyi Rong
2021-04-19 9:36 ` Zhang, Qi Z
2021-04-19 10:59 ` Ananyev, Konstantin
2021-04-20 5:31 ` Rong, Leyi
2021-04-20 5:29 ` [dpdk-dev] [PATCH v3] " Leyi Rong
2021-04-20 8:03 ` Ananyev, Konstantin
2021-04-20 14:00 ` Zhang, Qi Z
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).