[PATCH] net/ice: fix scalar Rx and Tx path segment

[PATCH] net/ice: fix scalar Rx and Tx path segment

[Mingjin Ye]

CRC is stripped by the hardware in the scattered Rx path. If the last
buffer packet length is '0', the scalar Tx path would send empty buffer
that causes the Tx queue to overflow.

This patch adds a judgment for the last buffer length to fix this issue,
so that it would free the mbuf associated to the last one if the last
buffer is empty.

Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/ice/ice_rxtx.c | 53 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0a2b0376ac..4f9c29eaf6 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
 			} else
 				rxm->data_len = (uint16_t)(rx_packet_len -
 							   RTE_ETHER_CRC_LEN);
+		} else if (rx_packet_len == 0) {
+			rte_pktmbuf_free_seg(rxm);
+			first_seg->nb_segs--;
+			last_seg->next = NULL;
 		}
 
 		first_seg->port = rxq->port_id;
@@ -2903,6 +2907,35 @@ ice_calc_pkt_desc(struct rte_mbuf *tx_pkt)
 	return count;
 }
 
+/*Check the number of valid mbufs and free the invalid mbufs*/
+static inline uint16_t
+ice_check_mbuf(struct rte_mbuf *tx_pkt)
+{
+	struct rte_mbuf *txd = tx_pkt;
+	struct rte_mbuf *txd_removal = NULL;
+	struct rte_mbuf *txd_pre = NULL;
+	uint16_t count = 0;
+	uint16_t removal = 0;
+
+	while (txd != NULL) {
+		if (removal == 1 || txd->data_len == 0) {
+			txd_removal = txd;
+			txd = txd->next;
+			if (removal == 0) {
+				removal = 1;
+				txd_pre->next = NULL;
+			}
+			rte_pktmbuf_free_seg(txd_removal);
+		} else {
+			++count;
+			txd_pre = txd;
+			txd = txd->next;
+		}
+	}
+
+	return count;
+}
+
 uint16_t
 ice_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 {
@@ -2960,11 +2993,27 @@ ice_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		 * the mbuf data size exceeds max data size that hw allows
 		 * per tx desc.
 		 */
-		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG)
+		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
 			nb_used = (uint16_t)(ice_calc_pkt_desc(tx_pkt) +
 					     nb_ctx);
-		else
+		} else {
+			nb_used = ice_check_mbuf(tx_pkt);
+			if (nb_used == 0) {
+				PMD_TX_LOG(ERR,
+				"Check packets is empty "
+				"(port=%d queue=%d)\n",
+				txq->port_id, txq->queue_id);
+				continue;
+			} else if (nb_used < tx_pkt->nb_segs) {
+				PMD_TX_LOG(WRINING,
+				"Check packets valid num ="
+				"%4u total num = %4u (port=%d queue=%d)\n",
+				nb_used, tx_pkt->nb_segs, txq->port_id, txq->queue_id);
+				tx_pkt->nb_segs = nb_used;
+			}
 			nb_used = (uint16_t)(tx_pkt->nb_segs + nb_ctx);
+		}
+
 		tx_last = (uint16_t)(tx_id + nb_used - 1);
 
 		/* Circular ring */
-- 
2.34.1

RE: [PATCH] net/ice: fix scalar Rx and Tx path segment

[Xu, Ke1]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Friday, November 4, 2022 1:21 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhou, YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> <mingjinx.ye@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>
> Subject: [PATCH] net/ice: fix scalar Rx and Tx path segment
> 
> CRC is stripped by the hardware in the scattered Rx path. If the last buffer
> packet length is '0', the scalar Tx path would send empty buffer that causes
> the Tx queue to overflow.
> 
> This patch adds a judgment for the last buffer length to fix this issue, so that
> it would free the mbuf associated to the last one if the last buffer is empty.
> 
> Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>


V1 patch verified and passed. Remains some compile errors to fix for redhat.

Tested-by: Ke Xu <ke1.xu@intel.com>


> ---
>  drivers/net/ice/ice_rxtx.c | 53 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 

[PATCH v2] net/ice: fix scalar Rx and Tx path segment

[Mingjin Ye]

CRC is stripped by the hardware in the scattered Rx path. If the last
buffer packet length is '0', the scalar Tx path would send empty buffer
that causes the Tx queue to overflow.

This patch adds a judgment for the last buffer length to fix this issue,
so that it would free the mbuf associated to the last one if the last
buffer is empty.

Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

v2:
	* Fix log level in ice_rxtx.c source file.
---
 drivers/net/ice/ice_rxtx.c | 53 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0a2b0376ac..b181f66aad 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
 			} else
 				rxm->data_len = (uint16_t)(rx_packet_len -
 							   RTE_ETHER_CRC_LEN);
+		} else if (rx_packet_len == 0) {
+			rte_pktmbuf_free_seg(rxm);
+			first_seg->nb_segs--;
+			last_seg->next = NULL;
 		}
 
 		first_seg->port = rxq->port_id;
@@ -2903,6 +2907,35 @@ ice_calc_pkt_desc(struct rte_mbuf *tx_pkt)
 	return count;
 }
 
+/*Check the number of valid mbufs and free the invalid mbufs*/
+static inline uint16_t
+ice_check_mbuf(struct rte_mbuf *tx_pkt)
+{
+	struct rte_mbuf *txd = tx_pkt;
+	struct rte_mbuf *txd_removal = NULL;
+	struct rte_mbuf *txd_pre = NULL;
+	uint16_t count = 0;
+	uint16_t removal = 0;
+
+	while (txd != NULL) {
+		if (removal == 1 || txd->data_len == 0) {
+			txd_removal = txd;
+			txd = txd->next;
+			if (removal == 0) {
+				removal = 1;
+				txd_pre->next = NULL;
+			}
+			rte_pktmbuf_free_seg(txd_removal);
+		} else {
+			++count;
+			txd_pre = txd;
+			txd = txd->next;
+		}
+	}
+
+	return count;
+}
+
 uint16_t
 ice_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 {
@@ -2960,11 +2993,27 @@ ice_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		 * the mbuf data size exceeds max data size that hw allows
 		 * per tx desc.
 		 */
-		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG)
+		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
 			nb_used = (uint16_t)(ice_calc_pkt_desc(tx_pkt) +
 					     nb_ctx);
-		else
+		} else {
+			nb_used = ice_check_mbuf(tx_pkt);
+			if (nb_used == 0) {
+				PMD_TX_LOG(ERR,
+				"Check packets is empty "
+				"(port=%d queue=%d)\n",
+				txq->port_id, txq->queue_id);
+				continue;
+			} else if (nb_used < tx_pkt->nb_segs) {
+				PMD_TX_LOG(DEBUG,
+				"Check packets valid num ="
+				"%4u total num = %4u (port=%d queue=%d)\n",
+				nb_used, tx_pkt->nb_segs, txq->port_id, txq->queue_id);
+				tx_pkt->nb_segs = nb_used;
+			}
 			nb_used = (uint16_t)(tx_pkt->nb_segs + nb_ctx);
+		}
+
 		tx_last = (uint16_t)(tx_id + nb_used - 1);
 
 		/* Circular ring */
-- 
2.34.1

RE: [PATCH v2] net/ice: fix scalar Rx and Tx path segment

[Xu, Ke1]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Wednesday, November 9, 2022 8:56 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>
> Subject: [PATCH v2] net/ice: fix scalar Rx and Tx path segment
> 
> CRC is stripped by the hardware in the scattered Rx path. If the last buffer
> packet length is '0', the scalar Tx path would send empty buffer that causes
> the Tx queue to overflow.
> 
> This patch adds a judgment for the last buffer length to fix this issue, so that
> it would free the mbuf associated to the last one if the last buffer is empty.
> 
> Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Tested and passed.

Tested-by: Ke XU <ke1.xu@intel.com>


> 
> v2:
> 	* Fix log level in ice_rxtx.c source file.
> ---
>  drivers/net/ice/ice_rxtx.c | 53 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 

RE: [PATCH v2] net/ice: fix scalar Rx and Tx path segment

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Wednesday, November 9, 2022 8:56 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>
> Subject: [PATCH v2] net/ice: fix scalar Rx and Tx path segment
> 
> CRC is stripped by the hardware in the scattered Rx path. If the last buffer
> packet length is '0', the scalar Tx path would send empty buffer that causes
> the Tx queue to overflow.

Please separate this patch into two, one for Rx and one for Tx, they are independent.

For the Tx implementation, I think we can move them into tx_prepare where is place to check Tx violation.
 
> 
> This patch adds a judgment for the last buffer length to fix this issue, so that
> it would free the mbuf associated to the last one if the last buffer is empty.
> 
> Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> 
> v2:
> 	* Fix log level in ice_rxtx.c source file.
> ---
>  drivers/net/ice/ice_rxtx.c | 53 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index
> 0a2b0376ac..b181f66aad 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
>  			} else
>  				rxm->data_len = (uint16_t)(rx_packet_len -
> 
> RTE_ETHER_CRC_LEN);
> +		} else if (rx_packet_len == 0) {
> +			rte_pktmbuf_free_seg(rxm);
> +			first_seg->nb_segs--;
> +			last_seg->next = NULL;
>  		}
> 
>  		first_seg->port = rxq->port_id;
> @@ -2903,6 +2907,35 @@ ice_calc_pkt_desc(struct rte_mbuf *tx_pkt)
>  	return count;
>  }
> 
> +/*Check the number of valid mbufs and free the invalid mbufs*/ static
> +inline uint16_t ice_check_mbuf(struct rte_mbuf *tx_pkt) {
> +	struct rte_mbuf *txd = tx_pkt;
> +	struct rte_mbuf *txd_removal = NULL;
> +	struct rte_mbuf *txd_pre = NULL;
> +	uint16_t count = 0;
> +	uint16_t removal = 0;
> +
> +	while (txd != NULL) {
> +		if (removal == 1 || txd->data_len == 0) {
> +			txd_removal = txd;
> +			txd = txd->next;
> +			if (removal == 0) {
> +				removal = 1;
> +				txd_pre->next = NULL;
> +			}
> +			rte_pktmbuf_free_seg(txd_removal);
> +		} else {
> +			++count;
> +			txd_pre = txd;
> +			txd = txd->next;
> +		}
> +	}
> +
> +	return count;
> +}
> +
>  uint16_t
>  ice_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> { @@ -2960,11 +2993,27 @@ ice_xmit_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
>  		 * the mbuf data size exceeds max data size that hw allows
>  		 * per tx desc.
>  		 */
> -		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG)
> +		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
>  			nb_used = (uint16_t)(ice_calc_pkt_desc(tx_pkt) +
>  					     nb_ctx);
> -		else
> +		} else {
> +			nb_used = ice_check_mbuf(tx_pkt);
> +			if (nb_used == 0) {
> +				PMD_TX_LOG(ERR,
> +				"Check packets is empty "
> +				"(port=%d queue=%d)\n",
> +				txq->port_id, txq->queue_id);
> +				continue;
> +			} else if (nb_used < tx_pkt->nb_segs) {
> +				PMD_TX_LOG(DEBUG,
> +				"Check packets valid num ="
> +				"%4u total num = %4u (port=%d
> queue=%d)\n",
> +				nb_used, tx_pkt->nb_segs, txq->port_id, txq-
> >queue_id);
> +				tx_pkt->nb_segs = nb_used;
> +			}
>  			nb_used = (uint16_t)(tx_pkt->nb_segs + nb_ctx);
> +		}
> +
>  		tx_last = (uint16_t)(tx_id + nb_used - 1);
> 
>  		/* Circular ring */
> --
> 2.34.1

RE: [PATCH v2] net/ice: fix scalar Rx and Tx path segment

[Ye, MingjinX]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: 2022年11月10日 18:37
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>
> Subject: RE: [PATCH v2] net/ice: fix scalar Rx and Tx path segment
> 
> 
> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Wednesday, November 9, 2022 8:56 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Lu,
> > Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> > Li, Xiaoyun <xiaoyun.li@intel.com>; Ferruh Yigit
> > <ferruh.yigit@intel.com>
> > Subject: [PATCH v2] net/ice: fix scalar Rx and Tx path segment
> >
> > CRC is stripped by the hardware in the scattered Rx path. If the last
> > buffer packet length is '0', the scalar Tx path would send empty
> > buffer that causes the Tx queue to overflow.
> 
> Please separate this patch into two, one for Rx and one for Tx, they are
> independent.
> 
> For the Tx implementation, I think we can move them into tx_prepare where
> is place to check Tx violation.
Thanks for your suggestion, I will provide 2 new patches according to the new
scheme and promote it to the community.
> 
> >
> > This patch adds a judgment for the last buffer length to fix this
> > issue, so that it would free the mbuf associated to the last one if the last
> buffer is empty.
> >
> > Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> >
> > v2:
> > 	* Fix log level in ice_rxtx.c source file.
> > ---
> >  drivers/net/ice/ice_rxtx.c | 53
> > ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > index 0a2b0376ac..b181f66aad 100644
> > --- a/drivers/net/ice/ice_rxtx.c
> > +++ b/drivers/net/ice/ice_rxtx.c
> > @@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
> >  			} else
> >  				rxm->data_len = (uint16_t)(rx_packet_len -
> >
> > RTE_ETHER_CRC_LEN);
> > +		} else if (rx_packet_len == 0) {
> > +			rte_pktmbuf_free_seg(rxm);
> > +			first_seg->nb_segs--;
> > +			last_seg->next = NULL;
> >  		}
> >
> >  		first_seg->port = rxq->port_id;
> > @@ -2903,6 +2907,35 @@ ice_calc_pkt_desc(struct rte_mbuf *tx_pkt)
> >  	return count;
> >  }
> >
> > +/*Check the number of valid mbufs and free the invalid mbufs*/ static
> > +inline uint16_t ice_check_mbuf(struct rte_mbuf *tx_pkt) {
> > +	struct rte_mbuf *txd = tx_pkt;
> > +	struct rte_mbuf *txd_removal = NULL;
> > +	struct rte_mbuf *txd_pre = NULL;
> > +	uint16_t count = 0;
> > +	uint16_t removal = 0;
> > +
> > +	while (txd != NULL) {
> > +		if (removal == 1 || txd->data_len == 0) {
> > +			txd_removal = txd;
> > +			txd = txd->next;
> > +			if (removal == 0) {
> > +				removal = 1;
> > +				txd_pre->next = NULL;
> > +			}
> > +			rte_pktmbuf_free_seg(txd_removal);
> > +		} else {
> > +			++count;
> > +			txd_pre = txd;
> > +			txd = txd->next;
> > +		}
> > +	}
> > +
> > +	return count;
> > +}
> > +
> >  uint16_t
> >  ice_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t
> > nb_pkts) { @@ -2960,11 +2993,27 @@ ice_xmit_pkts(void *tx_queue,
> > struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> >  		 * the mbuf data size exceeds max data size that hw allows
> >  		 * per tx desc.
> >  		 */
> > -		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG)
> > +		if (ol_flags & RTE_MBUF_F_TX_TCP_SEG) {
> >  			nb_used = (uint16_t)(ice_calc_pkt_desc(tx_pkt) +
> >  					     nb_ctx);
> > -		else
> > +		} else {
> > +			nb_used = ice_check_mbuf(tx_pkt);
> > +			if (nb_used == 0) {
> > +				PMD_TX_LOG(ERR,
> > +				"Check packets is empty "
> > +				"(port=%d queue=%d)\n",
> > +				txq->port_id, txq->queue_id);
> > +				continue;
> > +			} else if (nb_used < tx_pkt->nb_segs) {
> > +				PMD_TX_LOG(DEBUG,
> > +				"Check packets valid num ="
> > +				"%4u total num = %4u (port=%d
> > queue=%d)\n",
> > +				nb_used, tx_pkt->nb_segs, txq->port_id,
> txq-
> > >queue_id);
> > +				tx_pkt->nb_segs = nb_used;
> > +			}
> >  			nb_used = (uint16_t)(tx_pkt->nb_segs + nb_ctx);
> > +		}
> > +
> >  		tx_last = (uint16_t)(tx_id + nb_used - 1);
> >
> >  		/* Circular ring */
> > --
> > 2.34.1

[PATCH v3 1/2] net/ice: fix scalar Rx path segment

[Mingjin Ye]

CRC is stripped by the hardware in the scattered Rx path. The last buffer
is invalid if it's packet length is zero.

This patch adds a judgment for the last buffer length to fix this issue,
it would free the mbuf associated to the last one if the last buffer is
empty.

Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/ice/ice_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0a2b0376ac..e6ddd2513d 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
 			} else
 				rxm->data_len = (uint16_t)(rx_packet_len -
 							   RTE_ETHER_CRC_LEN);
+		} else if (rx_packet_len == 0) {
+			rte_pktmbuf_free_seg(rxm);
+			first_seg->nb_segs--;
+			last_seg->next = NULL;
 		}
 
 		first_seg->port = rxq->port_id;
-- 
2.34.1

RE: [PATCH v3 1/2] net/ice: fix scalar Rx path segment

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Friday, November 11, 2022 8:04 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>
> Subject: [PATCH v3 1/2] net/ice: fix scalar Rx path segment
> 
> CRC is stripped by the hardware in the scattered Rx path. The last buffer is
> invalid if it's packet length is zero.
> 
> This patch adds a judgment for the last buffer length to fix this issue, it would
> free the mbuf associated to the last one if the last buffer is empty.
> 
> Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

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

Applied to dpdk-next-net-intel.

Thanks
Qi

[PATCH v3 2/2] net/ice: fix scalar Tx path segment

[Mingjin Ye]

The scalar Tx path would send empty buffer that causes the Tx queue to
overflow.

This patch adds the last buffer length judgment in tx_prepare to fix this
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: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/ice/ice_rxtx.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index e6ddd2513d..69358f6a3a 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 invalid mbuf*/
+static inline uint16_t
+ice_check_mbuf(struct rte_mbuf *tx_pkt)
+{
+	struct rte_mbuf *txd = tx_pkt;
+
+	while (txd != NULL) {
+		if (txd->data_len == 0)
+			return -1;
+		txd = txd->next;
+	}
+
+	return 0;
+}
+
 uint16_t
 ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	      uint16_t nb_pkts)
@@ -3653,6 +3669,7 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	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;
+	uint16_t nb_used;
 
 	for (i = 0; i < nb_pkts; i++) {
 		m = tx_pkts[i];
@@ -3689,6 +3706,13 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			rte_errno = -ret;
 			return i;
 		}
+
+		if (!(ol_flags & RTE_MBUF_F_TX_TCP_SEG) &&
+			ice_check_mbuf(m)) {
+			rte_errno = EINVAL;
+			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf data_len=[0]");
+			return i;
+		}
 	}
 	return i;
 }
-- 
2.34.1

RE: [PATCH v3 2/2] net/ice: fix scalar Tx path segment

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Friday, November 11, 2022 8:04 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>; Liu, KevinX <kevinx.liu@intel.com>
> Subject: [PATCH v3 2/2] net/ice: fix scalar Tx path segment
> 
> The scalar Tx path would send empty buffer that causes the Tx queue to
> overflow.
> 
> This patch adds the last buffer length judgment in tx_prepare to fix this 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: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
>  drivers/net/ice/ice_rxtx.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c index
> e6ddd2513d..69358f6a3a 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 invalid mbuf*/
> +static inline uint16_t
> +ice_check_mbuf(struct rte_mbuf *tx_pkt) {

Better to name the function to exactly match what it does. 
e.g.: ice_check_emtpy_mbuf
and also declare it as inline.

> +	struct rte_mbuf *txd = tx_pkt;
> +
> +	while (txd != NULL) {
> +		if (txd->data_len == 0)
> +			return -1;
> +		txd = txd->next;
> +	}
> +
> +	return 0;
> +}
> +
>  uint16_t
>  ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
>  	      uint16_t nb_pkts)
> @@ -3653,6 +3669,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
>  	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;
> +	uint16_t nb_used;
> 
>  	for (i = 0; i < nb_pkts; i++) {
>  		m = tx_pkts[i];
> @@ -3689,6 +3706,13 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
>  			rte_errno = -ret;
>  			return i;
>  		}
> +
> +		if (!(ol_flags & RTE_MBUF_F_TX_TCP_SEG) &&
> +			ice_check_mbuf(m)) {

Why "!(ol_flags & RTE_MBUF_F_TX_TCP_SEG)" is needed here?
A empty mbuf with TSO enabled is still acceptable?

> +			rte_errno = EINVAL;
> +			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf
> data_len=[0]");
> +			return i;
> +		}
>  	}
>  	return i;
>  }
> --
> 2.34.1

RE: [PATCH v3 2/2] net/ice: fix scalar Tx path segment

[Ye, MingjinX]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: 2022年11月11日 13:09
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>; Liu, KevinX <kevinx.liu@intel.com>
> Subject: RE: [PATCH v3 2/2] net/ice: fix scalar Tx path segment
> 
> 
> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Friday, November 11, 2022 8:04 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > Ferruh Yigit <ferruh.yigit@intel.com>; Li, Xiaoyun
> > <xiaoyun.li@intel.com>; Liu, KevinX <kevinx.liu@intel.com>
> > Subject: [PATCH v3 2/2] net/ice: fix scalar Tx path segment
> >
> > The scalar Tx path would send empty buffer that causes the Tx queue to
> > overflow.
> >
> > This patch adds the last buffer length judgment in tx_prepare to fix
> > this 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: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> >  drivers/net/ice/ice_rxtx.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > index e6ddd2513d..69358f6a3a 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 invalid mbuf*/
> > +static inline uint16_t
> > +ice_check_mbuf(struct rte_mbuf *tx_pkt) {
> 
> Better to name the function to exactly match what it does.
> e.g.: ice_check_emtpy_mbuf
> and also declare it as inline.
will optimize.
> 
> > +	struct rte_mbuf *txd = tx_pkt;
> > +
> > +	while (txd != NULL) {
> > +		if (txd->data_len == 0)
> > +			return -1;
> > +		txd = txd->next;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  uint16_t
> >  ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
> >  	      uint16_t nb_pkts)
> > @@ -3653,6 +3669,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> >  	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;
> > +	uint16_t nb_used;
> >
> >  	for (i = 0; i < nb_pkts; i++) {
> >  		m = tx_pkts[i];
> > @@ -3689,6 +3706,13 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> >  			rte_errno = -ret;
> >  			return i;
> >  		}
> > +
> > +		if (!(ol_flags & RTE_MBUF_F_TX_TCP_SEG) &&
> > +			ice_check_mbuf(m)) {
> 
> Why "!(ol_flags & RTE_MBUF_F_TX_TCP_SEG)" is needed here?
> A empty mbuf with TSO enabled is still acceptable?
Should not be expected to be implemented in test-pmd, should detect.
> 
> > +			rte_errno = EINVAL;
> > +			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf
> > data_len=[0]");
> > +			return i;
> > +		}
> >  	}
> >  	return i;
> >  }
> > --
> > 2.34.1

RE: [PATCH v3 2/2] net/ice: fix scalar Tx path segment

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Friday, November 11, 2022 4:31 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>; Liu, KevinX <kevinx.liu@intel.com>
> Subject: RE: [PATCH v3 2/2] net/ice: fix scalar Tx path segment
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: 2022年11月11日 13:09
> > To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ferruh
> > Yigit <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> > Liu, KevinX <kevinx.liu@intel.com>
> > Subject: RE: [PATCH v3 2/2] net/ice: fix scalar Tx path segment
> >
> >
> >
> > > -----Original Message-----
> > > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > > Sent: Friday, November 11, 2022 8:04 PM
> > > To: dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > > YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> > > <mingjinx.ye@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > > Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> > > <wenzhuo.lu@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Li,
> > > Xiaoyun <xiaoyun.li@intel.com>; Liu, KevinX <kevinx.liu@intel.com>
> > > Subject: [PATCH v3 2/2] net/ice: fix scalar Tx path segment
> > >
> > > The scalar Tx path would send empty buffer that causes the Tx queue
> > > to overflow.
> > >
> > > This patch adds the last buffer length judgment in tx_prepare to fix
> > > this 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: stable@dpdk.org
> > >
> > > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > > ---
> > >  drivers/net/ice/ice_rxtx.c | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > >
> > > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > > index e6ddd2513d..69358f6a3a 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 invalid mbuf*/
> > > +static inline uint16_t
> > > +ice_check_mbuf(struct rte_mbuf *tx_pkt) {
> >
> > Better to name the function to exactly match what it does.
> > e.g.: ice_check_emtpy_mbuf
> > and also declare it as inline.
> will optimize.
> >
> > > +	struct rte_mbuf *txd = tx_pkt;
> > > +
> > > +	while (txd != NULL) {
> > > +		if (txd->data_len == 0)
> > > +			return -1;
> > > +		txd = txd->next;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  uint16_t
> > >  ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
> > >  	      uint16_t nb_pkts)
> > > @@ -3653,6 +3669,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > >  	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;
> > > +	uint16_t nb_used;
> > >
> > >  	for (i = 0; i < nb_pkts; i++) {
> > >  		m = tx_pkts[i];
> > > @@ -3689,6 +3706,13 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > >  			rte_errno = -ret;
> > >  			return i;
> > >  		}
> > > +
> > > +		if (!(ol_flags & RTE_MBUF_F_TX_TCP_SEG) &&
> > > +			ice_check_mbuf(m)) {
> >
> > Why "!(ol_flags & RTE_MBUF_F_TX_TCP_SEG)" is needed here?
> > A empty mbuf with TSO enabled is still acceptable?
> Should not be expected to be implemented in test-pmd, should detect.

Why related with testpmd?, testpmd is not the only application, this is the question for how to identify a violated mbuf.

> >
> > > +			rte_errno = EINVAL;
> > > +			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf
> > > data_len=[0]");
> > > +			return i;
> > > +		}
> > >  	}
> > >  	return i;
> > >  }
> > > --
> > > 2.34.1

[PATCH v4 1/2] net/ice: fix scalar Rx path segment

[Mingjin Ye]

CRC is stripped by the hardware in the scattered Rx path. The last buffer
is invalid if it's packet length is zero.

This patch adds a judgment for the last buffer length to fix this issue,
it would free the mbuf associated to the last one if the last buffer is
empty.

Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/ice/ice_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0a2b0376ac..e6ddd2513d 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
 			} else
 				rxm->data_len = (uint16_t)(rx_packet_len -
 							   RTE_ETHER_CRC_LEN);
+		} else if (rx_packet_len == 0) {
+			rte_pktmbuf_free_seg(rxm);
+			first_seg->nb_segs--;
+			last_seg->next = NULL;
 		}
 
 		first_seg->port = rxq->port_id;
-- 
2.34.1

[PATCH v4 2/2] net/ice: fix scalar Tx path segment

[Mingjin Ye]

The scalar Tx path would send empty buffer that causes the Tx queue to
overflow.

This patch adds the last buffer length judgment in tx_prepare to fix this
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: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
v3:
	* delete unused variable.
---
 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..c1cde65a39 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 emtpy mbuf*/
+static inline uint16_t
+ice_check_emtpy_mbuf(struct rte_mbuf *tx_pkt)
+{
+	struct rte_mbuf *txd = tx_pkt;
+
+	while (txd != NULL) {
+		if (txd->data_len == 0)
+			return -1;
+		txd = txd->next;
+	}
+
+	return 0;
+}
+
 uint16_t
 ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	      uint16_t nb_pkts)
@@ -3689,6 +3705,12 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			rte_errno = -ret;
 			return i;
 		}
+
+		if (ice_check_emtpy_mbuf(m) != 0) {
+			rte_errno = EINVAL;
+			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf data_len=[0]");
+			return i;
+		}
 	}
 	return i;
 }
-- 
2.34.1

[PATCH v4 1/2] net/ice: fix scalar Rx path segment

[Mingjin Ye]

CRC is stripped by the hardware in the scattered Rx path. The last buffer
is invalid if it's packet length is zero.

This patch adds a judgment for the last buffer length to fix this issue,
it would free the mbuf associated to the last one if the last buffer is
empty.

Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
 drivers/net/ice/ice_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0a2b0376ac..e6ddd2513d 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -2111,6 +2111,10 @@ ice_recv_scattered_pkts(void *rx_queue,
 			} else
 				rxm->data_len = (uint16_t)(rx_packet_len -
 							   RTE_ETHER_CRC_LEN);
+		} else if (rx_packet_len == 0) {
+			rte_pktmbuf_free_seg(rxm);
+			first_seg->nb_segs--;
+			last_seg->next = NULL;
 		}
 
 		first_seg->port = rxq->port_id;
-- 
2.34.1

RE: [PATCH v4 1/2] net/ice: fix scalar Rx path segment

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Saturday, November 12, 2022 12:13 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>
> Subject: [PATCH v4 1/2] net/ice: fix scalar Rx path segment
> 
> CRC is stripped by the hardware in the scattered Rx path. The last buffer is
> invalid if it's packet length is zero.
> 
> This patch adds a judgment for the last buffer length to fix this issue, it would
> free the mbuf associated to the last one if the last buffer is empty.
> 
> Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

v3 already be applied, no new change on v4, will ignore this patch.

RE: [PATCH v4 1/2] net/ice: fix scalar Rx path segment

[Xu, Ke1]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Friday, November 11, 2022 5:03 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li,
> Xiaoyun <xiaoyun.li@intel.com>
> Subject: RE: [PATCH v4 1/2] net/ice: fix scalar Rx path segment
> 
> 
> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Saturday, November 12, 2022 12:13 AM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li,
> > Xiaoyun <xiaoyun.li@intel.com>
> > Subject: [PATCH v4 1/2] net/ice: fix scalar Rx path segment
> >
> > CRC is stripped by the hardware in the scattered Rx path. The last
> > buffer is invalid if it's packet length is zero.
> >
> > This patch adds a judgment for the last buffer length to fix this
> > issue, it would free the mbuf associated to the last one if the last buffer is
> empty.
> >
> > Fixes: 6eac0b7fde95 ("net/ice: support advance Rx/Tx")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> 
> v3 already be applied, no new change on v4, will ignore this patch.

Tested with v4 patch, passed.

Tested-by: Ke Xu <ke1.xu@intel.com>

[PATCH v4 2/2] net/ice: fix scalar Tx path segment

[Mingjin Ye]

The scalar Tx path would send empty buffer that causes the Tx queue to
overflow.

This patch adds the last buffer length judgment in tx_prepare to fix this
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: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@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*/
+static inline uint16_t
+ice_check_empty_mbuf(struct rte_mbuf *tx_pkt)
+{
+	struct rte_mbuf *txd = tx_pkt;
+
+	while (txd != NULL) {
+		if (txd->data_len == 0)
+			return -1;
+		txd = txd->next;
+	}
+
+	return 0;
+}
+
 uint16_t
 ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	      uint16_t nb_pkts)
@@ -3689,6 +3705,12 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 			rte_errno = -ret;
 			return i;
 		}
+
+		if (ice_check_empty_mbuf(m) != 0) {
+			rte_errno = EINVAL;
+			PMD_DRV_LOG(ERR, "INVALID mbuf:	last mbuf data_len=[0]");
+			return i;
+		}
 	}
 	return i;
 }
-- 
2.34.1

RE: [PATCH v4 2/2] net/ice: fix scalar Tx path segment

[Zhang, Qi Z]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Saturday, November 12, 2022 12:13 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>; Zhang, Qi
> Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> KevinX <kevinx.liu@intel.com>
> Subject: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
> 
> The scalar Tx path would send empty buffer that causes the Tx queue to
> overflow.
> 
> This patch adds the last buffer length judgment in tx_prepare to fix this 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: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

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

Applied to dpdk-next-net-intel.

Thanks
Qi

RE: [PATCH v4 2/2] net/ice: fix scalar Tx path segment

[Xu, Ke1]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Friday, November 11, 2022 5:02 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ferruh
> Yigit <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> KevinX <kevinx.liu@intel.com>
> Subject: RE: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
> 
> 
> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Saturday, November 12, 2022 12:13 AM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu,
> > Jingjing <jingjing.wu@intel.com>; Ferruh Yigit
> > <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Liu,
> > KevinX <kevinx.liu@intel.com>
> > Subject: [PATCH v4 2/2] net/ice: fix scalar Tx path segment
> >
> > The scalar Tx path would send empty buffer that causes the Tx queue to
> > overflow.
> >
> > This patch adds the last buffer length judgment in tx_prepare to fix
> > this 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: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> 
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> 
> Applied to dpdk-next-net-intel.
> 
> Thanks
> Qi

Tested and passed.
Tested-by: Ke Xu <ke1.xu@intel.com>

Re: [PATCH v4 2/2] net/ice: fix scalar Tx path segment

[David Marchand]

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.ye@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: stable@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@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

Re: [PATCH v4 2/2] net/ice: fix scalar Tx path segment

[David Marchand]

On Tue, Sep 19, 2023 at 10:15 AM David Marchand
<david.marchand@redhat.com> wrote:
> 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.ye@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: stable@dpdk.org
> >
> > Signed-off-by: Mingjin Ye <mingjinx.ye@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.

Ping.


-- 
David Marchand