DPDK patches and discussions
 help / color / mirror / Atom feed
From: Panu Matilainen <pmatilai@redhat.com>
To: David Marchand <david.marchand@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] eal: move plugin loading to eal/common
Date: Wed, 21 Oct 2015 13:54:21 +0300	[thread overview]
Message-ID: <56276EDD.7060002@redhat.com> (raw)
In-Reply-To: <CALwxeUukFXRfa34XYm1RyOLf_beJA2L7hJMVuaM-CH1H6q87Xg@mail.gmail.com>

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 -



>

  reply	other threads:[~2015-10-21 10:54 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-25 11:58 [dpdk-dev] [PATCH 0/2] Add support for driver directories Panu Matilainen
2015-09-25 11:58 ` [dpdk-dev] [PATCH 1/2] eal: refactor plugin list append from eal_parse_args() to a helper function Panu Matilainen
2015-09-25 11:58 ` [dpdk-dev] [PATCH 2/2] eal: add support for driver directory concept Panu Matilainen
2015-09-25 12:35 ` [dpdk-dev] [PATCH 0/2] Add support for driver directories David Marchand
2015-09-25 13:00   ` Panu Matilainen
2015-10-14 10:41     ` Panu Matilainen
2015-10-14 11:55       ` David Marchand
2015-10-16 11:58 ` [dpdk-dev] [PATCH 0/5 v2] " Panu Matilainen
2015-10-16 11:58   ` [dpdk-dev] [PATCH 1/5] eal: refactor plugin list append from eal_parse_args() to a helper function Panu Matilainen
2015-10-16 11:58     ` [dpdk-dev] [PATCH 2/5] eal: refactor plugin init " Panu Matilainen
2015-10-16 11:58     ` [dpdk-dev] [PATCH 3/5] eal: move plugin loading to eal/common Panu Matilainen
2015-10-16 11:58     ` [dpdk-dev] [PATCH 4/5] eal: add an error code to plugin init for the next step Panu Matilainen
2015-10-16 12:59       ` Bruce Richardson
2015-10-16 13:14         ` Panu Matilainen
2015-10-16 13:38           ` Panu Matilainen
2015-10-21  8:14             ` Thomas Monjalon
2015-10-16 11:58     ` [dpdk-dev] [PATCH 5/5] eal: add support for driver directory concept Panu Matilainen
2015-10-16 12:57     ` [dpdk-dev] [PATCH 1/5] eal: refactor plugin list append from eal_parse_args() to a helper function Bruce Richardson
2015-10-16 13:07       ` Panu Matilainen
2015-10-21  8:29   ` [dpdk-dev] [PATCH 0/2 v3] Add support for driver directories Panu Matilainen
2015-10-21  8:29   ` [dpdk-dev] [PATCH 1/2] eal: move plugin loading to eal/common Panu Matilainen
2015-10-21 10:15     ` David Marchand
2015-10-21 10:54       ` Panu Matilainen [this message]
2015-10-21 11:09         ` David Marchand
2015-10-21 11:15           ` Bruce Richardson
2015-10-21 11:53             ` Thomas Monjalon
2015-10-21 12:07               ` Panu Matilainen
2015-10-21  8:29   ` [dpdk-dev] [PATCH 2/2] eal: add support for driver directory concept Panu Matilainen
2015-10-21  8:44     ` Thomas Monjalon
2015-10-21  9:43       ` Panu Matilainen
2015-11-10 14:28   ` [dpdk-dev] [PATCH v4 0/2] Add support for driver directories Panu Matilainen
2015-11-10 15:04     ` David Marchand
2015-11-12 15:52       ` Thomas Monjalon
2015-12-03  2:07         ` Stephen Hemminger
2015-12-03  2:26           ` Thomas Monjalon
2015-12-03  7:59             ` Panu Matilainen
2015-11-10 14:28   ` [dpdk-dev] [PATCH v4 1/2] eal: move plugin loading to eal/common Panu Matilainen
2015-11-10 14:28   ` [dpdk-dev] [PATCH v4 2/2] eal: add support for driver directory concept Panu Matilainen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56276EDD.7060002@redhat.com \
    --to=pmatilai@redhat.com \
    --cc=david.marchand@6wind.com \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).