On Tue, Mar 27, 2018 at 07:53:46PM -0400, Neil Horman wrote:

On Tue, Mar 27, 2018 at 08:48:01PM +0000, Richardson, Bruce wrote:

quoted

quoted

-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
Sent: Tuesday, March 27, 2018 9:35 PM
To: Richardson, Bruce <redacted>
Cc: Neil Horman <nhorman@tuxdriver.com>; dev@dpdk.org; Wiles, Keith
[off-list ref]
Subject: Re: [dpdk-dev] [PATCH v3 10/20] eal/dev: implement device
iteration initialization

On Tue, Mar 27, 2018 at 09:28:07PM +0100, Bruce Richardson wrote:

quoted

On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:

quoted

On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman wrote:

quoted

On Tue, Mar 27, 2018 at 02:40:00PM +0200, Gaëtan Rivet wrote:

quoted

On Tue, Mar 27, 2018 at 07:47:50AM -0400, Neil Horman wrote:

quoted

On Tue, Mar 27, 2018 at 01:18:34AM +0200, Gaetan Rivet wrote:

quoted

Parse a device description.
Split this description in their relevant part for each layers.
No dynamic allocation is performed.

Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: "Wiles, Keith" <redacted>
Signed-off-by: Gaetan Rivet <redacted>
---

This version uses librte_kvargs.

Otherwise, this looks pretty good to me

Please look into the librte_kvargs compatibility patch as well
(quite short). I'm very unhappy about the logging hack.
There is always the solution of setting a function pointer on
rte_log with the proper loglevels and so on.
Ideally rte_log could be made independent (starting skimming EAL
from all the fat), but this is much less trivial.

just posted about that.  I agree with Keith, I don't think you
should need that patch.  RTE_LOG just calls rte_vlog which contains

this code:

quoted

quoted

quoted

if (f == NULL) {
                f = default_log_stream;
                if (f == NULL) {
                        /*
                         * Grab the current value of stderr here,

rather than

quoted

quoted

quoted

                         * just initializing default_log_stream to

stderr. This

quoted

quoted

quoted

                         * ensures that we will always use the

current value

quoted

quoted

quoted

                         * of stderr, even if the application closes

and

quoted

quoted

quoted

                         * reopens it.
                         */
                        f = stderr;
                }
        }
}

Which I read as saying that the logging library should back off to
stderr if its not initialized yet.  If you've encountered a
problem that made you need that logging patch, it seems like you
should be able to drop it, and we need to fix the logging library.

Can you elaborate on what you ran into here?

quoted

quoted

quoted

Neil

Neat. The issue is that rte_log.h is not symlink-ed until librte_eal
is processed. rte_log cannot be included.

Sure it can - just pass -I/path/to/eal as a cflag.

/Bruce

When you put it this way... :)

Sure, this is a solution, of which early symlink was a genericization.
I'm just not a fan of having co-dependent libraries.

But this will probably come to that.

The other alternative is what was done with rte_compat.h - create a new lib
just with a single header file in it. Works ok too, and may seem less hacky
to some folks.

/Bruce

This seems like a good alternative to me.  I'm not entirely sure why the logging
functions are integrated to EAL anyway.  

Neil
 

As I said earlier:

quoted

quoted

quoted

quoted

quoted

quoted

Ideally rte_log could be made independent (starting skimming EAL
from all the fat), but this is much less trivial.

rte_log could certainly stand on its own. I quickly attempted to make it
a library, but this is too much work at this point. I think the EAL
should be broken down, it is too monolithic. Problem is that many
other libraries / applications, now relies on parts of the EAL that
would need to be moved.

So any effort in this direction will be difficult to undertake (and
badly received by the community, with good reasons), especially when
the workaround is an additional -I cflag.

-- 
Gaëtan Rivet
6WIND