Re: [ovs-dev] [PATCH v5 04/10] dpif-netdev: Register packet processing cores to KA framework.

2017-10-13 Thread Fischetti, Antonio 
Hi Bhanu,
a couple of minor comments inline.

-Antonio

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-boun...@openvswitch.org]
> On Behalf Of Bhanuprakash Bodireddy
> Sent: Friday, September 15, 2017 5:40 PM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v5 04/10] dpif-netdev: Register packet processing
> cores to KA framework.
> 
> This commit registers the packet processing PMD threads to keepalive
> framework. Only PMDs that have rxqs mapped will be registered and
> actively monitored by KA framework.
> 
> This commit spawns a keepalive thread that will dispatch heartbeats to
> PMD threads. The pmd threads respond to heartbeats by marking themselves
> alive. As long as PMD responds to heartbeats it is considered 'healthy'.
> 
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com>
> ---
>  lib/dpif-netdev.c |  79 ++
>  lib/keepalive.c   | 191 
> --
>  lib/keepalive.h   |  20 ++
>  lib/ovs-thread.c  |   6 ++
>  lib/ovs-thread.h  |   1 +
>  lib/util.c|  22 +++
>  lib/util.h|   1 +
>  7 files changed, 316 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index ca74df8..da419d5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -49,6 +49,7 @@
>  #include "flow.h"
>  #include "hmapx.h"
>  #include "id-pool.h"
> +#include "keepalive.h"
>  #include "latch.h"
>  #include "netdev.h"
>  #include "netdev-vport.h"
> @@ -591,6 +592,7 @@ struct dp_netdev_pmd_thread {
>  uint64_t last_reload_seq;
>  atomic_bool reload; /* Do we need to reload ports? */
>  pthread_t thread;
> +pid_t tid;  /* Thread id of this pmd thread. */
>  unsigned core_id;   /* CPU core id of this pmd thread. */
>  int numa_id;/* numa node id of this pmd thread. */
>  bool isolated;
> @@ -1018,6 +1020,72 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>  *n = k;
>  }
> 
> +static void *
> +ovs_keepalive(void *f_ OVS_UNUSED)
> +{
> +pthread_detach(pthread_self());
> +
> +for (;;) {
> +int interval;

[Antonio] shouldn't we put 'interval' declaration outside of the for (;;) loop?
   int interval;
   for (;;) {
   interval =...
   xnanosleep(..
   }

> +
> +interval = get_ka_interval();
> +xnanosleep(interval);
> +}
> +
> +return NULL;
> +}
> +
> +/* Kickstart 'ovs_keepalive' thread. */
> +static void
> +ka_thread_start(struct dp_netdev *dp)
> +{
> +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +if (ovsthread_once_start()) {
> +ovs_thread_create("ovs_keepalive", ovs_keepalive, dp);
> +
> +ovsthread_once_done();
> +}
> +}
> +
> +/* Register the datapath threads. This gets invoked on every datapath
> + * reconfiguration. The pmd thread[s] having rxq[s] mapped will be
> + * registered to KA framework.
> + */
> +static void
> +ka_register_datapath_threads(struct dp_netdev *dp)
> +{
> +if (!ka_is_enabled()) {
> +return;
> +}
> +
> +ka_thread_start(dp);
> +
> +ka_reload_datapath_threads_begin();
> +
> +struct dp_netdev_pmd_thread *pmd;
> +CMAP_FOR_EACH (pmd, node, >poll_threads) {
> +/*  Register only PMD threads. */
> +if (pmd->core_id != NON_PMD_CORE_ID) {
> +/* Skip PMD thread with no rxqs mapping. */
> +if (OVS_UNLIKELY(!hmap_count(>poll_list))) {
> +/* Rxq mapping changes due to datapath reconfiguration.
> + * If no rxqs mapped to PMD now due to reconfiguration,
> + * unregister the pmd thread. */
> +ka_unregister_thread(pmd->tid);
> +continue;
> +}
> +
> +ka_register_thread(pmd->tid);
> +VLOG_INFO("Registered PMD thread [%d] on Core[%d] to KA
> framework",
> +  pmd->tid, pmd->core_id);
> +}
> +}
> +ka_cache_registered_threads();
> +
> +ka_reload_datapath_threads_end();
> +}
> +
>  static void
>  dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
>const char *argv[], void *aux OVS_UNUSED)
> @@ -3821,6 +3889,9 @@ reconfigure_datapath(struct dp_netdev *dp)
> 
>  /* Reload affected pmd threads. */
>  reload_affected_pmds(dp);
> +
> +/* Register datapath threads to

[ovs-dev] [PATCH v5 04/10] dpif-netdev: Register packet processing cores to KA framework.

2017-09-15 Thread Bhanuprakash Bodireddy 
This commit registers the packet processing PMD threads to keepalive
framework. Only PMDs that have rxqs mapped will be registered and
actively monitored by KA framework.

This commit spawns a keepalive thread that will dispatch heartbeats to
PMD threads. The pmd threads respond to heartbeats by marking themselves
alive. As long as PMD responds to heartbeats it is considered 'healthy'.

Signed-off-by: Bhanuprakash Bodireddy 
---
 lib/dpif-netdev.c |  79 ++
 lib/keepalive.c   | 191 --
 lib/keepalive.h   |  20 ++
 lib/ovs-thread.c  |   6 ++
 lib/ovs-thread.h  |   1 +
 lib/util.c|  22 +++
 lib/util.h|   1 +
 7 files changed, 316 insertions(+), 4 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index ca74df8..da419d5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -49,6 +49,7 @@
 #include "flow.h"
 #include "hmapx.h"
 #include "id-pool.h"
+#include "keepalive.h"
 #include "latch.h"
 #include "netdev.h"
 #include "netdev-vport.h"
@@ -591,6 +592,7 @@ struct dp_netdev_pmd_thread {
 uint64_t last_reload_seq;
 atomic_bool reload; /* Do we need to reload ports? */
 pthread_t thread;
+pid_t tid;  /* Thread id of this pmd thread. */
 unsigned core_id;   /* CPU core id of this pmd thread. */
 int numa_id;/* numa node id of this pmd thread. */
 bool isolated;
@@ -1018,6 +1020,72 @@ sorted_poll_thread_list(struct dp_netdev *dp,
 *n = k;
 }
 
+static void *
+ovs_keepalive(void *f_ OVS_UNUSED)
+{
+pthread_detach(pthread_self());
+
+for (;;) {
+int interval;
+
+interval = get_ka_interval();
+xnanosleep(interval);
+}
+
+return NULL;
+}
+
+/* Kickstart 'ovs_keepalive' thread. */
+static void
+ka_thread_start(struct dp_netdev *dp)
+{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+if (ovsthread_once_start()) {
+ovs_thread_create("ovs_keepalive", ovs_keepalive, dp);
+
+ovsthread_once_done();
+}
+}
+
+/* Register the datapath threads. This gets invoked on every datapath
+ * reconfiguration. The pmd thread[s] having rxq[s] mapped will be
+ * registered to KA framework.
+ */
+static void
+ka_register_datapath_threads(struct dp_netdev *dp)
+{
+if (!ka_is_enabled()) {
+return;
+}
+
+ka_thread_start(dp);
+
+ka_reload_datapath_threads_begin();
+
+struct dp_netdev_pmd_thread *pmd;
+CMAP_FOR_EACH (pmd, node, >poll_threads) {
+/*  Register only PMD threads. */
+if (pmd->core_id != NON_PMD_CORE_ID) {
+/* Skip PMD thread with no rxqs mapping. */
+if (OVS_UNLIKELY(!hmap_count(>poll_list))) {
+/* Rxq mapping changes due to datapath reconfiguration.
+ * If no rxqs mapped to PMD now due to reconfiguration,
+ * unregister the pmd thread. */
+ka_unregister_thread(pmd->tid);
+continue;
+}
+
+ka_register_thread(pmd->tid);
+VLOG_INFO("Registered PMD thread [%d] on Core[%d] to KA framework",
+  pmd->tid, pmd->core_id);
+}
+}
+ka_cache_registered_threads();
+
+ka_reload_datapath_threads_end();
+}
+
 static void
 dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
   const char *argv[], void *aux OVS_UNUSED)
@@ -3821,6 +3889,9 @@ reconfigure_datapath(struct dp_netdev *dp)
 
 /* Reload affected pmd threads. */
 reload_affected_pmds(dp);
+
+/* Register datapath threads to KA monitoring. */
+ka_register_datapath_threads(dp);
 }
 
 /* Returns true if one of the netdevs in 'dp' requires a reconfiguration */
@@ -4023,6 +4094,8 @@ pmd_thread_main(void *f_)
 
 /* Stores the pmd thread's 'pmd' to 'per_pmd_key'. */
 ovsthread_setspecific(pmd->dp->per_pmd_key, pmd);
+/* Stores tid in to 'pmd->tid'. */
+ovsthread_settid(>tid);
 ovs_numa_thread_setaffinity_core(pmd->core_id);
 dpdk_set_lcore_id(pmd->core_id);
 poll_cnt = pmd_load_queues_and_ports(pmd, _list);
@@ -4056,6 +4129,9 @@ reload:
   : PMD_CYCLES_IDLE);
 }
 
+/* Mark PMD thread alive. */
+ka_mark_pmd_thread_alive(pmd->tid);
+
 if (lc++ > 1024) {
 bool reload;
 
@@ -4089,6 +4165,9 @@ reload:
 }
 
 emc_cache_uninit(>flow_cache);
+
+ka_unregister_thread(pmd->tid);
+
 free(poll_list);
 pmd_free_cached_ports(pmd);
 return NULL;
diff --git a/lib/keepalive.c b/lib/keepalive.c
index 1f151f6..da4defd 100644
--- a/lib/keepalive.c
+++ b/lib/keepalive.c
@@ -19,6 +19,7 @@
 #include "keepalive.h"
 #include "lib/vswitch-idl.h"
 #include "openvswitch/vlog.h"
+#include "process.h"
 #include "seq.h"
 #include "timeval.h"
 
@@ -28,11 +29,18 @@ static bool keepalive_enable