[dpdk-dev] [PATCH] Don't inline string functions

[dpdk-dev] [PATCH] Don't inline string functions

[Stephen Hemminger]

It makes no sense to inline string functions, in fact snprintf
can't be inlined because the function supports variable number of
arguments.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
Resend because of lack of response

 lib/librte_eal/bsdapp/eal/Makefile             |    1 
 lib/librte_eal/common/eal_common_string_fns.c  |   96 +++++++++++++++++++++++++
 lib/librte_eal/common/include/rte_string_fns.h |   57 +-------------
 lib/librte_eal/linuxapp/eal/Makefile           |    1 
 4 files changed, 102 insertions(+), 53 deletions(-)

--- a/lib/librte_eal/bsdapp/eal/Makefile	2014-05-02 16:31:12.519570116 -0700
+++ b/lib/librte_eal/bsdapp/eal/Makefile	2014-05-02 16:31:12.519570116 -0700
@@ -67,6 +67,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) +=
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_tailqs.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_errno.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_cpuflags.c
+SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_string_fns.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_devargs.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_BSDAPP) += eal_common_vdev.c
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/lib/librte_eal/common/eal_common_string_fns.c	2014-05-02 16:31:12.519570116 -0700
@@ -0,0 +1,96 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 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 <string.h>
+#include <stdio.h>
+#include <stdarg.h>
+
+#include <rte_string_fns.h>
+
+/* safe version os snprintf */
+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;
+}
+
+/* split string into tokens */
+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;
+}
--- a/lib/librte_eal/common/include/rte_string_fns.h	2014-05-02 16:31:12.519570116 -0700
+++ b/lib/librte_eal/common/include/rte_string_fns.h	2014-05-02 16:31:12.519570116 -0700
@@ -72,33 +72,9 @@ 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, ...);
 
 /**
  * Takes string "string" parameter and splits it at character "delim"
@@ -126,34 +102,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
 }
--- a/lib/librte_eal/linuxapp/eal/Makefile	2014-05-02 16:31:12.519570116 -0700
+++ b/lib/librte_eal/linuxapp/eal/Makefile	2014-05-02 16:31:12.519570116 -0700
@@ -75,6 +75,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) +
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_tailqs.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_errno.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_cpuflags.c
+SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_string_fns.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_hexdump.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_devargs.c
 SRCS-$(CONFIG_RTE_LIBRTE_EAL_LINUXAPP) += eal_common_vdev.c

Re: [dpdk-dev] [PATCH] Don't inline string functions

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Wednesday, May 14, 2014 4:20 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] Don't inline string functions
> 
> It makes no sense to inline string functions, in fact snprintf can't be inlined
> because the function supports variable number of arguments.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH] Don't inline string functions

[Thomas Monjalon]

2014-05-14 08:19, Stephen Hemminger:
> It makes no sense to inline string functions, in fact snprintf
> can't be inlined because the function supports variable number of
> arguments.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> ---
> Resend because of lack of response

Please do not resend patches. I prefer a ping request in the original thread.

By the way, you didn't take into account my first comments:
	http://dpdk.org/ml/archives/dev/2014-April/002028.html
and I'm still waiting answers to these patches:
	http://dpdk.org/ml/archives/dev/2014-March/001673.html
	http://dpdk.org/ml/archives/dev/2014-March/001676.html

PS: I was going to apply your patches with these fixes:
	add FILE argument to debug functions
		[Thomas: fix quota_watermark example]
	eal: dont inline string functions
		[Thomas: update includes]

-- 
Thomas

Re: [dpdk-dev] [PATCH] Don't inline string functions

[Stephen Hemminger]

On Wed, 14 May 2014 17:47:01 +0200
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2014-05-14 08:19, Stephen Hemminger:
> > It makes no sense to inline string functions, in fact snprintf
> > can't be inlined because the function supports variable number of
> > arguments.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > 
> > ---
> > Resend because of lack of response
> 
> Please do not resend patches. I prefer a ping request in the original thread.
> 
> By the way, you didn't take into account my first comments:
> 	http://dpdk.org/ml/archives/dev/2014-April/002028.html

All the issues there are changed. includes, copyright etc.


> and I'm still waiting answers to these patches:
> 	http://dpdk.org/ml/archives/dev/2014-March/001673.html

This is in the rte_sched group. patch 1

> 	http://dpdk.org/ml/archives/dev/2014-March/001676.html

This is more of a mess than want to get into right now.
header conflicts are nuisance.