[dpdk-dev] Application framework vs. library

[dpdk-dev] Application framework vs. library

[Steffen.Bauch]

Hi,

as a developer of a product that uses DPDK I inherited a custom changeset
towards DPDK 2.0 to allow better integration into our application stack. I
am currently underway of updating the used DPDK release and would like to
get rid of these local changes in the future by influencing the upstream
development to some degree.

Concerning the integration we try to cope with the circumstance that
DPDK is more comparable to an application framework and less to a
library. The main part of the changes deal with EAL and specifically with
rte_eal_init() and prune or replace functionality. The pruned features
are in the responsibility of other proprietary libraries and modules
within our application. For illustration purposes, I have included an
old changeset towards v2.0.0. for reference, but due to the nature
of the whole changeset it does not make sense to apply it directly,
as there is a total lack of generality in the specific implementation
(e.g. only the linuxapp part is customized, incomplete implemented concept
in termination behavior ...). Nonetheless I am underway of researching
concepts and approaches that are general enough for upstream inclusion.

The main reason for attaching this patchset is to be able to discuss
possible approaches of further development, as I would like to get
feedback on possible development approaches to address the named problems.

In detail the changes address the following areas:

Logging behavior

Opening a connection to the system logger for our program is within the
responsibility of our specific logging module that is also used in the
proprietary parts of the application. For this reason openlog() should
not be called in our use case. In addition, using rte_openlog_stream()
is not usable before rte_eal_init() as logging init will overwrite the
used log stream. For this reason a large part of the init logs can not
be handled by a custom log stream. Btw, after removal of the log caching,
is there a fundamental difference between early log and normal logging?

A possible approach for the future could be to initialize rte_logs.file
to NULL (in fact it is, as it is static) and only set it during
initialization if it is NULL with the goal to be able to use
rte_openlog_stream() before rte_eal_init(). As the openlog() call is
only relevant when the default log stream is used (and architecture
dependent!) I introduced a specific entry point for calling openlog. The
main complexity to this changeset is added due to two reasons. First
reason is the circumstance that rte_eal_common_log_init() is used several
times during early logging and real logging initialization. Second
aspect is that a user might revert to the use of the default_log_stream
after a custom log stream has be used straight from the beginning (even
before rte_eal_init()). A dedicated changeset towards v16.07 for this
improvement is attached for review, feedback and possible inclusion.

Process termination behavior

As the DPDK functionality is only a small part of the whole application
ownership of the running process is not with DPDK and it is not allowed
to cancel the running process as a result of an error. For this reason,
the current changeset instead of calling rte_panic or rte_exit returns
an error code and handles it in the calling function. Unfortunately this
change was only implemented in rte_eal_init() and not in other places
(drivers, libs ...), so there is currently no safety that an unknown
code part terminates the whole application on the spot. Future changes
and patches could change the error handling in a more thorough approach,
but I would like to have some feedback and opinions before I start the
heavy-lifting work in over 700 code locations. Even some interfaces
might be affected, as lots of functions return void now,

Thread creation

In our application thread creation and setting the affinity is already
handled in a proprietary module. Frames are polled and processed
in non-EAL pthreads. For this reason the lcore thread creation in
rte_eal_init() is completely removed as we do not want additional threads
to be assigned to processing CPUs. Unfortunately setting the coremask
to 0 is not feasible directly. For the non-EAL threads a custom function
within the application sets the per_thread _lcore variable to the right
value to try to enable mbuf cacheing. The changeset also disables the
interrupt handling thread as in our application it currently seems not
necessary (maybe make this optional?)

In an experiment I removed the error check for the non-zero number of
lcores and quick-fixed certain locations where rte_lcore_count() is used
for memory size computations and at least got a running application.

A possible approach could be to enhance enum rte_lcore_role_t with an
auxiliary thread type and introduce additional eal arguments to assign
lcores with these auxiliary role.

I would like to receive feedback on possible development directions.

Thank you,

Steffen Bauch




-- 

Steffen Bauch
Senior Software Engineer Platform & Technology

R&S Cybersecurity ipoque GmbH
Augustusplatz 9, D-04109 Leipzig
Phone: + 49 341 59403 0
Email: steffen.bauch@rohde-schwarz.com
Internet: www.ipoque.com

Trade register Amtsgericht Leipzig HRB21462
Gesellschaft mit beschränkter Haftung (GmbH)
Dirk Czepluch - Managing Director
From sagi@grimberg.me  Mon Aug  1 10:44:25 2016
Return-Path: <sagi@grimberg.me>
Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134])
 by dpdk.org (Postfix) with ESMTP id 99B3F2C0C
 for <dev@dpdk.org>; Mon,  1 Aug 2016 10:44:25 +0200 (CEST)
Received: from bzq-82-81-101-184.red.bezeqint.net ([82.81.101.184]
 helo=bombadil.infradead.org)
 by merlin.infradead.org with esmtpsa (Exim 4.85_2 #1 (Red Hat Linux))
 id 1bU8pk-00089b-B4 for dev@dpdk.org; Mon, 01 Aug 2016 08:44:24 +0000
From: Sagi Grimberg <sagi@grimberg.me>
To: dev@dpdk.org
Date: Mon,  1 Aug 2016 11:44:21 +0300
Message-Id: <1470041061-8059-1-git-send-email-sagi@grimberg.me>
X-Mailer: git-send-email 1.9.1
Subject: [dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 01 Aug 2016 08:44:25 -0000

The user is allowed to call ->rx_pkt_burst() even without free
mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
on the first iteration (where pkt is still NULL). This would cause us
to deref a NULL pkt (reset refcount and free).

Fix this by checking the pkt before freeing it.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/net/mlx5/mlx5_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index fce3381ae87a..a07cc4794023 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1572,7 +1572,7 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 		rte_prefetch0(wqe);
 		rep = rte_mbuf_raw_alloc(rxq->mp);
 		if (unlikely(rep == NULL)) {
-			while (pkt != seg) {
+			while (pkt && pkt != seg) {
 				assert(pkt != (*rxq->elts)[idx]);
 				seg = NEXT(pkt);
 				rte_mbuf_refcnt_set(pkt, 0);
--
1.9.1

Re: [dpdk-dev] Application framework vs. library

[Thomas Monjalon]

Hi,

Thanks for explaining your context.
Your concerns are known under the term "usability enhancements".
As they require some API changes, we won't make them in the release 16.11.
But we must work on it now to be able to announce the API changes for 17.02
before the 16.11 release.

2016-08-01 10:18, Steffen.Bauch@rohde-schwarz.com:
> Concerning the integration we try to cope with the circumstance that
> DPDK is more comparable to an application framework and less to a
> library.

I think DPDK should be easier to use and considered as a normal library.


[...]
> Logging behavior
> 
> Opening a connection to the system logger for our program is within the
> responsibility of our specific logging module that is also used in the
> proprietary parts of the application. For this reason openlog() should
> not be called in our use case. In addition, using rte_openlog_stream()
> is not usable before rte_eal_init() as logging init will overwrite the
> used log stream. For this reason a large part of the init logs can not
> be handled by a custom log stream. Btw, after removal of the log caching,
> is there a fundamental difference between early log and normal logging?

You're right. Now that the log history is removed, we can rework the log
initialization and reconsider early logging.

> A possible approach for the future could be to initialize rte_logs.file
> to NULL (in fact it is, as it is static) and only set it during
> initialization if it is NULL with the goal to be able to use
> rte_openlog_stream() before rte_eal_init(). As the openlog() call is
> only relevant when the default log stream is used (and architecture
> dependent!) I introduced a specific entry point for calling openlog. The
> main complexity to this changeset is added due to two reasons. First
> reason is the circumstance that rte_eal_common_log_init() is used several
> times during early logging and real logging initialization. Second
> aspect is that a user might revert to the use of the default_log_stream
> after a custom log stream has be used straight from the beginning (even
> before rte_eal_init()). A dedicated changeset towards v16.07 for this
> improvement is attached for review, feedback and possible inclusion.

Before entering in the proposed design for a custom log handler, we should
discuss the desired features in DPDK logging.
Is there some developer who would like to see the logs improved to be
dynamically tuned in run-time live?
Or should we just allow a custom log handler and provide only a default
minimal handler?


> Process termination behavior
> 
> As the DPDK functionality is only a small part of the whole application
> ownership of the running process is not with DPDK and it is not allowed
> to cancel the running process as a result of an error. For this reason,
> the current changeset instead of calling rte_panic or rte_exit returns
> an error code and handles it in the calling function. Unfortunately this
> change was only implemented in rte_eal_init() and not in other places
> (drivers, libs ...), so there is currently no safety that an unknown
> code part terminates the whole application on the spot. Future changes
> and patches could change the error handling in a more thorough approach,
> but I would like to have some feedback and opinions before I start the
> heavy-lifting work in over 700 code locations. Even some interfaces
> might be affected, as lots of functions return void now,

Yes, please let's remove every panic/exit calls from DPDK.
The only reason it is still there is that nobody took the time to remove
these traces from the early support of bare metal environment (now dropped).


> Thread creation
> 
> In our application thread creation and setting the affinity is already
> handled in a proprietary module. Frames are polled and processed
> in non-EAL pthreads. For this reason the lcore thread creation in
> rte_eal_init() is completely removed as we do not want additional threads
> to be assigned to processing CPUs. Unfortunately setting the coremask
> to 0 is not feasible directly. For the non-EAL threads a custom function
> within the application sets the per_thread _lcore variable to the right
> value to try to enable mbuf cacheing. The changeset also disables the
> interrupt handling thread as in our application it currently seems not
> necessary (maybe make this optional?)
> 
> In an experiment I removed the error check for the non-zero number of
> lcores and quick-fixed certain locations where rte_lcore_count() is used
> for memory size computations and at least got a running application.
> 
> A possible approach could be to enhance enum rte_lcore_role_t with an
> auxiliary thread type and introduce additional eal arguments to assign
> lcores with these auxiliary role.

My personal opinion is that DPDK (as a library) should not create some
threads at all.
The loops and workload strategy is the responsibility of the application.

Re: [dpdk-dev] Application framework vs. library

[Wiles, Keith]

> On Aug 1, 2016, at 5:04 AM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> Hi,
> 
> Thanks for explaining your context.
> Your concerns are known under the term "usability enhancements".
> As they require some API changes, we won't make them in the release 16.11.
> But we must work on it now to be able to announce the API changes for 17.02
> before the 16.11 release.
> 
> 2016-08-01 10:18, Steffen.Bauch@rohde-schwarz.com:
>> Concerning the integration we try to cope with the circumstance that
>> DPDK is more comparable to an application framework and less to a
>> library.
> 
> I think DPDK should be easier to use and considered as a normal library.
> 
> 
> [...]
>> Logging behavior
>> 
>> Opening a connection to the system logger for our program is within the
>> responsibility of our specific logging module that is also used in the
>> proprietary parts of the application. For this reason openlog() should
>> not be called in our use case. In addition, using rte_openlog_stream()
>> is not usable before rte_eal_init() as logging init will overwrite the
>> used log stream. For this reason a large part of the init logs can not
>> be handled by a custom log stream. Btw, after removal of the log caching,
>> is there a fundamental difference between early log and normal logging?
> 
> You're right. Now that the log history is removed, we can rework the log
> initialization and reconsider early logging.
> 
>> A possible approach for the future could be to initialize rte_logs.file
>> to NULL (in fact it is, as it is static) and only set it during
>> initialization if it is NULL with the goal to be able to use
>> rte_openlog_stream() before rte_eal_init(). As the openlog() call is
>> only relevant when the default log stream is used (and architecture
>> dependent!) I introduced a specific entry point for calling openlog. The
>> main complexity to this changeset is added due to two reasons. First
>> reason is the circumstance that rte_eal_common_log_init() is used several
>> times during early logging and real logging initialization. Second
>> aspect is that a user might revert to the use of the default_log_stream
>> after a custom log stream has be used straight from the beginning (even
>> before rte_eal_init()). A dedicated changeset towards v16.07 for this
>> improvement is attached for review, feedback and possible inclusion.
> 
> Before entering in the proposed design for a custom log handler, we should
> discuss the desired features in DPDK logging.
> Is there some developer who would like to see the logs improved to be
> dynamically tuned in run-time live?
> Or should we just allow a custom log handler and provide only a default
> minimal handler?

I would suggest we have a custom log handler for applications that have their own logging system. I believe we still need a logging system in DPDK as a complete ‘none’ library type system. DPDK was designed as a complete system (or appears that way), but I understand moving toward a library design is reasonable. I just do not want to lose the complete system design as we should be able to have both.

The only down side to a library design is testing becomes more complex IMO as we do not have a standard configuration(s) and “complete” applications. DPDK is a complex designed system and it can become difficult to test the library interactions if we do not have a clean way to test the complete set of libraries.

> 
> 
>> Process termination behavior
>> 
>> As the DPDK functionality is only a small part of the whole application
>> ownership of the running process is not with DPDK and it is not allowed
>> to cancel the running process as a result of an error. For this reason,
>> the current changeset instead of calling rte_panic or rte_exit returns
>> an error code and handles it in the calling function. Unfortunately this
>> change was only implemented in rte_eal_init() and not in other places
>> (drivers, libs ...), so there is currently no safety that an unknown
>> code part terminates the whole application on the spot. Future changes
>> and patches could change the error handling in a more thorough approach,
>> but I would like to have some feedback and opinions before I start the
>> heavy-lifting work in over 700 code locations. Even some interfaces
>> might be affected, as lots of functions return void now,
> 
> Yes, please let's remove every panic/exit calls from DPDK.
> The only reason it is still there is that nobody took the time to remove
> these traces from the early support of bare metal environment (now dropped).

Removing the panic calls is find, but the patch just replaces them with return -1 and will not do as you need to know why the calls failed. Replacing them with returning an error code and calling a log output would be much more reasonable.

> 
> 
>> Thread creation
>> 
>> In our application thread creation and setting the affinity is already
>> handled in a proprietary module. Frames are polled and processed
>> in non-EAL pthreads. For this reason the lcore thread creation in
>> rte_eal_init() is completely removed as we do not want additional threads
>> to be assigned to processing CPUs. Unfortunately setting the coremask
>> to 0 is not feasible directly. For the non-EAL threads a custom function
>> within the application sets the per_thread _lcore variable to the right
>> value to try to enable mbuf cacheing. The changeset also disables the
>> interrupt handling thread as in our application it currently seems not
>> necessary (maybe make this optional?)
>> 
>> In an experiment I removed the error check for the non-zero number of
>> lcores and quick-fixed certain locations where rte_lcore_count() is used
>> for memory size computations and at least got a running application.
>> 
>> A possible approach could be to enhance enum rte_lcore_role_t with an
>> auxiliary thread type and introduce additional eal arguments to assign
>> lcores with these auxiliary role.
> 
> My personal opinion is that DPDK (as a library) should not create some
> threads at all.
> The loops and workload strategy is the responsibility of the application.

Removing the threading support in DPDK is reasonable, but we also need to keep the support for applications that are using DPDK as is. We should not toss out the ‘none’ library design just to make it harder for everyone that does not view DPDK as just a library to use.

We need to support both is a seamless way and be able to test DPDK without having to write a bunch of ‘application’ code to support the new library only design.

Regards,
Keith

Re: [dpdk-dev] Application framework vs. library

[Steffen.Bauch]

diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
b/lib/librte_eal/linuxapp/eal/eal.c
index bd770cf..f63f2f8 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -664,12 +664,6 @@ eal_check_mem_on_local_socket(void)
                        "memory on local socket!\n");
 }
 
-static int
-sync_func(__attribute__((unused)) void *arg)
-{
-       return 0;
-}
-
 inline static void
 rte_eal_mcfg_complete(void)
 {
@@ -699,26 +693,17 @@ rte_eal_iopl_init(void)
 int
 rte_eal_init(int argc, char **argv)
 {
-       int i, fctret, ret;
-       pthread_t thread_id;
-       static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
+       int fctret;
        struct shared_driver *solib = NULL;
        const char *logid;
-       char cpuset[RTE_CPU_AFFINITY_STR_LEN];
-
-       if (!rte_atomic32_test_and_set(&run_once))
-               return -1;
 
-       logid = strrchr(argv[0], '/');
-       logid = strdup(logid ? logid + 1: argv[0]);
-
-       thread_id = pthread_self();
+       logid = NULL;
 
        if (rte_eal_log_early_init() < 0)
-               rte_panic("Cannot init early logs\n");
+               return -1;
 
        if (rte_eal_cpu_init() < 0)
-               rte_panic("Cannot detect lcores\n");
+               return -1;
 
        fctret = eal_parse_args(argc, argv);
        if (fctret < 0)
@@ -731,7 +716,7 @@ rte_eal_init(int argc, char **argv)
                        internal_config.process_type != RTE_PROC_SECONDARY 
&&
                        internal_config.xen_dom0_support == 0 &&
                        eal_hugepage_info_init() < 0)
-               rte_panic("Cannot get hugepage information\n");
+               return -1;
 
        if (internal_config.memory == 0 && internal_config.force_sockets 
== 0) {
                if (internal_config.no_hugetlbfs)
@@ -756,41 +741,43 @@ rte_eal_init(int argc, char **argv)
        rte_config_init();
 
        if (rte_eal_pci_init() < 0)
-               rte_panic("Cannot init PCI\n");
+               return -1;
 
 #ifdef RTE_LIBRTE_IVSHMEM
        if (rte_eal_ivshmem_init() < 0)
-               rte_panic("Cannot init IVSHMEM\n");
+               return -1;
 #endif
 
        if (rte_eal_memory_init() < 0)
-               rte_panic("Cannot init memory\n");
+               return -1;
 
        /* the directories are locked during eal_hugepage_info_init */
        eal_hugedirs_unlock();
 
        if (rte_eal_memzone_init() < 0)
-               rte_panic("Cannot init memzone\n");
+               return -1;
 
        if (rte_eal_tailqs_init() < 0)
-               rte_panic("Cannot init tail queues for objects\n");
+               return -1;
 
 #ifdef RTE_LIBRTE_IVSHMEM
        if (rte_eal_ivshmem_obj_init() < 0)
-               rte_panic("Cannot init IVSHMEM objects\n");
+               return -1;
 #endif
 
        if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0)
-               rte_panic("Cannot init logs\n");
+               return -1;
 
        if (rte_eal_alarm_init() < 0)
-               rte_panic("Cannot init interrupt-handling thread\n");
+               return -1;
 
+       /* interrupt handling will be initialized but the thread is 
patched to immediately exit */
        if (rte_eal_intr_init() < 0)
-               rte_panic("Cannot init interrupt-handling thread\n");
+               return -1;
 
+       /* timer stuff is initialized but hpet should be disabled in 
configuration */
        if (rte_eal_timer_init() < 0)
-               rte_panic("Cannot init HPET or TSC timers\n");
+               return -1;
 
        eal_check_mem_on_local_socket();
 
@@ -803,47 +790,12 @@ rte_eal_init(int argc, char **argv)
                        RTE_LOG(WARNING, EAL, "%s\n", dlerror());
        }
 
-       eal_thread_init_master(rte_config.master_lcore);
-
-       ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
-
-       RTE_LOG(DEBUG, EAL, "Master lcore %u is ready 
(tid=%x;cpuset=[%s%s])\n",
-               rte_config.master_lcore, (int)thread_id, cpuset,
-               ret == 0 ? "" : "...");
-
        if (rte_eal_dev_init() < 0)
-               rte_panic("Cannot init pmd devices\n");
-
-       RTE_LCORE_FOREACH_SLAVE(i) {
-
-               /*
-                * create communication pipes between master thread
-                * and children
-                */
-               if (pipe(lcore_config[i].pipe_master2slave) < 0)
-                       rte_panic("Cannot create pipe\n");
-               if (pipe(lcore_config[i].pipe_slave2master) < 0)
-                       rte_panic("Cannot create pipe\n");
-
-               lcore_config[i].state = WAIT;
-
-               /* create a thread for each lcore */
-               ret = pthread_create(&lcore_config[i].thread_id, NULL,
-                                    eal_thread_loop, NULL);
-               if (ret != 0)
-                       rte_panic("Cannot create thread\n");
-       }
-
-       /*
-        * Launch a dummy function on all slave lcores, so that master 
lcore
-        * knows they are all ready when this function returns.
-        */
-       rte_eal_mp_remote_launch(sync_func, NULL, SKIP_MASTER);
-       rte_eal_mp_wait_lcore();
+               return -1;
 
        /* Probe & Initialize PCI devices */
        if (rte_eal_pci_probe())
-               rte_panic("Cannot probe PCI\n");
+               return -1;
 
        return fctret;
 }
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c 
b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 66deda2..886e5e0 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -774,6 +774,9 @@ eal_intr_thread_main(__rte_unused void *arg)
 {
        struct epoll_event ev;
 
+       /* we do not need the interrupt handling and do not want the extra 
thread */
+       pthread_exit((void*)0);
+
        /* host thread, never break out */
        for (;;) {
                /* build up the epoll fd with all descriptors we are to
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c 
b/lib/librte_eal/linuxapp/eal/eal_log.c
index 0b133c3..7c4d569 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -91,7 +91,7 @@ static cookie_io_functions_t console_log_func = {
  * once memzones are available.
  */
 int
-rte_eal_log_init(const char *id, int facility)
+rte_eal_log_init(__attribute__((unused)) const char *id, 
__attribute__((unused)) int facility)
 {
        FILE *log_stream;
 
@@ -99,8 +99,6 @@ rte_eal_log_init(const char *id, int facility)
        if (log_stream == NULL)
                return -1;
 
-       openlog(id, LOG_NDELAY | LOG_PID, facility);
-
        if (rte_eal_common_log_init(log_stream) < 0)
                return -1;
 
-- 

Steffen Bauch
Senior Software Engineer Platform & Technology

R&S Cybersecurity ipoque GmbH
Augustusplatz 9, D-04109 Leipzig
Phone: + 49 341 59403 0
Email: steffen.bauch@rohde-schwarz.com
Internet: www.ipoque.com

Trade register Amtsgericht Leipzig HRB21462
Gesellschaft mit beschränkter Haftung (GmbH)
Dirk Czepluch - Managing Director

Re: [dpdk-dev] Application framework vs. library

[Steffen.Bauch]

>From c3be5420d921325559de9b1079354e1c4314220a Mon Sep 17 00:00:00 2001
From: Steffen Bauch <steffen.bauch@rohde-schwarz.com>
Date: Mon, 25 Jul 2016 16:13:02 +0200
Subject: [PATCH] allow the call to rte_openlog_stream() before 
rte_eal_init()

- only initialize the default_log_stream if it was not initialized
  before main initialization of EAL
- save facility and logid in rte_default_log_args structure
- call openlog only on setup of the default_log_stream in 
rte_openlog_stream
---
 lib/librte_eal/bsdapp/eal/eal_log.c     |  6 ++---
 lib/librte_eal/common/eal_common_log.c  | 47 
++++++++++++++++++++++++++++++---
 lib/librte_eal/common/eal_private.h     | 40 +++++++++++++++++++++++++---
 lib/librte_eal/common/include/rte_log.h |  7 +++++
 lib/librte_eal/linuxapp/eal/eal_log.c   | 24 +++++++++++++----
 5 files changed, 110 insertions(+), 14 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_log.c 
b/lib/librte_eal/bsdapp/eal/eal_log.c
index a425f7a..5abd906 100644
--- a/lib/librte_eal/bsdapp/eal/eal_log.c
+++ b/lib/librte_eal/bsdapp/eal/eal_log.c
@@ -42,9 +42,9 @@
  * once memzones are available.
  */
 int
-rte_eal_log_init(const char *id __rte_unused, int facility __rte_unused)
+rte_eal_log_init(const char *id, int facility)
 {
-       if (rte_eal_common_log_init(stderr) < 0)
+       if (rte_eal_common_log_init(stderr, id, facility) < 0)
                return -1;
        return 0;
 }
@@ -52,6 +52,6 @@ rte_eal_log_init(const char *id __rte_unused, int 
facility __rte_unused)
 int
 rte_eal_log_early_init(void)
 {
-       rte_openlog_stream(stderr);
+       rte_openlog_stream_initial(stderr);
        return 0;
 }
diff --git a/lib/librte_eal/common/eal_common_log.c 
b/lib/librte_eal/common/eal_common_log.c
index 7916c78..197e6c9 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -48,6 +48,14 @@ struct rte_logs rte_logs = {
        .file = NULL,
 };
 
+struct rte_default_log_args rte_default_log_args =
+{
+        .facility = 0,
+        .log_id = NULL,
+        .prepare_default_log = NULL,
+};
+
+
 static FILE *default_log_stream;
 
 /**
@@ -82,9 +90,30 @@ int
 rte_openlog_stream(FILE *f)
 {
        if (f == NULL)
+       {
+               if (rte_default_log_args.prepare_default_log)
+               {
+                       if 
(rte_default_log_args.prepare_default_log(rte_default_log_args.log_id,
+                                       rte_default_log_args.facility) < 
0)
+                       return -1;
+               }
                rte_logs.file = default_log_stream;
+       }
        else
+       {
                rte_logs.file = f;
+       }
+       return 0;
+}
+
+int
+rte_openlog_stream_initial(FILE *f)
+{
+       /* only initialize if not configured before rte_eal_init() */
+       if (NULL == rte_logs.file)
+       {
+               return rte_openlog_stream(f);
+       }
        return 0;
 }
 
@@ -176,14 +205,26 @@ rte_log(uint32_t level, uint32_t logtype, const char 
*format, ...)
        return ret;
 }
 
+int rte_eal_set_default_log_stream(FILE *default_log,  const char *id, 
int facility,
+                                    int (*prepare_log)(const char 
*log_id, int facility))
+{
+       rte_default_log_args.facility = facility;
+       rte_default_log_args.log_id = id;
+       rte_default_log_args.prepare_default_log = prepare_log;
+       default_log_stream = default_log;
+
+       return 0;
+}
+
 /*
  * called by environment-specific log init function
  */
 int
-rte_eal_common_log_init(FILE *default_log)
+rte_eal_common_log_init(FILE *default_log, const char *id, int facility,
+                        int (*prepare_log)(const char *log_id, int 
facility))
 {
-       default_log_stream = default_log;
-       rte_openlog_stream(default_log);
+       rte_eal_set_default_log_stream(default_log, id, facility, 
prepare_log);
+       rte_openlog_stream_initial(NULL); /* enable default log stream */
 
 #if RTE_LOG_LEVEL >= RTE_LOG_DEBUG
        RTE_LOG(NOTICE, EAL, "Debug logs available - lower 
performance\n");
diff --git a/lib/librte_eal/common/eal_private.h 
b/lib/librte_eal/common/eal_private.h
index 857dc3e..f12a00d 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -49,13 +49,26 @@ int rte_eal_memzone_init(void);
 /**
  * Common log initialization function (private to eal).
  *
- * @param default_log
- *   The default log stream to be used.
+ * @param default_log The default log stream to be used.
+ * @param id the id used for openlog call
+ * @param facility the facility used for openlog call
+ * @param prepare_log a platform specific log prepare function
  * @return
  *   - 0 on success
  *   - Negative on error
  */
-int rte_eal_common_log_init(FILE *default_log);
+int rte_eal_common_log_init(FILE *default_log, const char *id, int 
facility,
+                              int (*prepare_log)(const char *log_id, int 
facility));
+
+/**
+ * A function that only sets the log stream if it has not previously been 
set.
+ *
+ * This function is private to EAL.
+ *
+ * @return
+ *   0 on success, negative on error
+ */
+int rte_openlog_stream_initial(FILE *f);
 
 /**
  * Fill configuration with number of physical and logical processors
@@ -97,6 +110,27 @@ int rte_eal_memory_init(void);
 int rte_eal_timer_init(void);
 
 /**
+ * Perform necessary initialization for the default stream before it is 
used
+ *
+ * This function is private to EAL.
+ *
+ * @return
+ *   0 on success, negative on error
+ */
+int rte_eal_prepare_log(const char *id, int facility);
+
+/**
+ * Setup the default log stream and related parameters
+ *
+ * This function is private to EAL.
+ *
+ * @return
+ *   0 on success, negative on error
+ */
+int rte_eal_set_default_log_stream(FILE *default_log,  const char *id, 
int facility,
+                            int (*prepare_log)(const char *log_id, int 
facility));
+
+/**
  * Init early logs
  *
  * This function is private to EAL.
diff --git a/lib/librte_eal/common/include/rte_log.h 
b/lib/librte_eal/common/include/rte_log.h
index b1add04..4eb98a6 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -59,8 +59,15 @@ struct rte_logs {
        FILE *file;     /**< Pointer to current FILE* for logs. */
 };
 
+struct rte_default_log_args {
+       int facility;
+       const char *log_id;
+       int (*prepare_default_log)(const char *log_id, int facility);
+};
+
 /** Global log informations */
 extern struct rte_logs rte_logs;
+extern struct rte_default_log_args rte_default_log_args;
 
 /* SDK log type */
 #define RTE_LOGTYPE_EAL     0x00000001 /**< Log related to eal. */
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c 
b/lib/librte_eal/linuxapp/eal/eal_log.c
index d391100..052237b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -49,6 +49,8 @@
 
 #include "eal_private.h"
 
+static FILE *early_log_stream;
+
 /*
  * default log function
  */
@@ -82,10 +84,18 @@ static cookie_io_functions_t console_log_func = {
        .write = console_log_write,
 };
 
+static int
+rte_eal_prepare_default_log(const char *id, int facility)
+{
+       openlog(id, LOG_NDELAY | LOG_PID, facility);
+       return 0;
+}
+
 /*
  * set the log to default function, called during eal init process,
  * once memzones are available.
  */
+
 int
 rte_eal_log_init(const char *id, int facility)
 {
@@ -95,10 +105,14 @@ rte_eal_log_init(const char *id, int facility)
        if (log_stream == NULL)
                return -1;
 
-       openlog(id, LOG_NDELAY | LOG_PID, facility);
-
-       if (rte_eal_common_log_init(log_stream) < 0)
-               return -1;
+       if (rte_logs.file != early_log_stream) /* logging was initialized 
before rte_eal_init() */
+       {
+               rte_eal_set_default_log_stream(log_stream, id, facility, 
rte_eal_prepare_default_log);
+       } else
+       {
+               if (rte_eal_common_log_init(log_stream, id, facility, 
rte_eal_prepare_default_log) < 0)
+                       return -1;
+       }
 
        return 0;
 }
@@ -136,6 +150,6 @@ rte_eal_log_early_init(void)
                printf("Cannot configure early_log_stream\n");
                return -1;
        }
-       rte_openlog_stream(early_log_stream);
+       rte_openlog_stream_initial(early_log_stream);
        return 0;
 }
-- 
2.9.2

-- 

Steffen Bauch
Senior Software Engineer Platform & Technology

R&S Cybersecurity ipoque GmbH
Augustusplatz 9, D-04109 Leipzig
Phone: + 49 341 59403 0
Email: steffen.bauch@rohde-schwarz.com
Internet: www.ipoque.com

Trade register Amtsgericht Leipzig HRB21462
Gesellschaft mit beschränkter Haftung (GmbH)
Dirk Czepluch - Managing Director