[dpdk-dev] rte_eal_init() alternative?

[dpdk-dev] rte_eal_init() alternative?

[Montorsi, Francesco]

Hi all,

Currently it seems that the only way to initialize EAL is using rte_eal_init() function, correct?

I have the problem that rte_eal_init() will call rte_panic() whenever something fails to initialize or in other cases it will call exit().
In my application, I would rather like to attempt DPDK initialization. If it fails I don't want to exit.
Unfortunately I cannot even copy&paste the rte_eal_init() code into my application (removing rte_panic and exit calls) since it uses a lot of DPDK internal private functions.

I think that my requirements (avoid abort/exit calls when init fails) is a basic requirement... would you accept a patch that adds an alternative rte_eal_init() function that just returns an error code upon failure, instead of immediately exiting?

Thanks for your hard work!

Francesco Montorsi

Re: [dpdk-dev] rte_eal_init() alternative?

[Bruce Richardson]

On Wed, Sep 02, 2015 at 12:49:40PM +0000, Montorsi, Francesco wrote:
> Hi all,
> 
> Currently it seems that the only way to initialize EAL is using rte_eal_init() function, correct?
> 
> I have the problem that rte_eal_init() will call rte_panic() whenever something fails to initialize or in other cases it will call exit().
> In my application, I would rather like to attempt DPDK initialization. If it fails I don't want to exit.
> Unfortunately I cannot even copy&paste the rte_eal_init() code into my application (removing rte_panic and exit calls) since it uses a lot of DPDK internal private functions.
> 
> I think that my requirements (avoid abort/exit calls when init fails) is a basic requirement... would you accept a patch that adds an alternative rte_eal_init() function that just returns an error code upon failure, instead of immediately exiting?
> 
> Thanks for your hard work!
> 
> Francesco Montorsi
> 
I, for one, would welcome such a patch. I think the code is overly quick in
many places to panic or exit the app, when an error code would be more appropriate.
Feel free to also look at other libraries in DPDK too, if you like :-)

Regards,
/Bruce

Re: [dpdk-dev] rte_eal_init() alternative?

[Thomas Monjalon]

2015-09-02 13:56, Bruce Richardson:
> On Wed, Sep 02, 2015 at 12:49:40PM +0000, Montorsi, Francesco wrote:
> > Hi all,
> > 
> > Currently it seems that the only way to initialize EAL is using rte_eal_init() function, correct?
> > 
> > I have the problem that rte_eal_init() will call rte_panic() whenever something fails to initialize or in other cases it will call exit().
> > In my application, I would rather like to attempt DPDK initialization. If it fails I don't want to exit.
> > Unfortunately I cannot even copy&paste the rte_eal_init() code into my application (removing rte_panic and exit calls) since it uses a lot of DPDK internal private functions.
> > 
> > I think that my requirements (avoid abort/exit calls when init fails) is a basic requirement... would you accept a patch that adds an alternative rte_eal_init() function that just returns an error code upon failure, instead of immediately exiting?
> > 
> > Thanks for your hard work!
> > 
> > Francesco Montorsi
> > 
> I, for one, would welcome such a patch. I think the code is overly quick in
> many places to panic or exit the app, when an error code would be more appropriate.
> Feel free to also look at other libraries in DPDK too, if you like :-)

Yes but please, do not create an alternative init function.
We just need to replace panic/exit with error codes and be sure that apps
and examples handle them correctly.
Thanks

Re: [dpdk-dev] rte_eal_init() alternative?

[Jay Rolette]

On Wed, Sep 2, 2015 at 7:56 AM, Bruce Richardson <bruce.richardson@intel.com
> wrote:

> On Wed, Sep 02, 2015 at 12:49:40PM +0000, Montorsi, Francesco wrote:
> > Hi all,
> >
> > Currently it seems that the only way to initialize EAL is using
> rte_eal_init() function, correct?
> >
> > I have the problem that rte_eal_init() will call rte_panic() whenever
> something fails to initialize or in other cases it will call exit().
> > In my application, I would rather like to attempt DPDK initialization.
> If it fails I don't want to exit.
> > Unfortunately I cannot even copy&paste the rte_eal_init() code into my
> application (removing rte_panic and exit calls) since it uses a lot of DPDK
> internal private functions.
> >
> > I think that my requirements (avoid abort/exit calls when init fails) is
> a basic requirement... would you accept a patch that adds an alternative
> rte_eal_init() function that just returns an error code upon failure,
> instead of immediately exiting?
> >
> > Thanks for your hard work!
> >
> > Francesco Montorsi
> >
> I, for one, would welcome such a patch. I think the code is overly quick in
> many places to panic or exit the app, when an error code would be more
> appropriate.
> Feel free to also look at other libraries in DPDK too, if you like :-)
>
> Regards,
> /Bruce
>

+1

Re: [dpdk-dev] rte_eal_init() alternative?

[Don Provan]

Thomas Monjalon:
>Yes but please, do not create an alternative init function.
>We just need to replace panic/exit with error codes and be sure that apps and examples handle them correctly.

I understand your concerns, but the panics are really just the tip of the iceberg of the EAL library not realizing it's a library. It really makes no sense to think the library should define the application's command line, or that the PCI bus should be probed without considering whether this application is going to use PCI, and or to insist that EAL work be done on internal EAL threads.

So I'd say it's way past time to consider revamping initialization to start the process of ending the DPDK library's tail wagging the application's dog. Naturally this would have to be done while retaining the existing init routine on top of a real library initialization, but that's just an unfortunate artifact of the library's history, not a rational design decision for moving forward.

-don provan

Re: [dpdk-dev] rte_eal_init() alternative?

[Stephen Hemminger]

On Wed, 2 Sep 2015 18:17:40 +0000
Don Provan <dprovan@bivio.net> wrote:

> Thomas Monjalon:
> >Yes but please, do not create an alternative init function.
> >We just need to replace panic/exit with error codes and be sure that apps and examples handle them correctly.
> 
> I understand your concerns, but the panics are really just the tip of the iceberg of the EAL library not realizing it's a library. It really makes no sense to think the library should define the application's command line, or that the PCI bus should be probed without considering whether this application is going to use PCI, and or to insist that EAL work be done on internal EAL threads.
> 
> So I'd say it's way past time to consider revamping initialization to start the process of ending the DPDK library's tail wagging the application's dog. Naturally this would have to be done while retaining the existing init routine on top of a real library initialization, but that's just an unfortunate artifact of the library's history, not a rational design decision for moving forward.
> 
> -don provan
> 

You are welcome to submit patches with what you are proposing for review.
Theoretical requirements discussions will probably only result in more mail,
not new code. You know what you want, propose a solution.

As far as the command line. That is easily managed by realizing the application
doesn't have to pass the original command line into EAL. If you just view the
command line as a way to pass unstructured options; the application or infrastructure
can build up new values and pass it in.

I agree that initialization itself should try and not fail except in the
most extreme cases.  "ie I can't find /sys what is wrong" and should try
and adapt more "you asked for 128 cpu's but I see only 2, log it and continue"

Re: [dpdk-dev] rte_eal_init() alternative?

[Zoltan Kiss]

On 02/09/15 15:08, Jay Rolette wrote:
> On Wed, Sep 2, 2015 at 7:56 AM, Bruce Richardson <bruce.richardson@intel.com
>> wrote:
>
>> On Wed, Sep 02, 2015 at 12:49:40PM +0000, Montorsi, Francesco wrote:
>>> Hi all,
>>>
>>> Currently it seems that the only way to initialize EAL is using
>> rte_eal_init() function, correct?
>>>
>>> I have the problem that rte_eal_init() will call rte_panic() whenever
>> something fails to initialize or in other cases it will call exit().
>>> In my application, I would rather like to attempt DPDK initialization.
>> If it fails I don't want to exit.
>>> Unfortunately I cannot even copy&paste the rte_eal_init() code into my
>> application (removing rte_panic and exit calls) since it uses a lot of DPDK
>> internal private functions.
>>>
>>> I think that my requirements (avoid abort/exit calls when init fails) is
>> a basic requirement... would you accept a patch that adds an alternative
>> rte_eal_init() function that just returns an error code upon failure,
>> instead of immediately exiting?
>>>
>>> Thanks for your hard work!
>>>
>>> Francesco Montorsi
>>>
>> I, for one, would welcome such a patch. I think the code is overly quick in
>> many places to panic or exit the app, when an error code would be more
>> appropriate.
>> Feel free to also look at other libraries in DPDK too, if you like :-)
>>
>> Regards,
>> /Bruce
>>
>
> +1
>

+1

Re: [dpdk-dev] rte_eal_init() alternative?

[Marc Sune]

Stephen, Don,

On Wed, Sep 2, 2015 at 9:00 PM, Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Wed, 2 Sep 2015 18:17:40 +0000
> Don Provan <dprovan@bivio.net> wrote:
>
> > Thomas Monjalon:
> > >Yes but please, do not create an alternative init function.
> > >We just need to replace panic/exit with error codes and be sure that
> apps and examples handle them correctly.
> >
> > I understand your concerns, but the panics are really just the tip of
> the iceberg of the EAL library not realizing it's a library. It really
> makes no sense to think the library should define the application's command
> line, or that the PCI bus should be probed without considering whether this
> application is going to use PCI, and or to insist that EAL work be done on
> internal EAL threads.
> >
> > So I'd say it's way past time to consider revamping initialization to
> start the process of ending the DPDK library's tail wagging the
> application's dog. Naturally this would have to be done while retaining the
> existing init routine on top of a real library initialization, but that's
> just an unfortunate artifact of the library's history, not a rational
> design decision for moving forward.
>

That's one of the first things I was asking in the mailing list (argv):

http://dpdk.org/ml/archives/dev/2013-August/000374.html

I still think the same way.


> >
> > -don provan
> >
>
> You are welcome to submit patches with what you are proposing for review.
> Theoretical requirements discussions will probably only result in more
> mail,
> not new code. You know what you want, propose a solution.
>
> As far as the command line. That is easily managed by realizing the
> application
> doesn't have to pass the original command line into EAL. If you just view
> the
> command line as a way to pass unstructured options; the application or
> infrastructure
> can build up new values and pass it in.
>

Yes sure, and that's what all of us who are integrating DPDK in existing
applications is doing:

https://github.com/bisdn/xdpd/blob/stable/src/xdpd/drivers/gnu_linux_dpdk/src/hal-imp/driver.cc#L153-L157

But that's rather a workaround.

Instead, having an eal_init() API which only handles a fixed set of
arguments (non-argv) that can be used by existing applications, and build a
command line API on top of eal_init() that parses argv as of now (e.g.
eal_init_cl) seems to me cleaner. There are users that make use of the
parsing of argv (e.g. dpdk-pktgen) in DPDK, so I think it makes sense to
keep it.

So if you'd agree on this general proposal, I will try to allocate some
time for refactoring this.

Marc


>
> I agree that initialization itself should try and not fail except in the
> most extreme cases.  "ie I can't find /sys what is wrong" and should try
> and adapt more "you asked for 128 cpu's but I see only 2, log it and
> continue"
>
>

Re: [dpdk-dev] rte_eal_init() alternative?

[Thomas Monjalon]

2015-09-02 12:00, Stephen Hemminger:
> On Wed, 2 Sep 2015 18:17:40 +0000
> Don Provan <dprovan@bivio.net> wrote:
> 
> > Thomas Monjalon:
> > >Yes but please, do not create an alternative init function.
> > >We just need to replace panic/exit with error codes and be sure that apps and examples handle them correctly.
> > 
> > I understand your concerns, but the panics are really just the tip of the iceberg of the EAL library not realizing it's a library. It really makes no sense to think the library should define the application's command line, or that the PCI bus should be probed without considering whether this application is going to use PCI, and or to insist that EAL work be done on internal EAL threads.
> > 
> > So I'd say it's way past time to consider revamping initialization to start the process of ending the DPDK library's tail wagging the application's dog. Naturally this would have to be done while retaining the existing init routine on top of a real library initialization, but that's just an unfortunate artifact of the library's history, not a rational design decision for moving forward.
> > 
> > -don provan
> > 
> 
> You are welcome to submit patches with what you are proposing for review.
> Theoretical requirements discussions will probably only result in more mail,
> not new code. You know what you want, propose a solution.

+1
Everybody agree that DPDK should be more flexible.
We move from a bare metal framework to a real library.
They are shortcuts in original design which can be changed.

> As far as the command line. That is easily managed by realizing the application
> doesn't have to pass the original command line into EAL. If you just view the
> command line as a way to pass unstructured options; the application or infrastructure
> can build up new values and pass it in.
> 
> I agree that initialization itself should try and not fail except in the
> most extreme cases.  "ie I can't find /sys what is wrong" and should try
> and adapt more "you asked for 128 cpu's but I see only 2, log it and continue"
> 

Re: [dpdk-dev] rte_eal_init() alternative?

[Wiles, Keith]

On 9/2/15, 4:08 PM, "dev on behalf of Thomas Monjalon"
<dev-bounces@dpdk.org on behalf of thomas.monjalon@6wind.com> wrote:

>2015-09-02 12:00, Stephen Hemminger:
>> On Wed, 2 Sep 2015 18:17:40 +0000
>> Don Provan <dprovan@bivio.net> wrote:
>> 
>> > Thomas Monjalon:
>> > >Yes but please, do not create an alternative init function.
>> > >We just need to replace panic/exit with error codes and be sure that
>>apps and examples handle them correctly.
>> > 
>> > I understand your concerns, but the panics are really just the tip of
>>the iceberg of the EAL library not realizing it's a library. It really
>>makes no sense to think the library should define the application's
>>command line, or that the PCI bus should be probed without considering
>>whether this application is going to use PCI, and or to insist that EAL
>>work be done on internal EAL threads.
>> > 
>> > So I'd say it's way past time to consider revamping initialization to
>>start the process of ending the DPDK library's tail wagging the
>>application's dog. Naturally this would have to be done while retaining
>>the existing init routine on top of a real library initialization, but
>>that's just an unfortunate artifact of the library's history, not a
>>rational design decision for moving forward.
>> > 
>> > -don provan
>> > 
>> 
>> You are welcome to submit patches with what you are proposing for
>>review.
>> Theoretical requirements discussions will probably only result in more
>>mail,
>> not new code. You know what you want, propose a solution.
>
>+1
>Everybody agree that DPDK should be more flexible.
>We move from a bare metal framework to a real library.
>They are shortcuts in original design which can be changed.

+1
I was also asked at one point, at DPDK summit was, if a structure based
init routine would be better for applications. Instead of passing in
command line argvs he wanted to setup a structure in his application to be
passed into a new API. The old argvs rte_eal_init() would setup a
structure and call into the new rte_eal_struct_init() routine (I guess).

I could see the new API would return ERROR (plus errno) to the application
if called directly or return ERROR to rte_eal_init() to be handle from the
command line.

That stated I am not a big fan of huge structures being passed into a init
routine as that structure would need to be versioned and it will
grow/change. Plus he did not really want to deal in strings, so the
structure would be binary values and strings as required.

He also wanted DPDK to be more dynamic, meaning some of the values we have
in the config files should be more dynamic set via the structure and
possible have APIs to enable/disable or set the values. Each config option
would have to be looked at IMO, as some are not very easy to be dynamic
and maybe not reasonable at all.

I believe looking at the current problem of not doing a exit/panic is the
first step, next step, if everyone wants, is to look at how to initialize
DPDK from an application perspective as compared to a command line one.

>
>> As far as the command line. That is easily managed by realizing the
>>application
>> doesn't have to pass the original command line into EAL. If you just
>>view the
>> command line as a way to pass unstructured options; the application or
>>infrastructure
>> can build up new values and pass it in.
>> 
>> I agree that initialization itself should try and not fail except in the
>> most extreme cases.  "ie I can't find /sys what is wrong" and should try
>> and adapt more "you asked for 128 cpu's but I see only 2, log it and
>>continue"
>> 
>
>
>


‹ 
Regards,
++Keith
Intel Corporation

Re: [dpdk-dev] rte_eal_init() alternative?

[Don Provan]

From: Wiles, Keith:
>That stated I am not a big fan of huge structures being passed into
>a init routine as that structure would need to be versioned and it will
>grow/change. Plus he did not really want to deal in strings, so the
>structure would be binary values and strings as required.

A typical library has an init routine which establishes defaults, and
then the application adjusts parameters through targeted set routines
before starting to use the library operationally. In the argc/argv
wrapper, the parsing code would call one of those individual routines
when it parses the corresponding command line flag.

The idea that there has to be one massive init routine which is
passed every possible configuration parameter is more of the same
monolithic thinking that DPDK needs to shake.

-don provan
dprovan@bivio.net

Re: [dpdk-dev] rte_eal_init() alternative?

[Wiles, Keith]

On 9/8/15, 1:01 PM, "Don Provan" <dprovan@bivio.net> wrote:

>From: Wiles, Keith:
>>That stated I am not a big fan of huge structures being passed into
>>a init routine as that structure would need to be versioned and it will
>>grow/change. Plus he did not really want to deal in strings, so the
>>structure would be binary values and strings as required.
>
>A typical library has an init routine which establishes defaults, and
>then the application adjusts parameters through targeted set routines
>before starting to use the library operationally. In the argc/argv
>wrapper, the parsing code would call one of those individual routines
>when it parses the corresponding command line flag.
>
>The idea that there has to be one massive init routine which is
>passed every possible configuration parameter is more of the same
>monolithic thinking that DPDK needs to shake.

I am not sure DPDK has a lot of monolithic parts, so I do not fully
understand your statement, if you mean we should carefully add this
support in the correct way then I agree.

Some items may need to be passed into the first routine and other items
can be set via the setter routines. The problem is the current routine
does it all at once and it will need go be broken down to do as you say
with setter routines. Passing in a few options or a small structure can
still work, but we should not try to create a huge structure.

Today we have one API rte_eal_init() and breaking this up into 20 routines
is going to be a problem IMO, we need to strike a balance of some type
that does not use argc/argv or we add new APIs.
>
>-don provan
>dprovan@bivio.net
>


‹ 
Regards,
++Keith
Intel Corporation

Re: [dpdk-dev] rte_eal_init() alternative?

[Montorsi, Francesco]

Hi,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: mercoledì 2 settembre 2015 15:10
> To: Montorsi, Francesco <fmontorsi@empirix.com>
> Cc: dev@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] rte_eal_init() alternative?
> 
> 2015-09-02 13:56, Bruce Richardson:
> > On Wed, Sep 02, 2015 at 12:49:40PM +0000, Montorsi, Francesco wrote:
> > > Hi all,
> > >
> > > Currently it seems that the only way to initialize EAL is using rte_eal_init()
> function, correct?
> > >
> > > I have the problem that rte_eal_init() will call rte_panic() whenever
> something fails to initialize or in other cases it will call exit().
> > > In my application, I would rather like to attempt DPDK initialization. If it
> fails I don't want to exit.
> > > Unfortunately I cannot even copy&paste the rte_eal_init() code into my
> application (removing rte_panic and exit calls) since it uses a lot of DPDK
> internal private functions.
> > >
> > > I think that my requirements (avoid abort/exit calls when init fails) is a
> basic requirement... would you accept a patch that adds an alternative
> rte_eal_init() function that just returns an error code upon failure, instead of
> immediately exiting?
> > >
> > > Thanks for your hard work!
> > >
> > > Francesco Montorsi
> > >
> > I, for one, would welcome such a patch. I think the code is overly
> > quick in many places to panic or exit the app, when an error code would be
> more appropriate.
> > Feel free to also look at other libraries in DPDK too, if you like :-)
> 
> Yes but please, do not create an alternative init function.
> We just need to replace panic/exit with error codes and be sure that apps
> and examples handle them correctly.

To maintain compatibility with existing applications I think that perhaps the best would be to have a core initialization function rte_eal_init_raw() that never calls rte_panic() and returns an error code. Then we can maintain compatibility having an rte_eal_init() function that does call rte_panic() if rte_eal_init_raw() fails.

Something like the attached patch. 

Note that the attached patch exposes also a way to skip the argv/argc configuration process by directly providing a populated configuration structure...

Let me know what you think about it (the patch is just a draft and needs more work).

Thanks,
Francesco



	

Re: [dpdk-dev] rte_eal_init() alternative?

[Panu Matilainen]

On 10/08/2015 05:58 PM, Montorsi, Francesco wrote:
> Hi,
>
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> Sent: mercoledì 2 settembre 2015 15:10
>> To: Montorsi, Francesco <fmontorsi@empirix.com>
>> Cc: dev@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>
>> Subject: Re: [dpdk-dev] rte_eal_init() alternative?
>>
>> 2015-09-02 13:56, Bruce Richardson:
>>> On Wed, Sep 02, 2015 at 12:49:40PM +0000, Montorsi, Francesco wrote:
>>>> Hi all,
>>>>
>>>> Currently it seems that the only way to initialize EAL is using rte_eal_init()
>> function, correct?
>>>>
>>>> I have the problem that rte_eal_init() will call rte_panic() whenever
>> something fails to initialize or in other cases it will call exit().
>>>> In my application, I would rather like to attempt DPDK initialization. If it
>> fails I don't want to exit.
>>>> Unfortunately I cannot even copy&paste the rte_eal_init() code into my
>> application (removing rte_panic and exit calls) since it uses a lot of DPDK
>> internal private functions.
>>>>
>>>> I think that my requirements (avoid abort/exit calls when init fails) is a
>> basic requirement... would you accept a patch that adds an alternative
>> rte_eal_init() function that just returns an error code upon failure, instead of
>> immediately exiting?
>>>>
>>>> Thanks for your hard work!
>>>>
>>>> Francesco Montorsi
>>>>
>>> I, for one, would welcome such a patch. I think the code is overly
>>> quick in many places to panic or exit the app, when an error code would be
>> more appropriate.
>>> Feel free to also look at other libraries in DPDK too, if you like :-)
>>
>> Yes but please, do not create an alternative init function.
>> We just need to replace panic/exit with error codes and be sure that apps
>> and examples handle them correctly.
>
> To maintain compatibility with existing applications I think that
> perhaps the best would be to have a core initialization function
> rte_eal_init_raw() that never calls rte_panic() and returns an error
> code. Then we can maintain compatibility having an rte_eal_init()
> function that does call rte_panic() if rte_eal_init_raw() fails.

Note that callers are already required to check rte_eal_init() return 
code for errors, and any app failing to do so would be buggy to begin 
with. So just turning the panics into error returns is not an 
incompatible change.

I agree with Thomas here, lets just fix rte_eal_init() to do the right 
thing instead of adding alternatives just for the error return. 
Especially when _raw() in the name suggests that is not the thing you'd 
commonly want to use.

> Something like the attached patch.

It seems the patch missed the boat :)

> Note that the attached patch exposes also a way to skip the
> argv/argc configuration process by directly providing a populated
> configuration structure...
> Let me know what you think about it (the patch is just a draft and
> needs more work).

Can't comment on what I've not seen, but based on comments seen on this 
list, having an alternative way to initialize with structures would be 
welcomed by many. The downside is that those structures will need to be 
exposed in the API forever which means any changes there are subject to 
the ABI process.

	- Panu -


> Thanks,
> Francesco
>
>
>
> 	
>

Re: [dpdk-dev] rte_eal_init() alternative?

[Montorsi, Francesco]

Hi Panu,



> -----Original Message-----
> From: Panu Matilainen [mailto:pmatilai@redhat.com]
> Sent: venerdì 9 ottobre 2015 10:26
> To: Montorsi, Francesco <fmontorsi@empirix.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] rte_eal_init() alternative?
> 
> > Something like the attached patch.
> 
> It seems the patch missed the boat :)

Correct, sorry. I'm attaching it now. 


> 
> > Note that the attached patch exposes also a way to skip the argv/argc
> > configuration process by directly providing a populated configuration
> > structure...
> > Let me know what you think about it (the patch is just a draft and
> > needs more work).
> 
> Can't comment on what I've not seen, but based on comments seen on this
> list, having an alternative way to initialize with structures would be welcomed
> by many. The downside is that those structures will need to be exposed in
> the API forever which means any changes there are subject to the ABI
> process.
> 
Perhaps the init function taking a structure could be an exception for ABI changes... i.e., the format of the configuration is not garantueed to stay the same between different versions, and applications using a shared build of DPDK libraries must avoid using the configuration structure... would that be a possible solution?

Thanks,
Francesco

Re: [dpdk-dev] rte_eal_init() alternative?

[Montorsi, Francesco]

> > It seems the patch missed the boat :)
> 
> Correct, sorry. I'm attaching it now.
Ok, for some reason the email client is removing the attachment... I'm copying and pasting it:
(the points marked as TODO are functions that still contain rte_panic() calls...)




==== dpdk-2.1.0/lib/librte_eal/common/eal_common_log.c - dpdk-2.1.0/lib/librte_eal/common/eal_common_log.c ====
==== dpdk-2.1.0/lib/librte_eal/common/include/rte_eal.h - dpdk-2.1.0/lib/librte_eal/common/include/rte_eal.h ====
--- /tmp/tmp.6220.37	2015-10-08 16:15:22.402607404 +0200
+++ dpdk-2.1.0/lib/librte_eal/common/include/rte_eal.h	2015-10-08 15:57:21.442627152 +0200
@@ -141,6 +141,9 @@
  * returning. See also the rte_eal_get_configuration() function. Note:
  * This behavior may change in the future.
  *
+ * This function will log and eventually abort the entire application if
+ * initialization fails.
+ *
  * @param argc
  *   The argc argument that was given to the main() function.
  * @param argv
@@ -153,6 +156,27 @@
  *   - On failure, a negative error value.
  */
 int rte_eal_init(int argc, char **argv);
+
+/**
+ * Initialize the Environment Abstraction Layer (EAL).
+ *
+ * Please refer to rte_eal_init() for more information.
+ * The difference between rte_eal_init() and rte_eal_init_raw()
+ * is that the latter will never abort the entire process but rather
+ * will just log an error and return an error code.
+ *
+ * @param logid
+ *   A string that identifies the whole process, used to prefix log messages;
+ *   on Linux will be used as the 'ident' parameter of the syslog facility openlog().
+ * @param cfg
+ *   The internal configuration for RTE EAL.
+ * @return
+ *   - On success, zero.
+ *   - On failure, a negative error value.
+ */
+struct internal_config;
+int rte_eal_init_raw(const char* logid, struct internal_config *cfg);
+
 /**
  * Usage function typedef used by the application usage function.
  *
==== dpdk-2.1.0/lib/librte_eal/linuxapp/eal/eal.c - dpdk-2.1.0/lib/librte_eal/linuxapp/eal/eal.c ====
--- /tmp/tmp.6220.75	2015-10-08 16:15:22.406607404 +0200
+++ dpdk-2.1.0/lib/librte_eal/linuxapp/eal/eal.c	2015-10-08 16:15:10.106607628 +0200
@@ -178,7 +178,7 @@
  * on other parts, e.g. memzones, to detect if there are running secondary
  * processes. */
 static void
-rte_eal_config_create(void)
+rte_eal_config_create(void)			// TODO
 {
 	void *rte_mem_cfg_addr;
 	int retval;
@@ -232,7 +232,7 @@
 
 /* attach to an existing shared memory config */
 static void
-rte_eal_config_attach(void)
+rte_eal_config_attach(void)			// TODO
 {
 	struct rte_mem_config *mem_config;
 
@@ -258,7 +258,7 @@
 
 /* reattach the shared config at exact memory location primary process has it */
 static void
-rte_eal_config_reattach(void)
+rte_eal_config_reattach(void)		// TODO
 {
 	struct rte_mem_config *mem_config;
 	void *rte_mem_cfg_addr;
@@ -305,7 +305,7 @@
 
 /* Sets up rte_config structure with the pointer to shared memory config.*/
 static void
-rte_config_init(void)
+rte_config_init(void)		// TODO
 {
 	rte_config.process_type = internal_config.process_type;
 
@@ -724,25 +724,17 @@
 #endif
 }
 
-/* Launch threads, called at application init(). */
+
+/* Launch threads, called at application init(). Logs and aborts on critical errors. */
 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);
-	struct shared_driver *solib = NULL;
+	int fctret;
 	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();
-
 	if (rte_eal_log_early_init() < 0)
 		rte_panic("Cannot init early logs\n");
 
@@ -751,18 +743,54 @@
 	/* set log level as early as possible */
 	rte_set_log_level(internal_config.log_level);
 
-	if (rte_eal_cpu_init() < 0)
-		rte_panic("Cannot detect lcores\n");
-
 	fctret = eal_parse_args(argc, argv);
 	if (fctret < 0)
 		exit(1);
 
+	if (rte_eal_init_raw(logid, NULL) < 0)
+		rte_panic("Errors encountered during initialization. Cannot proceed.\n");
+
+	return fctret;
+}
+
+/* Library-style init(), will attempt initialization, log on errors and return;
+ * This function does not rte_panic() or exit() the whole process. */
+int
+rte_eal_init_raw(const char* logid, struct internal_config *cfg)
+{
+	int i, ret;
+	pthread_t thread_id;
+	static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
+	struct shared_driver *solib = NULL;
+	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
+
+	if (!rte_atomic32_test_and_set(&run_once))
+		return -1;
+
+	thread_id = pthread_self();
+	if (rte_eal_log_early_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init early logs\n");
+		return -1;
+	}
+
+	if (cfg)
+		memcpy(&internal_config, cfg, sizeof(*cfg));
+
+	/* set log level as early as possible */
+	rte_set_log_level(internal_config.log_level);
+
+	if (rte_eal_cpu_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot detect lcores\n");
+		return -1;
+	}
+
 	if (internal_config.no_hugetlbfs == 0 &&
 			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");
+			eal_hugepage_info_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot get hugepage information\n");
+		return -1;
+	}
 
 	if (internal_config.memory == 0 && internal_config.force_sockets == 0) {
 		if (internal_config.no_hugetlbfs)
@@ -786,42 +814,62 @@
 
 	rte_config_init();
 
-	if (rte_eal_pci_init() < 0)
-		rte_panic("Cannot init PCI\n");
+	if (rte_eal_pci_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init PCI\n");
+		return -1;
+	}
 
 #ifdef RTE_LIBRTE_IVSHMEM
-	if (rte_eal_ivshmem_init() < 0)
-		rte_panic("Cannot init IVSHMEM\n");
+	if (rte_eal_ivshmem_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init IVSHMEM\n");
+		return -1;
+	}
 #endif
 
-	if (rte_eal_memory_init() < 0)
-		rte_panic("Cannot init memory\n");
+	if (rte_eal_memory_init() < 0) {
+		RTE_LOG (ERR, EAL, "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");
+	if (rte_eal_memzone_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init memzone\n");
+		return -1;
+	}
 
-	if (rte_eal_tailqs_init() < 0)
-		rte_panic("Cannot init tail queues for objects\n");
+	if (rte_eal_tailqs_init() < 0) {
+		RTE_LOG (ERR, EAL, "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");
+	if (rte_eal_ivshmem_obj_init() < 0) {
+		RTE_LOG (ERR, EAL, "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");
+	if (rte_eal_log_init(logid, internal_config.syslog_facility) < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init logs\n");
+		return -1;
+	}
 
-	if (rte_eal_alarm_init() < 0)
-		rte_panic("Cannot init interrupt-handling thread\n");
+	if (rte_eal_alarm_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init alarm\n");
+		return -1;
+	}
 
-	if (rte_eal_intr_init() < 0)
-		rte_panic("Cannot init interrupt-handling thread\n");
+	if (rte_eal_intr_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init interrupt-handling thread\n");
+		return -1;
+	}
 
-	if (rte_eal_timer_init() < 0)
-		rte_panic("Cannot init HPET or TSC timers\n");
+	if (rte_eal_timer_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init HPET or TSC timers\n");
+		return -1;
+	}
 
 	eal_check_mem_on_local_socket();
 
@@ -842,8 +890,10 @@
 		rte_config.master_lcore, (int)thread_id, cpuset,
 		ret == 0 ? "" : "...");
 
-	if (rte_eal_dev_init() < 0)
-		rte_panic("Cannot init pmd devices\n");
+	if (rte_eal_dev_init() < 0) {
+		RTE_LOG (ERR, EAL, "Cannot init pmd devices\n");
+		return -1;
+	}
 
 	RTE_LCORE_FOREACH_SLAVE(i) {
 
@@ -851,18 +901,24 @@
 		 * 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");
+		if (pipe(lcore_config[i].pipe_master2slave) < 0) {
+			RTE_LOG (ERR, EAL, "Cannot create pipe\n");
+			return -1;
+		}
+		if (pipe(lcore_config[i].pipe_slave2master) < 0) {
+			RTE_LOG (ERR, EAL, "Cannot create pipe\n");
+			return -1;
+		}
 
 		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");
+		if (ret != 0) {
+			RTE_LOG (ERR, EAL, "Cannot create thread\n");
+			return -1;
+		}
 	}
 
 	/*
@@ -873,12 +929,15 @@
 	rte_eal_mp_wait_lcore();
 
 	/* Probe & Initialize PCI devices */
-	if (rte_eal_pci_probe())
-		rte_panic("Cannot probe PCI\n");
+	if (rte_eal_pci_probe()) {
+		RTE_LOG (ERR, EAL, "Cannot probe PCI\n");
+		return -1;
+	}
 
-	return fctret;
+	return 0;
 }
 
+
 /* get core role */
 enum rte_lcore_role_t
 rte_eal_lcore_role(unsigned lcore_id)

Re: [dpdk-dev] rte_eal_init() alternative?

[Panu Matilainen]

On 10/09/2015 01:03 PM, Montorsi, Francesco wrote:
> Hi Panu,
>
>
>
>> -----Original Message-----
>> From: Panu Matilainen [mailto:pmatilai@redhat.com]
>> Sent: venerdì 9 ottobre 2015 10:26
>> To: Montorsi, Francesco <fmontorsi@empirix.com>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] rte_eal_init() alternative?
>>
>>> Something like the attached patch.
>>
>> It seems the patch missed the boat :)
>
> Correct, sorry. I'm attaching it now.
>
>
>>
>>> Note that the attached patch exposes also a way to skip the argv/argc
>>> configuration process by directly providing a populated configuration
>>> structure...
>>> Let me know what you think about it (the patch is just a draft and
>>> needs more work).
>>
>> Can't comment on what I've not seen, but based on comments seen on this
>> list, having an alternative way to initialize with structures would be welcomed
>> by many. The downside is that those structures will need to be exposed in
>> the API forever which means any changes there are subject to the ABI
>> process.
>>
> Perhaps the init function taking a structure could be an exception
> for ABI changes... i.e., the format of the configuration is not
> garantueed to stay the same between different versions, and
> applications using a shared build of DPDK libraries must avoid using
> the configuration structure... would that be a possible solution?

Sorry but no, down the path of exceptions lies madness. It'd also be 
giving the middle finger to people using DPDK as a shared library.

Exported structs are always a PITA and even more so in something like 
configuration which is expected to keep expanding and/or otherwise 
changing.

I'd much rather see an rte_eal_init() which takes struct *rte_cfgfile as 
the configuration argument. That, plus maybe enhance librte_cfgfile to 
allow constructing one entirely in memory + setting values in addition 
to getting.

	- Panu -




> Thanks,
> Francesco
>
>
>

Re: [dpdk-dev] rte_eal_init() alternative?

[Panu Matilainen]

On 10/09/2015 01:13 PM, Montorsi, Francesco wrote:
>>> It seems the patch missed the boat :)
>>
>> Correct, sorry. I'm attaching it now.
> Ok, for some reason the email client is removing the attachment... I'm copying and pasting it:
> (the points marked as TODO are functions that still contain rte_panic() calls...)

I actually did receive the attachment from the previous mail, but 
inlined patches are far better for commenting purposes.

> + */
> +struct internal_config;
> +int rte_eal_init_raw(const char* logid, struct internal_config *cfg);

Like the name indicates, struct internal_config is internal to 
librte_eal, you'd need to "export" the eal_internal_cfg.h header for 
this to be useful to users outside librte_eal itself. But I'd say 
there's a reason why its internal...

> -	if (rte_eal_pci_init() < 0)
> -		rte_panic("Cannot init PCI\n");
> +	if (rte_eal_pci_init() < 0) {
> +		RTE_LOG (ERR, EAL, "Cannot init PCI\n");
> +		return -1;
> +	}
>
>   #ifdef RTE_LIBRTE_IVSHMEM
> -	if (rte_eal_ivshmem_init() < 0)
> -		rte_panic("Cannot init IVSHMEM\n");
> +	if (rte_eal_ivshmem_init() < 0) {
> +		RTE_LOG (ERR, EAL, "Cannot init IVSHMEM\n");
> +		return -1;
> +	}
>   #endif
>
> -	if (rte_eal_memory_init() < 0)
> -		rte_panic("Cannot init memory\n");
> +	if (rte_eal_memory_init() < 0) {
> +		RTE_LOG (ERR, EAL, "Cannot init memory\n");
> +		return -1;
> +	}
[...]

Something like that, sure. The big question with this conversion is what 
to do with already allocated/initialized resources in case of failure, 
which I'd guess is the reason rte_panic() is there - to avoid having to 
deal with all that.

Getting to a point where all or even most inialization can be undone in 
case of failure is likely going to be a long road, I think many 
subsystems dont even have a shutdown function. To beging with, EAL 
itself doesn't have one :)

Anyway, one has to start someplace. But in order to make the cleanup 
eventually possible, I'd suggest using a common point of exit instead of 
a dozen returns, ie something in spirit of

{
	[...]

	if (rte_eal_pci_init() < 0) {
		RTE_LOG (ERR, EAL, "Cannot init PCI\n");
		goto err;
	}

	if (rte_eal_memory_init() < 0)
		RTE_LOG (ERR, EAL, "Cannot init memory\n");
		goto err;
	}

	[...]

	return 0;

err:
	/* TODO: undo all initialization work */
         return -1;
}

	- Panu -

Re: [dpdk-dev] rte_eal_init() alternative?

[Thomas F Herbert]

On 10/9/15 11:40 AM, Panu Matilainen wrote:
> On 10/09/2015 01:03 PM, Montorsi, Francesco wrote:
>> Hi Panu,
>>
>>
>>
>>> -----Original Message-----
>>> From: Panu Matilainen [mailto:pmatilai@redhat.com]
>>> Sent: venerdì 9 ottobre 2015 10:26
>>> To: Montorsi, Francesco <fmontorsi@empirix.com>; Thomas Monjalon
>>> <thomas.monjalon@6wind.com>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [dpdk-dev] rte_eal_init() alternative?
>>>
>>>> Something like the attached patch.
>>>
>>> It seems the patch missed the boat :)
>>
>> Correct, sorry. I'm attaching it now.
>>
>>
>>>
>>>> Note that the attached patch exposes also a way to skip the argv/argc
>>>> configuration process by directly providing a populated configuration
>>>> structure...
>>>> Let me know what you think about it (the patch is just a draft and
>>>> needs more work).
>>>
>>> Can't comment on what I've not seen, but based on comments seen on this
>>> list, having an alternative way to initialize with structures would 
>>> be welcomed
>>> by many. The downside is that those structures will need to be 
>>> exposed in
>>> the API forever which means any changes there are subject to the ABI
>>> process.
>>>
>> Perhaps the init function taking a structure could be an exception
>> for ABI changes... i.e., the format of the configuration is not
>> garantueed to stay the same between different versions, and
>> applications using a shared build of DPDK libraries must avoid using
>> the configuration structure... would that be a possible solution?
>
> Sorry but no, down the path of exceptions lies madness. It'd also be 
> giving the middle finger to people using DPDK as a shared library.
>
> Exported structs are always a PITA and even more so in something like 
> configuration which is expected to keep expanding and/or otherwise 
> changing.
>
> I'd much rather see an rte_eal_init() which takes struct *rte_cfgfile 
> as the configuration argument. That, plus maybe enhance librte_cfgfile 
> to allow constructing one entirely in memory + setting values in 
> addition to getting.
>
>     - Panu -
It is very difficult for application writers to write their own command 
parsers with implementation for -h option. How about a function that 
would verify the init parameters and return with a benign error if the 
options are not correct.
>
>
>
>
>> Thanks,
>> Francesco
>>
>>
>>
>

-- 
Thomas F Herbert Red Hat