[dpdk-dev] [PATCH] log: respect rte_openlog_stream calls before rte_eal_init
Before this patch, rte_eal_init invoked rte_openlog_stream, cancelling
any application-specific logger and making it it impossible for an
application to capture the initial log messages generated during
rte_eal_init. With this patch, applications can capture all of the
log messages.
Signed-off-by: John Ousterhout
---
lib/librte_eal/bsdapp/eal/eal_log.c | 3 +--
lib/librte_eal/common/eal_common_log.c | 13 +++--
lib/librte_eal/common/include/rte_log.h | 2 +-
lib/librte_eal/linuxapp/eal/eal_log.c | 3 +--
4 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c
b/lib/librte_eal/bsdapp/eal/eal_log.c
index a425f7a..c5b1dee 100644
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ b/lib/librte_eal/bsdapp/eal/eal_log.c
@@ -52,6 +52,5 @@ rte_eal_log_init(const char *id __rte_unused, int facility
__rte_unused)
int
rte_eal_log_early_init(void)
{
- rte_openlog_stream(stderr);
- return 0;
+ return rte_eal_common_log_init(stderr);
}
diff --git a/lib/librte_eal/common/eal_common_log.c
b/lib/librte_eal/common/eal_common_log.c
index 967991a..cfc4c8b 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,11 +48,12 @@ struct rte_logs rte_logs = {
.file = NULL,
};
+/* Stream to use for logging if rte_logs.file is NULL */
static FILE *default_log_stream;
/**
* This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
*/
struct log_cur_msg {
uint32_t loglevel; /**< log level - see rte_log.h */
@@ -68,10 +69,7 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
int
rte_openlog_stream(FILE *f)
{
- if (f == NULL)
- rte_logs.file = default_log_stream;
- else
- rte_logs.file = f;
+ rte_logs.file = f;
return 0;
}
@@ -127,6 +125,8 @@ rte_vlog(uint32_t level, uint32_t logtype, const char
*format, va_list ap)
{
int ret;
FILE *f = rte_logs.file;
+ if (f == NULL)
+ f = default_log_stream;
if ((level > rte_logs.level) || !(logtype & rte_logs.type))
return 0;
@@ -158,7 +158,8 @@ rte_log(uint32_t level, uint32_t logtype, const char
*format, ...)
}
/*
- * called by environment-specific log init function
+ * Called during initialization to specify where to log messages if
+ * openlog_stream hasn't been called.
*/
int
rte_eal_common_log_init(FILE *default_log)
diff --git a/lib/librte_eal/common/include/rte_log.h
b/lib/librte_eal/common/include/rte_log.h
index 919563c..d99baf3 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -54,7 +54,7 @@ extern "C" {
struct rte_logs {
uint32_t type; /**< Bitfield with enabled logs. */
uint32_t level; /**< Log level. */
- FILE *file; /**< Pointer to current FILE* for logs. */
+ FILE *file; /**< Output file set by rte_openlog_stream, or NULL. */
};
/** Global log informations */
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c
b/lib/librte_eal/linuxapp/eal/eal_log.c
index d391100..aafc41c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -136,6 +136,5 @@ rte_eal_log_early_init(void)
printf("Cannot configure early_log_stream\n");
return -1;
}
- rte_openlog_stream(early_log_stream);
- return 0;
+ return rte_eal_common_log_init(early_log_stream);
}
--
2.8.3
[dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init
Before this patch, application-specific loggers could not be
installed before rte_eal_init completed (the initialization process
called rte_openlog_stream, overwriting any previously installed
logger). This made it impossible for an application to capture the
initial log messages generated during rte_eal_init. This patch changes
initialization so that information from a previous call to
rte_openlog_stream is not lost. Specifically:
* The default log stream is now maintained separately from an
application-specific log stream installed with rte_openlog_stream.
* rte_eal_common_log_init has been renamed to rte_eal_log_set_default,
since this is all it does. It no longer invokes rte_openlog_stream; it
just updates the default stream. Also, this method now returns void,
rather than int, since there are no errors.
* The "early log" mechanism (e.g. rte_eal_log_early_init) has been
removed; all of the desired functionality can be achieved by calling
rte_eal_log_set_default.
Signed-off-by: John Ousterhout
v2:
* Removed the early log mechanism, renamed rte_eal_common_log_init.
Note: I see from the code that Linux and BSD set different default streams:
Linux uses stdout, while BSD uses stderr. This patch retains the distinction,
though I'm not sure why it is there.
---
lib/librte_eal/bsdapp/eal/eal.c | 3 +--
lib/librte_eal/bsdapp/eal/eal_log.c | 11 +
lib/librte_eal/common/eal_common_log.c | 19 +++-
lib/librte_eal/common/eal_private.h | 16 -
lib/librte_eal/common/include/rte_log.h | 2 +-
lib/librte_eal/linuxapp/eal/eal.c | 3 +--
lib/librte_eal/linuxapp/eal/eal_log.c | 40 +
7 files changed, 17 insertions(+), 77 deletions(-)
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a0c8f8c..a1ef75b 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -501,8 +501,7 @@ rte_eal_init(int argc, char **argv)
thread_id = pthread_self();
- if (rte_eal_log_early_init() < 0)
- rte_panic("Cannot init early logs\n");
+ rte_eal_log_set_default(stderr);
eal_log_level_parse(argc, argv);
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c
b/lib/librte_eal/bsdapp/eal/eal_log.c
index a425f7a..6b2c6da 100644
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ b/lib/librte_eal/bsdapp/eal/eal_log.c
@@ -44,14 +44,5 @@
int
rte_eal_log_init(const char *id __rte_unused, int facility __rte_unused)
{
- if (rte_eal_common_log_init(stderr) < 0)
- return -1;
- return 0;
-}
-
-int
-rte_eal_log_early_init(void)
-{
- rte_openlog_stream(stderr);
- return 0;
+ rte_eal_set_default(stderr);
}
diff --git a/lib/librte_eal/common/eal_common_log.c
b/lib/librte_eal/common/eal_common_log.c
index 967991a..2181cfa 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,11 +48,12 @@ struct rte_logs rte_logs = {
.file = NULL,
};
+/* Stream to use for logging if rte_logs.file is NULL */
static FILE *default_log_stream;
/**
* This global structure stores some informations about the message
- * that is currently beeing processed by one lcore
+ * that is currently being processed by one lcore
*/
struct log_cur_msg {
uint32_t loglevel; /**< log level - see rte_log.h */
@@ -68,10 +69,7 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
int
rte_openlog_stream(FILE *f)
{
- if (f == NULL)
- rte_logs.file = default_log_stream;
- else
- rte_logs.file = f;
+ rte_logs.file = f;
return 0;
}
@@ -127,6 +125,8 @@ rte_vlog(uint32_t level, uint32_t logtype, const char
*format, va_list ap)
{
int ret;
FILE *f = rte_logs.file;
+ if (f == NULL)
+ f = default_log_stream;
if ((level > rte_logs.level) || !(logtype & rte_logs.type))
return 0;
@@ -158,17 +158,14 @@ rte_log(uint32_t level, uint32_t logtype, const char
*format, ...)
}
/*
- * called by environment-specific log init function
+ * Called by environment-specific initialization functions.
*/
-int
-rte_eal_common_log_init(FILE *default_log)
+void
+rte_eal_log_set_default(FILE *default_log)
{
default_log_stream = default_log;
- rte_openlog_stream(default_log);
#if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
RTE_LOG(NOTICE, EAL, "Debug logs available - lower performance\n");
#endif
-
- return 0;
}
diff --git a/lib/librte_eal/common/eal_private.h
b/lib/librte_eal/common/eal_private.h
index 19f7535..a037994 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -47,7 +47,9 @@
int rte_eal_memzone_init(void);
/**
- * Common log initialization function (private to eal).
+ * Common log initialization function (private to eal). Determines
+ * where log
[dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init
On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon
wrote:
>
> 2016-10-10 15:39, John Ousterhout:
> > ...
> >
> > Note: I see from the code that Linux and BSD set different default
streams:
> > Linux uses stdout, while BSD uses stderr. This patch retains the
distinction,
> > though I'm not sure why it is there.
>
> I don't know either.
> What is best between stdout and stderr for logs?
I would guess that stdout makes more sense, since most log entries describe
normal operation, not errors. I'm happy to make these consistent, but this
would introduce a behavior change for BSD (which currently uses stderr);
would that be considered antisocial?
> [...]
> > -int
> > -rte_eal_log_early_init(void)
> > -{
> > - rte_openlog_stream(stderr);
> > - return 0;
> > + rte_eal_set_default(stderr);
>
> It should be rte_eal_log_set_default.
Oops, right; will fix.
>
> [...]
> > /*
> > - * called by environment-specific log init function
> > + * Called by environment-specific initialization functions.
> > */
> > -int
> > -rte_eal_common_log_init(FILE *default_log)
> > +void
> > +rte_eal_log_set_default(FILE *default_log)
> > {
> > default_log_stream = default_log;
> > - rte_openlog_stream(default_log);
> >
> > #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> > RTE_LOG(NOTICE, EAL, "Debug logs available - lower
performance\n");
> > #endif
> > -
> > - return 0;
> > }
>
> Do we really need a function for that?
> Why not just initialize default_log_stream statically?
Right now, different platforms have different defaults. BSD uses stderr
always. Linux starts out with stdout as the default, but later during
initialization it switches to a different default that logs messages both
to standard output and also to syslog. I don't have enough experience with
DPDK to know whether a single approach is really right for all systems (or
which approach it should be), and since I'm a DPDK newbie I thought it best
to take a more conservative approach and avoid behavioral changes. My
personal preference would be to minimize mission creep with this patch and
leave that behavior in place for someone with more expertise to deal with
later (and this issue is orthogonal to the main goal of the patch). But, if
unifying the log defaults is considered essential to the patch (and is
noncontroversial), I'm willing to implement it.
> [...]
> > /**
> > - * Common log initialization function (private to eal).
> > + * Common log initialization function (private to eal). Determines
> > + * where log data is written when no call to eal_openlog_stream is
> > + * in effect.
>
> It should be rte_openlog_stream.
Oops; fixed.
Thanks for the comments.
-John-
[dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init
All of your suggestions look reasonable and fairly straightforward; I'll
work on a new patch that includes them.
Given that rte_eal_log_init is a no-op (and won't even be invoked), would
it be better to remove that function completely, and even delete the file
containing it (eal_log.c), or is it better to retain the empty function in
order to maintain a parallel structure with Linux? Personally I'd lean
towards deleting the file. As it stands, the interface to that function
doesn't even make sense for BSD; the arguments were chosen for Linux and
are ignored in BSD.
Let me know your preference.
-John-
On Tue, Oct 11, 2016 at 1:30 PM, Thomas Monjalon
wrote:
> 2016-10-11 09:30, John Ousterhout:
> > On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon <
> thomas.monjalon at 6wind.com>
> > wrote:
> > >
> > > 2016-10-10 15:39, John Ousterhout:
> > > > ...
> > > >
> > > > Note: I see from the code that Linux and BSD set different default
> > streams:
> > > > Linux uses stdout, while BSD uses stderr. This patch retains the
> > distinction,
> > > > though I'm not sure why it is there.
> > >
> > > I don't know either.
> > > What is best between stdout and stderr for logs?
> >
> > I would guess that stdout makes more sense, since most log entries
> describe
> > normal operation, not errors. I'm happy to make these consistent, but
> this
> > would introduce a behavior change for BSD (which currently uses stderr);
> > would that be considered antisocial?
>
> No, that's OK to use stdout on BSD.
>
> > > > -int
> > > > -rte_eal_common_log_init(FILE *default_log)
> > > > +void
> > > > +rte_eal_log_set_default(FILE *default_log)
> > > > {
> > > > default_log_stream = default_log;
> > > > - rte_openlog_stream(default_log);
> > > >
> > > > #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
> > > > RTE_LOG(NOTICE, EAL, "Debug logs available - lower
> > performance\n");
> > > > #endif
> > > > -
> > > > - return 0;
> > > > }
> > >
> > > Do we really need a function for that?
> > > Why not just initialize default_log_stream statically?
> >
> > Right now, different platforms have different defaults. BSD uses stderr
> > always. Linux starts out with stdout as the default, but later during
> > initialization it switches to a different default that logs messages both
> > to standard output and also to syslog. I don't have enough experience
> with
> > DPDK to know whether a single approach is really right for all systems
> (or
> > which approach it should be), and since I'm a DPDK newbie I thought it
> best
> > to take a more conservative approach and avoid behavioral changes. My
> > personal preference would be to minimize mission creep with this patch
> and
> > leave that behavior in place for someone with more expertise to deal with
> > later (and this issue is orthogonal to the main goal of the patch). But,
> if
> > unifying the log defaults is considered essential to the patch (and is
> > noncontroversial), I'm willing to implement it.
>
> OK sorry, I'm mixing things.
>
> 1/ When removing early log functions, you are replacing early init with
> a default set to stderr/stdout via rte_eal_log_set_default.
> I think you can just set statically to stdout:
> static FILE *default_log_stream = stdout;
>
> 2/ Yes, on Linux, a more complex stream with stdout + syslog is set.
> It is OK to use rte_eal_log_set_default for that usage.
> Note that there is a stream which is not used and can be removed in
> eal_private.h:
> extern FILE *eal_default_log_stream;
> Other note: rte_eal_log_set_default is not a public function so should be
> named eal_log_set_default.
>
> 3/ When calling rte_eal_log_set_default on BSD from rte_eal_log_init,
> you can keep stderr but an empty function would be better because
> it is not called and already set (to stderr or stdout if 1/).
>
> 4/ rte_eal_log_init can be called earlier to replace early init.
>
> 5/ It would be simpler to understand by splitting in two patches
> (remove early log + use non default log)
>
> I understand that you prefer to focus on your fix and I'm more or less
> suggesting a cleanup of logging. That's why I can do the first cleanup
> patch if you are really not confortable with it. (I feel you could do it)
> Just let me know.
>
[dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init
Don's argument for stderr over stdout makes sense to me. Does anyone else disagree? -John- On Tue, Oct 11, 2016 at 3:16 PM, Don Provan wrote: > > -Original Message- > > From: John Ousterhout [mailto:ouster at cs.stanford.edu] > > Sent: Tuesday, October 11, 2016 9:30 AM > > To: Thomas Monjalon > > Cc: dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls > > before rte_eal_init > > > > On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon > > > > wrote: > > > I don't know either. > > > What is best between stdout and stderr for logs? > > > > I would guess that stdout makes more sense, since most log entries > describe > > normal operation, not errors. I'm happy to make these consistent, but > this > > would introduce a behavior change for BSD (which currently uses stderr); > > would that be considered antisocial? > > I've never seen a pronouncement or anything, but as a linux programmer, > my attitude is that stdout should be the output the application is > producing > when carrying out its function. Debugging output isn't part of what the > application is trying to accomplish, so it should be sent to stderr where > it > can be segregated from the functional output when needed. > -don > dprovan at bivio.net > >
[dpdk-dev] [PATCH v3] log: respect rte_openlog_stream calls before rte_eal_init
Before this patch, application-specific loggers could not be
installed before rte_eal_init completed (the initialization process
called rte_openlog_stream, overwriting any previously installed
logger). This made it impossible for an application to capture the
initial log messages generated during rte_eal_init. This patch changes
initialization so that information from a previous call to
rte_openlog_stream is not lost. Specifically:
* The default log stream is now maintained separately from an
application-specific log stream installed with rte_openlog_stream.
* rte_eal_common_log_init has been renamed to eal_log_set_default,
since this is all it does. It no longer invokes rte_openlog_stream; it
just updates the default stream. Also, this method now returns void,
rather than int, since there are no errors.
This patch also removes the "early log" mechanism and cleans up the
log initialization mechanism:
* The default log stream defaults to stderr on all platforms if
eal_log_set_default hasn't been invoked (Linux used to use stdout
during the first part of initialization).
* Removed rte_eal_log_early_init; all of the desired functionality can
be achieved by calling eal_log_set_default.
* Removed lib/librte_eal/bsdapp/eal/eal_log.c: it contained only one
function, rte_eal_log_init, which is not needed or invoked for BSD.
* Removed declaration for eal_default_log_stream in rte_log.h (it's now
private to eal_common_log.c).
* Moved call to rte_eal_log_init earlier in rte_eal_init for Linux, so
that it starts using the preferrred log ASAP.
Signed-off-by: John Ousterhout
v3:
* Made stderr the initial default log stream for Linux.
* Deleted lib/librte_eal/bsdapp/eal/eal_log.c.
* Deleted declaration for eal_default_log_stream in rte_log.h.
* Moved rte_eal_log_init call for Linux.
v2:
* Removed the early log mechanism, renamed rte_eal_common_log_init.
---
lib/librte_eal/bsdapp/eal/Makefile | 3 +-
lib/librte_eal/bsdapp/eal/eal.c | 6
lib/librte_eal/bsdapp/eal/eal_log.c | 57 -
lib/librte_eal/common/eal_common_log.c | 29 ++---
lib/librte_eal/common/eal_private.h | 16 +++--
lib/librte_eal/common/include/rte_log.h | 5 +--
lib/librte_eal/linuxapp/eal/eal.c | 9 ++
lib/librte_eal/linuxapp/eal/eal_log.c | 40 +--
8 files changed, 28 insertions(+), 137 deletions(-)
delete mode 100644 lib/librte_eal/bsdapp/eal/eal_log.c
diff --git a/lib/librte_eal/bsdapp/eal/Makefile
b/lib/librte_eal/bsdapp/eal/Makefile
index 7a0fea5..34340c1 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -50,12 +50,11 @@ EXPORT_MAP := rte_eal_version.map
LIBABIVER := 3
-# specific to linuxapp exec-env
+# specific to bsdapp exec-env
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_memory.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_hugepage_info.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_thread.c
-SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_log.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_pci.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_debug.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_lcore.c
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a0c8f8c..6a6ae86 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -501,9 +501,6 @@ rte_eal_init(int argc, char **argv)
thread_id = pthread_self();
- if (rte_eal_log_early_init() < 0)
- rte_panic("Cannot init early logs\n");
-
eal_log_level_parse(argc, argv);
/* set log level as early as possible */
@@ -552,9 +549,6 @@ rte_eal_init(int argc, char **argv)
if (rte_eal_tailqs_init() < 0)
rte_panic("Cannot init tail queues for objects\n");
-/* if (rte_eal_log_init(argv[0], internal_config.syslog_facility) < 0)
- rte_panic("Cannot init logs\n");*/
-
if (rte_eal_alarm_init() < 0)
rte_panic("Cannot init interrupt-handling thread\n");
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c
b/lib/librte_eal/bsdapp/eal/eal_log.c
deleted file mode 100644
index a425f7a..000
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/*-
- * 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 docum
[dpdk-dev] [PATCH v4] log: respect rte_openlog_stream calls before rte_eal_init
Before this patch, application-specific loggers could not be
installed before rte_eal_init completed (the initialization process
called rte_openlog_stream, overwriting any previously installed
logger). This made it impossible for an application to capture the
initial log messages generated during rte_eal_init. This patch changes
initialization so that information from a previous call to
rte_openlog_stream is not lost. Specifically:
* The default log stream is now maintained separately from an
application-specific log stream installed with rte_openlog_stream.
* rte_eal_common_log_init has been renamed to eal_log_set_default,
since this is all it does. It no longer invokes rte_openlog_stream; it
just updates the default stream. Also, this method now returns void,
rather than int, since there are no errors.
This patch also removes the "early log" mechanism and cleans up the
log initialization mechanism:
* The default log stream defaults to stderr on all platforms if
eal_log_set_default hasn't been invoked (Linux used to use stdout
during the first part of initialization).
* Removed rte_eal_log_early_init; all of the desired functionality can
be achieved by calling eal_log_set_default.
* Removed lib/librte_eal/bsdapp/eal/eal_log.c: it contained only one
function, rte_eal_log_init, which is not needed or invoked for BSD.
* Removed declaration for eal_default_log_stream in rte_log.h (it's now
private to eal_common_log.c).
* Moved call to rte_eal_log_init earlier in rte_eal_init for Linux, so
that it starts using the preferrred log ASAP.
Signed-off-by: John Ousterhout
v4:
* Fixed problems from checkpatches.
v3:
* Made stderr the initial default log stream for Linux.
* Deleted lib/librte_eal/bsdapp/eal/eal_log.c.
* Deleted declaration for eal_default_log_stream in rte_log.h.
* Moved rte_eal_log_init call for Linux.
v2:
* Removed the early log mechanism, renamed rte_eal_common_log_init.
---
lib/librte_eal/bsdapp/eal/Makefile | 3 +-
lib/librte_eal/bsdapp/eal/eal.c | 6
lib/librte_eal/bsdapp/eal/eal_log.c | 57 -
lib/librte_eal/common/eal_common_log.c | 30 ++---
lib/librte_eal/common/eal_private.h | 16 +++--
lib/librte_eal/common/include/rte_log.h | 5 +--
lib/librte_eal/linuxapp/eal/eal.c | 9 ++
lib/librte_eal/linuxapp/eal/eal_log.c | 40 +--
8 files changed, 29 insertions(+), 137 deletions(-)
delete mode 100644 lib/librte_eal/bsdapp/eal/eal_log.c
diff --git a/lib/librte_eal/bsdapp/eal/Makefile
b/lib/librte_eal/bsdapp/eal/Makefile
index 7a0fea5..34340c1 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -50,12 +50,11 @@ EXPORT_MAP := rte_eal_version.map
LIBABIVER := 3
-# specific to linuxapp exec-env
+# specific to bsdapp exec-env
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_memory.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_hugepage_info.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_thread.c
-SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_log.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_pci.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_debug.c
SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_lcore.c
diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index a0c8f8c..6a6ae86 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -501,9 +501,6 @@ rte_eal_init(int argc, char **argv)
thread_id = pthread_self();
- if (rte_eal_log_early_init() < 0)
- rte_panic("Cannot init early logs\n");
-
eal_log_level_parse(argc, argv);
/* set log level as early as possible */
@@ -552,9 +549,6 @@ rte_eal_init(int argc, char **argv)
if (rte_eal_tailqs_init() < 0)
rte_panic("Cannot init tail queues for objects\n");
-/* if (rte_eal_log_init(argv[0], internal_config.syslog_facility) < 0)
- rte_panic("Cannot init logs\n");*/
-
if (rte_eal_alarm_init() < 0)
rte_panic("Cannot init interrupt-handling thread\n");
diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c
b/lib/librte_eal/bsdapp/eal/eal_log.c
deleted file mode 100644
index a425f7a..000
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ /dev/null
@@ -1,57 +0,0 @@
-/*-
- * 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 d
[dpdk-dev] [PATCH v4] log: respect rte_openlog_stream calls before rte_eal_init
Suppose an application starts up, calls rte_eal_init, then later on invokes
code like this:
fclose(stderr);
stderr = fopen("foo", "w");
This might happen if it is using stderr for its log, but decides to roll
the log over to a new file.
Now stderr has changed. However, if DPDK made a copy of it with a
statement like this:
FILE *default_log_stream = stderr;
then default_log_stream will continue to refer to the old log file, not the
new one.
Thus, it's better to grab the value of stderr at the last possible moment
before logging.
-John-
On Wed, Oct 12, 2016 at 12:47 PM, Thomas Monjalon wrote:
> 2016-10-12 12:38, John Ousterhout:
> > @@ -127,6 +125,19 @@ rte_vlog(uint32_t level, uint32_t logtype, const
> char *format, va_list ap)
> > {
> > int ret;
> > FILE *f = rte_logs.file;
> > + if (f == NULL) {
> > + f = default_log_stream;
> > + if (f == NULL) {
> > + /*
> > + * Grab the current value of stderr here, rather
> than
> > + * just initializing default_log_stream to stderr.
> This
> > + * ensures that we will always use the current
> value
> > + * of stderr, even if the application closes and
> > + * reopens it.
> > + */
> > + f = stderr;
> > + }
> > + }
>
> I don't understand this big comment.
> What is the difference with initializing default_log_stream to stderr?
> What do you mean by "if the application closes and reopens it"?
>
[dpdk-dev] [PATCH] eal: avoid unnecessary conflicts over rte_config file
Before this patch, DPDK used the file ~/.rte_config as a lock to detect
potential interference between multiple DPDK applications running on the
same machine. However, if a single user ran DPDK applications concurrently
on several different machines, and if the user's home directory was shared
between the machines via NFS, DPDK would incorrectly detect conflicts
for all but the first application and abort them. This patch fixes the
problem by incorporating the machine name into the config file name (e.g.,
~/.rte_hostname_config).
Signed-off-by: John Ousterhout
---
doc/guides/prog_guide/multi_proc_support.rst | 11 +++
lib/librte_eal/common/eal_common_proc.c | 8 ++--
lib/librte_eal/common/eal_filesystem.h | 15 +--
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/doc/guides/prog_guide/multi_proc_support.rst
b/doc/guides/prog_guide/multi_proc_support.rst
index badd102..a54fa1c 100644
--- a/doc/guides/prog_guide/multi_proc_support.rst
+++ b/doc/guides/prog_guide/multi_proc_support.rst
@@ -129,10 +129,13 @@ Support for this usage scenario is provided using the
``--file-prefix`` paramete
By default, the EAL creates hugepage files on each hugetlbfs filesystem using
the rtemap_X filename,
where X is in the range 0 to the maximum number of hugepages -1.
-Similarly, it creates shared configuration files, memory mapped in each
process, using the /var/run/.rte_config filename,
-when run as root (or $HOME/.rte_config when run as a non-root user;
-if filesystem and device permissions are set up to allow this).
-The rte part of the filenames of each of the above is configurable using the
file-prefix parameter.
+Similarly, it creates shared configuration files, memory mapped in each
process.
+When run as root, the name of the configuration file will be
+/var/run/.rte_*host*_config, where *host* is the name of the machine.
+When run as a non-root user, the the name of the configuration file will be
+$HOME/.rte_*host*_config (if filesystem and device permissions are set up to
allow this).
+If the ``--file-prefix`` parameter has been specified, its value will be used
+in place of "rte" in the file names.
In addition to specifying the file-prefix parameter,
any DPDK applications that are to be run side-by-side must explicitly limit
their memory use.
diff --git a/lib/librte_eal/common/eal_common_proc.c
b/lib/librte_eal/common/eal_common_proc.c
index 12e0fca..517aa0c 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -45,12 +45,8 @@ rte_eal_primary_proc_alive(const char *config_file_path)
if (config_file_path)
config_fd = open(config_file_path, O_RDONLY);
- else {
- char default_path[PATH_MAX+1];
- snprintf(default_path, PATH_MAX, RUNTIME_CONFIG_FMT,
-default_config_dir, "rte");
- config_fd = open(default_path, O_RDONLY);
- }
+ else
+ config_fd = open(eal_runtime_config_path(), O_RDONLY);
if (config_fd < 0)
return 0;
diff --git a/lib/librte_eal/common/eal_filesystem.h
b/lib/librte_eal/common/eal_filesystem.h
index fdb4a70..4929aa3 100644
--- a/lib/librte_eal/common/eal_filesystem.h
+++ b/lib/librte_eal/common/eal_filesystem.h
@@ -41,7 +41,7 @@
#define EAL_FILESYSTEM_H
/** Path of rte config file. */
-#define RUNTIME_CONFIG_FMT "%s/.%s_config"
+#define RUNTIME_CONFIG_FMT "%s/.%s_%s_config"
#include
#include
@@ -59,11 +59,22 @@ eal_runtime_config_path(void)
static char buffer[PATH_MAX]; /* static so auto-zeroed */
const char *directory = default_config_dir;
const char *home_dir = getenv("HOME");
+ static char nameBuffer[1000];
+ int result;
if (getuid() != 0 && home_dir != NULL)
directory = home_dir;
+
+ /*
+* Include the name of the host in the config file path. Otherwise,
+* if DPDK applications run on different hosts but share a home
+* directory (e.g. via NFS), they will choose the same config
+* file and conflict unnecessarily.
+*/
+ result = gethostname(nameBuffer, sizeof(nameBuffer)-1);
snprintf(buffer, sizeof(buffer) - 1, RUNTIME_CONFIG_FMT, directory,
- internal_config.hugefile_prefix);
+ internal_config.hugefile_prefix,
+ (result == 0) ? nameBuffer : "unknown-host");
return buffer;
}
--
2.8.3
[dpdk-dev] [PATCH] eal: avoid unnecessary conflicts over rte_config file
It's true that users can patch around this problem (and I started off doing
just that), but why impose this inconvenience on users when DPDK can just
"do the right thing" to begin with? For example, it took me several hours
to figure out why the problem was occurring and then to hunt down the
--file-prefix solution. Is there some reason why it would be a bad idea to
fix this in DPDK?
-John-
On Thu, Oct 13, 2016 at 3:53 AM, Ananyev, Konstantin <
konstantin.ananyev at intel.com> wrote:
>
> Hi John,
>
> > Before this patch, DPDK used the file ~/.rte_config as a lock to detect
> > potential interference between multiple DPDK applications running on the
> > same machine. However, if a single user ran DPDK applications
> concurrently
> > on several different machines, and if the user's home directory was
> shared
> > between the machines via NFS, DPDK would incorrectly detect conflicts
> > for all but the first application and abort them. This patch fixes the
> > problem by incorporating the machine name into the config file name
> (e.g.,
> > ~/.rte_hostname_config).
> >
> > Signed-off-by: John Ousterhout
> > ---
> > doc/guides/prog_guide/multi_proc_support.rst | 11 +++
> > lib/librte_eal/common/eal_common_proc.c | 8 ++--
> > lib/librte_eal/common/eal_filesystem.h | 15 +--
> > 3 files changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/doc/guides/prog_guide/multi_proc_support.rst
> b/doc/guides/prog_guide/multi_proc_support.rst
> > index badd102..a54fa1c 100644
> > --- a/doc/guides/prog_guide/multi_proc_support.rst
> > +++ b/doc/guides/prog_guide/multi_proc_support.rst
> > @@ -129,10 +129,13 @@ Support for this usage scenario is provided using
> the ``--file-prefix`` paramete
> >
> > By default, the EAL creates hugepage files on each hugetlbfs filesystem
> using the rtemap_X filename,
> > where X is in the range 0 to the maximum number of hugepages -1.
> > -Similarly, it creates shared configuration files, memory mapped in each
> process, using the /var/run/.rte_config filename,
> > -when run as root (or $HOME/.rte_config when run as a non-root user;
> > -if filesystem and device permissions are set up to allow this).
> > -The rte part of the filenames of each of the above is configurable
> using the file-prefix parameter.
> > +Similarly, it creates shared configuration files, memory mapped in each
> process.
> > +When run as root, the name of the configuration file will be
> > +/var/run/.rte_*host*_config, where *host* is the name of the machine.
> > +When run as a non-root user, the the name of the configuration file
> will be
> > +$HOME/.rte_*host*_config (if filesystem and device permissions are set
> up to allow this).
> > +If the ``--file-prefix`` parameter has been specified, its value will
> be used
> > +in place of "rte" in the file names.
>
> I am not sure that we need to handle all such cases inside EAL.
> User can easily overcome that problem by just adding something like:
> --file-prefix=`uname -n`
> to his command-line.
> Konstantin
>
> >
> > In addition to specifying the file-prefix parameter,
> > any DPDK applications that are to be run side-by-side must explicitly
> limit their memory use.
> > diff --git a/lib/librte_eal/common/eal_common_proc.c
> b/lib/librte_eal/common/eal_common_proc.c
> > index 12e0fca..517aa0c 100644
> > --- a/lib/librte_eal/common/eal_common_proc.c
> > +++ b/lib/librte_eal/common/eal_common_proc.c
> > @@ -45,12 +45,8 @@ rte_eal_primary_proc_alive(const char
> *config_file_path)
> >
> > if (config_file_path)
> > config_fd = open(config_file_path, O_RDONLY);
> > - else {
> > - char default_path[PATH_MAX+1];
> > - snprintf(default_path, PATH_MAX, RUNTIME_CONFIG_FMT,
> > - default_config_dir, "rte");
> > - config_fd = open(default_path, O_RDONLY);
> > - }
> > + else
> > + config_fd = open(eal_runtime_config_path(), O_RDONLY);
> > if (config_fd < 0)
> > return 0;
> >
> > diff --git a/lib/librte_eal/common/eal_filesystem.h
> b/lib/librte_eal/common/eal_filesystem.h
> > index fdb4a70..4929aa3 100644
> > --- a/lib/librte_eal/common/eal_filesystem.h
> > +++ b/lib/librte_eal/common/eal_filesystem.h
> > @@ -41,7 +41,7 @@
> > #define EAL_FILESYSTEM_H
> >
> > /** Path of rte config file. */
> > -#define RUNTIME_CONFIG_FMT "%s/.%s_config"
> > +#define RUNTIME_CONFIG_FMT "
[dpdk-dev] [PATCH] eal: avoid unnecessary conflicts over rte_config file
Hi Harry, But, given the existence of the --file-prefix option, isn't it already unsafe for Collectd to check only for .rte_config? If it's important for other programs to be able to find the config files, it seems to me that a more robust mechanism is needed than the current one. -John- On Thu, Oct 13, 2016 at 9:07 AM, Van Haaren, Harry < harry.van.haaren at intel.com> wrote: > Hi John, > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of John Ousterhout > > Subject: Re: [dpdk-dev] [PATCH] eal: avoid unnecessary conflicts over > rte_config file > > > > > For example, it took me several hours > > to figure out why the problem was occurring and then to hunt down the > > --file-prefix solution. Is there some reason why it would be a bad idea > to > > fix this in DPDK? > > Right now, DPDK will by default always use a consistent .rte_config > location, which allows other applications to monitor that. For example, > Collectd is able to monitor a DPDK primary process by checking if the > .rte_config file exists at its default location[1]. > > If we change the default behavior of DPDK, other projects can no longer > rely on that file being created, and these monitoring projects must be > updated in sync with DPDK to avoid breakage if versions mismatch. > > Although I see your use-case, I think using the --file-prefix as > Konstantin suggests is a better solution in this case. > > Regards, -Harry > > [1] https://github.com/collectd/collectd/blob/master/src/dpdkstat.c#L60 > > > > On Thu, Oct 13, 2016 at 3:53 AM, Ananyev, Konstantin < > > konstantin.ananyev at intel.com> wrote: > > > > > > > > Hi John, > > > > > > > Before this patch, DPDK used the file ~/.rte_config as a lock to > detect > > > > potential interference between multiple DPDK applications running on > the > > > > same machine. However, if a single user ran DPDK applications > > > concurrently > > > > on several different machines, and if the user's home directory was > > > shared > > > > between the machines via NFS, DPDK would incorrectly detect conflicts > > > > for all but the first application and abort them. This patch fixes > the > > > > problem by incorporating the machine name into the config file name > > > (e.g., > > > > ~/.rte_hostname_config). > > > > > > > > Signed-off-by: John Ousterhout > > > > --- > > > > doc/guides/prog_guide/multi_proc_support.rst | 11 +++ > > > > lib/librte_eal/common/eal_common_proc.c | 8 ++-- > > > > lib/librte_eal/common/eal_filesystem.h | 15 +-- > > > > 3 files changed, 22 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/doc/guides/prog_guide/multi_proc_support.rst > > > b/doc/guides/prog_guide/multi_proc_support.rst > > > > index badd102..a54fa1c 100644 > > > > --- a/doc/guides/prog_guide/multi_proc_support.rst > > > > +++ b/doc/guides/prog_guide/multi_proc_support.rst > > > > @@ -129,10 +129,13 @@ Support for this usage scenario is provided > using > > > the ``--file-prefix`` paramete > > > > > > > > By default, the EAL creates hugepage files on each hugetlbfs > filesystem > > > using the rtemap_X filename, > > > > where X is in the range 0 to the maximum number of hugepages -1. > > > > -Similarly, it creates shared configuration files, memory mapped in > each > > > process, using the /var/run/.rte_config filename, > > > > -when run as root (or $HOME/.rte_config when run as a non-root user; > > > > -if filesystem and device permissions are set up to allow this). > > > > -The rte part of the filenames of each of the above is configurable > > > using the file-prefix parameter. > > > > +Similarly, it creates shared configuration files, memory mapped in > each > > > process. > > > > +When run as root, the name of the configuration file will be > > > > +/var/run/.rte_*host*_config, where *host* is the name of the > machine. > > > > +When run as a non-root user, the the name of the configuration file > > > will be > > > > +$HOME/.rte_*host*_config (if filesystem and device permissions are > set > > > up to allow this). > > > > +If the ``--file-prefix`` parameter has been specified, its value > will > > > be used > > > > +in place of "rte" in the file names. > > > > > > I am not sure that
[dpdk-dev] [PATCH] eal: avoid unnecessary conflicts over rte_config file
It sounds like my patch would break some existing software, so it probably
doesn't make sense right now.
I'd still argue that the current mechanism has a number of problems, and it
should probably undergo a comprehensive overhaul at some point in the
future.
-John-
On Thu, Oct 13, 2016 at 2:39 PM, Tahhan, Maryam
wrote:
> > Hi John,
> >
> > > Before this patch, DPDK used the file ~/.rte_config as a lock to
> > > detect potential interference between multiple DPDK applications
> > > running on the same machine. However, if a single user ran DPDK
> > > applications concurrently on several different machines, and if the
> > > user's home directory was shared between the machines via NFS, DPDK
> > > would incorrectly detect conflicts for all but the first application
> > > and abort them. This patch fixes the problem by incorporating the
> > > machine name into the config file name (e.g., ~/.rte_hostname_config).
> > >
> > > Signed-off-by: John Ousterhout
> > > ---
> > > doc/guides/prog_guide/multi_proc_support.rst | 11 +++
> > > lib/librte_eal/common/eal_common_proc.c | 8 ++--
> > > lib/librte_eal/common/eal_filesystem.h | 15 +--
> > > 3 files changed, 22 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/doc/guides/prog_guide/multi_proc_support.rst
> > > b/doc/guides/prog_guide/multi_proc_support.rst
> > > index badd102..a54fa1c 100644
> > > --- a/doc/guides/prog_guide/multi_proc_support.rst
> > > +++ b/doc/guides/prog_guide/multi_proc_support.rst
> > > @@ -129,10 +129,13 @@ Support for this usage scenario is provided
> > > using the ``--file-prefix`` paramete
> > >
> > > By default, the EAL creates hugepage files on each hugetlbfs
> > > filesystem using the rtemap_X filename, where X is in the range 0 to
> the
> > maximum number of hugepages -1.
> > > -Similarly, it creates shared configuration files, memory mapped in
> > > each process, using the /var/run/.rte_config filename, -when run as
> > > root (or $HOME/.rte_config when run as a non-root user; -if filesystem
> and
> > device permissions are set up to allow this).
> > > -The rte part of the filenames of each of the above is configurable
> using the
> > file-prefix parameter.
> > > +Similarly, it creates shared configuration files, memory mapped in
> each
> > process.
> > > +When run as root, the name of the configuration file will be
> > > +/var/run/.rte_*host*_config, where *host* is the name of the machine.
> > > +When run as a non-root user, the the name of the configuration file
> > > +will be $HOME/.rte_*host*_config (if filesystem and device permissions
> > are set up to allow this).
> > > +If the ``--file-prefix`` parameter has been specified, its value will
> > > +be used in place of "rte" in the file names.
> >
> > I am not sure that we need to handle all such cases inside EAL.
> > User can easily overcome that problem by just adding something like:
> > --file-prefix=`uname -n`
> > to his command-line.
> > Konstantin
> >
>
> I agree with Konstantin, there's no need to include the hostname in the
> rte config file + I'm not sure this will be backward compatible with
> existing DPDK applications that use a secondary processes that use the
> config file (as in, multiprocess DPDK applications in use would all need to
> be updated). What Konstantin suggests fixes the issue you were encountering
> without breaking backward compatibility.
> Is addition, the hostname is not unique... you could in theory have 2
> hosts with the same hostname and encounter the issue you were seeing again.
>
> > >
> > > In addition to specifying the file-prefix parameter, any DPDK
> > > applications that are to be run side-by-side must explicitly limit
> their
> > memory use.
> > > diff --git a/lib/librte_eal/common/eal_common_proc.c
> > > b/lib/librte_eal/common/eal_common_proc.c
> > > index 12e0fca..517aa0c 100644
> > > --- a/lib/librte_eal/common/eal_common_proc.c
> > > +++ b/lib/librte_eal/common/eal_common_proc.c
> > > @@ -45,12 +45,8 @@ rte_eal_primary_proc_alive(const char
> > > *config_file_path)
> > >
> > > if (config_file_path)
> > > config_fd = open(config_file_path, O_RDONLY);
> > > - else {
> > > - char default_path[PATH_MAX+1];
> > > - snprintf(default_path, PATH_MAX, RUNTIME_CONFIG_FMT,
> > > -def
