[ovs-dev] [PATCH v4 2/7] Keepalive: Add initial keepalive support.
Fischetti, Antonio
antonio.fischetti at intel.com
Tue Sep 5 07:03:01 UTC 2017
Comments inline.
> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org]
> On Behalf Of Bhanuprakash Bodireddy
> Sent: Tuesday, August 22, 2017 10:41 AM
> To: dev at openvswitch.org
> Cc: i.maximets at samsung.com
> Subject: [ovs-dev] [PATCH v4 2/7] Keepalive: Add initial keepalive support.
>
> This commit introduces the initial keepalive support by adding
> 'keepalive' module and also helper and initialization functions
> that will be invoked by later commits.
>
> This commit adds new ovsdb column "keepalive" that shows the status
> of the datapath threads. This is implemented for DPDK datapath and
> only status of PMD threads is reported.
>
> For eg:
> To enable keepalive feature.
> 'ovs-vsctl --no-wait set Open_vSwitch . other_config:enable-keepalive=true'
[Antonio]
It would help saying that this command must be run 'before'
launching ovs-vswitchd otherwise has no effect.
>
> To set timer interval of 5000ms for monitoring packet processing cores.
> 'ovs-vsctl --no-wait set Open_vSwitch . \
> other_config:keepalive-interval="5000"
>
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
> ---
> lib/automake.mk | 2 +
> lib/keepalive.c | 183 +++++++++++++++++++++++++++++++++++++++++++++
> lib/keepalive.h | 87 +++++++++++++++++++++
> vswitchd/bridge.c | 3 +
> vswitchd/vswitch.ovsschema | 8 +-
> vswitchd/vswitch.xml | 49 ++++++++++++
> 6 files changed, 330 insertions(+), 2 deletions(-)
> create mode 100644 lib/keepalive.c
> create mode 100644 lib/keepalive.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 2415f4c..0d99f0a 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -110,6 +110,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/json.c \
> lib/jsonrpc.c \
> lib/jsonrpc.h \
> + lib/keepalive.c \
> + lib/keepalive.h \
> lib/lacp.c \
> lib/lacp.h \
> lib/latch.h \
> diff --git a/lib/keepalive.c b/lib/keepalive.c
> new file mode 100644
> index 0000000..ac73a42
> --- /dev/null
> +++ b/lib/keepalive.c
> @@ -0,0 +1,183 @@
> +/*
> + * Copyright (c) 2014, 2015, 2016, 2017 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +
> +#include "keepalive.h"
> +#include "lib/vswitch-idl.h"
> +#include "openvswitch/vlog.h"
> +#include "timeval.h"
> +
> +VLOG_DEFINE_THIS_MODULE(keepalive);
> +
> +static bool keepalive_enable = false; /* Keepalive disabled by default */
> +static bool ka_init_status = ka_init_failure; /* Keepalive initialization */
> +static uint32_t keepalive_timer_interval; /* keepalive timer interval */
> +static struct keepalive_info *ka_info = NULL;
> +
> +inline bool
> +ka_is_enabled(void)
> +{
> + return keepalive_enable;
> +}
> +
> +inline int
> +ka_get_pmd_tid(unsigned core_idx)
> +{
> + if (ka_is_enabled()) {
> + return ka_info->thread_id[core_idx];
> + }
> +
> + return -EINVAL;
> +}
> +
> +void
> +ka_set_pmd_state_ts(unsigned core_id, enum keepalive_state state,
> + uint64_t last_alive)
> +{
> + struct ka_process_info *pinfo;
> + int tid = ka_get_pmd_tid(core_id);
> +
> + ovs_mutex_lock(&ka_info->proclist_mutex);
> + HMAP_FOR_EACH_WITH_HASH (pinfo, node, hash_int(tid, 0),
> + &ka_info->process_list) {
> + if ((pinfo->core_id == core_id) && (pinfo->tid == tid)) {
> + pinfo->core_state = state;
> + pinfo->core_last_seen_times = last_alive;
> + }
> + }
> + ovs_mutex_unlock(&ka_info->proclist_mutex);
> +}
> +
> +/* Retrieve and return the keepalive timer interval from OVSDB. */
> +static uint32_t
> +ka_get_timer_interval(const struct smap *ovs_other_config OVS_UNUSED)
> +{
> +#define OVS_KEEPALIVE_TIMEOUT 1000 /* Default timeout set to 1000ms */
> + uint32_t ka_interval;
> +
> + /* Timer granularity in milliseconds
> + * Defaults to OVS_KEEPALIVE_TIMEOUT(ms) if not set */
> + ka_interval = smap_get_int(ovs_other_config, "keepalive-interval",
> + OVS_KEEPALIVE_TIMEOUT);
> +
> + VLOG_INFO("Keepalive timer interval set to %"PRIu32" (ms)\n",
> ka_interval);
> + return ka_interval;
> +}
> +
> +/*
> + * This function shall be invoked periodically to write the core status and
> + * last seen timestamp of the cores in to keepalive info structure.
> + */
> +void
> +ka_update_core_state(const int core_id, const enum keepalive_state core_state,
> + uint64_t last_alive, void *ptr_data OVS_UNUSED)
> +{
> + switch (core_state) {
> + case KA_STATE_ALIVE:
> + case KA_STATE_MISSING:
> + ka_set_pmd_state_ts(core_id, KA_STATE_ALIVE, last_alive);
> + break;
> + case KA_STATE_UNUSED:
> + case KA_STATE_DOZING:
> + case KA_STATE_SLEEP:
> + case KA_STATE_DEAD:
> + case KA_STATE_GONE:
> + ka_set_pmd_state_ts(core_id, core_state, last_alive);
> + break;
[Antonio]
Just for robustness - should core_state be corrupted for some reason -
I'd add the default switch case here,
something like?
+ default:
+ VLOG_DBG("Unexpected %d value for core_state.", core_state);
or alternatively defining a __KA_STATE_MAX and using OVS_NOT_REACHED()?
> + }
> +}
> +
> +static void
> +keepalive_register_relay_cb(ka_relay_cb cb, void *aux)
> +{
> + ka_info->relay_cb = cb;
> + ka_info->relay_cb_data = aux;
> +}
> +
> +static struct keepalive_info *
> +keepalive_info_create(void)
> +{
> + struct keepalive_info *ka_info;
> + ka_info = xzalloc(sizeof *ka_info);
> + if (ka_info != NULL) {
> + ka_info->init_time = time_wall_msec();
> + }
> +
> + return ka_info;
> +}
> +
> +static int
> +ka_init__(void)
> +{
> + if ((ka_info = keepalive_info_create()) == NULL) {
> + VLOG_ERR("OvS Keepalive - initialization failed.");
> + return -1;
> + }
> +
> + keepalive_register_relay_cb(ka_update_core_state, NULL);
> + ovs_mutex_init(&ka_info->proclist_mutex);
> + hmap_init(&ka_info->process_list);
> +
> + return 0;
> +}
> +
> +void
> +ka_init(const struct smap *ovs_other_config)
> +{
> + if (smap_get_bool(ovs_other_config, "enable-keepalive", false)) {
> + static struct ovsthread_once once_enable = OVSTHREAD_ONCE_INITIALIZER;
> +
> + if (ovsthread_once_start(&once_enable)) {
> + keepalive_enable = true;
> + VLOG_INFO("OvS Keepalive enabled.");
> +
> + keepalive_timer_interval =
> + ka_get_timer_interval(ovs_other_config);
> +
> + int err = ka_init__();
> + if (!err) {
> + VLOG_INFO("OvS Keepalive - initialized.");
> + ka_init_status = ka_init_success;
> + }
> +
> + ovsthread_once_done(&once_enable);
> + }
> + }
> +}
> +
> +void
> +ka_destroy(void)
> +{
> + if (ka_info) {
> + struct ka_process_info *pinfo;
> +
> + ovs_mutex_lock(&ka_info->proclist_mutex);
> + HMAP_FOR_EACH_POP (pinfo, node, &ka_info->process_list) {
> + free(pinfo);
> + }
> + ovs_mutex_unlock(&ka_info->proclist_mutex);
> +
> + hmap_destroy(&ka_info->process_list);
> + ovs_mutex_destroy(&ka_info->proclist_mutex);
> +
> + free(ka_info);
> + }
> +}
> diff --git a/lib/keepalive.h b/lib/keepalive.h
> new file mode 100644
> index 0000000..d133398
> --- /dev/null
> +++ b/lib/keepalive.h
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (c) 2016 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef KEEPALIVE_H
> +#define KEEPALIVE_H
> +
> +#include <stdint.h>
> +#include "openvswitch/hmap.h"
> +#include "ovs-thread.h"
> +
> +#define KA_DP_MAXCORES 128
> +
> +struct smap;
> +
> +enum keepalive_state {
> + KA_STATE_UNUSED = 0,
> + KA_STATE_ALIVE = 1,
> + KA_STATE_MISSING = 4,
> + KA_STATE_DEAD = 2,
> + KA_STATE_GONE = 3,
> + KA_STATE_DOZING = 5,
> + KA_STATE_SLEEP = 6,
> +};
> +
> +typedef void (*ka_relay_cb)(int, enum keepalive_state, uint64_t, void *);
> +
> +struct ka_process_info {
> + int tid;
> + int core_id;
> + enum keepalive_state core_state;
> + uint64_t core_last_seen_times;
> + struct hmap_node node;
> +};
> +
> +struct keepalive_info {
> + /* Mutex for 'process_list'. */
> + struct ovs_mutex proclist_mutex;
> +
> + /* List of process/threads monitored by KA framework. */
> + struct hmap process_list OVS_GUARDED;
> +
> + /* Store Datapath threads 'tid'.
> + * In case of DPDK there can be max of KA_DP_MAXCORES threads. */
> + pid_t thread_id[KA_DP_MAXCORES];
> +
> + /* Datapath cores state. */
> + enum keepalive_state state_flags[KA_DP_MAXCORES];
> +
> + uint64_t init_time;
> + /* Last seen timestamps. */
> + uint64_t last_alive[KA_DP_MAXCORES];
> +
> + /* Active cores (non-zero if the core should be monitored). */
> + uint8_t active_cores[KA_DP_MAXCORES];
> +
> + /** Relay handler. */
> + ka_relay_cb relay_cb;
> + void *relay_cb_data;
> +};
> +
> +enum keepalive_status {
> + ka_init_failure = 0,
> + ka_init_success
> +};
> +
> +void ka_init(const struct smap *);
> +void ka_destroy(void);
> +void ka_set_pmd_state_ts(unsigned, enum keepalive_state, uint64_t);
> +void ka_update_core_state(const int, const enum keepalive_state,
> + uint64_t, void *);
> +
> +int ka_get_pmd_tid(unsigned core);
> +bool ka_is_enabled(void);
> +#endif /* keepalive.h */
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index a8cbae7..aaa8d9b 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -34,6 +34,7 @@
> #include "hmapx.h"
> #include "if-notifier.h"
> #include "jsonrpc.h"
> +#include "keepalive.h"
> #include "lacp.h"
> #include "mac-learning.h"
> #include "mcast-snooping.h"
> @@ -506,6 +507,7 @@ bridge_exit(bool delete_datapath)
> bridge_destroy(br, delete_datapath);
> }
> ovsdb_idl_destroy(idl);
> + ka_destroy();
> }
>
> /* Looks at the list of managers in 'ovs_cfg' and extracts their remote IP
> @@ -2953,6 +2955,7 @@ bridge_run(void)
> if (cfg) {
> netdev_set_flow_api_enabled(&cfg->other_config);
> dpdk_init(&cfg->other_config);
> + ka_init(&cfg->other_config);
> }
>
> /* Initialize the ofproto library. This only needs to run once, but
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 90e50b6..c56a64c 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
> {"name": "Open_vSwitch",
> - "version": "7.15.1",
> - "cksum": "3682332033 23608",
> + "version": "7.16.0",
> + "cksum": "3631938350 23762",
> "tables": {
> "Open_vSwitch": {
> "columns": {
> @@ -30,6 +30,10 @@
> "type": {"key": "string", "value": "string",
> "min": 0, "max": "unlimited"},
> "ephemeral": true},
> + "keepalive": {
> + "type": {"key": "string", "value": "string", "min": 0,
> + "max": "unlimited"},
> + "ephemeral": true},
> "ovs_version": {
> "type": {"key": {"type": "string"},
> "min": 0, "max": 1}},
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 074535b..4302fd5 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -568,6 +568,55 @@
> </p>
> </column>
> </group>
> +
> + <group title="Keepalive">
> + <p>
> + The <code>keepalive</code> column contains key-value pairs that
> + report health of datapath threads in Open vSwitch. These are
> updated
> + periodically (based on the keepalive-interval).
> + </p>
> +
> + <column name="other_config" key="enable-keepalive"
> + type='{"type": "boolean"}'>
> + Keepalive is disabled by default to avoid overhead in the common
> + case when heartbeat monitoring is not useful. Set this value to
> + <code>true</code> to enable keepalive <ref column="keepalive"/>
> + column or to <code>false</code> to explicitly disable it.
> + </column>
> +
> + <column name="other_config" key="keepalive-interval"
> + type='{"type": "integer", "minInteger": 1}'>
> + <p>
> + Specifies the keepalive interval value.
> + </p>
> + <p>
> + If not specified, this will be set to 1000 milliseconds (default
> + value). Changing this value requires restarting the daemon.
> + </p>
> + </column>
> +
> + <column name="keepalive" key="PMD_ID">
> + <p>
> + One such key-value pair, with <code>ID</code> replaced by the
> + PMD thread, will exist for each active PMD thread. The value is a
> + comma-separated list of PMD thread status, core number and the
> + last seen timestamp of PMD thread. In respective order, these
[Antonio]
Nit: one missing space
last seen timestamp of PMD thread. In respective order, these
> + values are:
> + </p>
> +
> + <ol>
> + <li>Status of PMD thread. Valid values include ALIVE, MISSING,
> + DEAD, GONE, DOZING, SLEEPING.</li>
[Antonio]
Nit: should SLEEPING be changed into SLEEP as it is referred all over the code?
This is not so much for esthetics as for a cautious approach with the
state machine names and any future changes.
> + <li>Core id of PMD thread.</li>
> + <li>Last seen timestamp(epoch) of the PMD core.</li>
> + </ol>
> +
> + <p>
> + This is only valid for OvS-DPDK Datapath and PMD threads status
> + is implemented currently.
> + </p>
> + </column>
> + </group>
> </group>
>
> <group title="Version Reporting">
> --
> 2.4.11
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list