-----Original Message-----
From: David Marchand <redacted>
Sent: Monday, September 25, 2023 6:30 PM
To: Zhang, Qi Z <redacted>
Cc: dev@dpdk.org; ktraynor@redhat.com; mkp@redhat.com;
dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
[off-list ref]
Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments

On Thu, Sep 21, 2023 at 12:43 PM Zhang, Qi Z [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: David Marchand <redacted>
Sent: Thursday, September 21, 2023 1:55 PM
To: Zhang, Qi Z <redacted>
Cc: dev@dpdk.org; ktraynor@redhat.com; mkp@redhat.com;
dexia.li@jaguarmicro.com; stable@dpdk.org; Yang, Qiming
[off-list ref]; Kevin Liu [off-list ref]
Subject: Re: [PATCH 2/2] net/ice: fix TSO with big segments

On Thu, Sep 21, 2023 at 7:48 AM Zhang, Qi Z [off-list ref]

wrote:

quoted

quoted

quoted

quoted

-----Original Message-----
From: David Marchand <redacted>
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
[off-list ref]; Zhang, Qi Z [off-list ref];
Kevin Liu [off-list ref]
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 <redacted>
---
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

quoted

quoted

quoted

quoted

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,

quoted

quoted

quoted

quoted

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

quoted

                      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.

quoted

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