Non-argv dependant rte_eal_init() call

Non-argv dependant rte_eal_init() call

[Marc Sune]

Dear all,

Sorry in advance if there is another API for this and I haven't found 
it, or if there is a strong reason for having it this way. I've seen 
that in the case of both baremetal and Linux applications, the way to 
initialize EAL is passing argv:

<code>
//...
     /* init EAL */
     ret = rte_eal_init(argc, argv);
     if (ret < 0)
         rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n");
     argc -= ret;
     argv += ret;
//...
</code>

However, this is a little bit annoying in the case of GNU/Linux 
user-space applications, hence using DPDK as a library, when porting 
them to DPDK (specially in case of multi-platform applications, like in 
our case), since they are not necessarily designed to be changing the 
main routines in a per platform basis. In our case they are even in 
separate autotools package, since the library providing DPDK based 
services needs to be distributed also in binary version, linking to 
non-DPDK aware code.

In our case, we are right now simply faking the argv, which is a little 
bit ugly:
<code>
//...
  37         const char* argv[EAL_ARGS] = {"./fake", "-c",CORE_MASK, 
"-n",NUM_CACHE_LINES, ""};
//...
  53         ret = rte_eal_init(EAL_ARGS, (char**)argv);
  54         if (ret < 0)
  55                 rte_exit(EXIT_FAILURE, "rte_eal_init failed");
//...
</code>

IMHO it would make more sense to have actually two calls, adding a 
library-like initialization. Something like:

<code>
/*
* In the comments a warning that this should be called at the very 
beginning of the program.
*...
*/
int rte_eal_init(eal_coremask_t core_mask, unsigned int num_of_lines 
/*More parameters here...*/);

/*
*
*/
int rte_eal_init_argv(int argc, char **argv);

</code>

Btw, the same applies to the mangling of the main() (MAIN) routine. Is 
this really necessary? Isn't it enough to clearly state in the 
documentation that certain API calls need to be made on the very 
beginning of the application?

Best
Marc

---
BISDN GmbH
Marc Suñé Clos
Christburger Straße 45, 10405 Berlin, Germany

Berlin Institute for Software Defined Networks - BISDN GmbH
Managing Directors: Dr.-Ing. Hagen Woesner, Andreas Köpsel

Commercial register: Amtsgericht Berlin-Charlottenburg HRB 141569 B
VAT ID No: DE283257294
Corporate seat: Berlin

Re: Non-argv dependant rte_eal_init() call

[Thomas Monjalon]

Hello,

01/08/2013 17:37, Marc Sune :
> In our case, we are right now simply faking the argv, which is a little
> bit ugly:
> <code>
> //...
>   37         const char* argv[EAL_ARGS] = {"./fake", "-c",CORE_MASK,
> "-n",NUM_CACHE_LINES, ""};
> //...
>   53         ret = rte_eal_init(EAL_ARGS, (char**)argv);
>   54         if (ret < 0)
>   55                 rte_exit(EXIT_FAILURE, "rte_eal_init failed");
> //...
> </code>

You should provide a better binary name because in your example, your logs 
will be prefixed with "fake" which is, I agree with you, a little bit ugly ;)

> IMHO it would make more sense to have actually two calls, adding a
> library-like initialization. Something like:
> 
> <code>
> /*
> * In the comments a warning that this should be called at the very
> beginning of the program.
> *...
> */
> int rte_eal_init(eal_coremask_t core_mask, unsigned int num_of_lines
> /*More parameters here...*/);
> 
> /*
> *
> */
> int rte_eal_init_argv(int argc, char **argv);
> 
> </code>

The problem with your proposal is that the number of options is static.
So when adding a new option in future releases, all the applications should be 
updated to give a (probably null) value for this new option.
Not sure it is an improvement.

> Btw, the same applies to the mangling of the main() (MAIN) routine. Is
> this really necessary? Isn't it enough to clearly state in the
> documentation that certain API calls need to be made on the very
> beginning of the application?

Not sure to understand this point.
MAIN is only defined in examples for the bare-metal use case.
What is the link with the API ?

-- 
Thomas

Re: Non-argv dependant rte_eal_init() call

[Marc Sune]

Dear Thomas,

Regarding the MAIN, then I understand is not really necessary for Linux 
user-space applications, and that is there in the examples because they 
can run both baremetal and userspace... this is fine.

Regarding the rte_eal_init(), if the concern is the number of parameters 
and backwards compatibility, a typical solution is to create a struct 
containing the parameters:

<code>
typedef struct eal_init_params{
     uint64_t coremask;
     unsigned int num_of_cache_lines;
     /* Add here more parmeters in future versions... */
}eal_init_params_t;

int rte_eal_init(eal_init_params_t* params);
</code>

Therefore the user code, is always backwards compatible (provided that 
is properly recompiled).

In any case, and besides the struct approach, I think is more elegant to 
add a parameter to a function call if you jump to a newer version of the 
DPDK, than having to create a fake C string array or forcing the 
applications to add extra DPDK parameters in the executable.

Just my 2 cents ;)

Best
marc

On 01/08/13 18:01, Thomas Monjalon wrote:
> Hello,
>
> 01/08/2013 17:37, Marc Sune :
>> In our case, we are right now simply faking the argv, which is a little
>> bit ugly:
>> <code>
>> //...
>>    37         const char* argv[EAL_ARGS] = {"./fake", "-c",CORE_MASK,
>> "-n",NUM_CACHE_LINES, ""};
>> //...
>>    53         ret = rte_eal_init(EAL_ARGS, (char**)argv);
>>    54         if (ret < 0)
>>    55                 rte_exit(EXIT_FAILURE, "rte_eal_init failed");
>> //...
>> </code>
> You should provide a better binary name because in your example, your logs
> will be prefixed with "fake" which is, I agree with you, a little bit ugly ;)
>
>> IMHO it would make more sense to have actually two calls, adding a
>> library-like initialization. Something like:
>>
>> <code>
>> /*
>> * In the comments a warning that this should be called at the very
>> beginning of the program.
>> *...
>> */
>> int rte_eal_init(eal_coremask_t core_mask, unsigned int num_of_lines
>> /*More parameters here...*/);
>>
>> /*
>> *
>> */
>> int rte_eal_init_argv(int argc, char **argv);
>>
>> </code>
> The problem with your proposal is that the number of options is static.
> So when adding a new option in future releases, all the applications should be
> updated to give a (probably null) value for this new option.
> Not sure it is an improvement.
>
>> Btw, the same applies to the mangling of the main() (MAIN) routine. Is
>> this really necessary? Isn't it enough to clearly state in the
>> documentation that certain API calls need to be made on the very
>> beginning of the application?
> Not sure to understand this point.
> MAIN is only defined in examples for the bare-metal use case.
> What is the link with the API ?
>

Re: Non-argv dependant rte_eal_init() call

[Antti Kantee]

On 1.8.2013 19:13, Marc Sune wrote:
> Regarding the rte_eal_init(), if the concern is the number of parameters
> and backwards compatibility, a typical solution is to create a struct
> containing the parameters:
>
> <code>
> typedef struct eal_init_params{
>      uint64_t coremask;
>      unsigned int num_of_cache_lines;
>      /* Add here more parmeters in future versions... */
> }eal_init_params_t;
>
> int rte_eal_init(eal_init_params_t* params);
> </code>
>
> Therefore the user code, is always backwards compatible (provided that
> is properly recompiled).

I don't think that's a good interface because:
1) like you say, you need to recompile everything always to make sure 
the passed struct is of the right size
2) it's less obvious how to pass optional parameters, or more 
accurately, how to not pass them.  You could add some 
eal_init_defaults() interface, but see "3".
3) with every DPDK upgrade you need to evaluate new members of the 
struct to determine their default values.  Mandatory parameters need to 
be addressed either way, but at least the current scheme gives an 
explicit error if you omit one instead of defaulting to some perhaps 
unwanted behavior.

I think the current way of passing of a string tuple vector is fine, 
though I agree it's a little counter-intuitive when you need to invent 
argv[0] in case you're not just passing in argv[] opaquely.  I pass 
"if_dpdk" from my TCP driver, and I haven't lost too much sleep over it.

My only annoyance is that eal_init() takes a non-const.

   - antti

Re: Non-argv dependant rte_eal_init() call

[Marc Sune]

[-- Attachment #1: Type: text/plain, Size: 3506 bytes --]

Well, I would not want to get into religious discussions here :), but 
concerning 1) and 3) you have to compile anyway your final applications, 
since as far as I've seen current DPDK makefiles are only compiling 
static versions of the lib.

Moreover, I don't think it is feasible to assume that the future 
versions of DPDK are going to maintain the exact same headers (APIs and 
data structures), basically due to the new HW supported and additional 
features that DPDK is going to (hopefully) support. So at the very end 
this requires a recompilation anyway. But very likely you know more 
about this detail than I do, so I could be wrong here..

Along this, I personally think that in end-user applications these 
parameters (CPU_CORES, channels...) will be either compile time 
constants or config file parameters, rather than arguments to the 
program. At least in our case, we plan to optimize it for several 
platforms and so on, but as a final application there is no need (and it 
can be harmful) to expose this to the enduser. Besides, most of the 
programs have already their own parameters that are meaningful for the 
application.

Unless you are profiling, or simply checking SDK examples (in here yes, 
it is great, this is why I think it must be kept), I think that having 
such DPDK HW specific argvs is not that useful.

Concerning 2), this has a trivial solution, which is define a static 
initializer and a (likely inlined) struct initalizer; e.g. pthreads.h does:

|int pthread_mutex_init(pthread_mutex_t */mutex/,
     const pthread_mutexattr_t */attr/);
pthread_mutex_t/mutex/  = PTHREAD_MUTEX_INITIALIZER;|


In any case, for the moment I will continue faking argv's. If I get more 
upset about this piece of code, I will try to implement this call and 
send the patch here for discussion. At the very end, it was only a 
suggestion ;)

Best
marc

On 01/08/13 18:47, Antti Kantee wrote:
> On 1.8.2013 19:13, Marc Sune wrote:
>> Regarding the rte_eal_init(), if the concern is the number of parameters
>> and backwards compatibility, a typical solution is to create a struct
>> containing the parameters:
>>
>> <code>
>> typedef struct eal_init_params{
>>      uint64_t coremask;
>>      unsigned int num_of_cache_lines;
>>      /* Add here more parmeters in future versions... */
>> }eal_init_params_t;
>>
>> int rte_eal_init(eal_init_params_t* params);
>> </code>
>>
>> Therefore the user code, is always backwards compatible (provided that
>> is properly recompiled).
>
> I don't think that's a good interface because:
> 1) like you say, you need to recompile everything always to make sure 
> the passed struct is of the right size
> 2) it's less obvious how to pass optional parameters, or more 
> accurately, how to not pass them.  You could add some 
> eal_init_defaults() interface, but see "3".
> 3) with every DPDK upgrade you need to evaluate new members of the 
> struct to determine their default values.  Mandatory parameters need 
> to be addressed either way, but at least the current scheme gives an 
> explicit error if you omit one instead of defaulting to some perhaps 
> unwanted behavior.
>
> I think the current way of passing of a string tuple vector is fine, 
> though I agree it's a little counter-intuitive when you need to invent 
> argv[0] in case you're not just passing in argv[] opaquely.  I pass 
> "if_dpdk" from my TCP driver, and I haven't lost too much sleep over it.
>
> My only annoyance is that eal_init() takes a non-const.
>
>   - antti


[-- Attachment #2: Type: text/html, Size: 4774 bytes --]

Re: Non-argv dependant rte_eal_init() call

[Stephen Hemminger]

On Thu, 01 Aug 2013 17:37:35 +0200
Marc Sune <marc.sune-kpkqNMk1I7M@public.gmane.org> wrote:

> Dear all,
> 
> Sorry in advance if there is another API for this and I haven't found 
> it, or if there is a strong reason for having it this way. I've seen 
> that in the case of both baremetal and Linux applications, the way to 
> initialize EAL is passing argv:
> 
> <code>
> //...
>      /* init EAL */
>      ret = rte_eal_init(argc, argv);
>      if (ret < 0)
>          rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n");
>      argc -= ret;
>      argv += ret;
> //...
> </code>
> 
> However, this is a little bit annoying in the case of GNU/Linux 
> user-space applications, hence using DPDK as a library, when porting 
> them to DPDK (specially in case of multi-platform applications, like in 
> our case), since they are not necessarily designed to be changing the 
> main routines in a per platform basis. In our case they are even in 
> separate autotools package, since the library providing DPDK based 
> services needs to be distributed also in binary version, linking to 
> non-DPDK aware code.
> 
> In our case, we are right now simply faking the argv, which is a little 
> bit ugly:
> <code>
> //...
>   37         const char* argv[EAL_ARGS] = {"./fake", "-c",CORE_MASK, 
> "-n",NUM_CACHE_LINES, ""};
> //...
>   53         ret = rte_eal_init(EAL_ARGS, (char**)argv);
>   54         if (ret < 0)
>   55                 rte_exit(EXIT_FAILURE, "rte_eal_init failed");
> //...
> </code>
> 
> IMHO it would make more sense to have actually two calls, adding a 
> library-like initialization. Something like:
> 
> <code>
> /*
> * In the comments a warning that this should be called at the very 
> beginning of the program.
> *...
> */
> int rte_eal_init(eal_coremask_t core_mask, unsigned int num_of_lines 
> /*More parameters here...*/);
> 
> /*
> *
> */
> int rte_eal_init_argv(int argc, char **argv);
> 
> </code>
> 
> Btw, the same applies to the mangling of the main() (MAIN) routine. Is 
> this really necessary? Isn't it enough to clearly state in the 
> documentation that certain API calls need to be made on the very 
> beginning of the application?


We found it more convenient to handle application arguments first before
calling rte_eal_init().  Mostly because application needs to start as daemon,
and eal_init spawns threads.

main(argc, argv) {
	progname = strrchr (argv[0], '/');
	progname = strdup(progname ? progname + 1 : argv[0]);

	ret = parse_args(argc, argv);
	if (ret < 0)
		return -1;
	argc -= ret;
	argv += ret;
...
	if (daemon_mode) {
		if (daemon(1,1) < 0)
			rte_panic("daemon failed\n");
	}

	/* workaround fact that EAL expects progname as first argument */
	argv[0] = progname;
	ret = rte_eal_init(argc, argv);
	if (ret < 0)
		return -1;

Re: Non-argv dependant rte_eal_init() call

[Marc Sune]

Thanks Stephen for the hack.

Unfortunately, our main already has parameters, and are all 
platform(architecture) agnostic, so this would break the assumption that 
arguments should be platform agnostic.

But anyway thanks ;)
marc

On 01/08/13 19:06, Stephen Hemminger wrote:
> On Thu, 01 Aug 2013 17:37:35 +0200
> Marc Sune <marc.sune-kpkqNMk1I7M@public.gmane.org> wrote:
>
>> Dear all,
>>
>> Sorry in advance if there is another API for this and I haven't found
>> it, or if there is a strong reason for having it this way. I've seen
>> that in the case of both baremetal and Linux applications, the way to
>> initialize EAL is passing argv:
>>
>> <code>
>> //...
>>       /* init EAL */
>>       ret = rte_eal_init(argc, argv);
>>       if (ret < 0)
>>           rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n");
>>       argc -= ret;
>>       argv += ret;
>> //...
>> </code>
>>
>> However, this is a little bit annoying in the case of GNU/Linux
>> user-space applications, hence using DPDK as a library, when porting
>> them to DPDK (specially in case of multi-platform applications, like in
>> our case), since they are not necessarily designed to be changing the
>> main routines in a per platform basis. In our case they are even in
>> separate autotools package, since the library providing DPDK based
>> services needs to be distributed also in binary version, linking to
>> non-DPDK aware code.
>>
>> In our case, we are right now simply faking the argv, which is a little
>> bit ugly:
>> <code>
>> //...
>>    37         const char* argv[EAL_ARGS] = {"./fake", "-c",CORE_MASK,
>> "-n",NUM_CACHE_LINES, ""};
>> //...
>>    53         ret = rte_eal_init(EAL_ARGS, (char**)argv);
>>    54         if (ret < 0)
>>    55                 rte_exit(EXIT_FAILURE, "rte_eal_init failed");
>> //...
>> </code>
>>
>> IMHO it would make more sense to have actually two calls, adding a
>> library-like initialization. Something like:
>>
>> <code>
>> /*
>> * In the comments a warning that this should be called at the very
>> beginning of the program.
>> *...
>> */
>> int rte_eal_init(eal_coremask_t core_mask, unsigned int num_of_lines
>> /*More parameters here...*/);
>>
>> /*
>> *
>> */
>> int rte_eal_init_argv(int argc, char **argv);
>>
>> </code>
>>
>> Btw, the same applies to the mangling of the main() (MAIN) routine. Is
>> this really necessary? Isn't it enough to clearly state in the
>> documentation that certain API calls need to be made on the very
>> beginning of the application?
>
> We found it more convenient to handle application arguments first before
> calling rte_eal_init().  Mostly because application needs to start as daemon,
> and eal_init spawns threads.
>
> main(argc, argv) {
> 	progname = strrchr (argv[0], '/');
> 	progname = strdup(progname ? progname + 1 : argv[0]);
>
> 	ret = parse_args(argc, argv);
> 	if (ret < 0)
> 		return -1;
> 	argc -= ret;
> 	argv += ret;
> ...
> 	if (daemon_mode) {
> 		if (daemon(1,1) < 0)
> 			rte_panic("daemon failed\n");
> 	}
>
> 	/* workaround fact that EAL expects progname as first argument */
> 	argv[0] = progname;
> 	ret = rte_eal_init(argc, argv);
> 	if (ret < 0)
> 		return -1;
>
>

Re: Non-argv dependant rte_eal_init() call

[Stephen Hemminger]

On Thu, 01 Aug 2013 20:17:43 +0200
Marc Sune <marc.sune-kpkqNMk1I7M@public.gmane.org> wrote:

> Thanks Stephen for the hack.
> 
> Unfortunately, our main already has parameters, and are all 
> platform(architecture) agnostic, so this would break the assumption that 
> arguments should be platform agnostic.
> 
> But anyway thanks ;)
> marc
> 

Also, our application is started by an init script and there is some shell
magic to attempt to deduce number of cpus (not hard) and number of memory
channels (kind of messy).  I do think the DPDK should work without any arguments
and do this itself. The CPU part is easy by looking at /sys, but the memory
channel information has to come from DMI which is a real ugly mess.

Re: Non-argv dependant rte_eal_init() call

[Balazs Nemeth]

[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]

Letting DPDK deduce de cpus and memchannels is just a move of feautures in
your application. In some cases you might want to limit the number of
cores. It's just simple a way to allow more control when initializing DPDK.

Kind Regards

Balazs Nemeth
Hasselt University, Belgium
2nd Master in Computer Science
On Aug 1, 2013 9:23 PM, "Stephen Hemminger" <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
wrote:

> On Thu, 01 Aug 2013 20:17:43 +0200
> Marc Sune <marc.sune-kpkqNMk1I7M@public.gmane.org> wrote:
>
> > Thanks Stephen for the hack.
> >
> > Unfortunately, our main already has parameters, and are all
> > platform(architecture) agnostic, so this would break the assumption that
> > arguments should be platform agnostic.
> >
> > But anyway thanks ;)
> > marc
> >
>
> Also, our application is started by an init script and there is some shell
> magic to attempt to deduce number of cpus (not hard) and number of memory
> channels (kind of messy).  I do think the DPDK should work without any
> arguments
> and do this itself. The CPU part is easy by looking at /sys, but the memory
> channel information has to come from DMI which is a real ugly mess.
>
>
>

[-- Attachment #2: Type: text/html, Size: 1633 bytes --]