diff mbox

[dpdk-dev,v3,7/7] net/mlx4: remove empty Tx segment support

Message ID 1509358049-18854-8-git-send-email-matan@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Matan Azrad Oct. 30, 2017, 10:07 a.m. UTC 
  Move empty segment case processing to debug mode.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx4/mlx4_rxtx.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Adrien Mazarguil Oct. 30, 2017, 2:24 p.m. UTC | #1 
On Mon, Oct 30, 2017 at 10:07:29AM +0000, Matan Azrad wrote:
> Move empty segment case processing to debug mode.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Whoa, I think there's a misunderstanding here. Nothing prevents applications
from attempting to send zero-length segments, and the PMD must survive this
somehow.

I think this commit should be dropped, more below.

> ---
>  drivers/net/mlx4/mlx4_rxtx.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c
> index 482c399..c005a41 100644
> --- a/drivers/net/mlx4/mlx4_rxtx.c
> +++ b/drivers/net/mlx4/mlx4_rxtx.c
> @@ -305,15 +305,18 @@ static int handle_multi_segs(struct rte_mbuf *buf,
>  			return -1;
>  		}
>  #endif /* NDEBUG */
> -		if (likely(sbuf->data_len)) {
> -			byte_count = rte_cpu_to_be_32(sbuf->data_len);
> -		} else {
> +		byte_count = rte_cpu_to_be_32(sbuf->data_len);
> +#ifndef NDEBUG
> +		if (unlikely(!sbuf->data_len)) {
> +			DEBUG("%p: Empty segment is not allowed",
> +					(void *)txq);
>  			/*
>  			 * Zero length segment is treated as inline segment
>  			 * with zero data.
>  			 */
>  			byte_count = RTE_BE32(0x80000000);
>  		}
> +#endif /* NDEBUG */

This change means outside of debug mode and according to PRM, a zero-length
segment is interpreted as containing 2 GiB worth of data, which guarantees
some sort of crash.

To properly enforce such a limitation, you'd need a check (possibly
unlikely()) to reject the packet and stop the TX function at this point
anyway. Such a check negates any kind of optimization brought by this
commit, as small as it is.

You'd better leave the existing code unmodified in my opinion.
diff mbox

Patch

diff --git a/drivers/net/mlx4/mlx4_rxtx.c b/drivers/net/mlx4/mlx4_rxtx.c
index 482c399..c005a41 100644
--- a/drivers/net/mlx4/mlx4_rxtx.c
+++ b/drivers/net/mlx4/mlx4_rxtx.c
@@ -305,15 +305,18 @@  static int handle_multi_segs(struct rte_mbuf *buf,
 			return -1;
 		}
 #endif /* NDEBUG */
-		if (likely(sbuf->data_len)) {
-			byte_count = rte_cpu_to_be_32(sbuf->data_len);
-		} else {
+		byte_count = rte_cpu_to_be_32(sbuf->data_len);
+#ifndef NDEBUG
+		if (unlikely(!sbuf->data_len)) {
+			DEBUG("%p: Empty segment is not allowed",
+					(void *)txq);
 			/*
 			 * Zero length segment is treated as inline segment
 			 * with zero data.
 			 */
 			byte_count = RTE_BE32(0x80000000);
 		}
+#endif /* NDEBUG */
 		/*
 		 * If the data segment is not at the beginning of a
 		 * Tx basic block (TXBB) then write the byte count,