From: David Marchand <david.marchand@redhat.com>
To: "Zhang, Qi Z" <qi.z.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"ktraynor@redhat.com" <ktraynor@redhat.com>,
"mkp@redhat.com" <mkp@redhat.com>,
"dexia.li@jaguarmicro.com" <dexia.li@jaguarmicro.com>,
"stable@dpdk.org" <stable@dpdk.org>,
"Yang, Qiming" <qiming.yang@intel.com>
Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments
Date: Mon, 25 Sep 2023 12:30:27 +0200 [thread overview]
Message-ID: <CAJFAV8yzk76eqM02i-=Dq8uJuhSfaBqJpnKLS4F=PF2yezWDLQ@mail.gmail.com> (raw)
In-Reply-To: <DM4PR11MB59941A24731B37BCC5A7E54DD7F8A@DM4PR11MB5994.namprd11.prod.outlook.com>
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
next prev parent reply other threads:[~2023-09-25 10:30 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 14:04 [PATCH 1/2] net/iavf: " David Marchand
2023-09-19 14:04 ` [PATCH 2/2] net/ice: " David Marchand
2023-09-21 5:48 ` Zhang, Qi Z
2023-09-21 5:55 ` David Marchand
2023-09-21 10:42 ` Zhang, Qi Z
2023-09-25 10:30 ` David Marchand [this message]
2023-09-27 4:05 ` Zhang, Qi Z
2023-09-27 9:41 ` [PATCH v2 1/4] net/iavf: remove log from Tx prepare datapath function David Marchand
2023-09-27 9:41 ` [PATCH v2 2/4] net/iavf: fix TSO with big segments David Marchand
2023-09-27 9:41 ` [PATCH v2 3/4] net/ice: remove log from Tx prepare datapath function David Marchand
2023-09-27 9:41 ` [PATCH v2 4/4] net/ice: fix TSO with big segments David Marchand
2023-09-27 12:36 ` Zhang, Qi Z
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJFAV8yzk76eqM02i-=Dq8uJuhSfaBqJpnKLS4F=PF2yezWDLQ@mail.gmail.com' \
--to=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=dexia.li@jaguarmicro.com \
--cc=ktraynor@redhat.com \
--cc=mkp@redhat.com \
--cc=qi.z.zhang@intel.com \
--cc=qiming.yang@intel.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).