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

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand at redhat.com>
> Sent: Thursday, September 21, 2023 1:55 PM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>
> Cc: dev at dpdk.org; ktraynor at redhat.com; mkp at redhat.com;
> dexia.li at jaguarmicro.com; stable at dpdk.org; Yang, Qiming
> <qiming.yang at intel.com>; Kevin Liu <kevinx.liu at 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 at intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand at redhat.com>
> > > Sent: Tuesday, September 19, 2023 10:05 PM
> > > To: dev at dpdk.org
> > > Cc: ktraynor at redhat.com; mkp at redhat.com; dexia.li at jaguarmicro.com;
> > > stable at dpdk.org; Yang, Qiming <qiming.yang at intel.com>; Zhang, Qi Z
> > > <qi.z.zhang at intel.com>; Kevin Liu <kevinx.liu at 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 at dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand at 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 at 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