From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp2.cs.Stanford.EDU (smtp2.cs.stanford.edu [171.64.64.26]) by dpdk.org (Postfix) with ESMTP id EC1216CB7 for ; Tue, 11 Oct 2016 23:47:28 +0200 (CEST) Received: from mail-it0-f52.google.com ([209.85.214.52]:36204) by smtp2.cs.Stanford.EDU with esmtpsa (TLSv1.2:AES128-GCM-SHA256:128) (Exim 4.84_2) (envelope-from ) id 1bu4tT-0003rp-1r for dev@dpdk.org; Tue, 11 Oct 2016 14:47:28 -0700 Received: by mail-it0-f52.google.com with SMTP id l13so121057878itl.1 for ; Tue, 11 Oct 2016 14:47:27 -0700 (PDT) X-Gm-Message-State: AA6/9RlDN/rcMCJvuELkAZ41G1IgIU66JJ+o1FPjvXL4CufulYKzyuwRxueREe9eYyNGZxwIZubKPNy/2hUIlA== X-Received: by 10.107.7.135 with SMTP id g7mr159771ioi.15.1476222446681; Tue, 11 Oct 2016 14:47:26 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.98.67 with HTTP; Tue, 11 Oct 2016 14:46:46 -0700 (PDT) In-Reply-To: <39898498.0kdAxWznnB@xps13> References: <20160928204244.8288-1-ouster@cs.stanford.edu> <2532748.dRiGlJefCg@xps13> <39898498.0kdAxWznnB@xps13> From: John Ousterhout Date: Tue, 11 Oct 2016 14:46:46 -0700 X-Gmail-Original-Message-ID: Message-ID: To: Thomas Monjalon Cc: dev@dpdk.org X-Spam-Score: -1.0 X-Spam-Level: X-Spam-Checker-Version: SpamAssassin on smtp2.cs.Stanford.EDU X-Scan-Signature: b7af135983bf854fd3162ef3c6b6525a Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2] log: respect rte_openlog_stream calls before rte_eal_init X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Oct 2016 21:47:29 -0000 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@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. >