DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Ed Czeck <ed.czeck@atomicrules.com>,
	dev@dpdk.org, bruce.richardson@intel.com
Cc: shepard.siegel@atomicrules.com, john.miller@atomicrules.com
Subject: Re: [dpdk-dev] [PATCH] net/ark: fix meson build
Date: Thu, 20 Aug 2020 12:16:45 +0100	[thread overview]
Message-ID: <5715777c-a36a-8edd-7e06-2afebdef6c37@intel.com> (raw)
In-Reply-To: <20200819204514.22187-1-ed.czeck@atomicrules.com>

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.czeck@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.

  reply	other threads:[~2020-08-20 11:16 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-19 15:35 Ed Czeck
2020-08-19 16:29 ` Ferruh Yigit
2020-08-19 20:45 ` Ed Czeck
2020-08-20 11:16   ` Ferruh Yigit [this message]
2020-08-20 15:41     ` Ed Czeck
2020-08-21  9:44       ` Ferruh Yigit
2020-08-20 21:55 ` [dpdk-dev] [PATCH 1/2] net/ark: remove compile time log macros in favor of run time log control Ed Czeck
2020-08-20 21:55   ` [dpdk-dev] [PATCH 2/2] net/ark remove ARK_TX_PAD_TO_60 configuration macro Ed Czeck
2020-08-21  9:50     ` Ferruh Yigit
2020-08-24 13:36 ` [dpdk-dev] [PATCH 1/2] net/ark: remove compile time log macros in favor of run time log control Ed Czeck
2020-08-24 13:36   ` [dpdk-dev] [PATCH 2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro Ed Czeck
2020-08-24 14:55     ` Ferruh Yigit
2020-08-24 21:51       ` Ed Czeck
2020-08-25  7:43         ` Ferruh Yigit
2020-08-24 14:37   ` [dpdk-dev] [PATCH 1/2] net/ark: remove compile time log macros in favor of run time log control Ferruh Yigit
2020-08-24 14:40     ` Bruce Richardson
2020-08-24 15:09       ` Ferruh Yigit
2020-08-24 21:40         ` Ed Czeck
2020-08-25  7:44           ` Ferruh Yigit
2020-08-26 15:24 ` Ed Czeck
2020-08-26 15:24   ` [dpdk-dev] [PATCH 2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro Ed Czeck
2020-08-27 16:11 ` [dpdk-dev] [PATCH 1/2] net/ark: remove compile time log macros in favor of run time log control Ed Czeck
2020-08-27 16:11   ` [dpdk-dev] [PATCH 2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro Ed Czeck
2020-09-01 11:17     ` Ferruh Yigit
2020-09-08 19:20 ` [dpdk-dev] [PATCH v7 1/2] net/ark: remove compile time log macros in favor of run time log control Ed Czeck
2020-09-08 19:20   ` [dpdk-dev] [PATCH v7 2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro Ed Czeck
2020-09-09 13:33   ` [dpdk-dev] [PATCH v7 1/2] net/ark: remove compile time log macros in favor of run time log control Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5715777c-a36a-8edd-7e06-2afebdef6c37@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ed.czeck@atomicrules.com \
    --cc=john.miller@atomicrules.com \
    --cc=shepard.siegel@atomicrules.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).