[PATCH 1/2] net/iavf: fix TSO with big segments

[PATCH 1/2] net/iavf: fix TSO with big segments

[David Marchand]

Packets to be segmented with TSO are usually larger than MTU.
Plus, a single segment for the whole packet may be used: in OVS case,
an external rte_malloc'd buffer is used for packets received
from vhost-user ports.

Before this fix, TSO packets were dropped by net/iavf with the following
message:
2023-09-18T14:08:52.739Z|00610|dpdk(pmd-c31/id:11)|ERR|iavf_prep_pkts():
	INVALID mbuf: bad data_len=[2962]

Remove the check on data_len.

Besides, logging an error level message in a datapath function may
slow down the whole application. It is better not to log anything.

Fixes: 19ee91c6bd9a ("net/iavf: check illegal packet sizes")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_rxtx.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index f7df4665d1..0396099708 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -3611,7 +3611,6 @@ iavf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	struct rte_mbuf *m;
 	struct iavf_tx_queue *txq = tx_queue;
 	struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
-	uint16_t max_frame_size = dev->data->mtu + IAVF_ETH_OVERHEAD;
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct iavf_adapter *adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 
@@ -3640,11 +3639,8 @@ iavf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			return i;
 		}
 
-		/* check the data_len in mbuf */
-		if (m->data_len < IAVF_TX_MIN_PKT_LEN ||
-			m->data_len > max_frame_size) {
+		if (m->pkt_len < IAVF_TX_MIN_PKT_LEN) {
 			rte_errno = EINVAL;
-			PMD_DRV_LOG(ERR, "INVALID mbuf: bad data_len=[%hu]", m->data_len);
 			return i;
 		}
 
-- 
2.41.0

[PATCH 2/2] net/ice: fix TSO with big segments

[David Marchand]

Packets to be segmented with TSO are usually larger than MTU.
Plus, a single segment for the whole packet may be used: in OVS case,
an external rte_malloc'd buffer is used for packets received
from vhost-user ports.

Before this fix, TSO packets were dropped by net/ice with the following
message:
2023-09-18T13:34:31.064Z|00020|dpdk(pmd-c31/id:22)|ERR|ice_prep_pkts():
	INVALID mbuf: bad data_len=[2962]

Remove the check on data_len.

Besides, logging an error level message in a datapath function may
slow down the whole application. It is better not to log anything.

Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Note: there may be some followup patch later, as some additional check
has been added in ice_prep_pkts.
For context, see: http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3OqpNc5usTt3Rw@mail.gmail.com/T/#u

---
 drivers/net/ice/ice_rxtx.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 64c4486b4b..80c4284200 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	int i, ret;
 	uint64_t ol_flags;
 	struct rte_mbuf *m;
-	struct ice_tx_queue *txq = tx_queue;
-	struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
-	uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
 
 	for (i = 0; i < nb_pkts; i++) {
 		m = tx_pkts[i];
@@ -3704,11 +3701,8 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			return i;
 		}
 
-		/* check the data_len in mbuf */
-		if (m->data_len < ICE_TX_MIN_PKT_LEN ||
-			m->data_len > max_frame_size) {
+		if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
 			rte_errno = EINVAL;
-			PMD_DRV_LOG(ERR, "INVALID mbuf: bad data_len=[%hu]", m->data_len);
 			return i;
 		}
 
-- 
2.41.0

RE: [PATCH 2/2] net/ice: fix TSO with big segments

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, September 19, 2023 10:05 PM
> To: dev@dpdk.org
> Cc: ktraynor@redhat.com; mkp@redhat.com; dexia.li@jaguarmicro.com;
> stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> Subject: [PATCH 2/2] net/ice: fix TSO with big segments
> 
> Packets to be segmented with TSO are usually larger than MTU.
> Plus, a single segment for the whole packet may be used: in OVS case, an
> external rte_malloc'd buffer is used for packets received from vhost-user
> ports.
> 
> Before this fix, TSO packets were dropped by net/ice with the following
> message:
> 2023-09-18T13:34:31.064Z|00020|dpdk(pmd-
> c31/id:22)|ERR|ice_prep_pkts():
> 	INVALID mbuf: bad data_len=[2962]
> 
> Remove the check on data_len.
> 
> Besides, logging an error level message in a datapath function may slow
> down the whole application. It is better not to log anything.
> 
> Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Note: there may be some followup patch later, as some additional check has
> been added in ice_prep_pkts.
> For context, see:
> http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3
> OqpNc5usTt3Rw@mail.gmail.com/T/#u
> 
> ---
>  drivers/net/ice/ice_rxtx.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index
> 64c4486b4b..80c4284200 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
>  	int i, ret;
>  	uint64_t ol_flags;
>  	struct rte_mbuf *m;
> -	struct ice_tx_queue *txq = tx_queue;
> -	struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
> -	uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
> 
>  	for (i = 0; i < nb_pkts; i++) {
>  		m = tx_pkts[i];
> @@ -3704,11 +3701,8 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
>  			return i;
>  		}
> 
> -		/* check the data_len in mbuf */
> -		if (m->data_len < ICE_TX_MIN_PKT_LEN ||
> -			m->data_len > max_frame_size) {
> +		if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {

+1 

>  			rte_errno = EINVAL;
> -			PMD_DRV_LOG(ERR, "INVALID mbuf: bad
> data_len=[%hu]", m->data_len);

is it still worth to keep a debug level log here ? and it's better to unify the logging method in the same function.

>  			return i;
>  		}
> 
> --
> 2.41.0

Re: [PATCH 2/2] net/ice: fix TSO with big segments

[David Marchand]

On Thu, Sep 21, 2023 at 7:48 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, September 19, 2023 10:05 PM
> > To: dev@dpdk.org
> > Cc: ktraynor@redhat.com; mkp@redhat.com; dexia.li@jaguarmicro.com;
> > stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> > Subject: [PATCH 2/2] net/ice: fix TSO with big segments
> >
> > Packets to be segmented with TSO are usually larger than MTU.
> > Plus, a single segment for the whole packet may be used: in OVS case, an
> > external rte_malloc'd buffer is used for packets received from vhost-user
> > ports.
> >
> > Before this fix, TSO packets were dropped by net/ice with the following
> > message:
> > 2023-09-18T13:34:31.064Z|00020|dpdk(pmd-
> > c31/id:22)|ERR|ice_prep_pkts():
> >       INVALID mbuf: bad data_len=[2962]
> >
> > Remove the check on data_len.
> >
> > Besides, logging an error level message in a datapath function may slow
> > down the whole application. It is better not to log anything.
> >
> > Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > Note: there may be some followup patch later, as some additional check has
> > been added in ice_prep_pkts.
> > For context, see:
> > http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3
> > OqpNc5usTt3Rw@mail.gmail.com/T/#u
> >
> > ---
> >  drivers/net/ice/ice_rxtx.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index
> > 64c4486b4b..80c4284200 100644
> > --- a/drivers/net/ice/ice_rxtx.c
> > +++ b/drivers/net/ice/ice_rxtx.c
> > @@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> >       int i, ret;
> >       uint64_t ol_flags;
> >       struct rte_mbuf *m;
> > -     struct ice_tx_queue *txq = tx_queue;
> > -     struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
> > -     uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
> >
> >       for (i = 0; i < nb_pkts; i++) {
> >               m = tx_pkts[i];
> > @@ -3704,11 +3701,8 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> >                       return i;
> >               }
> >
> > -             /* check the data_len in mbuf */
> > -             if (m->data_len < ICE_TX_MIN_PKT_LEN ||
> > -                     m->data_len > max_frame_size) {
> > +             if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
>
> +1
>
> >                       rte_errno = EINVAL;
> > -                     PMD_DRV_LOG(ERR, "INVALID mbuf: bad
> > data_len=[%hu]", m->data_len);
>
> is it still worth to keep a debug level log here ? and it's better to unify the logging method in the same function.

Logging data_len is incorrect.

There are no log in other drivers.

If anything, the logging may happen in the application invoking
rte_eth_tx_prepare.

I am against keeping those logs.


-- 
David Marchand

RE: [PATCH 2/2] net/ice: fix TSO with big segments

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, September 21, 2023 1:55 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; ktraynor@redhat.com; mkp@redhat.com;
> dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments
> 
> On Thu, Sep 21, 2023 at 7:48 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Tuesday, September 19, 2023 10:05 PM
> > > To: dev@dpdk.org
> > > Cc: ktraynor@redhat.com; mkp@redhat.com; dexia.li@jaguarmicro.com;
> > > stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> > > Subject: [PATCH 2/2] net/ice: fix TSO with big segments
> > >
> > > Packets to be segmented with TSO are usually larger than MTU.
> > > Plus, a single segment for the whole packet may be used: in OVS
> > > case, an external rte_malloc'd buffer is used for packets received
> > > from vhost-user ports.
> > >
> > > Before this fix, TSO packets were dropped by net/ice with the
> > > following
> > > message:
> > > 2023-09-18T13:34:31.064Z|00020|dpdk(pmd-
> > > c31/id:22)|ERR|ice_prep_pkts():
> > >       INVALID mbuf: bad data_len=[2962]
> > >
> > > Remove the check on data_len.
> > >
> > > Besides, logging an error level message in a datapath function may
> > > slow down the whole application. It is better not to log anything.
> > >
> > > Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > > Note: there may be some followup patch later, as some additional
> > > check has been added in ice_prep_pkts.
> > > For context, see:
> > >
> http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3
> > > OqpNc5usTt3Rw@mail.gmail.com/T/#u
> > >
> > > ---
> > >  drivers/net/ice/ice_rxtx.c | 8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > > index
> > > 64c4486b4b..80c4284200 100644
> > > --- a/drivers/net/ice/ice_rxtx.c
> > > +++ b/drivers/net/ice/ice_rxtx.c
> > > @@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > >       int i, ret;
> > >       uint64_t ol_flags;
> > >       struct rte_mbuf *m;
> > > -     struct ice_tx_queue *txq = tx_queue;
> > > -     struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
> > > -     uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
> > >
> > >       for (i = 0; i < nb_pkts; i++) {
> > >               m = tx_pkts[i];
> > > @@ -3704,11 +3701,8 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > >                       return i;
> > >               }
> > >
> > > -             /* check the data_len in mbuf */
> > > -             if (m->data_len < ICE_TX_MIN_PKT_LEN ||
> > > -                     m->data_len > max_frame_size) {
> > > +             if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
> >
> > +1
> >
> > >                       rte_errno = EINVAL;
> > > -                     PMD_DRV_LOG(ERR, "INVALID mbuf: bad
> > > data_len=[%hu]", m->data_len);
> >
> > is it still worth to keep a debug level log here ? and it's better to unify the
> logging method in the same function.
> 
> Logging data_len is incorrect.
> 
> There are no log in other drivers.
> 
> If anything, the logging may happen in the application invoking
> rte_eth_tx_prepare.
> 
> I am against keeping those logs.


I'm still hesitant to remove these logs until we find a way to provide equivalent diagnostic information for users,  because similar request comes directly from some of our customers.

There could be several options to consider, such as counting the errors and reporting them in xstats or introducing devargs for on purpose diagnostic routine with log printing.

How about split the issue into two parts. One part focuses on fixing the 'data_len' check and keeping the log in sync (this patch target to), while the other part deals with removing the error log and implementing diagnostics. 

 
> 
> 
> --
> David Marchand

Re: [PATCH 2/2] net/ice: fix TSO with big segments

[David Marchand]

On Thu, Sep 21, 2023 at 12:43 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, September 21, 2023 1:55 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; ktraynor@redhat.com; mkp@redhat.com;
> > dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
> > <qiming.yang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> > Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments
> >
> > On Thu, Sep 21, 2023 at 7:48 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: David Marchand <david.marchand@redhat.com>
> > > > Sent: Tuesday, September 19, 2023 10:05 PM
> > > > To: dev@dpdk.org
> > > > Cc: ktraynor@redhat.com; mkp@redhat.com; dexia.li@jaguarmicro.com;
> > > > stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> > > > Subject: [PATCH 2/2] net/ice: fix TSO with big segments
> > > >
> > > > Packets to be segmented with TSO are usually larger than MTU.
> > > > Plus, a single segment for the whole packet may be used: in OVS
> > > > case, an external rte_malloc'd buffer is used for packets received
> > > > from vhost-user ports.
> > > >
> > > > Before this fix, TSO packets were dropped by net/ice with the
> > > > following
> > > > message:
> > > > 2023-09-18T13:34:31.064Z|00020|dpdk(pmd-
> > > > c31/id:22)|ERR|ice_prep_pkts():
> > > >       INVALID mbuf: bad data_len=[2962]
> > > >
> > > > Remove the check on data_len.
> > > >
> > > > Besides, logging an error level message in a datapath function may
> > > > slow down the whole application. It is better not to log anything.
> > > >
> > > > Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > > Note: there may be some followup patch later, as some additional
> > > > check has been added in ice_prep_pkts.
> > > > For context, see:
> > > >
> > http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3
> > > > OqpNc5usTt3Rw@mail.gmail.com/T/#u
> > > >
> > > > ---
> > > >  drivers/net/ice/ice_rxtx.c | 8 +-------
> > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > > > index
> > > > 64c4486b4b..80c4284200 100644
> > > > --- a/drivers/net/ice/ice_rxtx.c
> > > > +++ b/drivers/net/ice/ice_rxtx.c
> > > > @@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > > struct rte_mbuf **tx_pkts,
> > > >       int i, ret;
> > > >       uint64_t ol_flags;
> > > >       struct rte_mbuf *m;
> > > > -     struct ice_tx_queue *txq = tx_queue;
> > > > -     struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
> > > > -     uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
> > > >
> > > >       for (i = 0; i < nb_pkts; i++) {
> > > >               m = tx_pkts[i];
> > > > @@ -3704,11 +3701,8 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > > struct rte_mbuf **tx_pkts,
> > > >                       return i;
> > > >               }
> > > >
> > > > -             /* check the data_len in mbuf */
> > > > -             if (m->data_len < ICE_TX_MIN_PKT_LEN ||
> > > > -                     m->data_len > max_frame_size) {
> > > > +             if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
> > >
> > > +1
> > >
> > > >                       rte_errno = EINVAL;
> > > > -                     PMD_DRV_LOG(ERR, "INVALID mbuf: bad
> > > > data_len=[%hu]", m->data_len);
> > >
> > > is it still worth to keep a debug level log here ? and it's better to unify the
> > logging method in the same function.
> >
> > Logging data_len is incorrect.
> >
> > There are no log in other drivers.
> >
> > If anything, the logging may happen in the application invoking
> > rte_eth_tx_prepare.
> >
> > I am against keeping those logs.
>
>
> I'm still hesitant to remove these logs until we find a way to provide equivalent diagnostic information for users,  because similar request comes directly from some of our customers.
>
> There could be several options to consider, such as counting the errors and reporting them in xstats or introducing devargs for on purpose diagnostic routine with log printing.

This check indicates a programmatic error, in a datapath function.
Keeping some log here while it could be triggered with packets is scary.


Thinking about some xstats, what makes this check on the min packet
length different from other checks in this helper?

If we added a xstats for this check, we would have a super specialised
counter for only this driver;
And nobody would be able to make some sense of it without reading this
driver code.


-- 
David Marchand

RE: [PATCH 2/2] net/ice: fix TSO with big segments

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, September 25, 2023 6:30 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; ktraynor@redhat.com; mkp@redhat.com;
> dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
> <qiming.yang@intel.com>
> Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments
> 
> On Thu, Sep 21, 2023 at 12:43 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Thursday, September 21, 2023 1:55 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; ktraynor@redhat.com; mkp@redhat.com;
> > > dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
> > > <qiming.yang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> > > Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments
> > >
> > > On Thu, Sep 21, 2023 at 7:48 AM Zhang, Qi Z <qi.z.zhang@intel.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: David Marchand <david.marchand@redhat.com>
> > > > > Sent: Tuesday, September 19, 2023 10:05 PM
> > > > > To: dev@dpdk.org
> > > > > Cc: ktraynor@redhat.com; mkp@redhat.com;
> > > > > dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
> > > > > <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > > > > Kevin Liu <kevinx.liu@intel.com>
> > > > > Subject: [PATCH 2/2] net/ice: fix TSO with big segments
> > > > >
> > > > > Packets to be segmented with TSO are usually larger than MTU.
> > > > > Plus, a single segment for the whole packet may be used: in OVS
> > > > > case, an external rte_malloc'd buffer is used for packets
> > > > > received from vhost-user ports.
> > > > >
> > > > > Before this fix, TSO packets were dropped by net/ice with the
> > > > > following
> > > > > message:
> > > > > 2023-09-18T13:34:31.064Z|00020|dpdk(pmd-
> > > > > c31/id:22)|ERR|ice_prep_pkts():
> > > > >       INVALID mbuf: bad data_len=[2962]
> > > > >
> > > > > Remove the check on data_len.
> > > > >
> > > > > Besides, logging an error level message in a datapath function
> > > > > may slow down the whole application. It is better not to log anything.
> > > > >
> > > > > Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> > > > > Cc: stable@dpdk.org
> > > > >
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > > ---
> > > > > Note: there may be some followup patch later, as some additional
> > > > > check has been added in ice_prep_pkts.
> > > > > For context, see:
> > > > >
> > >
> http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3
> > > > > OqpNc5usTt3Rw@mail.gmail.com/T/#u
> > > > >
> > > > > ---
> > > > >  drivers/net/ice/ice_rxtx.c | 8 +-------
> > > > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ice/ice_rxtx.c
> > > > > b/drivers/net/ice/ice_rxtx.c index
> > > > > 64c4486b4b..80c4284200 100644
> > > > > --- a/drivers/net/ice/ice_rxtx.c
> > > > > +++ b/drivers/net/ice/ice_rxtx.c
> > > > > @@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void
> *tx_queue,
> > > > > struct rte_mbuf **tx_pkts,
> > > > >       int i, ret;
> > > > >       uint64_t ol_flags;
> > > > >       struct rte_mbuf *m;
> > > > > -     struct ice_tx_queue *txq = tx_queue;
> > > > > -     struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
> > > > > -     uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
> > > > >
> > > > >       for (i = 0; i < nb_pkts; i++) {
> > > > >               m = tx_pkts[i];
> > > > > @@ -3704,11 +3701,8 @@ ice_prep_pkts(__rte_unused void
> > > > > *tx_queue, struct rte_mbuf **tx_pkts,
> > > > >                       return i;
> > > > >               }
> > > > >
> > > > > -             /* check the data_len in mbuf */
> > > > > -             if (m->data_len < ICE_TX_MIN_PKT_LEN ||
> > > > > -                     m->data_len > max_frame_size) {
> > > > > +             if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
> > > >
> > > > +1
> > > >
> > > > >                       rte_errno = EINVAL;
> > > > > -                     PMD_DRV_LOG(ERR, "INVALID mbuf: bad
> > > > > data_len=[%hu]", m->data_len);
> > > >
> > > > is it still worth to keep a debug level log here ? and it's better
> > > > to unify the
> > > logging method in the same function.
> > >
> > > Logging data_len is incorrect.
> > >
> > > There are no log in other drivers.
> > >
> > > If anything, the logging may happen in the application invoking
> > > rte_eth_tx_prepare.
> > >
> > > I am against keeping those logs.
> >
> >
> > I'm still hesitant to remove these logs until we find a way to provide
> equivalent diagnostic information for users,  because similar request comes
> directly from some of our customers.
> >
> > There could be several options to consider, such as counting the errors and
> reporting them in xstats or introducing devargs for on purpose diagnostic
> routine with log printing.
> 
> This check indicates a programmatic error, in a datapath function.
> Keeping some log here while it could be triggered with packets is scary.

Its on purpose, user should be aware of this limitation, it is still helps if the traffic is not busy.

> 
> 
> Thinking about some xstats, what makes this check on the min packet length
> different from other checks in this helper?

I agree that the current implementation lacks consistency in log printing. 
but, if this patch is intended to address not only the data_len check but also the removal of log printing, it should remove all log entries. Otherwise, we should consider splitting it into two separate patches.

Btw, we have a new design to provide a more comprehensive diagnostic solution which will not rely on tx_pkt_prep. So, it is acceptable to remove these log entries. Would you mind submit v2 address above request?

> 
> If we added a xstats for this check, we would have a super specialised counter
> for only this driver; And nobody would be able to make some sense of it
> without reading this driver code.

Not sure, does xstats can be used to report vendor specific counters for diagnose purpose? At least, it's not a bad idea for me.

Regards
Qi

> 
> 
> --
> David Marchand

[PATCH v2 1/4] net/iavf: remove log from Tx prepare datapath function

[David Marchand]

iavf_prep_pkts reports to the application that the packet is invalid
(from the driver pov). Having a log message only in this branch is not
consistent with all other checks in this function and it may slow down
the application if such invalid packets are being sent continuously.

Fixes: 19ee91c6bd9a ("net/iavf: check illegal packet sizes")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_rxtx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index 0484988d13..21a06b8351 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -3669,7 +3669,6 @@ iavf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 		if (m->data_len < IAVF_TX_MIN_PKT_LEN ||
 			m->data_len > max_frame_size) {
 			rte_errno = EINVAL;
-			PMD_DRV_LOG(ERR, "INVALID mbuf: bad data_len=[%hu]", m->data_len);
 			return i;
 		}
 
-- 
2.41.0

[PATCH v2 2/4] net/iavf: fix TSO with big segments

[David Marchand]

Packets to be segmented with TSO are usually larger than MTU.
Plus, a single segment for the whole packet may be used: in OVS case,
an external rte_malloc'd buffer is used for packets received
from vhost-user ports.

Before this fix, TSO packets were dropped by net/iavf with the following
message:
2023-09-18T14:08:52.739Z|00610|dpdk(pmd-c31/id:11)|ERR|iavf_prep_pkts():
	INVALID mbuf: bad data_len=[2962]

Remove the check on data_len.

Fixes: 19ee91c6bd9a ("net/iavf: check illegal packet sizes")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- moved log removal in a separate patch,

---
 drivers/net/iavf/iavf_rxtx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
index 21a06b8351..c6ef6af1d8 100644
--- a/drivers/net/iavf/iavf_rxtx.c
+++ b/drivers/net/iavf/iavf_rxtx.c
@@ -3636,7 +3636,6 @@ iavf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	struct rte_mbuf *m;
 	struct iavf_tx_queue *txq = tx_queue;
 	struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
-	uint16_t max_frame_size = dev->data->mtu + IAVF_ETH_OVERHEAD;
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 	struct iavf_adapter *adapter = IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
 
@@ -3665,9 +3664,7 @@ iavf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			return i;
 		}
 
-		/* check the data_len in mbuf */
-		if (m->data_len < IAVF_TX_MIN_PKT_LEN ||
-			m->data_len > max_frame_size) {
+		if (m->pkt_len < IAVF_TX_MIN_PKT_LEN) {
 			rte_errno = EINVAL;
 			return i;
 		}
-- 
2.41.0

[PATCH v2 3/4] net/ice: remove log from Tx prepare datapath function

[David Marchand]

ice_prep_pkts reports to the application that the packet is invalid
(from the driver pov). Having a log message in those branches is not
consistent with all other checks in this function and it may slow down
the application if such invalid packets are being sent continuously.

Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
Fixes: 688cb2f2c61e ("net/ice: fix scalar Tx path segment")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ice/ice_rxtx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index e07c6d1f15..d5513e9e93 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -3708,7 +3708,6 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 		if (m->data_len < ICE_TX_MIN_PKT_LEN ||
 			m->data_len > max_frame_size) {
 			rte_errno = EINVAL;
-			PMD_DRV_LOG(ERR, "INVALID mbuf: bad data_len=[%hu]", m->data_len);
 			return i;
 		}
 
@@ -3727,7 +3726,6 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 
 		if (ice_check_empty_mbuf(m) != 0) {
 			rte_errno = EINVAL;
-			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf data_len=[0]");
 			return i;
 		}
 	}
-- 
2.41.0

[PATCH v2 4/4] net/ice: fix TSO with big segments

[David Marchand]

Packets to be segmented with TSO are usually larger than MTU.
Plus, a single segment for the whole packet may be used: in OVS case,
an external rte_malloc'd buffer is used for packets received
from vhost-user ports.

Before this fix, TSO packets were dropped by net/ice with the following
message:
2023-09-18T13:34:31.064Z|00020|dpdk(pmd-c31/id:22)|ERR|ice_prep_pkts():
	INVALID mbuf: bad data_len=[2962]

Remove the check on data_len.

Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Note: I am still waiting for feedback and there may be some followup patch
later wrt ice_prep_pkts.
For context, see: http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3OqpNc5usTt3Rw@mail.gmail.com/T/#u

Changes since v1:
- moved log removal in a separate patch,

---
 drivers/net/ice/ice_rxtx.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index d5513e9e93..ee9cb7b955 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	int i, ret;
 	uint64_t ol_flags;
 	struct rte_mbuf *m;
-	struct ice_tx_queue *txq = tx_queue;
-	struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
-	uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
 
 	for (i = 0; i < nb_pkts; i++) {
 		m = tx_pkts[i];
@@ -3704,9 +3701,7 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			return i;
 		}
 
-		/* check the data_len in mbuf */
-		if (m->data_len < ICE_TX_MIN_PKT_LEN ||
-			m->data_len > max_frame_size) {
+		if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
 			rte_errno = EINVAL;
 			return i;
 		}
-- 
2.41.0

RE: [PATCH v2 4/4] net/ice: fix TSO with big segments

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, September 27, 2023 5:42 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Kevin Liu <kevinx.liu@intel.com>
> Subject: [PATCH v2 4/4] net/ice: fix TSO with big segments
> 
> Packets to be segmented with TSO are usually larger than MTU.
> Plus, a single segment for the whole packet may be used: in OVS case, an
> external rte_malloc'd buffer is used for packets received from vhost-user
> ports.
> 
> Before this fix, TSO packets were dropped by net/ice with the following
> message:
> 2023-09-18T13:34:31.064Z|00020|dpdk(pmd-
> c31/id:22)|ERR|ice_prep_pkts():
> 	INVALID mbuf: bad data_len=[2962]
> 
> Remove the check on data_len.
> 
> Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Note: I am still waiting for feedback and there may be some followup patch
> later wrt ice_prep_pkts.
> For context, see:
> http://inbox.dpdk.org/dev/CAJFAV8yOa3ShkVdEXHfnmOEmUTwV3e75Bu9U3
> OqpNc5usTt3Rw@mail.gmail.com/T/#u
> 
> Changes since v1:
> - moved log removal in a separate patch,
> 
> ---
>  drivers/net/ice/ice_rxtx.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index
> d5513e9e93..ee9cb7b955 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -3685,9 +3685,6 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
>  	int i, ret;
>  	uint64_t ol_flags;
>  	struct rte_mbuf *m;
> -	struct ice_tx_queue *txq = tx_queue;
> -	struct rte_eth_dev *dev = &rte_eth_devices[txq->port_id];
> -	uint16_t max_frame_size = dev->data->mtu + ICE_ETH_OVERHEAD;
> 
>  	for (i = 0; i < nb_pkts; i++) {
>  		m = tx_pkts[i];
> @@ -3704,9 +3701,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
>  			return i;
>  		}
> 
> -		/* check the data_len in mbuf */
> -		if (m->data_len < ICE_TX_MIN_PKT_LEN ||
> -			m->data_len > max_frame_size) {
> +		if (m->pkt_len < ICE_TX_MIN_PKT_LEN) {
>  			rte_errno = EINVAL;
>  			return i;
>  		}
> --
> 2.41.0

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

Applied to dpdk-next-net-intel.

Thanks
Qi