Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

+Adrien on To: line

Email user/client fail on original. :-(

----- Forwarded message from Bruce Richardson <bruce.richardson@intel.com> -----

Date: Fri, 6 Nov 2015 11:49:05 +0000
From: Bruce Richardson <bruce.richardson@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>, Thomas Monjalon <thomas.monjalon@6wind.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header
User-Agent: Mutt/1.5.23 (2014-03-12)

On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> Bruce is asking for a consensus about -pedantic, whether we want to do the
> extra effort to support it in DPDK. Since I like checking for -pedantic
> errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
> debugging mode. There is currently no established rule in DPDK against this.
> 
> I'm arguing that most C headers (C compiler, libc, most libraries, even the
> Linux kernel in uapi to an extent) provide standards compliant includes
> because they cannot predict or force particular compilation flags on
> user applications.
> 
> If we consider DPDK as a system wide library, I think we should do it as
> well in all installed header files. If we choose not to, then we must
> document that our code is not standard, -pedantic is unsupported and I'll
> have to drop it from mlx4 and mlx5.
> 
> -- 
> Adrien Mazarguil
> 6WIND

Hi Adrien,

I'm trying to dig into this a bit more now, and try out using a static inline
function, but I'm having trouble getting DPDK to compile with the mlx drivers
turned on in the config. I'm trying to follow the instructions here:
http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly called out what
requirements are for compilation vs requirements for running the PMD.

I'm running Fedora 23, and installed the libibverbs-devel package, but when I
compile I get the following error:

== Build drivers/net/mlx4
  CC mlx4.o
  /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function ‘txq_cleanup’:
  /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error: storage size of ‘params’ isn’t known
    struct ibv_exp_release_intf_params params;
                                       ^
compilation terminated due to -Wfatal-errors.

Any suggestions on the fix for this?

Thanks,
/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

Hi Bruce,

On Fri, Nov 06, 2015 at 11:52:44AM +0000, Bruce Richardson wrote:
> +Adrien on To: line
> 
> Email user/client fail on original. :-(
> 
> ----- Forwarded message from Bruce Richardson <bruce.richardson@intel.com> -----
> 
> Date: Fri, 6 Nov 2015 11:49:05 +0000
> From: Bruce Richardson <bruce.richardson@intel.com>
> To: Stephen Hemminger <stephen@networkplumber.org>, Thomas Monjalon <thomas.monjalon@6wind.com>, dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header
> User-Agent: Mutt/1.5.23 (2014-03-12)
> 
> On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > Bruce is asking for a consensus about -pedantic, whether we want to do the
> > extra effort to support it in DPDK. Since I like checking for -pedantic
> > errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
> > debugging mode. There is currently no established rule in DPDK against this.
> > 
> > I'm arguing that most C headers (C compiler, libc, most libraries, even the
> > Linux kernel in uapi to an extent) provide standards compliant includes
> > because they cannot predict or force particular compilation flags on
> > user applications.
> > 
> > If we consider DPDK as a system wide library, I think we should do it as
> > well in all installed header files. If we choose not to, then we must
> > document that our code is not standard, -pedantic is unsupported and I'll
> > have to drop it from mlx4 and mlx5.
> > 
> > -- 
> > Adrien Mazarguil
> > 6WIND
> 
> Hi Adrien,
> 
> I'm trying to dig into this a bit more now, and try out using a static inline
> function, but I'm having trouble getting DPDK to compile with the mlx drivers
> turned on in the config. I'm trying to follow the instructions here:
> http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly called out what
> requirements are for compilation vs requirements for running the PMD.
> 
> I'm running Fedora 23, and installed the libibverbs-devel package, but when I
> compile I get the following error:
> 
> == Build drivers/net/mlx4
>   CC mlx4.o
>   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function ‘txq_cleanup’:
>   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error: storage size of ‘params’ isn’t known
>     struct ibv_exp_release_intf_params params;
>                                        ^
> compilation terminated due to -Wfatal-errors.
> 
> Any suggestions on the fix for this?

This is a known issue, libibverbs-devel package from Fedora 23 most likely
does not support extended types and functions required by mlx4. You should
remove the packages that come with your distribution and install libraries
versions from Mellanox OFED as described in the next section:

 http://dpdk.org/doc/guides/nics/mlx4.html#getting-mellanox-ofed

Note: no need to fully install OFED for compilation checks, you can extract
an updated libibverbs package from the archive.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Richardson, Bruce]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> 
> Hi Bruce,
> 
> On Fri, Nov 06, 2015 at 11:52:44AM +0000, Bruce Richardson wrote:
> > +Adrien on To: line
> >
> > Email user/client fail on original. :-(
> >
> > ----- Forwarded message from Bruce Richardson
> > <bruce.richardson@intel.com> -----
> >
> > Date: Fri, 6 Nov 2015 11:49:05 +0000
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > To: Stephen Hemminger <stephen@networkplumber.org>, Thomas Monjalon
> > <thomas.monjalon@6wind.com>, dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking
> > macros to header
> > User-Agent: Mutt/1.5.23 (2014-03-12)
> >
> > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > Bruce is asking for a consensus about -pedantic, whether we want to
> > > do the extra effort to support it in DPDK. Since I like checking for
> > > -pedantic errors, it's enabled for mlx4 and mlx5 when compiling
> > > these drivers in debugging mode. There is currently no established
> rule in DPDK against this.
> > >
> > > I'm arguing that most C headers (C compiler, libc, most libraries,
> > > even the Linux kernel in uapi to an extent) provide standards
> > > compliant includes because they cannot predict or force particular
> > > compilation flags on user applications.
> > >
> > > If we consider DPDK as a system wide library, I think we should do
> > > it as well in all installed header files. If we choose not to, then
> > > we must document that our code is not standard, -pedantic is
> > > unsupported and I'll have to drop it from mlx4 and mlx5.
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> >
> > Hi Adrien,
> >
> > I'm trying to dig into this a bit more now, and try out using a static
> > inline function, but I'm having trouble getting DPDK to compile with
> > the mlx drivers turned on in the config. I'm trying to follow the
> instructions here:
> > http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly called
> > out what requirements are for compilation vs requirements for running
> the PMD.
> >
> > I'm running Fedora 23, and installed the libibverbs-devel package, but
> > when I compile I get the following error:
> >
> > == Build drivers/net/mlx4
> >   CC mlx4.o
> >   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function
> ‘txq_cleanup’:
> >   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error:
> storage size of ‘params’ isn’t known
> >     struct ibv_exp_release_intf_params params;
> >                                        ^ compilation terminated due to
> > -Wfatal-errors.
> >
> > Any suggestions on the fix for this?
> 
> This is a known issue, libibverbs-devel package from Fedora 23 most likely
> does not support extended types and functions required by mlx4. You should
> remove the packages that come with your distribution and install libraries
> versions from Mellanox OFED as described in the next section:
> 
>  http://dpdk.org/doc/guides/nics/mlx4.html#getting-mellanox-ofed
> 
> Note: no need to fully install OFED for compilation checks, you can
> extract an updated libibverbs package from the archive.
> 
> --
> Adrien Mazarguil
> 6WIND

Hi again,

I've installed the libibverbs and libibverbs-devel packages from the mellanox site, 
but I'm still getting the same error. Anything else I might be missing?

$ rpm -qa | grep mlnx
libibverbs-devel-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
libmlx5-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64
libmlx4-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
libibverbs-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
libmlx4-devel-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
libmlx5-devel-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64

Regards,
/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Fri, Nov 06, 2015 at 02:39:31PM +0000, Richardson, Bruce wrote:
[...]
> > > Hi Adrien,
> > >
> > > I'm trying to dig into this a bit more now, and try out using a static
> > > inline function, but I'm having trouble getting DPDK to compile with
> > > the mlx drivers turned on in the config. I'm trying to follow the
> > instructions here:
> > > http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly called
> > > out what requirements are for compilation vs requirements for running
> > the PMD.
> > >
> > > I'm running Fedora 23, and installed the libibverbs-devel package, but
> > > when I compile I get the following error:
> > >
> > > == Build drivers/net/mlx4
> > >   CC mlx4.o
> > >   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function
> > ‘txq_cleanup’:
> > >   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error:
> > storage size of ‘params’ isn’t known
> > >     struct ibv_exp_release_intf_params params;
> > >                                        ^ compilation terminated due to
> > > -Wfatal-errors.
> > >
> > > Any suggestions on the fix for this?
> > 
> > This is a known issue, libibverbs-devel package from Fedora 23 most likely
> > does not support extended types and functions required by mlx4. You should
> > remove the packages that come with your distribution and install libraries
> > versions from Mellanox OFED as described in the next section:
> > 
> >  http://dpdk.org/doc/guides/nics/mlx4.html#getting-mellanox-ofed
> > 
> > Note: no need to fully install OFED for compilation checks, you can
> > extract an updated libibverbs package from the archive.
> > 
> > --
> > Adrien Mazarguil
> > 6WIND
> 
> Hi again,
> 
> I've installed the libibverbs and libibverbs-devel packages from the mellanox site, 
> but I'm still getting the same error. Anything else I might be missing?
> 
> $ rpm -qa | grep mlnx
> libibverbs-devel-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
> libmlx5-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64
> libmlx4-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
> libibverbs-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
> libmlx4-devel-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
> libmlx5-devel-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64

That's weird, 'struct ibv_exp_release_intf_param' must be defined in
/usr/include/infiniband/verbs_exp.h, itself included by infiniband/verbs.h,
both normally part of the libibverbs-devel package above.

Make sure you don't have an old version of infiniband/verbs.h somewhere else
such as in /usr/local/include after a manual compilation of libibverbs.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Richardson, Bruce]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> 
> On Fri, Nov 06, 2015 at 02:39:31PM +0000, Richardson, Bruce wrote:
> [...]
> > > > Hi Adrien,
> > > >
> > > > I'm trying to dig into this a bit more now, and try out using a
> > > > static inline function, but I'm having trouble getting DPDK to
> > > > compile with the mlx drivers turned on in the config. I'm trying
> > > > to follow the
> > > instructions here:
> > > > http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly
> > > > called out what requirements are for compilation vs requirements
> > > > for running
> > > the PMD.
> > > >
> > > > I'm running Fedora 23, and installed the libibverbs-devel package,
> > > > but when I compile I get the following error:
> > > >
> > > > == Build drivers/net/mlx4
> > > >   CC mlx4.o
> > > >   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function
> > > ‘txq_cleanup’:
> > > >   /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error:
> > > storage size of ‘params’ isn’t known
> > > >     struct ibv_exp_release_intf_params params;
> > > >                                        ^ compilation terminated
> > > > due to -Wfatal-errors.
> > > >
> > > > Any suggestions on the fix for this?
> > >
> > > This is a known issue, libibverbs-devel package from Fedora 23 most
> > > likely does not support extended types and functions required by
> > > mlx4. You should remove the packages that come with your
> > > distribution and install libraries versions from Mellanox OFED as
> described in the next section:
> > >
> > >  http://dpdk.org/doc/guides/nics/mlx4.html#getting-mellanox-ofed
> > >
> > > Note: no need to fully install OFED for compilation checks, you can
> > > extract an updated libibverbs package from the archive.
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> >
> > Hi again,
> >
> > I've installed the libibverbs and libibverbs-devel packages from the
> > mellanox site, but I'm still getting the same error. Anything else I
> might be missing?
> >
> > $ rpm -qa | grep mlnx
> > libibverbs-devel-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
> > libmlx5-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64
> > libmlx4-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
> > libibverbs-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
> > libmlx4-devel-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
> > libmlx5-devel-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64
> 
> That's weird, 'struct ibv_exp_release_intf_param' must be defined in
> /usr/include/infiniband/verbs_exp.h, itself included by
> infiniband/verbs.h, both normally part of the libibverbs-devel package
> above.
> 
> Make sure you don't have an old version of infiniband/verbs.h somewhere
> else such as in /usr/local/include after a manual compilation of
> libibverbs.
> 
> --
> Adrien Mazarguil
> 6WIND

Thanks, that fixed it. There was a copy of the verbs headers in /usr/local/include, which is strange because I never remember having ever tried compiling up ibverbs before.

Anyway, problem solved for now.
Thanks for your help.
/Bruce

[dpdk-dev] [PATCH v2 0/4] ethdev: minor cleanup

[Bruce Richardson]

This patchset performs two cleanups:
1. Four functions in ethdev.c which were enabled for debug only have been
  merged into their inlined header-file counterparts. This change required that
  a number of macros be renamed and moved to the header file too. The macro changes
  are in patches 1 & 2, and the elimination of the separate debug fns are in patch 3.
2. Checks for valid function pointers are added to the API calls for reading
  the descriptor ring count, and checking for a valid descriptor. This is because
  these functions are not implemented by most drivers, and so it's far safer to
  have the check.

---

V2 Changes:
* Rebased to latest DPDK codebase
* Changed type from uint32_t to int for the count function, on the basis of
feedback received.

Bruce Richardson (4):
  ethdev: rename macros to have RTE_ETH prefix
  ethdev: move error checking macros to header
  ethdev: remove duplicated debug functions
  ethdev: check driver support for functions

 lib/librte_ether/rte_ethdev.c | 674 ++++++++++++++++++------------------------
 lib/librte_ether/rte_ethdev.h | 121 ++++++--
 2 files changed, 375 insertions(+), 420 deletions(-)

-- 
2.4.3

[dpdk-dev] [PATCH v3 0/4] ethdev: minor cleanup

[Bruce Richardson]

This patchset performs two cleanups:
1. Four functions in ethdev.c which were enabled for debug only have been
  merged into their inlined header-file counterparts. This change required that
  a number of macros be renamed and moved to the header file too. The macro changes
  are in patches 1 & 2, and the elimination of the separate debug fns are in patch 3.
2. Checks for valid function pointers are added to the API calls for reading
  the descriptor ring count, and checking for a valid descriptor. This is because
  these functions are not implemented by most drivers, and so it's far safer to
  have the check.

---

V3 Changes:
* Rebased to latest DPDK codebase
* Fixed checkpatch issues in patches 2 and 3.

V2 Changes:
* Rebased to latest DPDK codebase
* Changed type from uint32_t to int for the count function, on the basis of
feedback received.

Bruce Richardson (4):
  ethdev: rename macros to have RTE_ETH prefix
  ethdev: move error checking macros to header
  ethdev: remove duplicated debug functions
  ethdev: check driver support for functions

 lib/librte_ether/rte_ethdev.c | 602 ++++++++++++++++++------------------------
 lib/librte_ether/rte_ethdev.h | 120 ++++++---
 2 files changed, 338 insertions(+), 384 deletions(-)

-- 
2.4.3

[dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

Move the function ptr and port id checking macros to the header file, so
that they can be used in the static inline functions there. In doxygen
comments, mark them as for internal use only.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 38 ------------------------------
 lib/librte_ether/rte_ethdev.h | 54 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 21f213f..f95c4d2 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -69,14 +69,6 @@
 #include "rte_ether.h"
 #include "rte_ethdev.h"
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
-#define RTE_PMD_DEBUG_TRACE(fmt, args...) do {                        \
-		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args); \
-	} while (0)
-#else
-#define RTE_PMD_DEBUG_TRACE(fmt, args...)
-#endif
-
 /* Macros for checking for restricting functions to primary instance only */
 #define PROC_PRIMARY_OR_ERR_RET(retval) do { \
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \
@@ -92,36 +84,6 @@
 	} \
 } while (0)
 
-/* Macros to check for invalid function pointers in dev_ops structure */
-#define RTE_ETH_FPTR_OR_ERR_RET(func, retval) do { \
-	if ((func) == NULL) { \
-		RTE_PMD_DEBUG_TRACE("Function not supported\n"); \
-		return (retval); \
-	} \
-} while (0)
-
-#define RTE_ETH_FPTR_OR_RET(func) do { \
-	if ((func) == NULL) { \
-		RTE_PMD_DEBUG_TRACE("Function not supported\n"); \
-		return; \
-	} \
-} while (0)
-
-/* Macros to check for valid port */
-#define RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, retval) do {		\
-	if (!rte_eth_dev_is_valid_port(port_id)) {		\
-		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); \
-		return retval;					\
-	}							\
-} while (0)
-
-#define RTE_ETH_VALID_PORTID_OR_RET(port_id) do {			\
-	if (!rte_eth_dev_is_valid_port(port_id)) {		\
-		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); \
-		return;						\
-	}							\
-} while (0)
-
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
 static struct rte_eth_dev_data *rte_eth_dev_data;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 7cf4af8..334fc7b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -925,6 +925,60 @@ struct rte_eth_dev_callback;
 /** @internal Structure to keep track of registered callbacks */
 TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
 
+/**
+ * @internal
+ *  Macro to print a message if in debugging mode
+ */
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
+		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
+#else
+#define RTE_PMD_DEBUG_TRACE(fmt, args...)
+#endif
+
+/**
+ * @internal
+ *  Macro to check for invalid function pointer in dev_ops structure
+ */
+#define RTE_ETH_FPTR_OR_ERR_RET(func, retval) do { \
+	if ((func) == NULL) { \
+		RTE_PMD_DEBUG_TRACE("Function not supported\n"); \
+		return retval; \
+	} \
+} while (0)
+/**
+ * @internal
+ *  Macro to check for invalid function pointer in dev_ops structure
+ */
+#define RTE_ETH_FPTR_OR_RET(func) do { \
+	if ((func) == NULL) { \
+		RTE_PMD_DEBUG_TRACE("Function not supported\n"); \
+		return; \
+	} \
+} while (0)
+
+/**
+ * @internal
+ * Macro to check for valid port id
+ */
+#define RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, retval) do {		\
+	if (!rte_eth_dev_is_valid_port(port_id)) {		\
+		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); \
+		return retval;					\
+	}							\
+} while (0)
+
+/**
+ * @internal
+ * Macro to check for valid port id
+ */
+#define RTE_ETH_VALID_PORTID_OR_RET(port_id) do {			\
+	if (!rte_eth_dev_is_valid_port(port_id)) {		\
+		RTE_PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); \
+		return;						\
+	}							\
+} while (0)
+
 /*
  * Definitions of all functions exported by an Ethernet driver through the
  * the generic structure of type *eth_dev_ops* supplied in the *rte_eth_dev*
-- 
2.4.3

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Thomas Monjalon]

2015-11-03 12:00, Bruce Richardson:
> Move the function ptr and port id checking macros to the header file, so
> that they can be used in the static inline functions there. In doxygen
> comments, mark them as for internal use only.
[...]
> +/**
> + * @internal
> + *  Macro to print a message if in debugging mode
> + */
> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> +#else
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> +#endif

It does not compile because Mellanox drivers are pedantic:

In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
/home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
/home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
 #define RTE_PMD_DEBUG_TRACE(fmt, args...) \

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> 2015-11-03 12:00, Bruce Richardson:
> > Move the function ptr and port id checking macros to the header file, so
> > that they can be used in the static inline functions there. In doxygen
> > comments, mark them as for internal use only.
> [...]
> > +/**
> > + * @internal
> > + *  Macro to print a message if in debugging mode
> > + */
> > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > +#else
> > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > +#endif
> 
> It does not compile because Mellanox drivers are pedantic:
> 
> In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
>  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \

I suggest something like the following definitions as a pedantic-proof and
standard compliant method (one drawback being that it cannot be done with a
single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
automatically appends a line feed:

 #ifdef RTE_LIBRTE_ETHDEV_DEBUG

 #define STRIP(a, b) a
 #define OPAREN (
 #define CPAREN )
 #define COMMA ,

 #define RTE_PMD_DEBUG_TRACE(...) \
         RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)

 #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
         RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)

 #define RTE_PMD_DEBUG_TRACE__(...) \
         RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)

 #else /* RTE_LIBRTE_ETHDEV_DEBUG */

 #define RTE_PMD_DEBUG_TRACE(...)

 #endif /* RTE_LIBRTE_ETHDEV_DEBUG */

STRIP() and other helper macros are used to manage the dangling comma issue
when __VA_ARGS__ is empty as in the first call below:

 RTE_PMD_DEBUG_TRACE("foo\n");
 RTE_PMD_DEBUG_TRACE("foo %u\n", 42);

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

On Wed, Nov 04, 2015 at 11:24:18AM +0100, Adrien Mazarguil wrote:
> On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > 2015-11-03 12:00, Bruce Richardson:
> > > Move the function ptr and port id checking macros to the header file, so
> > > that they can be used in the static inline functions there. In doxygen
> > > comments, mark them as for internal use only.
> > [...]
> > > +/**
> > > + * @internal
> > > + *  Macro to print a message if in debugging mode
> > > + */
> > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > +#else
> > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > +#endif
> > 
> > It does not compile because Mellanox drivers are pedantic:
> > 
> > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> >  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> 
> I suggest something like the following definitions as a pedantic-proof and
> standard compliant method (one drawback being that it cannot be done with a
> single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> automatically appends a line feed:
> 
>  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> 
>  #define STRIP(a, b) a
>  #define OPAREN (
>  #define CPAREN )
>  #define COMMA ,
> 
>  #define RTE_PMD_DEBUG_TRACE(...) \
>          RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> 
>  #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
>          RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> 
>  #define RTE_PMD_DEBUG_TRACE__(...) \
>          RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> 
>  #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> 
>  #define RTE_PMD_DEBUG_TRACE(...)
> 
>  #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> 
> STRIP() and other helper macros are used to manage the dangling comma issue
> when __VA_ARGS__ is empty as in the first call below:
> 
>  RTE_PMD_DEBUG_TRACE("foo\n");
>  RTE_PMD_DEBUG_TRACE("foo %u\n", 42);
> 
> -- 
> Adrien Mazarguil
> 6WIND

Is this really the best way around this? It looks like a lot more complicated
than the original solution. Do we really need to support the -pedantic flag
in our header files?

/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Wed, Nov 04, 2015 at 02:10:50PM +0000, Bruce Richardson wrote:
> On Wed, Nov 04, 2015 at 11:24:18AM +0100, Adrien Mazarguil wrote:
> > On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > > 2015-11-03 12:00, Bruce Richardson:
> > > > Move the function ptr and port id checking macros to the header file, so
> > > > that they can be used in the static inline functions there. In doxygen
> > > > comments, mark them as for internal use only.
> > > [...]
> > > > +/**
> > > > + * @internal
> > > > + *  Macro to print a message if in debugging mode
> > > > + */
> > > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > > +#else
> > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > > +#endif
> > > 
> > > It does not compile because Mellanox drivers are pedantic:
> > > 
> > > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> > >  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > 
> > I suggest something like the following definitions as a pedantic-proof and
> > standard compliant method (one drawback being that it cannot be done with a
> > single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> > automatically appends a line feed:
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > 
> >  #define STRIP(a, b) a
> >  #define OPAREN (
> >  #define CPAREN )
> >  #define COMMA ,
> > 
> >  #define RTE_PMD_DEBUG_TRACE(...) \
> >          RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> > 
> >  #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
> >          RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> > 
> >  #define RTE_PMD_DEBUG_TRACE__(...) \
> >          RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> > 
> >  #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> > 
> >  #define RTE_PMD_DEBUG_TRACE(...)
> > 
> >  #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> > 
> > STRIP() and other helper macros are used to manage the dangling comma issue
> > when __VA_ARGS__ is empty as in the first call below:
> > 
> >  RTE_PMD_DEBUG_TRACE("foo\n");
> >  RTE_PMD_DEBUG_TRACE("foo %u\n", 42);
> > 
> > -- 
> > Adrien Mazarguil
> > 6WIND
> 
> Is this really the best way around this? It looks like a lot more complicated
> than the original solution. Do we really need to support the -pedantic flag
> in our header files?

I know you didn't ask me directly but I think we should, at least for
exposed/installed headers. What we do internally does not matter, but we
cannot prevent user applications from adding -pedantic if they want to.

The above solution is one I suggest, perhaps it can be done in a different
manner if you think it's too complicated, although it's difficult using
macros only.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Stephen Hemminger]

On Wed, 4 Nov 2015 11:24:18 +0100
Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:

> On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > 2015-11-03 12:00, Bruce Richardson:
> > > Move the function ptr and port id checking macros to the header file, so
> > > that they can be used in the static inline functions there. In doxygen
> > > comments, mark them as for internal use only.
> > [...]
> > > +/**
> > > + * @internal
> > > + *  Macro to print a message if in debugging mode
> > > + */
> > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > +#else
> > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > +#endif
> > 
> > It does not compile because Mellanox drivers are pedantic:
> > 
> > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> >  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> 
> I suggest something like the following definitions as a pedantic-proof and
> standard compliant method (one drawback being that it cannot be done with a
> single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> automatically appends a line feed:
> 
>  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> 
>  #define STRIP(a, b) a
>  #define OPAREN (
>  #define CPAREN )
>  #define COMMA ,
> 
>  #define RTE_PMD_DEBUG_TRACE(...) \
>          RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> 
>  #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
>          RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> 
>  #define RTE_PMD_DEBUG_TRACE__(...) \
>          RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> 
>  #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> 
>  #define RTE_PMD_DEBUG_TRACE(...)
> 
>  #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> 
> STRIP() and other helper macros are used to manage the dangling comma issue
> when __VA_ARGS__ is empty as in the first call below:
> 
>  RTE_PMD_DEBUG_TRACE("foo\n");
>  RTE_PMD_DEBUG_TRACE("foo %u\n", 42);

That solution is really ugly.

Why not do something that keeps the expected checks.

diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index ede0dca..f3a3d34 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -99,6 +99,8 @@ extern struct rte_logs rte_logs;
 #define RTE_LOG_INFO     7U  /**< Informational.                    */
 #define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
 
+#define RTE_LOG_DISABLED 99U /**< Never printed			    */
+
 /** The default log stream. */
 extern FILE *eal_default_log_stream;
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index eee1194..e431f2e 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -931,6 +931,61 @@ struct rte_eth_dev_callback;
 /** @internal Structure to keep track of registered callbacks */
 TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
 
+/**
+ * @internal
+ *  Macro to print a message if in debugging mode
+ */
+#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
+	RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
+#else
+#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
+	RTE_LOG(DISABLED, PMD, "%s: " fmt, __func__, ## args)
+#endif

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Wed, Nov 04, 2015 at 10:39:57AM -0800, Stephen Hemminger wrote:
> On Wed, 4 Nov 2015 11:24:18 +0100
> Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> 
> > On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > > 2015-11-03 12:00, Bruce Richardson:
> > > > Move the function ptr and port id checking macros to the header file, so
> > > > that they can be used in the static inline functions there. In doxygen
> > > > comments, mark them as for internal use only.
> > > [...]
> > > > +/**
> > > > + * @internal
> > > > + *  Macro to print a message if in debugging mode
> > > > + */
> > > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > > +#else
> > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > > +#endif
> > > 
> > > It does not compile because Mellanox drivers are pedantic:
> > > 
> > > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> > >  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > 
> > I suggest something like the following definitions as a pedantic-proof and
> > standard compliant method (one drawback being that it cannot be done with a
> > single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> > automatically appends a line feed:
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > 
> >  #define STRIP(a, b) a
> >  #define OPAREN (
> >  #define CPAREN )
> >  #define COMMA ,
> > 
> >  #define RTE_PMD_DEBUG_TRACE(...) \
> >          RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> > 
> >  #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
> >          RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> > 
> >  #define RTE_PMD_DEBUG_TRACE__(...) \
> >          RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> > 
> >  #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> > 
> >  #define RTE_PMD_DEBUG_TRACE(...)
> > 
> >  #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> > 
> > STRIP() and other helper macros are used to manage the dangling comma issue
> > when __VA_ARGS__ is empty as in the first call below:
> > 
> >  RTE_PMD_DEBUG_TRACE("foo\n");
> >  RTE_PMD_DEBUG_TRACE("foo %u\n", 42);
> 
> That solution is really ugly.

I won't argue against this as it's obviously more complex than the original
method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
have to modify their code. They shouldn't care about the implementation.

Also note that we can do much cleaner code if we drop the all macros
implementation using a (much easier to debug) static inline function,
only perhaps with a wrapper macro that provides __LINE__, __func__ and
__FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.

> Why not do something that keeps the expected checks.

Sure but it's not the issue, we're discussing errors related to
-pedantic. I've only made the above suggestion to pass its pedantic
checks. RTE_LOG_DISABLED can be managed with these macros as well.

> diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> index ede0dca..f3a3d34 100644
> --- a/lib/librte_eal/common/include/rte_log.h
> +++ b/lib/librte_eal/common/include/rte_log.h
> @@ -99,6 +99,8 @@ extern struct rte_logs rte_logs;
>  #define RTE_LOG_INFO     7U  /**< Informational.                    */
>  #define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
>  
> +#define RTE_LOG_DISABLED 99U /**< Never printed			    */
> +
>  /** The default log stream. */
>  extern FILE *eal_default_log_stream;
>  
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index eee1194..e431f2e 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -931,6 +931,61 @@ struct rte_eth_dev_callback;
>  /** @internal Structure to keep track of registered callbacks */
>  TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
>  
> +/**
> + * @internal
> + *  Macro to print a message if in debugging mode
> + */
> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> +	RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> +#else
> +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> +	RTE_LOG(DISABLED, PMD, "%s: " fmt, __func__, ## args)
> +#endif

My previous message was probably not clear enough about the reason for this
error. With -pedantic, GCC complains about these bits:

- "args..." causing "error: ISO C does not permit named variadic
  macros", as in C function you cannot put an ellipsis directly behind a
  token without a comma.

- ", ## args" for which I can't recall the error, but pasting a comma and
  args is also nonstandard, especially when args happens to be empty.
  Without -pedantic, GCC silently drops the comma.

Bruce is asking for a consensus about -pedantic, whether we want to do the
extra effort to support it in DPDK. Since I like checking for -pedantic
errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
debugging mode. There is currently no established rule in DPDK against this.

I'm arguing that most C headers (C compiler, libc, most libraries, even the
Linux kernel in uapi to an extent) provide standards compliant includes
because they cannot predict or force particular compilation flags on
user applications.

If we consider DPDK as a system wide library, I think we should do it as
well in all installed header files. If we choose not to, then we must
document that our code is not standard, -pedantic is unsupported and I'll
have to drop it from mlx4 and mlx5.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> On Wed, Nov 04, 2015 at 10:39:57AM -0800, Stephen Hemminger wrote:
> > On Wed, 4 Nov 2015 11:24:18 +0100
> > Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> > 
> > > On Wed, Nov 04, 2015 at 02:19:36AM +0100, Thomas Monjalon wrote:
> > > > 2015-11-03 12:00, Bruce Richardson:
> > > > > Move the function ptr and port id checking macros to the header file, so
> > > > > that they can be used in the static inline functions there. In doxygen
> > > > > comments, mark them as for internal use only.
> > > > [...]
> > > > > +/**
> > > > > + * @internal
> > > > > + *  Macro to print a message if in debugging mode
> > > > > + */
> > > > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > > > +		RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > > > > +#else
> > > > > +#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > > > > +#endif
> > > > 
> > > > It does not compile because Mellanox drivers are pedantic:
> > > > 
> > > > In file included from /home/thomas/projects/dpdk/dpdk/drivers/net/mlx4/mlx4.c:78:0:
> > > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h: At top level:
> > > > /home/thomas/projects/dpdk/dpdk/x86_64-native-linuxapp-gcc-shared-next/include/rte_ethdev.h:933:38: error: ISO C does not permit named variadic macros [-Werror=variadic-macros]
> > > >  #define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > > 
> > > I suggest something like the following definitions as a pedantic-proof and
> > > standard compliant method (one drawback being that it cannot be done with a
> > > single macro), see PMD_DRV_LOG() in drivers/net/mlx5/mlx5_utils.h which also
> > > automatically appends a line feed:
> > > 
> > >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > 
> > >  #define STRIP(a, b) a
> > >  #define OPAREN (
> > >  #define CPAREN )
> > >  #define COMMA ,
> > > 
> > >  #define RTE_PMD_DEBUG_TRACE(...) \
> > >          RTE_PMD_DEBUG_TRACE_(__VA_ARGS__ STRIP OPAREN, CPAREN)
> > > 
> > >  #define RTE_PMD_DEBUG_TRACE_(fmt, ...) \
> > >          RTE_PMD_DEBUG_TRACE__(fmt COMMA __func__, __VA_ARGS__)
> > > 
> > >  #define RTE_PMD_DEBUG_TRACE__(...) \
> > >          RTE_LOG(ERR, PMD, "%s: " __VA_ARGS__)
> > > 
> > >  #else /* RTE_LIBRTE_ETHDEV_DEBUG */
> > > 
> > >  #define RTE_PMD_DEBUG_TRACE(...)
> > > 
> > >  #endif /* RTE_LIBRTE_ETHDEV_DEBUG */
> > > 
> > > STRIP() and other helper macros are used to manage the dangling comma issue
> > > when __VA_ARGS__ is empty as in the first call below:
> > > 
> > >  RTE_PMD_DEBUG_TRACE("foo\n");
> > >  RTE_PMD_DEBUG_TRACE("foo %u\n", 42);
> > 
> > That solution is really ugly.
> 
> I won't argue against this as it's obviously more complex than the original
> method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
> have to modify their code. They shouldn't care about the implementation.
> 
> Also note that we can do much cleaner code if we drop the all macros
> implementation using a (much easier to debug) static inline function,
> only perhaps with a wrapper macro that provides __LINE__, __func__ and
> __FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.
> 

+1 to this. I was planning to seeing if a static inline could help here, but
haven't had the chance to try it yet.

> > Why not do something that keeps the expected checks.
> 
> Sure but it's not the issue, we're discussing errors related to
> -pedantic. I've only made the above suggestion to pass its pedantic
> checks. RTE_LOG_DISABLED can be managed with these macros as well.
> 
> > diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
> > index ede0dca..f3a3d34 100644
> > --- a/lib/librte_eal/common/include/rte_log.h
> > +++ b/lib/librte_eal/common/include/rte_log.h
> > @@ -99,6 +99,8 @@ extern struct rte_logs rte_logs;
> >  #define RTE_LOG_INFO     7U  /**< Informational.                    */
> >  #define RTE_LOG_DEBUG    8U  /**< Debug-level messages.             */
> >  
> > +#define RTE_LOG_DISABLED 99U /**< Never printed			    */
> > +
> >  /** The default log stream. */
> >  extern FILE *eal_default_log_stream;
> >  
> > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> > index eee1194..e431f2e 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -931,6 +931,61 @@ struct rte_eth_dev_callback;
> >  /** @internal Structure to keep track of registered callbacks */
> >  TAILQ_HEAD(rte_eth_dev_cb_list, rte_eth_dev_callback);
> >  
> > +/**
> > + * @internal
> > + *  Macro to print a message if in debugging mode
> > + */
> > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > +	RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > +#else
> > +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > +	RTE_LOG(DISABLED, PMD, "%s: " fmt, __func__, ## args)
> > +#endif
> 
> My previous message was probably not clear enough about the reason for this
> error. With -pedantic, GCC complains about these bits:
> 
> - "args..." causing "error: ISO C does not permit named variadic
>   macros", as in C function you cannot put an ellipsis directly behind a
>   token without a comma.
> 
> - ", ## args" for which I can't recall the error, but pasting a comma and
>   args is also nonstandard, especially when args happens to be empty.
>   Without -pedantic, GCC silently drops the comma.
> 
> Bruce is asking for a consensus about -pedantic, whether we want to do the
> extra effort to support it in DPDK. Since I like checking for -pedantic
> errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
> debugging mode. There is currently no established rule in DPDK against this.
> 
> I'm arguing that most C headers (C compiler, libc, most libraries, even the
> Linux kernel in uapi to an extent) provide standards compliant includes
> because they cannot predict or force particular compilation flags on
> user applications.
> 
> If we consider DPDK as a system wide library, I think we should do it as
> well in all installed header files. If we choose not to, then we must
> document that our code is not standard, -pedantic is unsupported and I'll
> have to drop it from mlx4 and mlx5.
> 
> -- 

I'm in favour in principle of being standards compliant so long as the cost is
not extremely high. If we have to go through a lot of macro gymnastics to get
things working in order to support pedantic, I'd be of the opinion that the cost
of supporting that particular flag is too high. Each one will probably have his
own opinion of this.

Hopefully a static inline can provide a good compromise solution that everyone
can live with. I'll look at it as soon as I can.

/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> Bruce is asking for a consensus about -pedantic, whether we want to do the
> extra effort to support it in DPDK. Since I like checking for -pedantic
> errors, it's enabled for mlx4 and mlx5 when compiling these drivers in
> debugging mode. There is currently no established rule in DPDK against this.
> 
> I'm arguing that most C headers (C compiler, libc, most libraries, even the
> Linux kernel in uapi to an extent) provide standards compliant includes
> because they cannot predict or force particular compilation flags on
> user applications.
> 
> If we consider DPDK as a system wide library, I think we should do it as
> well in all installed header files. If we choose not to, then we must
> document that our code is not standard, -pedantic is unsupported and I'll
> have to drop it from mlx4 and mlx5.
> 
> -- 
> Adrien Mazarguil
> 6WIND

Hi Adrien,

I'm trying to dig into this a bit more now, and try out using a static inline
function, but I'm having trouble getting DPDK to compile with the mlx drivers
turned on in the config. I'm trying to follow the instructions here:
http://dpdk.org/doc/guides/nics/mlx4.html, but it's not clearly called out what
requirements are for compilation vs requirements for running the PMD.

I'm running Fedora 23, and installed the libibverbs-devel package, but when I
compile I get the following error:

== Build drivers/net/mlx4
  CC mlx4.o
  /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c: In function ‘txq_cleanup’:
  /home/bruce/ethdev-cleanup/drivers/net/mlx4/mlx4.c:886:37: error: storage size of ‘params’ isn’t known
    struct ibv_exp_release_intf_params params;
                                       ^
compilation terminated due to -Wfatal-errors.

Any suggestions on the fix for this?

Thanks,
/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> 
> I won't argue against this as it's obviously more complex than the original
> method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
> have to modify their code. They shouldn't care about the implementation.
> 
> Also note that we can do much cleaner code if we drop the all macros
> implementation using a (much easier to debug) static inline function,
> only perhaps with a wrapper macro that provides __LINE__, __func__ and
> __FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.
> 
Getting something working with __FILE__ and probably __LINE__ would be easy enough
with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
[since the pre-processor has no idea what function you are in].

However, using func, here is the best I've come up with so far. It's not that
pretty, but it's probably easier to work with than the macro version.

 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
 -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
 +#define RTE_PMD_DEBUG_TRACE(...) \
 +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
 +
 +static inline void
 +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
 +{
 +       static __thread char buffer[128];
 +       char *out_buf = buffer;
 +       unsigned count;
 +       va_list ap;
 +
 +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
 +       if (count >= sizeof(buffer)) { // truncated output
 +               char *new_buf = malloc(count + 1);
 +               if (new_buf == NULL) // no memory, just print 128 chars
 +                       goto print_buffer;
 +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
 +               va_start(ap, fmt);
 +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
 +               va_end(ap);
 +               free(new_buf);
 +               return;
 +       }
 +
 +print_buffer:
 +       va_start(ap, fmt);
 +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
 +       va_end(ap);
 +}
  #else
  #define RTE_PMD_DEBUG_TRACE(fmt, args...)
  #endif

Comments or improvements?

/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > 
> > I won't argue against this as it's obviously more complex than the original
> > method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
> > have to modify their code. They shouldn't care about the implementation.
> > 
> > Also note that we can do much cleaner code if we drop the all macros
> > implementation using a (much easier to debug) static inline function,
> > only perhaps with a wrapper macro that provides __LINE__, __func__ and
> > __FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.
> > 
> Getting something working with __FILE__ and probably __LINE__ would be easy enough
> with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
> [since the pre-processor has no idea what function you are in].
> 
> However, using func, here is the best I've come up with so far. It's not that
> pretty, but it's probably easier to work with than the macro version.
> 
>  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
>  +#define RTE_PMD_DEBUG_TRACE(...) \
>  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
>  +
>  +static inline void
>  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>  +{
>  +       static __thread char buffer[128];
>  +       char *out_buf = buffer;
>  +       unsigned count;
>  +       va_list ap;
>  +
>  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
>  +       if (count >= sizeof(buffer)) { // truncated output
>  +               char *new_buf = malloc(count + 1);
>  +               if (new_buf == NULL) // no memory, just print 128 chars
>  +                       goto print_buffer;
>  +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
>  +               va_start(ap, fmt);
>  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
>  +               va_end(ap);
>  +               free(new_buf);
>  +               return;
>  +       }
>  +
>  +print_buffer:
>  +       va_start(ap, fmt);
>  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
>  +       va_end(ap);
>  +}
>   #else
>   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
>   #endif
> 
> Comments or improvements?
> 
> /Bruce

And here's the version if we are happy to have file and line number instead of
function name. I think this might be the best option.

/Bruce

 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
 -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
 +#define RTE_PMD_DEBUG_TRACE(...) \
 +       RTE_LOG(ERR, PMD, __FILE__", " RTE_STR(__LINE__) ": " __VA_ARGS__)
  #else
 -#define RTE_PMD_DEBUG_TRACE(fmt, args...)
 +#define RTE_PMD_DEBUG_TRACE(...)
  #endif

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
> On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > 
> > > I won't argue against this as it's obviously more complex than the original
> > > method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
> > > have to modify their code. They shouldn't care about the implementation.
> > > 
> > > Also note that we can do much cleaner code if we drop the all macros
> > > implementation using a (much easier to debug) static inline function,
> > > only perhaps with a wrapper macro that provides __LINE__, __func__ and
> > > __FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.
> > > 
> > Getting something working with __FILE__ and probably __LINE__ would be easy enough
> > with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
> > [since the pre-processor has no idea what function you are in].
> > 
> > However, using func, here is the best I've come up with so far. It's not that
> > pretty, but it's probably easier to work with than the macro version.
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> >  +#define RTE_PMD_DEBUG_TRACE(...) \
> >  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
> >  +
> >  +static inline void
> >  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> >  +{
> >  +       static __thread char buffer[128];
> >  +       char *out_buf = buffer;
> >  +       unsigned count;
> >  +       va_list ap;
> >  +
> >  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
> >  +       if (count >= sizeof(buffer)) { // truncated output
> >  +               char *new_buf = malloc(count + 1);
> >  +               if (new_buf == NULL) // no memory, just print 128 chars
> >  +                       goto print_buffer;
> >  +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
> >  +               va_start(ap, fmt);
> >  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
> >  +               va_end(ap);
> >  +               free(new_buf);
> >  +               return;
> >  +       }
> >  +
> >  +print_buffer:
> >  +       va_start(ap, fmt);
> >  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
> >  +       va_end(ap);
> >  +}
> >   #else
> >   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
> >   #endif
> > 
> > Comments or improvements?

Such a function shouldn't malloc() anything. The entire line should fit on
the stack (crashing is fine if it does not, then it's probably a bug). We
did something in two passes along these lines in mlx5_defs.h (not pretty but
quite useful):

 /* Allocate a buffer on the stack and fill it with a printf format string. */
 #define MKSTR(name, ...) \
         char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
         \
         snprintf(name, sizeof(name), __VA_ARGS__)

Untested but I guess modifying that function accordingly would look like:

 static inline void
 rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
 {
         va_list ap;
         va_start(ap, fmt);

         static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];

         va_end(ap);
	 va_start(ap, fmt);
         vsnprintf(buffer, sizeof(buffer), fmt, ap);
	 va_end(ap);
         rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name, buffer);
 }

> And here's the version if we are happy to have file and line number instead of
> function name. I think this might be the best option.
> 
> /Bruce
> 
>  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
>  +#define RTE_PMD_DEBUG_TRACE(...) \
>  +       RTE_LOG(ERR, PMD, __FILE__", " RTE_STR(__LINE__) ": " __VA_ARGS__)
>   #else
>  -#define RTE_PMD_DEBUG_TRACE(fmt, args...)
>  +#define RTE_PMD_DEBUG_TRACE(...)
>   #endif

Much cleaner indeed, however __func__ might be useful when comparing log
outputs from different source code versions. I think we should keep it.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Mon, Nov 09, 2015 at 02:39:05PM +0100, Adrien Mazarguil wrote:
> On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
> > On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> > > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > > 
> > > > I won't argue against this as it's obviously more complex than the original
> > > > method, however note that users of the RTE_PMD_DEBUG_TRACE() macro do not
> > > > have to modify their code. They shouldn't care about the implementation.
> > > > 
> > > > Also note that we can do much cleaner code if we drop the all macros
> > > > implementation using a (much easier to debug) static inline function,
> > > > only perhaps with a wrapper macro that provides __LINE__, __func__ and
> > > > __FILE__ as arguments. Nontrival code shouldn't be done in macros anyway.
> > > > 
> > > Getting something working with __FILE__ and probably __LINE__ would be easy enough
> > > with a helper macro, but __func__ is not so easy as it's not a preprocessor symbol
> > > [since the pre-processor has no idea what function you are in].
> > > 
> > > However, using func, here is the best I've come up with so far. It's not that
> > > pretty, but it's probably easier to work with than the macro version.
> > > 
> > >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > >  +#define RTE_PMD_DEBUG_TRACE(...) \
> > >  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
> > >  +
> > >  +static inline void
> > >  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> > >  +{
> > >  +       static __thread char buffer[128];
> > >  +       char *out_buf = buffer;
> > >  +       unsigned count;
> > >  +       va_list ap;
> > >  +
> > >  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name, fmt);
> > >  +       if (count >= sizeof(buffer)) { // truncated output
> > >  +               char *new_buf = malloc(count + 1);
> > >  +               if (new_buf == NULL) // no memory, just print 128 chars
> > >  +                       goto print_buffer;
> > >  +               snprintf(new_buf, count + 1, "%s: %s", func_name, fmt);
> > >  +               va_start(ap, fmt);
> > >  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
> > >  +               va_end(ap);
> > >  +               free(new_buf);
> > >  +               return;
> > >  +       }
> > >  +
> > >  +print_buffer:
> > >  +       va_start(ap, fmt);
> > >  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
> > >  +       va_end(ap);
> > >  +}
> > >   #else
> > >   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > >   #endif
> > > 
> > > Comments or improvements?
> 
> Such a function shouldn't malloc() anything. The entire line should fit on
> the stack (crashing is fine if it does not, then it's probably a bug). We
> did something in two passes along these lines in mlx5_defs.h (not pretty but
> quite useful):
> 
>  /* Allocate a buffer on the stack and fill it with a printf format string. */
>  #define MKSTR(name, ...) \
>          char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
>          \
>          snprintf(name, sizeof(name), __VA_ARGS__)
> 
> Untested but I guess modifying that function accordingly would look like:
> 
>  static inline void
>  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>  {
>          va_list ap;
>          va_start(ap, fmt);
> 
>          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];

Of course this line should read:

         static __thread char buffer[vsnprintf(NULL, 0, fmt, ap) + 1];

>          va_end(ap);
>          va_start(ap, fmt);
>          vsnprintf(buffer, sizeof(buffer), fmt, ap);
>          va_end(ap);
>          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name, buffer);
>  }
> 
> > And here's the version if we are happy to have file and line number instead of
> > function name. I think this might be the best option.
> > 
> > /Bruce
> > 
> >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> >  +#define RTE_PMD_DEBUG_TRACE(...) \
> >  +       RTE_LOG(ERR, PMD, __FILE__", " RTE_STR(__LINE__) ": " __VA_ARGS__)
> >   #else
> >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...)
> >  +#define RTE_PMD_DEBUG_TRACE(...)
> >   #endif
> 
> Much cleaner indeed, however __func__ might be useful when comparing log
> outputs from different source code versions. I think we should keep it.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Richardson, Bruce]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Monday, November 9, 2015 1:39 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> to header
> 
> On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
> > On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
> > > On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
> > > >
> > > > I won't argue against this as it's obviously more complex than the
> original
> > > > method, however note that users of the RTE_PMD_DEBUG_TRACE() macro
> do not
> > > > have to modify their code. They shouldn't care about the
> implementation.
> > > >
> > > > Also note that we can do much cleaner code if we drop the all macros
> > > > implementation using a (much easier to debug) static inline
> function,
> > > > only perhaps with a wrapper macro that provides __LINE__, __func__
> and
> > > > __FILE__ as arguments. Nontrival code shouldn't be done in macros
> anyway.
> > > >
> > > Getting something working with __FILE__ and probably __LINE__ would be
> easy enough
> > > with a helper macro, but __func__ is not so easy as it's not a
> preprocessor symbol
> > > [since the pre-processor has no idea what function you are in].
> > >
> > > However, using func, here is the best I've come up with so far. It's
> not that
> > > pretty, but it's probably easier to work with than the macro version.
> > >
> > >  #ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > >  -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
> > >  -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
> > >  +#define RTE_PMD_DEBUG_TRACE(...) \
> > >  +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
> > >  +
> > >  +static inline void
> > >  +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> > >  +{
> > >  +       static __thread char buffer[128];
> > >  +       char *out_buf = buffer;
> > >  +       unsigned count;
> > >  +       va_list ap;
> > >  +
> > >  +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name,
> fmt);
> > >  +       if (count >= sizeof(buffer)) { // truncated output
> > >  +               char *new_buf = malloc(count + 1);
> > >  +               if (new_buf == NULL) // no memory, just print 128
> chars
> > >  +                       goto print_buffer;
> > >  +               snprintf(new_buf, count + 1, "%s: %s", func_name,
> fmt);
> > >  +               va_start(ap, fmt);
> > >  +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
> > >  +               va_end(ap);
> > >  +               free(new_buf);
> > >  +               return;
> > >  +       }
> > >  +
> > >  +print_buffer:
> > >  +       va_start(ap, fmt);
> > >  +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
> > >  +       va_end(ap);
> > >  +}
> > >   #else
> > >   #define RTE_PMD_DEBUG_TRACE(fmt, args...)
> > >   #endif
> > >
> > > Comments or improvements?
> 
> Such a function shouldn't malloc() anything. The entire line should fit on
> the stack (crashing is fine if it does not, then it's probably a bug). We
> did something in two passes along these lines in mlx5_defs.h (not pretty
> but
> quite useful):
> 
>  /* Allocate a buffer on the stack and fill it with a printf format
> string. */
>  #define MKSTR(name, ...) \
>          char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
>          \
>          snprintf(name, sizeof(name), __VA_ARGS__)
> 
> Untested but I guess modifying that function accordingly would look like:
> 
>  static inline void
>  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>  {
>          va_list ap;
>          va_start(ap, fmt);
> 
>          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> 
>          va_end(ap);
> 	 va_start(ap, fmt);
>          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> 	 va_end(ap);
>          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> buffer);
>  }
> 

Looks a much better option.

>From this, though, I assume then that we are only looking to support the -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic with the pre-gcc-5 versions won't allow that to work though, as variably sized arrays only came in with c99, and were gnu extensions before that.

Regards,
/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Declan Doherty]

On 09/11/15 14:02, Richardson, Bruce wrote:
>
>
>> -----Original Message-----
>> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
>> Sent: Monday, November 9, 2015 1:39 PM
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
>> to header
>>
>> On Fri, Nov 06, 2015 at 05:22:27PM +0000, Bruce Richardson wrote:
>>> On Fri, Nov 06, 2015 at 05:10:07PM +0000, Bruce Richardson wrote:
>>>> On Thu, Nov 05, 2015 at 04:09:18PM +0100, Adrien Mazarguil wrote:
>>>>>
>>>>> I won't argue against this as it's obviously more complex than the
>> original
>>>>> method, however note that users of the RTE_PMD_DEBUG_TRACE() macro
>> do not
>>>>> have to modify their code. They shouldn't care about the
>> implementation.
>>>>>
>>>>> Also note that we can do much cleaner code if we drop the all macros
>>>>> implementation using a (much easier to debug) static inline
>> function,
>>>>> only perhaps with a wrapper macro that provides __LINE__, __func__
>> and
>>>>> __FILE__ as arguments. Nontrival code shouldn't be done in macros
>> anyway.
>>>>>
>>>> Getting something working with __FILE__ and probably __LINE__ would be
>> easy enough
>>>> with a helper macro, but __func__ is not so easy as it's not a
>> preprocessor symbol
>>>> [since the pre-processor has no idea what function you are in].
>>>>
>>>> However, using func, here is the best I've come up with so far. It's
>> not that
>>>> pretty, but it's probably easier to work with than the macro version.
>>>>
>>>>   #ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>>   -#define RTE_PMD_DEBUG_TRACE(fmt, args...) \
>>>>   -               RTE_LOG(ERR, PMD, "%s: " fmt, __func__, ## args)
>>>>   +#define RTE_PMD_DEBUG_TRACE(...) \
>>>>   +               rte_pmd_debug_trace(__func__, __VA_ARGS__)
>>>>   +
>>>>   +static inline void
>>>>   +rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>>>>   +{
>>>>   +       static __thread char buffer[128];
>>>>   +       char *out_buf = buffer;
>>>>   +       unsigned count;
>>>>   +       va_list ap;
>>>>   +
>>>>   +       count = snprintf(buffer, sizeof(buffer), "%s: %s", func_name,
>> fmt);
>>>>   +       if (count >= sizeof(buffer)) { // truncated output
>>>>   +               char *new_buf = malloc(count + 1);
>>>>   +               if (new_buf == NULL) // no memory, just print 128
>> chars
>>>>   +                       goto print_buffer;
>>>>   +               snprintf(new_buf, count + 1, "%s: %s", func_name,
>> fmt);
>>>>   +               va_start(ap, fmt);
>>>>   +               rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, buffer, ap);
>>>>   +               va_end(ap);
>>>>   +               free(new_buf);
>>>>   +               return;
>>>>   +       }
>>>>   +
>>>>   +print_buffer:
>>>>   +       va_start(ap, fmt);
>>>>   +       rte_vlog(RTE_LOG_ERR, RTE_LOGTYPE_PMD, out_buf, ap);
>>>>   +       va_end(ap);
>>>>   +}
>>>>    #else
>>>>    #define RTE_PMD_DEBUG_TRACE(fmt, args...)
>>>>    #endif
>>>>
>>>> Comments or improvements?
>>
>> Such a function shouldn't malloc() anything. The entire line should fit on
>> the stack (crashing is fine if it does not, then it's probably a bug). We
>> did something in two passes along these lines in mlx5_defs.h (not pretty
>> but
>> quite useful):
>>
>>   /* Allocate a buffer on the stack and fill it with a printf format
>> string. */
>>   #define MKSTR(name, ...) \
>>           char name[snprintf(NULL, 0, __VA_ARGS__) + 1]; \
>>           \
>>           snprintf(name, sizeof(name), __VA_ARGS__)
>>
>> Untested but I guess modifying that function accordingly would look like:
>>
>>   static inline void
>>   rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
>>   {
>>           va_list ap;
>>           va_start(ap, fmt);
>>
>>           static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
>>
>>           va_end(ap);
>> 	 va_start(ap, fmt);
>>           vsnprintf(buffer, sizeof(buffer), fmt, ap);
>> 	 va_end(ap);
>>           rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
>> buffer);
>>   }
>>
>
> Looks a much better option.
>
>  From this, though, I assume then that we are only looking to support the -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic with the pre-gcc-5 versions won't allow that to work though, as variably sized arrays only came in with c99, and were gnu extensions before that.
>
> Regards,
> /Bruce
>

I had made some similar changes in the cryptodev patch set to allow 
these macros to be shared between the ethdev and cryptodev, but I wasn't 
aware of the -pedantic build issues. I've incorporate the changes from 
patch 1 & 2 discussed here into the cryptodev patch set. See patch 2/10 
(http://dpdk.org/ml/archives/dev/2015-November/027888.html) for the 
implementation of the rte_pmf_debug_trace function. Any comments or 
ack's are welcome :)

Declan

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
[...]
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
[...]
> > Untested but I guess modifying that function accordingly would look like:
> > 
> >  static inline void
> >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)
> >  {
> >          va_list ap;
> >          va_start(ap, fmt);
> > 
> >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > 
> >          va_end(ap);
> > 	 va_start(ap, fmt);
> >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > 	 va_end(ap);
> >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > buffer);
> >  }
> > 
> 
> Looks a much better option.
> 
> From this, though, I assume then that we are only looking to support the -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic with the pre-gcc-5 versions won't allow that to work though, as variably sized arrays only came in with c99, and were gnu extensions before that.

Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
it's meaningless.

However pre-GCC 5 is fine for most if not all features we use, see:

 https://gcc.gnu.org/c99status.html

Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
as we use a version that implements -std=gnu99 (or -std=c99 to be really
pedantic), it's fine.

Besides DPDK already uses C99 extensively, even a few C11 features (such as
embedded anonymous struct definitions) currently supported in C99 mode as
compiler extensions. I think we can safely ignore compilers that don't
support common C99 features.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Richardson, Bruce]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Tuesday, November 10, 2015 4:08 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> to header
> 
> On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
> [...]
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> [...]
> > > Untested but I guess modifying that function accordingly would look
> like:
> > >
> > >  static inline void
> > >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)  {
> > >          va_list ap;
> > >          va_start(ap, fmt);
> > >
> > >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > >
> > >          va_end(ap);
> > > 	 va_start(ap, fmt);
> > >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > > 	 va_end(ap);
> > >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > > buffer);  }
> > >
> >
> > Looks a much better option.
> >
> > From this, though, I assume then that we are only looking to support the
> -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic
> with the pre-gcc-5 versions won't allow that to work though, as variably
> sized arrays only came in with c99, and were gnu extensions before that.
> 
> Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
> it's meaningless.
> 
> However pre-GCC 5 is fine for most if not all features we use, see:
> 
>  https://gcc.gnu.org/c99status.html
> 
> Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
> macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
> as we use a version that implements -std=gnu99 (or -std=c99 to be really
> pedantic), it's fine.
> 
> Besides DPDK already uses C99 extensively, even a few C11 features (such
> as
> embedded anonymous struct definitions) currently supported in C99 mode as
> compiler extensions. I think we can safely ignore compilers that don't
> support common C99 features.
> 
> --
> Adrien Mazarguil
> 6WIND

Actually my point was slightly different. 
If we are supporting "-pedantic" in header files because "we don't know what compiler flags users are going to pass when", then we need to support it in C90 mode to do the job properly. If you take gcc 4.8 and compile some code with "-pedantic" as the only C-flag you are going to get lots of errors because it will default to C90 mode with pedantic, which means no varargs macros at all. 
This limits the usefulness of supporting pedantic flag at all in our header files, since we only support it in certain situations with non-latest compilers.

/Bruce

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Adrien Mazarguil]

On Tue, Nov 10, 2015 at 04:21:10PM +0000, Richardson, Bruce wrote:
> 
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Tuesday, November 10, 2015 4:08 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> > <thomas.monjalon@6wind.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> > to header
> > 
> > On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
> > [...]
> > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > [...]
> > > > Untested but I guess modifying that function accordingly would look
> > like:
> > > >
> > > >  static inline void
> > > >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)  {
> > > >          va_list ap;
> > > >          va_start(ap, fmt);
> > > >
> > > >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > > >
> > > >          va_end(ap);
> > > > 	 va_start(ap, fmt);
> > > >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > > > 	 va_end(ap);
> > > >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > > > buffer);  }
> > > >
> > >
> > > Looks a much better option.
> > >
> > > From this, though, I assume then that we are only looking to support the
> > -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic
> > with the pre-gcc-5 versions won't allow that to work though, as variably
> > sized arrays only came in with c99, and were gnu extensions before that.
> > 
> > Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
> > it's meaningless.
> > 
> > However pre-GCC 5 is fine for most if not all features we use, see:
> > 
> >  https://gcc.gnu.org/c99status.html
> > 
> > Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
> > macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
> > as we use a version that implements -std=gnu99 (or -std=c99 to be really
> > pedantic), it's fine.
> > 
> > Besides DPDK already uses C99 extensively, even a few C11 features (such
> > as
> > embedded anonymous struct definitions) currently supported in C99 mode as
> > compiler extensions. I think we can safely ignore compilers that don't
> > support common C99 features.
> > 
> > --
> > Adrien Mazarguil
> > 6WIND
> 
> Actually my point was slightly different. 
> If we are supporting "-pedantic" in header files because "we don't know what compiler flags users are going to pass when", then we need to support it in C90 mode to do the job properly. If you take gcc 4.8 and compile some code with "-pedantic" as the only C-flag you are going to get lots of errors because it will default to C90 mode with pedantic, which means no varargs macros at all. 

Agreed, exported headers should actually be C90 compliant for these reasons
but C99 would be a start. I didn't know GCC 5 switched to C99 by default
(don't worry, I do not intend to go back to C90).

> This limits the usefulness of supporting pedantic flag at all in our header files, since we only support it in certain situations with non-latest compilers.

I won't deny this, as the goal is partly to appease pedantic people like
myself. Using standard methods for doing things should be preferred over
extensions for compatibility with the unknown, unless there is no other way.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros to header

[Bruce Richardson]

On Tue, Nov 10, 2015 at 06:12:28PM +0100, Adrien Mazarguil wrote:
> On Tue, Nov 10, 2015 at 04:21:10PM +0000, Richardson, Bruce wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > Sent: Tuesday, November 10, 2015 4:08 PM
> > > To: Richardson, Bruce <bruce.richardson@intel.com>
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Thomas Monjalon
> > > <thomas.monjalon@6wind.com>; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: move error checking macros
> > > to header
> > > 
> > > On Mon, Nov 09, 2015 at 02:02:28PM +0000, Richardson, Bruce wrote:
> > > [...]
> > > > > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > > [...]
> > > > > Untested but I guess modifying that function accordingly would look
> > > like:
> > > > >
> > > > >  static inline void
> > > > >  rte_pmd_debug_trace(const char *func_name, const char *fmt, ...)  {
> > > > >          va_list ap;
> > > > >          va_start(ap, fmt);
> > > > >
> > > > >          static __thread char buffer[vsnprintf(NULL, 0, fmt, ap)];
> > > > >
> > > > >          va_end(ap);
> > > > > 	 va_start(ap, fmt);
> > > > >          vsnprintf(buffer, sizeof(buffer), fmt, ap);
> > > > > 	 va_end(ap);
> > > > >          rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: %s", func_name,
> > > > > buffer);  }
> > > > >
> > > >
> > > > Looks a much better option.
> > > >
> > > > From this, though, I assume then that we are only looking to support the
> > > -pedantic flag in conjuction with c99 mode or above. Supporting -pedantic
> > > with the pre-gcc-5 versions won't allow that to work though, as variably
> > > sized arrays only came in with c99, and were gnu extensions before that.
> > > 
> > > Right, -pedantic must follow a given standard such as -std=gnu99 otherwise
> > > it's meaningless.
> > > 
> > > However pre-GCC 5 is fine for most if not all features we use, see:
> > > 
> > >  https://gcc.gnu.org/c99status.html
> > > 
> > > Mixed code and declarations are supported since GCC 3.0, __VA_ARGS__ in
> > > macros since GCC 2.95 and variable length arrays since GCC 0.9, so as long
> > > as we use a version that implements -std=gnu99 (or -std=c99 to be really
> > > pedantic), it's fine.
> > > 
> > > Besides DPDK already uses C99 extensively, even a few C11 features (such
> > > as
> > > embedded anonymous struct definitions) currently supported in C99 mode as
> > > compiler extensions. I think we can safely ignore compilers that don't
> > > support common C99 features.
> > > 
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> > 
> > Actually my point was slightly different. 
> > If we are supporting "-pedantic" in header files because "we don't know what compiler flags users are going to pass when", then we need to support it in C90 mode to do the job properly. If you take gcc 4.8 and compile some code with "-pedantic" as the only C-flag you are going to get lots of errors because it will default to C90 mode with pedantic, which means no varargs macros at all. 
> 
> Agreed, exported headers should actually be C90 compliant for these reasons
> but C99 would be a start. I didn't know GCC 5 switched to C99 by default
> (don't worry, I do not intend to go back to C90).
> 
Actually, it's even better than C99, the default is now C11 (or gnu11 to be pedantic about it :-) )

https://gcc.gnu.org/gcc-5/changes.html

Top line item is: 
	"The default mode for C is now -std=gnu11 instead of -std=gnu89"

I believe clang is making a similar change to a c11-based default. \o/

/Bruce