From: John Ousterhout <ouster@cs.stanford.edu>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init
Date: Tue, 11 Oct 2016 14:46:46 -0700 [thread overview]
Message-ID: <CAGXJAmx-OsCvwwrxp6gMT+s9PcCSN0zvzaO7UU3T9o4aE0g_9Q@mail.gmail.com> (raw)
In-Reply-To: <39898498.0kdAxWznnB@xps13>
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 <thomas.monjalon@6wind.com>
wrote:
> 2016-10-11 09:30, John Ousterhout:
> > On Tue, Oct 11, 2016 at 1:08 AM, Thomas Monjalon <
> thomas.monjalon@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.
>
next prev parent reply other threads:[~2016-10-11 21:47 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-28 20:42 [dpdk-dev] [PATCH] " John Ousterhout
2016-09-30 15:01 ` Thomas Monjalon
2016-10-10 22:39 ` [dpdk-dev] [PATCH v2] " John Ousterhout
2016-10-11 8:08 ` Thomas Monjalon
2016-10-11 16:30 ` John Ousterhout
2016-10-11 20:30 ` Thomas Monjalon
2016-10-11 21:46 ` John Ousterhout [this message]
2016-10-12 7:09 ` Thomas Monjalon
2016-10-11 22:16 ` Don Provan
2016-10-12 0:22 ` John Ousterhout
2016-10-12 19:29 ` [dpdk-dev] [PATCH v3] " John Ousterhout
2016-10-12 19:38 ` [dpdk-dev] [PATCH v4] " John Ousterhout
2016-10-12 19:47 ` Thomas Monjalon
2016-10-12 21:17 ` John Ousterhout
2016-10-13 20:03 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGXJAmx-OsCvwwrxp6gMT+s9PcCSN0zvzaO7UU3T9o4aE0g_9Q@mail.gmail.com \
--to=ouster@cs.stanford.edu \
--cc=dev@dpdk.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).