Re: [dpdk-dev] [PATCH 2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro

[Ferruh Yigit <ferruh.yigit@intel.com>]

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?