[dpdk-dev,1/2] eal: move plugin loading to eal/common
Commit Message
There's no good reason to limit plugins to Linux, make it available
on FreeBSD too. Refactor the plugin code from Linux EAL to common
helper functions, also check for potential errors during initialization.
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
lib/librte_eal/bsdapp/eal/eal.c | 3 ++
lib/librte_eal/common/eal_common_options.c | 53 ++++++++++++++++++++++++++++++
lib/librte_eal/common/eal_options.h | 1 +
lib/librte_eal/linuxapp/eal/eal.c | 39 ++--------------------
4 files changed, 59 insertions(+), 37 deletions(-)
Comments
On Wed, Oct 21, 2015 at 10:29 AM, Panu Matilainen <pmatilai@redhat.com>
wrote:
> There's no good reason to limit plugins to Linux, make it available
> on FreeBSD too. Refactor the plugin code from Linux EAL to common
> helper functions, also check for potential errors during initialization.
>
Not sure about this "potential errors".
>
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> ---
> lib/librte_eal/bsdapp/eal/eal.c | 3 ++
> lib/librte_eal/common/eal_common_options.c | 53
> ++++++++++++++++++++++++++++++
> lib/librte_eal/common/eal_options.h | 1 +
> lib/librte_eal/linuxapp/eal/eal.c | 39 ++--------------------
> 4 files changed, 59 insertions(+), 37 deletions(-)
>
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
> b/lib/librte_eal/bsdapp/eal/eal.c
> index 1b6f705..f07a3c3 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
>
> rte_eal_mcfg_complete();
>
> + if (eal_plugins_init() < 0)
> + rte_panic("Cannot init plugins\n");
> +
>
Why testing for error when this cannot happen (see below) ?
> eal_thread_init_master(rte_config.master_lcore);
>
> ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index 1f459ac..b542868 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
>
[snip]
+
> +int
> +eal_plugins_init(void)
> +{
> + struct shared_driver *solib = NULL;
> +
> + TAILQ_FOREACH(solib, &solib_list, next) {
> + RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
> + solib->lib_handle = dlopen(solib->name, RTLD_NOW);
> + if (solib->lib_handle == NULL)
> + RTE_LOG(WARNING, EAL, "%s\n", dlerror());
> + }
> + return 0;
> +}
I can't see a non 0 return here.
Btw, returning an error here would change current behavior of dpdk loading
drivers.
Not sure we want this as people may rely on this loading not failing.
On 10/21/2015 01:15 PM, David Marchand wrote:
> On Wed, Oct 21, 2015 at 10:29 AM, Panu Matilainen <pmatilai@redhat.com>
> wrote:
>
>> There's no good reason to limit plugins to Linux, make it available
>> on FreeBSD too. Refactor the plugin code from Linux EAL to common
>> helper functions, also check for potential errors during initialization.
>>
>
> Not sure about this "potential errors".
>
>
>>
>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
>> ---
>> lib/librte_eal/bsdapp/eal/eal.c | 3 ++
>> lib/librte_eal/common/eal_common_options.c | 53
>> ++++++++++++++++++++++++++++++
>> lib/librte_eal/common/eal_options.h | 1 +
>> lib/librte_eal/linuxapp/eal/eal.c | 39 ++--------------------
>> 4 files changed, 59 insertions(+), 37 deletions(-)
>>
>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
>> b/lib/librte_eal/bsdapp/eal/eal.c
>> index 1b6f705..f07a3c3 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal.c
>> @@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
>>
>> rte_eal_mcfg_complete();
>>
>> + if (eal_plugins_init() < 0)
>> + rte_panic("Cannot init plugins\n");
>> +
>>
>
> Why testing for error when this cannot happen (see below) ?
This is just a preparation for the next patch which will add a
possibility of failure. Its done here to get the new "infrastructure" in
place in one commit, which seemed to be the preferred option by others.
>> eal_thread_init_master(rte_config.master_lcore);
>>
>> ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
>> diff --git a/lib/librte_eal/common/eal_common_options.c
>> b/lib/librte_eal/common/eal_common_options.c
>> index 1f459ac..b542868 100644
>> --- a/lib/librte_eal/common/eal_common_options.c
>> +++ b/lib/librte_eal/common/eal_common_options.c
>>
>
> [snip]
>
> +
>> +int
>> +eal_plugins_init(void)
>> +{
>> + struct shared_driver *solib = NULL;
>> +
>> + TAILQ_FOREACH(solib, &solib_list, next) {
>> + RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
>> + solib->lib_handle = dlopen(solib->name, RTLD_NOW);
>> + if (solib->lib_handle == NULL)
>> + RTE_LOG(WARNING, EAL, "%s\n", dlerror());
>> + }
>> + return 0;
>> +}
>
>
> I can't see a non 0 return here.
Yes, there is none, see above.
> Btw, returning an error here would change current behavior of dpdk loading
> drivers.
> Not sure we want this as people may rely on this loading not failing.
Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
actually fairly questionable behavior. Why would you load drivers with
-d if you dont care about them getting loaded? Well, maybe to handle an
"everything" case but that's much better handled with the driver
directory thing.
So actually the current patches make things a bit inconsistent, why
should driver directories cause a failure if individual drivers do not?
The question is, which behavior is the one people want: I personally
would rather make -dgiddy.goo fail rather than just warn and chug away
but its not exactly a deal-breaker for me.
- Panu -
>
On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:
>
> Btw, returning an error here would change current behavior of dpdk loading
>> drivers.
>> Not sure we want this as people may rely on this loading not failing.
>>
>
> Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
> actually fairly questionable behavior. Why would you load drivers with -d
> if you dont care about them getting loaded? Well, maybe to handle an
> "everything" case but that's much better handled with the driver directory
> thing.
>
> So actually the current patches make things a bit inconsistent, why should
> driver directories cause a failure if individual drivers do not? The
> question is, which behavior is the one people want: I personally would
> rather make -dgiddy.goo fail rather than just warn and chug away but its
> not exactly a deal-breaker for me.
>
Neither to me.
I agree on the principle of failing when passing wrong stuff, it is saner.
I just want to make sure nobody complains about this change later.
Thomas ? Bruce ?
On Wed, Oct 21, 2015 at 01:09:24PM +0200, David Marchand wrote:
> On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
> wrote:
> >
> > Btw, returning an error here would change current behavior of dpdk loading
> >> drivers.
> >> Not sure we want this as people may rely on this loading not failing.
> >>
> >
> > Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
> > actually fairly questionable behavior. Why would you load drivers with -d
> > if you dont care about them getting loaded? Well, maybe to handle an
> > "everything" case but that's much better handled with the driver directory
> > thing.
> >
> > So actually the current patches make things a bit inconsistent, why should
> > driver directories cause a failure if individual drivers do not? The
> > question is, which behavior is the one people want: I personally would
> > rather make -dgiddy.goo fail rather than just warn and chug away but its
> > not exactly a deal-breaker for me.
> >
>
> Neither to me.
> I agree on the principle of failing when passing wrong stuff, it is saner.
> I just want to make sure nobody complains about this change later.
>
> Thomas ? Bruce ?
>
Error early rather than later. If the -d flag doesn't work crash then, rather
than leaving people having to scroll-back to find why their NIC isn't working.
/Bruce
2015-10-21 12:15, Bruce Richardson:
> On Wed, Oct 21, 2015 at 01:09:24PM +0200, David Marchand wrote:
> > On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
> > wrote:
> > >
> > > Btw, returning an error here would change current behavior of dpdk loading
> > >> drivers.
> > >> Not sure we want this as people may rely on this loading not failing.
> > >>
> > >
> > > Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
> > > actually fairly questionable behavior. Why would you load drivers with -d
> > > if you dont care about them getting loaded? Well, maybe to handle an
> > > "everything" case but that's much better handled with the driver directory
> > > thing.
> > >
> > > So actually the current patches make things a bit inconsistent, why should
> > > driver directories cause a failure if individual drivers do not? The
> > > question is, which behavior is the one people want: I personally would
> > > rather make -dgiddy.goo fail rather than just warn and chug away but its
> > > not exactly a deal-breaker for me.
> > >
> >
> > Neither to me.
> > I agree on the principle of failing when passing wrong stuff, it is saner.
> > I just want to make sure nobody complains about this change later.
> >
> > Thomas ? Bruce ?
>
> Error early rather than later. If the -d flag doesn't work crash then, rather
> than leaving people having to scroll-back to find why their NIC isn't working.
Yes, no reason to ignore errors.
On 10/21/2015 02:53 PM, Thomas Monjalon wrote:
> 2015-10-21 12:15, Bruce Richardson:
>> On Wed, Oct 21, 2015 at 01:09:24PM +0200, David Marchand wrote:
>>> On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
>>> wrote:
>>>>
>>>> Btw, returning an error here would change current behavior of dpdk loading
>>>>> drivers.
>>>>> Not sure we want this as people may rely on this loading not failing.
>>>>>
>>>>
>>>> Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
>>>> actually fairly questionable behavior. Why would you load drivers with -d
>>>> if you dont care about them getting loaded? Well, maybe to handle an
>>>> "everything" case but that's much better handled with the driver directory
>>>> thing.
>>>>
>>>> So actually the current patches make things a bit inconsistent, why should
>>>> driver directories cause a failure if individual drivers do not? The
>>>> question is, which behavior is the one people want: I personally would
>>>> rather make -dgiddy.goo fail rather than just warn and chug away but its
>>>> not exactly a deal-breaker for me.
>>>>
>>>
>>> Neither to me.
>>> I agree on the principle of failing when passing wrong stuff, it is saner.
>>> I just want to make sure nobody complains about this change later.
>>>
>>> Thomas ? Bruce ?
>>
>> Error early rather than later. If the -d flag doesn't work crash then, rather
>> than leaving people having to scroll-back to find why their NIC isn't working.
>
> Yes, no reason to ignore errors.
Okay so we all vigorously agree on this :) Good then, I'll fix the error
behavior too in the next version.
- Panu -
@@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
rte_eal_mcfg_complete();
+ if (eal_plugins_init() < 0)
+ rte_panic("Cannot init plugins\n");
+
eal_thread_init_master(rte_config.master_lcore);
ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
@@ -39,6 +39,7 @@
#include <limits.h>
#include <errno.h>
#include <getopt.h>
+#include <dlfcn.h>
#include <rte_eal.h>
#include <rte_log.h>
@@ -93,6 +94,20 @@ eal_long_options[] = {
{0, 0, NULL, 0 }
};
+TAILQ_HEAD(shared_driver_list, shared_driver);
+
+/* Definition for shared object drivers. */
+struct shared_driver {
+ TAILQ_ENTRY(shared_driver) next;
+
+ char name[PATH_MAX];
+ void* lib_handle;
+};
+
+/* List of external loadable drivers */
+static struct shared_driver_list solib_list =
+TAILQ_HEAD_INITIALIZER(solib_list);
+
static int lcores_parsed;
static int master_lcore_parsed;
static int mem_parsed;
@@ -134,6 +149,38 @@ eal_reset_internal_config(struct internal_config *internal_cfg)
internal_cfg->create_uio_dev = 0;
}
+static int
+eal_plugin_add(const char *path)
+{
+ struct shared_driver *solib;
+
+ solib = malloc(sizeof(*solib));
+ if (solib == NULL) {
+ RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+ return -1;
+ }
+ memset(solib, 0, sizeof(*solib));
+ strncpy(solib->name, path, PATH_MAX-1);
+ solib->name[PATH_MAX-1] = 0;
+ TAILQ_INSERT_TAIL(&solib_list, solib, next);
+
+ return 0;
+}
+
+int
+eal_plugins_init(void)
+{
+ struct shared_driver *solib = NULL;
+
+ TAILQ_FOREACH(solib, &solib_list, next) {
+ RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
+ solib->lib_handle = dlopen(solib->name, RTLD_NOW);
+ if (solib->lib_handle == NULL)
+ RTE_LOG(WARNING, EAL, "%s\n", dlerror());
+ }
+ return 0;
+}
+
/*
* Parse the coremask given as argument (hexadecimal string) and fill
* the global configuration (core role and core count) with the parsed
@@ -716,6 +763,11 @@ eal_parse_common_option(int opt, const char *optarg,
* even if info or warning messages are disabled */
RTE_LOG(CRIT, EAL, "RTE Version: '%s'\n", rte_version());
break;
+ /* force loading of external driver */
+ case 'd':
+ if (eal_plugin_add(optarg) == -1)
+ return -1;
+ break;
/* long options */
case OPT_NO_HUGE_NUM:
@@ -902,6 +954,7 @@ eal_common_usage(void)
" --"OPT_PROC_TYPE" Type of this process (primary|secondary|auto)\n"
" --"OPT_SYSLOG" Set syslog facility\n"
" --"OPT_LOG_LEVEL" Set default log level\n"
+ " -d LIB.so Add driver (can be used multiple times)\n"
" -v Display version information on startup\n"
" -h, --help This help\n"
"\nEAL options for DEBUG use only:\n"
@@ -93,5 +93,6 @@ int eal_adjust_config(struct internal_config *internal_cfg);
int eal_check_common_options(struct internal_config *internal_cfg);
void eal_common_usage(void);
enum rte_proc_type_t eal_proc_type_detect(void);
+int eal_plugins_init(void);
#endif /* EAL_OPTIONS_H */
@@ -43,7 +43,6 @@
#include <getopt.h>
#include <sys/file.h>
#include <fcntl.h>
-#include <dlfcn.h>
#include <stddef.h>
#include <errno.h>
#include <limits.h>
@@ -90,20 +89,6 @@
/* Allow the application to print its usage message too if set */
static rte_usage_hook_t rte_application_usage_hook = NULL;
-TAILQ_HEAD(shared_driver_list, shared_driver);
-
-/* Definition for shared object drivers. */
-struct shared_driver {
- TAILQ_ENTRY(shared_driver) next;
-
- char name[PATH_MAX];
- void* lib_handle;
-};
-
-/* List of external loadable drivers */
-static struct shared_driver_list solib_list =
-TAILQ_HEAD_INITIALIZER(solib_list);
-
/* early configuration structure, when memory config is not mmapped */
static struct rte_mem_config early_mem_config;
@@ -350,7 +335,6 @@ eal_usage(const char *prgname)
printf("\nUsage: %s ", prgname);
eal_common_usage();
printf("EAL Linux options:\n"
- " -d LIB.so Add driver (can be used multiple times)\n"
" --"OPT_SOCKET_MEM" Memory to allocate on sockets (comma separated values)\n"
" --"OPT_HUGE_DIR" Directory where hugetlbfs is mounted\n"
" --"OPT_FILE_PREFIX" Prefix for hugepage filenames\n"
@@ -538,7 +522,6 @@ eal_parse_args(int argc, char **argv)
char **argvopt;
int option_index;
char *prgname = argv[0];
- struct shared_driver *solib;
argvopt = argv;
@@ -568,19 +551,6 @@ eal_parse_args(int argc, char **argv)
eal_usage(prgname);
exit(EXIT_SUCCESS);
- /* force loading of external driver */
- case 'd':
- solib = malloc(sizeof(*solib));
- if (solib == NULL) {
- RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
- return -1;
- }
- memset(solib, 0, sizeof(*solib));
- strncpy(solib->name, optarg, PATH_MAX-1);
- solib->name[PATH_MAX-1] = 0;
- TAILQ_INSERT_TAIL(&solib_list, solib, next);
- break;
-
/* long options */
case OPT_XEN_DOM0_NUM:
#ifdef RTE_LIBRTE_XEN_DOM0
@@ -731,7 +701,6 @@ 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;
const char *logid;
char cpuset[RTE_CPU_AFFINITY_STR_LEN];
@@ -824,12 +793,8 @@ rte_eal_init(int argc, char **argv)
rte_eal_mcfg_complete();
- TAILQ_FOREACH(solib, &solib_list, next) {
- RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
- solib->lib_handle = dlopen(solib->name, RTLD_NOW);
- if (solib->lib_handle == NULL)
- RTE_LOG(WARNING, EAL, "%s\n", dlerror());
- }
+ if (eal_plugins_init() < 0)
+ rte_panic("Cannot init plugins\n");
eal_thread_init_master(rte_config.master_lcore);