Hello,
Replying on this old thread, as it seems no in depth review was done
but I need more info before fixing a bug added by ccf33dccf7aa
("net/ice: check illegal packet sizes").
On Fri, Nov 11, 2022 at 9:26 AM Mingjin Ye <mingjinx...@intel.com> wrote:
>
> The scalar Tx path would send empty buffer that causes the Tx queue to
> overflow.
This is too vague.
Please describe the exact issue.
Is it a sw (read net/ice driver) bug? a hw bug?
>
> This patch adds the last buffer length judgment in tx_prepare to fix this
This reads odd.
The added code will stop and return an error when the last segment
*that is evaluated by the code* is empty.
But on the other hand, it is easier to understand that the check is to
catch the *first* empty segment of a packet.
> issue, rte_errno will be set to EINVAL and returned if the last buffer is
> empty.
>
> Fixes: 17c7d0f9d6a4 ("net/ice: support basic Rx/Tx")
> Fixes: ccf33dccf7aa ("net/ice: check illegal packet sizes")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx...@intel.com>
>
> v3:
> * delete unused variable.
> * Full detection of mbufs.
> ---
> drivers/net/ice/ice_rxtx.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> index e6ddd2513d..9017353943 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -3643,6 +3643,22 @@ ice_set_tx_function_flag(struct rte_eth_dev *dev,
> struct ice_tx_queue *txq)
> #define ICE_MIN_TSO_MSS 64
> #define ICE_MAX_TSO_MSS 9728
> #define ICE_MAX_TSO_FRAME_SIZE 262144
> +
> +/*Check for empty mbuf*/
Missing spaces.
> +static inline uint16_t
> +ice_check_empty_mbuf(struct rte_mbuf *tx_pkt)
This function name is confusing.
In dpdk, a mbuf can be both a segment descriptor (part of a packet) or
a packet descriptor (in which case, it overlaps the first segment
descriptor).
So it is better to be explicit with "segment" or "packet" when
referring to a mbuf.
> +{
> + struct rte_mbuf *txd = tx_pkt;
> +
> + while (txd != NULL) {
> + if (txd->data_len == 0)
> + return -1;
> + txd = txd->next;
> + }
There is nothing in the API that says that an empty segment is faulty.
With the current description, I understand that any empty segment
could lead to a bug, regardless of asking offloads (which
ice_prep_pkts is all about).
So a more valid fix is probably to skip empty segments in the tx
handler function(s) and not reject packets in ice_prep_pkts.
Another option could be to fix this "overflow" mentionned in the commitlog.
Looking fwd to explanations.
> +
> + return 0;
> +}
> +
> uint16_t
> ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
> uint16_t nb_pkts)
--
David Marchand
