Re: [ovs-dev] [PATCH v6 3/8] dpif-netdev: Enable heartbeats for DPDK datapath.

2018-01-22 Thread Kevin Traynor 
On 12/08/2017 12:04 PM, Bhanuprakash Bodireddy wrote:
> This commit adds heartbeat mechanism support for DPDK datapath. Heartbeats
> are sent to registered PMD threads at predefined intervals (as set in ovsdb
> with 'keepalive-interval').
> 
> The heartbeats are only enabled when there is atleast one port added to
> the bridge and with active PMD thread polling the port.
> 
> Signed-off-by: Bhanuprakash Bodireddy 
> ---
>  lib/dpif-netdev.c | 14 +-
>  lib/keepalive.c   | 42 ++
>  lib/keepalive.h   |  1 +
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c978a76..9021906 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1021,14 +1021,26 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>  }
>  
>  static void *
> -ovs_keepalive(void *f_ OVS_UNUSED)
> +ovs_keepalive(void *f_)
>  {
> +struct dp_netdev *dp = f_;
> +
>  pthread_detach(pthread_self());
>  
>  for (;;) {
> +bool hb_enable;
> +int n_pmds;
>  uint64_t interval;

I don't think you need any of these variables

>  
>  interval = get_ka_interval();
> +n_pmds = cmap_count(>poll_threads) - 1;
> +hb_enable = (n_pmds > 0) ? true : false;
> +
> +/* Dispatch heartbeats only if pmd[s] exist. */
> +if (hb_enable) {

Why is this needed? it's not atomic, so that suggests it wouldn't be a
problem to just call the dispatch_heartbeats() anyway.

> +dispatch_heartbeats();
> +}
> +
>  xnanosleep(interval * 1000 * 1000);
>  }
>  
> diff --git a/lib/keepalive.c b/lib/keepalive.c
> index b04877f..0e4b2b6 100644
> --- a/lib/keepalive.c
> +++ b/lib/keepalive.c
> @@ -284,6 +284,48 @@ ka_mark_pmd_thread_sleep(int tid)
>  }
>  }
>  
> +/* Dispatch heartbeats from 'ovs_keepalive' thread. */
> +void
> +dispatch_heartbeats(void)
> +{
> +struct ka_process_info *pinfo, *pinfo_next;
> +
> +/* Iterates over the list of processes in 'cached_process_list' map. */
> +HMAP_FOR_EACH_SAFE (pinfo, pinfo_next, node,
> +_info.cached_process_list) {
> +if (pinfo->state == KA_STATE_UNUSED) {
> +continue;
> +}
> +

Is the pinfo state atomic - you could be writing it as alive/sleep at
the same time as calling this, no?

> +switch (pinfo->state) {
> +case KA_STATE_UNUSED:
> +break;
> +case KA_STATE_ALIVE:
> +pinfo->state = KA_STATE_MISSING;
> +pinfo->last_seen_time = time_wall_msec();
> +break;
> +case KA_STATE_MISSING:
> +pinfo->state = KA_STATE_DEAD;
> +break;
> +case KA_STATE_DEAD:
> +pinfo->state = KA_STATE_GONE;
> +break;
> +case KA_STATE_GONE:
> +break;
> +case KA_STATE_SLEEP:
> +pinfo->state = KA_STATE_SLEEP;
> +pinfo->last_seen_time = time_wall_msec();

I don't think last_seen_time should be updated here

> +break;
> +default:
> +OVS_NOT_REACHED();
> +}
> +
> +/* Invoke 'ka_update_thread_state' cb function to update state info
> + * in to 'ka_info.process_list' map. */
> +ka_info.relay_cb(pinfo->tid, pinfo->state, pinfo->last_seen_time);
> +}
> +}
> +
>  void
>  ka_init(const struct smap *ovs_other_config)
>  {
> diff --git a/lib/keepalive.h b/lib/keepalive.h
> index 7674ea3..cbc2387 100644
> --- a/lib/keepalive.h
> +++ b/lib/keepalive.h
> @@ -100,6 +100,7 @@ void ka_free_cached_threads(void);
>  void ka_cache_registered_threads(void);
>  void ka_mark_pmd_thread_alive(int);
>  void ka_mark_pmd_thread_sleep(int);
> +void dispatch_heartbeats(void);
>  void ka_init(const struct smap *);
>  void ka_destroy(void);
>  
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6 3/8] dpif-netdev: Enable heartbeats for DPDK datapath.

2017-12-15 Thread Fischetti, Antonio 
LGTM,

Tested-by: Antonio Fischetti <antonio.fische...@intel.com>
Acked-by: Antonio Fischetti <antonio.fische...@intel.com>


> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Bhanuprakash Bodireddy
> Sent: Friday, December 8, 2017 12:04 PM
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH v6 3/8] dpif-netdev: Enable heartbeats for
> DPDK datapath.
> 
> This commit adds heartbeat mechanism support for DPDK datapath.
> Heartbeats
> are sent to registered PMD threads at predefined intervals (as set in
> ovsdb
> with 'keepalive-interval').
> 
> The heartbeats are only enabled when there is atleast one port added to
> the bridge and with active PMD thread polling the port.
> 
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com>
> ---
>  lib/dpif-netdev.c | 14 +-
>  lib/keepalive.c   | 42 ++
>  lib/keepalive.h   |  1 +
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c978a76..9021906 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1021,14 +1021,26 @@ sorted_poll_thread_list(struct dp_netdev *dp,
>  }
> 
>  static void *
> -ovs_keepalive(void *f_ OVS_UNUSED)
> +ovs_keepalive(void *f_)
>  {
> +struct dp_netdev *dp = f_;
> +
>  pthread_detach(pthread_self());
> 
>  for (;;) {
> +bool hb_enable;
> +int n_pmds;
>  uint64_t interval;
> 
>  interval = get_ka_interval();
> +n_pmds = cmap_count(>poll_threads) - 1;
> +hb_enable = (n_pmds > 0) ? true : false;
> +
> +/* Dispatch heartbeats only if pmd[s] exist. */
> +if (hb_enable) {
> +dispatch_heartbeats();
> +}
> +
>  xnanosleep(interval * 1000 * 1000);
>  }
> 
> diff --git a/lib/keepalive.c b/lib/keepalive.c
> index b04877f..0e4b2b6 100644
> --- a/lib/keepalive.c
> +++ b/lib/keepalive.c
> @@ -284,6 +284,48 @@ ka_mark_pmd_thread_sleep(int tid)
>  }
>  }
> 
> +/* Dispatch heartbeats from 'ovs_keepalive' thread. */
> +void
> +dispatch_heartbeats(void)
> +{
> +struct ka_process_info *pinfo, *pinfo_next;
> +
> +/* Iterates over the list of processes in 'cached_process_list'
> map. */
> +HMAP_FOR_EACH_SAFE (pinfo, pinfo_next, node,
> +_info.cached_process_list) {
> +if (pinfo->state == KA_STATE_UNUSED) {
> +continue;
> +}
> +
> +switch (pinfo->state) {
> +case KA_STATE_UNUSED:
> +break;
> +case KA_STATE_ALIVE:
> +pinfo->state = KA_STATE_MISSING;
> +pinfo->last_seen_time = time_wall_msec();
> +break;
> +case KA_STATE_MISSING:
> +pinfo->state = KA_STATE_DEAD;
> +break;
> +case KA_STATE_DEAD:
> +pinfo->state = KA_STATE_GONE;
> +break;
> +case KA_STATE_GONE:
> +break;
> +case KA_STATE_SLEEP:
> +pinfo->state = KA_STATE_SLEEP;
> +pinfo->last_seen_time = time_wall_msec();
> +break;
> +default:
> +OVS_NOT_REACHED();
> +}
> +
> +/* Invoke 'ka_update_thread_state' cb function to update state
> info
> + * in to 'ka_info.process_list' map. */
> +ka_info.relay_cb(pinfo->tid, pinfo->state, pinfo-
> >last_seen_time);
> +}
> +}
> +
>  void
>  ka_init(const struct smap *ovs_other_config)
>  {
> diff --git a/lib/keepalive.h b/lib/keepalive.h
> index 7674ea3..cbc2387 100644
> --- a/lib/keepalive.h
> +++ b/lib/keepalive.h
> @@ -100,6 +100,7 @@ void ka_free_cached_threads(void);
>  void ka_cache_registered_threads(void);
>  void ka_mark_pmd_thread_alive(int);
>  void ka_mark_pmd_thread_sleep(int);
> +void dispatch_heartbeats(void);
>  void ka_init(const struct smap *);
>  void ka_destroy(void);
> 
> --
> 2.4.11
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev