[2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro

Message ID 20200827161130.14978-2-ed.czeck@atomicrules.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series [1/2] net/ark: remove compile time log macros in favor of run time log control |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Ed Czeck Aug. 27, 2020, 4:11 p.m. UTC
  Replace behavior with RTE_LIBRTE_ARK_MIN_TX_PKTLEN
with a default value of 0.
Update documentation as needed.

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 doc/guides/nics/ark.rst         | 16 ++++++++----
 drivers/net/ark/ark_ethdev_tx.c | 43 ++++++++++++++++++---------------
 drivers/net/ark/ark_logs.h      |  8 ------
 3 files changed, 35 insertions(+), 32 deletions(-)
  

Comments

Ferruh Yigit Sept. 1, 2020, 11:17 a.m. UTC | #1
On 8/27/2020 5:11 PM, Ed Czeck wrote:
> Replace behavior with RTE_LIBRTE_ARK_MIN_TX_PKTLEN
> with a default value of 0.
> Update documentation as needed.

Can you please use versions in the patches, it makes easier to follow them?
Like '[PATCH v4 2/2]', -v# option to "git format-patch" or "git send-email" does
it automatically for you.

Also a changelog history that documents what changes in each version helps, a
good place to put it is just below the '---' after sign off, this way although
it stays in the patch, it is removed automatically by git while merging the patch.

> 
> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> ---
>  doc/guides/nics/ark.rst         | 16 ++++++++----
>  drivers/net/ark/ark_ethdev_tx.c | 43 ++++++++++++++++++---------------
>  drivers/net/ark/ark_logs.h      |  8 ------
>  3 files changed, 35 insertions(+), 32 deletions(-)
> 
> diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
> index c3ffcbbc2..c7ed4095f 100644
> --- a/doc/guides/nics/ark.rst
> +++ b/doc/guides/nics/ark.rst
> @@ -126,11 +126,10 @@ Configuration Information
>  
>    The following configuration options are available for the ARK PMD:
>  
> -   * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
> -     of the ARK PMD driver in the DPDK compilation.
> -

Hi Ed,

Can you leave out this piece in this patch? Yes it will go away eventually, but
it is not related logically to this change. Let's leave removing it to the patch
that removes Makefile which will be removing all relevant pieces as a whole.

> -   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
> -     packets are padded to 60 bytes to support downstream MACS.
> +   * **RTE_LIBRTE_ARK_MIN_TX_PKTLEN** (default 0): Sets the minimum
> +     packet length for tx packets to the FPGA.  Packets less than this
> +     length are padded to meet the requirement. This allows padding to
> +     be offloaded or remain in host software.
>  
>  
>  Building DPDK
> @@ -144,6 +143,13 @@ By default the ARK PMD library will be built into the DPDK library.
>  For configuring and using UIO and VFIO frameworks, please also refer :ref:`the
>  documentation that comes with DPDK suite <linux_gsg>`.
>  
> +To build with a non-zero minimum tx packet length, set the above macro in your
> +CFLAGS environment prior to the meson build step. I.e.,
> +
> +    export CFLAGS="-DRTE_LIBRTE_ARK_MIN_TX_PKTLEN=60"
> +    meson build
> +
> +
>  Supported ARK RTL PCIe Instances
>  --------------------------------
>  
> diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
> index 72624deb3..52ce2ed41 100644
> --- a/drivers/net/ark/ark_ethdev_tx.c
> +++ b/drivers/net/ark/ark_ethdev_tx.c
> @@ -14,6 +14,11 @@
>  #define ARK_TX_META_OFFSET (RTE_PKTMBUF_HEADROOM - ARK_TX_META_SIZE)
>  #define ARK_TX_MAX_NOCHAIN (RTE_MBUF_DEFAULT_DATAROOM)
>  
> +#ifndef RTE_LIBRTE_ARK_MIN_TX_PKTLEN
> +#define ARK_MIN_TX_PKTLEN 0
> +#else
> +#define ARK_MIN_TX_PKTLEN RTE_LIBRTE_ARK_MIN_TX_PKTLEN
> +#endif
>  
>  /* ************************************************************************* */
>  struct ark_tx_queue {
> @@ -104,28 +109,28 @@ eth_ark_xmit_pkts(void *vtxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>  	     ++nb) {
>  		mbuf = tx_pkts[nb];
>  
> -		if (ARK_TX_PAD_TO_60) {
> -			if (unlikely(rte_pktmbuf_pkt_len(mbuf) < 60)) {
> -				/* this packet even if it is small can be split,
> -				 * be sure to add to the end mbuf
> +#if ARK_MIN_TX_PKTLEN != 0


Previous "if (...)" approach was better, compiler was checking the code
independent from 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN' defined or not, and compiler was
optimizing out the code if it is not defined.
With the '#if' macro, we are losing the compiler check.

If there is no explicit reason, can you keep the old behavior here?
  

Patch

diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
index c3ffcbbc2..c7ed4095f 100644
--- a/doc/guides/nics/ark.rst
+++ b/doc/guides/nics/ark.rst
@@ -126,11 +126,10 @@  Configuration Information
 
   The following configuration options are available for the ARK PMD:
 
-   * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
-     of the ARK PMD driver in the DPDK compilation.
-
-   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
-     packets are padded to 60 bytes to support downstream MACS.
+   * **RTE_LIBRTE_ARK_MIN_TX_PKTLEN** (default 0): Sets the minimum
+     packet length for tx packets to the FPGA.  Packets less than this
+     length are padded to meet the requirement. This allows padding to
+     be offloaded or remain in host software.
 
 
 Building DPDK
@@ -144,6 +143,13 @@  By default the ARK PMD library will be built into the DPDK library.
 For configuring and using UIO and VFIO frameworks, please also refer :ref:`the
 documentation that comes with DPDK suite <linux_gsg>`.
 
+To build with a non-zero minimum tx packet length, set the above macro in your
+CFLAGS environment prior to the meson build step. I.e.,
+
+    export CFLAGS="-DRTE_LIBRTE_ARK_MIN_TX_PKTLEN=60"
+    meson build
+
+
 Supported ARK RTL PCIe Instances
 --------------------------------
 
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 72624deb3..52ce2ed41 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -14,6 +14,11 @@ 
 #define ARK_TX_META_OFFSET (RTE_PKTMBUF_HEADROOM - ARK_TX_META_SIZE)
 #define ARK_TX_MAX_NOCHAIN (RTE_MBUF_DEFAULT_DATAROOM)
 
+#ifndef RTE_LIBRTE_ARK_MIN_TX_PKTLEN
+#define ARK_MIN_TX_PKTLEN 0
+#else
+#define ARK_MIN_TX_PKTLEN RTE_LIBRTE_ARK_MIN_TX_PKTLEN
+#endif
 
 /* ************************************************************************* */
 struct ark_tx_queue {
@@ -104,28 +109,28 @@  eth_ark_xmit_pkts(void *vtxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	     ++nb) {
 		mbuf = tx_pkts[nb];
 
-		if (ARK_TX_PAD_TO_60) {
-			if (unlikely(rte_pktmbuf_pkt_len(mbuf) < 60)) {
-				/* this packet even if it is small can be split,
-				 * be sure to add to the end mbuf
+#if ARK_MIN_TX_PKTLEN != 0
+		if (unlikely(rte_pktmbuf_pkt_len(mbuf) < ARK_MIN_TX_PKTLEN)) {
+			/* this packet even if it is small can be split,
+			 * be sure to add to the end mbuf
+			 */
+			uint16_t to_add = ARK_MIN_TX_PKTLEN -
+				rte_pktmbuf_pkt_len(mbuf);
+			char *appended =
+				rte_pktmbuf_append(mbuf, to_add);
+
+			if (appended == 0) {
+				/* This packet is in error,
+				 * we cannot send it so just
+				 * count it and delete it.
 				 */
-				uint16_t to_add =
-					60 - rte_pktmbuf_pkt_len(mbuf);
-				char *appended =
-					rte_pktmbuf_append(mbuf, to_add);
-
-				if (appended == 0) {
-					/* This packet is in error,
-					 * we cannot send it so just
-					 * count it and delete it.
-					 */
-					queue->tx_errors += 1;
-					rte_pktmbuf_free(mbuf);
-					continue;
-				}
-				memset(appended, 0, to_add);
+				queue->tx_errors += 1;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
+			memset(appended, 0, to_add);
 		}
+#endif
 
 		if (unlikely(mbuf->nb_segs != 1)) {
 			stat = eth_ark_tx_jumbo(queue, mbuf);
diff --git a/drivers/net/ark/ark_logs.h b/drivers/net/ark/ark_logs.h
index c3d7e7d39..ca46d86c9 100644
--- a/drivers/net/ark/ark_logs.h
+++ b/drivers/net/ark/ark_logs.h
@@ -8,14 +8,6 @@ 
 #include <inttypes.h>
 #include <rte_log.h>
 
-
-/* Configuration option to pad TX packets to 60 bytes */
-#ifdef RTE_LIBRTE_ARK_PAD_TX
-#define ARK_TX_PAD_TO_60   1
-#else
-#define ARK_TX_PAD_TO_60   0
-#endif
-
 /* system camel case definition changed to upper case */
 #define PRIU32 PRIu32
 #define PRIU64 PRIu64