On 8/19/2020 9:45 PM, Ed Czeck wrote:
> * Rename net/ark specific CONFIG_RTE macros to local macros.
> * Change condition of ARK_PAD_TX to match behavior of meson build
> to makefile build.
> * Install header file needed for dynamic library.
> * Update doc as required.
>
> Signed-off-by: Ed Czeck <ed.cz...@atomicrules.com>
> ---
> doc/guides/nics/ark.rst | 24 ++++++++++++++----------
> drivers/net/ark/ark_logs.h | 16 +++++++---------
> drivers/net/ark/meson.build | 2 ++
> 3 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
> index 06e8c3374..4d8920cd0 100644
> --- a/doc/guides/nics/ark.rst
> +++ b/doc/guides/nics/ark.rst
> @@ -124,27 +124,31 @@ Configuration Information
>
> **DPDK Configuration Parameters**
>
> - The following configuration options are available for the ARK PMD:
> + The following compile-time 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.
> + * **ARK_NOPAD_TX**: When enabled TX
> + packets are not padded to 60 bytes to support downstream MACS.
>
> - * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y): When enabled TX
> - packets are padded to 60 bytes to support downstream MACS.
> -
> - * **CONFIG_RTE_LIBRTE_ARK_DEBUG_RX** (default n): Enables or disables
> debug
> + * **ARK_DEBUG_RX**: Enables debug
> logging and internal checking of RX ingress logic within the ARK PMD
> driver.
>
> - * **CONFIG_RTE_LIBRTE_ARK_DEBUG_TX** (default n): Enables or disables
> debug
> + * **ARK_DEBUG_TX**: Enables debug
> logging and internal checking of TX egress logic within the ARK PMD
> driver.
>
> - * **CONFIG_RTE_LIBRTE_ARK_DEBUG_STATS** (default n): Enables or disables
> debug
> + * **ARK_DEBUG_STATS**: Enables debug
> logging of detailed packet and performance statistics gathered in
> the PMD and FPGA.
>
> - * **CONFIG_RTE_LIBRTE_ARK_DEBUG_TRACE** (default n): Enables or disables
> debug
> + * **ARK_DEBUG_TRACE**: Enables debug
> logging of detailed PMD events and status.
Logging can be controlled in runtime, that is what we should use.
In data path, we use compile time flags because of the performance issues. So OK
to have 'CONFIG_RTE_LIBRTE_ARK_DEBUG_RX' & 'CONFIG_RTE_LIBRTE_ARK_DEBUG_TX' as
compile time flag, but why having compile time flag for rest?
>
> +Note that enabling debugging options may affect system performance.
> +These options may be set by specifying them in CFLAG
> +environment before the meson build set. E.g.::
> +
> + export CFLAGS="-DARK_DEBUG_TRACE"
> + meson build
> +
When you passed the flag as above, it is still global to all components in the
DPDK, this is not just for ark. What is the motivation to remove the
"RET_LIBRTE_" prefix?
>
> Building DPDK
> -------------
> diff --git a/drivers/net/ark/ark_logs.h b/drivers/net/ark/ark_logs.h
> index 44aac6102..125583475 100644
> --- a/drivers/net/ark/ark_logs.h
> +++ b/drivers/net/ark/ark_logs.h
> @@ -6,14 +6,12 @@
> #define _ARK_DEBUG_H_
>
> #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
> +#ifdef ARK_NOPAD_TX
> #define ARK_TX_PAD_TO_60 0
> +#else
> +#define ARK_TX_PAD_TO_60 1
> #endif
So you don't want to convert this to runtime configuration.
The point we are reducing compile time flags:
1) It forks the code paths and by time it leave not tested, even not compiled
code paths which may cause rotten code by time.
2) Multiple code paths will lead deployment problems. When you deploy an
application, you won't able to change the compile time configuration in customer
environment and need to re-compile (most probably re-test) and re-deploy it.
Also there is not easy way to figure out from binary in customer environment
that with which compile time flags it has been built.
Switching to CFLAGS="..." doesn't make above concerns go away and indeed it
makes (1) worst since hides the config options within the driver. Previously it
was possible to trigger each config option and do testing using scripts, now
since config options are hidden in driver we can't do even that.
Can you please detail why "ARK_TX_PAD_TO_60" is needed exactly?
And can you please justify why it has to be compile time config option?
<...>
> @@ -11,3 +11,5 @@ sources = files('ark_ddm.c',
> 'ark_pktgen.c',
> 'ark_rqp.c',
> 'ark_udm.c')
> +
> +install_headers('ark_ext.h')
>
Installing PMD header file is not required but this has an unique usage.
Ark PMD is wrapper to the external shared library which should implement the
functions that has prototypes in the 'ark_ext.h'.
Since this header is not needed by users of the dpdk library, but needed by
extension developers for the ark PMD, I think the header should not be
installed.
