[dpdk-dev] [PATCH v3 10/20] eal/dev: implement device iteration initialization

[Neil Horman]

On Wed, Mar 28, 2018 at 10:10:07AM +0200, Gaëtan Rivet wrote:
> 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:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> > > > Sent: Tuesday, March 27, 2018 9:35 PM
> > > > To: Richardson, Bruce <bruce.richardson at intel.com>
> > > > Cc: Neil Horman <nhorman at tuxdriver.com>; dev at dpdk.org; Wiles, Keith
> > > > <keith.wiles at intel.com>
> > > > 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:
> > > > > On Tue, Mar 27, 2018 at 10:20:40PM +0200, Gaëtan Rivet wrote:
> > > > > > On Tue, Mar 27, 2018 at 02:26:33PM -0400, Neil Horman wrote:
> > > > > > > On Tue, Mar 27, 2018 at 02:40:00PM +0200, Gaëtan Rivet wrote:
> > > > > > > > On Tue, Mar 27, 2018 at 07:47:50AM -0400, Neil Horman wrote:
> > > > > > > > > On Tue, Mar 27, 2018 at 01:18:34AM +0200, Gaetan Rivet wrote:
> > > > > > > > > > Parse a device description.
> > > > > > > > > > Split this description in their relevant part for each layers.
> > > > > > > > > > No dynamic allocation is performed.
> > > > > > > > > >
> > > > > > > > > > Cc: Neil Horman <nhorman at tuxdriver.com>
> > > > > > > > > > Cc: "Wiles, Keith" <keith.wiles at intel.com>
> > > > > > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > 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:
> > > > > > >
> > > > > > > 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;
> > > > > > >                 }
> > > > > > >         }
> > > > > > > }
> > > > > > >
> > > > > > > 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?
> > > > > > >
> > > > > > > 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:
> 
> > > > > > > > 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.
> 
I'm fine with just adding another include path to the CFLAGS for this purpose in
the short term, just saying I'm not sure why the log library got integrated into
EAL in the first place.

Neil

> -- 
> Gaëtan Rivet
> 6WIND
>