Hey Kevin, Thanks for the testing,~!
On Thu, Feb 5, 2015 at 10:29 AM, Traynor, Kevin <kevin.tray...@intel.com> wrote: > Hi, > > I've done some quick testing on this patch for different values of > NON_PMD_CORE_ID and it looks to be working fine. > > The only issue I've seen is that I'm isolcpus for all cores except core 0, > so when I change the NON_PMD_CORE_ID to non-zero, the pmd is being > scheduled on core 0 also and throughput suffers depending on system load. > > I'd suggest adding a comment beside the #define to warn that this value > affects where the pmd is scheduled, and it may be on a non-isolated core. > I'll do that, > Thanks, > Kevin. > > > -----Original Message----- > > From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di > Proietto > > Sent: Thursday, February 5, 2015 12:58 AM > > To: Alex Wang > > Cc: dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Allow changing > NON_PMD_CORE_ID for testing purpose. > > > > 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 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
