[dpdk-dev] [PATCH 1.6.1] don't inline rte_string_fns

[dpdk-dev] [PATCH 1.6.1] don't inline rte_string_fns

[Stephen Hemminger]

The function rte_snprintf() can never be inlined by Gcc. 
If compiled with -Winline it generates an error:
 function ‘rte_snprintf’ can never be inlined because it uses variable argument lists [-Werror=inline]

Therefore since both rte_snprintf and rte_strsplit are not performance sensitive
just move them to being real functions.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_eal_fs.c                         |    1 +
 lib/librte_eal/common/eal_common_string_fns.c  |   95 ++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_string_fns.h |   58 +--------------
 lib/librte_eal/linuxapp/eal/Makefile           |    1 +
 4 files changed, 101 insertions(+), 54 deletions(-)
 create mode 100644 lib/librte_eal/common/eal_common_string_fns.c

diff --git a/app/test/test_eal_fs.c b/app/test/test_eal_fs.c
index 0a7524d..2c4bdc6 100644
--- a/app/test/test_eal_fs.c
+++ b/app/test/test_eal_fs.c
@@ -36,6 +36,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <errno.h>
 
 /* eal_filesystem.h is not a public header file, so use relative path */
 #include "../../lib/librte_eal/linuxapp/eal/include/eal_filesystem.h"
diff --git a/lib/librte_eal/common/eal_common_string_fns.c b/lib/librte_eal/common/eal_common_string_fns.c
new file mode 100644
index 0000000..9c41a64
--- /dev/null
+++ b/lib/librte_eal/common/eal_common_string_fns.c
@@ -0,0 +1,95 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2013 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <errno.h>
+
+#include <rte_string_fns.h>
+
+int
+rte_snprintf(char *buffer, int buflen, const char *format, ...)
+{
+	int len;
+	va_list ap;
+
+	if (buffer == NULL && buflen != 0)
+		goto einval_error;
+	if (format == NULL) {
+		if (buflen > 0)
+			buffer[0] = '\0';
+		goto einval_error;
+	}
+
+	va_start(ap, format);
+	len = vsnprintf(buffer, buflen, format, ap);
+	va_end(ap);
+	if (len >= buflen && buflen > 0)
+		buffer[buflen - 1] = '\0';
+
+	return len;
+
+einval_error:
+	errno = EINVAL;
+	return -1;
+}
+
+int
+rte_strsplit(char *string, int stringlen,
+	     char **tokens, int maxtokens, char delim)
+{
+	int i, tok = 0;
+	int tokstart = 1; /* first token is right at start of string */
+
+	if (string == NULL || tokens == NULL)
+		goto einval_error;
+
+	for (i = 0; i < stringlen; i++) {
+		if (string[i] == '\0' || tok >= maxtokens)
+			break;
+		if (tokstart) {
+			tokstart = 0;
+			tokens[tok++] = &string[i];
+		}
+		if (string[i] == delim) {
+			string[i] = '\0';
+			tokstart = 1;
+		}
+	}
+	return tok;
+
+einval_error:
+	errno = EINVAL;
+	return -1;
+}
diff --git a/lib/librte_eal/common/include/rte_string_fns.h b/lib/librte_eal/common/include/rte_string_fns.h
index d8fde14..52bb77e 100644
--- a/lib/librte_eal/common/include/rte_string_fns.h
+++ b/lib/librte_eal/common/include/rte_string_fns.h
@@ -47,7 +47,6 @@ extern "C" {
 #include <stdio.h>
 #include <stdarg.h>
 #include <stddef.h>
-#include <errno.h>
 
 /**
  * Safer version of snprintf that writes up to buflen characters to
@@ -72,32 +71,8 @@ extern "C" {
  *   buffer been sufficiently big.
  *
  */
-static inline int
-rte_snprintf(char *buffer, int buflen, const char *format, ...)
-{
-	int len;
-	va_list ap;
-
-	if (buffer == NULL && buflen != 0)
-		goto einval_error;
-	if (format == NULL) {
-		if (buflen > 0)
-			buffer[0] = '\0';
-		goto einval_error;
-	}
-
-	va_start(ap, format);
-	len = vsnprintf(buffer, buflen, format, ap);
-	va_end(ap);
-	if (len >= buflen && buflen > 0)
-		buffer[buflen - 1] = '\0';
-
-	return len;
-
-einval_error:
-	errno = EINVAL;
-	return -1;
-}
+int
+rte_snprintf(char *buffer, int buflen, const char *format, ...);
 
 
 /**
@@ -126,34 +101,9 @@ einval_error:
  * @return
  *   The number of tokens in the tokens array.
  */
-static inline int
+int
 rte_strsplit(char *string, int stringlen,
-		char **tokens, int maxtokens, char delim)
-{
-	int i, tok = 0;
-	int tokstart = 1; /* first token is right at start of string */
-
-	if (string == NULL || tokens == NULL)
-		goto einval_error;
-
-	for (i = 0; i < stringlen; i++) {
-		if (string[i] == '\0' || tok >= maxtokens)
-			break;
-		if (tokstart) {
-			tokstart = 0;
-			tokens[tok++] = &string[i];
-		}
-		if (string[i] == delim) {
-			string[i] = '\0';
-			tokstart = 1;
-		}
-	}
-	return tok;
-
-einval_error:
-	errno = EINVAL;
-	return -1;
-}
+	     char **tokens, int maxtokens, char delim);
 
 #ifdef __cplusplus
 }
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index b525130..b25a093 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -78,6 +78,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_cpuflags.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_whitelist.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_nonpci_devs.c
+SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_string_fns.c
 
 CFLAGS_eal.o := -D_GNU_SOURCE
 CFLAGS_eal_thread.o := -D_GNU_SOURCE
-- 
1.7.10.4

Re: [dpdk-dev] [PATCH 1.6.1] don't inline rte_string_fns

[Thomas Monjalon]

Hi,

I have some minor comments below.

27/02/2014 09:18, Stephen Hemminger :
> The function rte_snprintf() can never be inlined by Gcc.
> If compiled with -Winline it generates an error:
>  function ‘rte_snprintf’ can never be inlined because it uses variable
> argument lists [-Werror=inline]
> 
> Therefore since both rte_snprintf and rte_strsplit are not performance
> sensitive just move them to being real functions.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

> --- /dev/null
> +++ b/lib/librte_eal/common/eal_common_string_fns.c
> @@ -0,0 +1,95 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2013 Intel Corporation. All rights reserved.

It is now 2014 in version 1.6.0.

[...]

> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <stddef.h>
> +#include <errno.h>

I think stddef.h is not needed.
 
[...]

> --- a/lib/librte_eal/common/include/rte_string_fns.h
> +++ b/lib/librte_eal/common/include/rte_string_fns.h
> @@ -47,7 +47,6 @@ extern "C" {
>  #include <stdio.h>
>  #include <stdarg.h>
>  #include <stddef.h>
> -#include <errno.h>
 
stdarg and stddef are not needed.
 
[...]

> +int
> +rte_snprintf(char *buffer, int buflen, const char *format, ...);
> 
> 

One blank line should be sufficient.

> +int
>  rte_strsplit(char *string, int stringlen,

Thank you
-- 
Thomas

Re: [dpdk-dev] [PATCH 1.6.1] don't inline rte_string_fns

[Thomas Monjalon]

Hi Stephen,
This patch is still pending.
Please send a v2.

2014-03-20 17:30, Thomas Monjalon:
> Hi,
> 
> I have some minor comments below.
> 
> 27/02/2014 09:18, Stephen Hemminger :
> > The function rte_snprintf() can never be inlined by Gcc.
> > 
> > If compiled with -Winline it generates an error:
> >  function ‘rte_snprintf’ can never be inlined because it uses variable
> > 
> > argument lists [-Werror=inline]
> > 
> > Therefore since both rte_snprintf and rte_strsplit are not performance
> > sensitive just move them to being real functions.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > --- /dev/null
> > +++ b/lib/librte_eal/common/eal_common_string_fns.c
> > @@ -0,0 +1,95 @@
> > +/*-
> > + *   BSD LICENSE
> > + *
> > + *   Copyright(c) 2010-2013 Intel Corporation. All rights reserved.
> 
> It is now 2014 in version 1.6.0.
> 
> [...]
> 
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <stddef.h>
> > +#include <errno.h>
> 
> I think stddef.h is not needed.
> 
> [...]
> 
> > --- a/lib/librte_eal/common/include/rte_string_fns.h
> > +++ b/lib/librte_eal/common/include/rte_string_fns.h
> > @@ -47,7 +47,6 @@ extern "C" {
> > 
> >  #include <stdio.h>
> >  #include <stdarg.h>
> >  #include <stddef.h>
> > 
> > -#include <errno.h>
> 
> stdarg and stddef are not needed.
> 
> [...]
> 
> > +int
> > +rte_snprintf(char *buffer, int buflen, const char *format, ...);
> 
> One blank line should be sufficient.
> 
> > +int
> > 
> >  rte_strsplit(char *string, int stringlen,
> 
> Thank you