Thanks for looking in to the patch Kevin, please see my reply inline.
> -----Original Message-----
> From: Traynor, Kevin
> Sent: Monday, April 25, 2016 6:08 PM
> To: Bodireddy, Bhanuprakash <bhanuprakash.bodire...@intel.com>;
> dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: Set pmd thread priority
>
> On 21/04/2016 16:16, Bhanuprakash Bodireddy wrote:
> > Set the DPDK pmd thread scheduling policy to SCHED_RR and static
> > priority to highest priority value of the policy. This is to deal with
> > pmd thread starvation case where another cpu hogging process can get
> > scheduled/affinitized to the same core where pmd is running there by
> > significantly impacting the datapath performance.
> >
> > The realtime scheduling policy is applied only when CPU mask is passed
> > to 'pmd-cpu-mask'. The exception to this is 'pmd-cpu-mask=1', where
> > the policy and priority shall not be applied to pmd thread spawned on
> core0.
> > For example:
> >
> > * In the absence of pmd-cpu-mask or if pmd-cpu-mask=1, one pmd
> > thread shall be created and affinitized to 'core 0' with default
> > scheduling policy and priority applied.
> >
> > * If pmd-cpu-mask is specified with CPU mask > 1, one or more pmd
> > threads shall be spawned on the corresponding core(s) in the mask
> > and real time scheduling policy SCHED_RR and highest static
> > priority is applied to the pmd thread(s).
> >
> > To reproduce use following commands:
> >
> > ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=6 taskset 0x2
> > cat /dev/zero > /dev/null &
>
> Even though it seems the most likely case - I'm not sure that we can always
> assume the user who put the non-OVS process on the core did so by mistake
> and would want us to increase our priority.
You've got a point. When the user explicitly sets pmd-cpu-mask, he is likely
looking at
Performance and wants to pin pmd threads to dedicated cores and unlikely to
run
any other process on the pmd cores.
Having said that, I have come across cases with HT enabled, users ran in to
issues
understanding 'thread_sliblings_list' and wrongly affinitize qemu threads
to the cores running pmds using 'taskset' there by significantly impacting the
datapath performance. This patch will mitigate such issues.
>
> >
> > Signed-off-by: Bhanuprakash Bodireddy
> <bhanuprakash.bodire...@intel.com>
> > ---
> > lib/dpif-netdev.c | 9 +++++++++
> > lib/netdev-dpdk.c | 14 ++++++++++++++
> > lib/netdev-dpdk.h | 1 +
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 1e8a37c..4a46816 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2670,6 +2670,15 @@ pmd_thread_main(void *f_)
> > /* Stores the pmd thread's 'pmd' to 'per_pmd_key'. */
> > ovsthread_setspecific(pmd->dp->per_pmd_key, pmd);
> > pmd_thread_setaffinity_cpu(pmd->core_id);
> > +
> > +#ifdef DPDK_NETDEV
> > + /* Set pmd thread's scheduling policy to SCHED_RR and priority to
> > + * highest priority of SCHED_RR policy, In absence of pmd-cpu-mask (or)
> > + * pmd-cpu-mask=1, default scheduling policy and priority shall
> > + * apply to pmd thread */
> > + if (pmd->core_id)
> > + pmd_thread_setpriority();
>
> Similar to above, I don't think we can assume anything special about
> core 0. This type of change sounds like something that would be better
> done at a layer above vswitch which has more system wide knowledge.
I stand to be corrected, Its very uncommon to isolate core0 or run HPC
apps/threads
on the core 0. Also on multicore systems to improve application performance and
mitigate
Interrupts, IRQs get explicity affinitized to Core 0. For this reasons I treat
Core 0 special.
> fwiw, it would be cleaner to remove the #ifdef from here and create a
> dummy fn in netdev-dpdk.h, also the 'if' needs {}
Agree.
> > +#endif
> > reload:
> > emc_cache_init(&pmd->flow_cache);
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index 208c5f5..6518c87 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2926,6 +2926,20 @@ pmd_thread_setaffinity_cpu(unsigned cpu)
> > return 0;
> > }
> >
> > +void
> > +pmd_thread_setpriority(void)
> > +{
> > + struct sched_param threadparam;
> > + int err;
> > +
> > + memset(&threadparam, 0, sizeof(threadparam));
> > + threadparam.sched_priority = sched_get_priority_max(SCHED_RR);
> > + err = pthread_setschedparam(pthread_self(), SCHED_RR,
> &threadparam);
> > + if (err) {
> > + VLOG_WARN("Thread priority error %d",err);
> > + }
> > +}
> > +
> > static bool
> > dpdk_thread_is_pmd(void)
> > {
> > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
> > index 646d3e2..168673b 100644
> > --- a/lib/netdev-dpdk.h
> > +++ b/lib/netdev-dpdk.h
> > @@ -26,6 +26,7 @@ int dpdk_init(int argc, char **argv);
> > void netdev_dpdk_register(void);
> > void free_dpdk_buf(struct dp_packet *);
> > int pmd_thread_setaffinity_cpu(unsigned cpu);
> > +void pmd_thread_setpriority(void);
> >
> > #else
> >
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
