Hey Alex,
I remember that calling certain DPDK functions (that we use to
initialize devices) with '_lcore_id !=0' resulted in an error. If this
is not the case anymore with DPDK 1.7.1 (i.e. if you tested this patch
with NON_PMD_CORE_ID!=0 and it worked), then I have no objection.
I would test this myself, but I don't have access to a DPDK capable
system right now. If you want to be more sure about this you can build
DPDK with CONFIG_RTE_LIBRTE_MBUF_DEBUG=y and watch for failed
assertions.
Last thing: we could also avoid using the _lcore_id to set the
affinity of a thread (e.g. a thread with '_lcore_id == 1' doesn't
necessarily need to be pinned to cpu 1). This would be another way to
achieve the same goal, but I prefer your approach, because it is more
consistent with DPDK internals.
Hope this helps. Let me know if there's anything else about this
Thanks,
Daniele
2015-02-05 0:14 GMT+01:00 Alex Wang <al...@nicira.com>:
> Hey Daniele,
>
> Do you still remember why you mentioned:
>
> "/* We have to use 0 to allow non pmd threads to perform certain DPDK
> * operations, like rte_eth_dev_configure(). */
> "
> in your commit: db73f716 (netdev-dpdk: Fix race condition with DPDK
> mempools in non pmd threads)
>
> This posted commit works during my manual test. And the
> rte_eth_dev_configure() in dpdk-1.7.1 does not require the caller lcore id
> to
> be 0.
>
> But Pravin mentioned that you may know more about why use 0 for non-pmd
> threads (to prevent crash?).
>
> Could you share some thoughts?
>
> Thanks,
> Alex Wang,
>
> On Wed, Feb 4, 2015 at 2:14 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>>
>> On Tue, Feb 3, 2015 at 5:54 PM, Alex Wang <al...@nicira.com> wrote:
>> > For testing purpose, developers may want to change the NON_PMD_CORE_ID
>> > and use a different core for non-pmd threads. Since the netdev-dpdk
>> > module is hard-coded to assert the non-pmd threads using core 0, such
>> > change will cause abortion of OVS.
>> >
>> > This commit fixes the assertion and allows changing NON_PMD_CORE_ID.
>> >
>> > Signed-off-by: Alex Wang <al...@nicira.com>
>> > ---
>> > lib/dpctl.c | 2 +-
>> > lib/dpif-netdev.h | 1 -
>> > lib/netdev-dpdk.c | 12 ++++++------
>> > lib/netdev-dpdk.h | 2 ++
>> > 4 files changed, 9 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/lib/dpctl.c b/lib/dpctl.c
>> > index 4c2614b..125023c 100644
>> > --- a/lib/dpctl.c
>> > +++ b/lib/dpctl.c
>> > @@ -31,11 +31,11 @@
>> > #include "dirs.h"
>> > #include "dpctl.h"
>> > #include "dpif.h"
>> > -#include "dpif-netdev.h"
>> > #include "dynamic-string.h"
>> > #include "flow.h"
>> > #include "match.h"
>> > #include "netdev.h"
>> > +#include "netdev-dpdk.h"
>> > #include "netlink.h"
>> > #include "odp-util.h"
>> > #include "ofp-parse.h"
>> > diff --git a/lib/dpif-netdev.h b/lib/dpif-netdev.h
>> > index d811507..410fcfa 100644
>> > --- a/lib/dpif-netdev.h
>> > +++ b/lib/dpif-netdev.h
>> > @@ -42,7 +42,6 @@ static inline void dp_packet_pad(struct ofpbuf *b)
>> >
>> > #define NR_QUEUE 1
>> > #define NR_PMD_THREADS 1
>> > -#define NON_PMD_CORE_ID 0
>> >
>> > #ifdef __cplusplus
>> > }
>> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> > index 0ede200..391695f 100644
>> > --- a/lib/netdev-dpdk.c
>> > +++ b/lib/netdev-dpdk.c
>> > @@ -1553,8 +1553,8 @@ pmd_thread_setaffinity_cpu(int cpu)
>> > VLOG_ERR("Thread affinity error %d",err);
>> > return err;
>> > }
>> > - /* lcore_id 0 is reseved for use by non pmd threads. */
>> > - ovs_assert(cpu);
>> > + /* NON_PMD_CORE_ID is reserved for use by non pmd threads. */
>> > + ovs_assert(cpu != NON_PMD_CORE_ID);
>>
>> > RTE_PER_LCORE(_lcore_id) = cpu;
>> >
>> > return 0;
>> > @@ -1563,13 +1563,13 @@ pmd_thread_setaffinity_cpu(int cpu)
>> > void
>> > thread_set_nonpmd(void)
>> > {
>> > - /* We have to use 0 to allow non pmd threads to perform certain
>> > DPDK
>> > - * operations, like rte_eth_dev_configure(). */
>> > - RTE_PER_LCORE(_lcore_id) = 0;
>> > + /* We have to use NON_PMD_CORE_ID to allow non-pmd threads to
>> > perform
>> > + * certain DPDK operations, like rte_eth_dev_configure(). */
>>
>>
>> The is not equivalent comment. have you confirmed that
>> rte_eth_dev_configure() works on any core?
>>
>> > + RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID;
>> > }
>> >
>> > static bool
>> > thread_is_pmd(void)
>> > {
>> > - return rte_lcore_id() != 0;
>> > + return rte_lcore_id() != NON_PMD_CORE_ID;
>> > }
>> > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
>> > index c24d6da..9a47165 100644
>> > --- a/lib/netdev-dpdk.h
>> > +++ b/lib/netdev-dpdk.h
>> > @@ -5,6 +5,8 @@
>> >
>> > struct dpif_packet;
>> >
>> > +#define NON_PMD_CORE_ID 0
>> > +
>> > #ifdef DPDK_NETDEV
>> >
>> > #include <rte_config.h>
>> > --
>> > 1.7.9.5
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
