From f199a6b3e58789fe337134c7c32a27565dabb614 Mon Sep 17 00:00:00 2001 From: "Anand H. Krishnan" Date: Fri, 3 Apr 2015 16:16:03 +0530 Subject: [PATCH] Return error to agent if an entry already existed in the flow table It is logically possible that agent and datapath are trying to create same flow simultaneously. If it so happens that agent gets the entry that datapath created and tries to update that entry assuming that the entry was created by it, then the hold count will never be compensated by a corresponding acted count, and hence vrouter's perception of the number of active hold entries can go wrong. To fix this, return error to agent if the flow it tried to create already existed. Other fixes: . If agent is changing the flow state to 'hold' from any other state, update the hold count entry. . Export the hold count statistics to 'flow' utility Change-Id: I24087baa5bf853b863f34e1b55882927d9114349 Partial-BUG: #1439069 --- dp-core/vr_flow.c | 68 +++++++++++++++++++++++++++++++++++++++++--- dp-core/vr_sandesh.c | 3 +- include/vr_flow.h | 2 ++ include/vrouter.h | 1 + linux/vrouter_mod.c | 7 +++++ sandesh/vr.sandesh | 6 ++++ utils/flow.c | 33 ++++++++++++++++++++- 7 files changed, 114 insertions(+), 6 deletions(-) diff --git a/dp-core/vr_flow.c b/dp-core/vr_flow.c index 680365d33..5ceefedce 100644 --- a/dp-core/vr_flow.c +++ b/dp-core/vr_flow.c @@ -44,6 +44,7 @@ struct vr_btable *vr_oflow_table; * is set by somebody and passed to agent for it to map */ unsigned char *vr_flow_path; +unsigned int vr_flow_hold_limit = 1; #if defined(__linux__) && defined(__KERNEL__) extern unsigned short vr_flow_major; @@ -700,9 +701,16 @@ vr_flow_entry_set_hold(struct vrouter *router, struct vr_flow_entry *flow_e) struct vr_flow_table_info *infop = router->vr_flow_table_info; cpu = vr_get_cpu(); + if (cpu >= vr_num_cpus) { + vr_printf("vrouter: Set HOLD failed (cpu %u num_cpus %u)\n", + cpu, vr_num_cpus); + return; + } + flow_e->fe_action = VR_FLOW_ACTION_HOLD; if (infop->vfti_hold_count[cpu] + 1 < infop->vfti_hold_count[cpu]) { + (void)__sync_add_and_fetch(&infop->vfti_oflows, 1); act_count = infop->vfti_action_count; if (act_count > infop->vfti_hold_count[cpu]) { (void)__sync_sub_and_fetch(&infop->vfti_action_count, @@ -735,7 +743,9 @@ vr_flow_lookup(struct vrouter *router, struct vr_flow *key, (pkt->vp_nh->nh_flags & NH_FLAG_RELAXED_POLICY)) return FLOW_FORWARD; - if (vr_flow_table_hold_count(router) > VR_MAX_FLOW_TABLE_HOLD_COUNT) { + if ((vr_flow_hold_limit) && + (vr_flow_table_hold_count(router) > + VR_MAX_FLOW_TABLE_HOLD_COUNT)) { vr_pfree(pkt, VP_DROP_FLOW_UNUSABLE); return FLOW_CONSUMED; } @@ -972,9 +982,13 @@ vr_add_flow(unsigned int rid, struct vr_flow *key, uint8_t type, struct vrouter *router = vrouter_get(rid); flow_e = vr_find_flow(router, key, type, fe_index); - if (!flow_e) + if (flow_e) { + /* a race between agent and dp. allow agent to handle this error */ + return NULL; + } else { flow_e = vr_find_free_entry(router, key, type, need_hold_queue, fe_index); + } return flow_e; } @@ -1135,6 +1149,7 @@ vr_flow_set(struct vrouter *router, vr_flow_req *req) { int ret; unsigned int fe_index; + struct vr_flow_entry *fe = NULL; struct vr_flow_table_info *infop = router->vr_flow_table_info; @@ -1170,6 +1185,8 @@ vr_flow_set(struct vrouter *router, vr_flow_req *req) fe = vr_add_flow_req(req, &fe_index); if (!fe) return -ENOSPC; + + infop->vfti_added++; } else { if ((req->fr_action == VR_FLOW_ACTION_HOLD) && (fe->fe_action != req->fr_action)) { @@ -1210,10 +1227,18 @@ vr_flow_set(struct vrouter *router, vr_flow_req *req) fe->fe_ecmp_nh_index = req->fr_ecmp_nh_index; fe->fe_src_nh_index = req->fr_src_nh_index; - fe->fe_action = req->fr_action; + + if ((req->fr_action == VR_FLOW_ACTION_HOLD) && + (fe->fe_action != VR_FLOW_ACTION_HOLD)) { + vr_flow_entry_set_hold(router, fe); + } else { + fe->fe_action = req->fr_action; + } + if (fe->fe_action == VR_FLOW_ACTION_DROP) fe->fe_drop_reason = (uint8_t)req->fr_drop_reason; - fe->fe_flags = req->fr_flags; + + fe->fe_flags = req->fr_flags; vr_flow_udp_src_port(router, fe); return vr_flow_schedule_transition(router, req, fe); @@ -1230,6 +1255,12 @@ vr_flow_req_destroy(vr_flow_req *req) req->fr_file_path = NULL; } + if (req->fr_hold_stat && req->fr_hold_stat_size) { + vr_free(req->fr_hold_stat); + req->fr_hold_stat = NULL; + req->fr_hold_stat_size = 0; + } + vr_free(req); return; @@ -1238,6 +1269,7 @@ vr_flow_req_destroy(vr_flow_req *req) vr_flow_req * vr_flow_req_get(vr_flow_req *ref_req) { + unsigned int hold_stat_size = vr_num_cpus * sizeof(uint32_t); vr_flow_req *req = vr_zalloc(sizeof(*req)); if (!req) @@ -1258,6 +1290,18 @@ vr_flow_req_get(vr_flow_req *ref_req) } } + req->fr_hold_stat = vr_zalloc(hold_stat_size); + if (!req->fr_hold_stat) { + if (vr_flow_path && req->fr_file_path) { + vr_free(req->fr_file_path); + req->fr_file_path = NULL; + } + + vr_free(req); + return NULL; + } + req->fr_hold_stat_size = hold_stat_size; + return req; } @@ -1268,7 +1312,10 @@ void vr_flow_req_process(void *s_req) { int ret = 0; + unsigned int i; bool need_destroy = false; + uint64_t hold_count = 0; + struct vrouter *router; vr_flow_req *req = (vr_flow_req *)s_req; vr_flow_req *resp = NULL; @@ -1293,6 +1340,19 @@ vr_flow_req_process(void *s_req) strncpy(resp->fr_file_path, vr_flow_path, VR_UNIX_PATH_MAX - 1); } + resp->fr_processed = router->vr_flow_table_info->vfti_action_count; + resp->fr_hold_oflows = router->vr_flow_table_info->vfti_oflows; + resp->fr_added = router->vr_flow_table_info->vfti_added; + resp->fr_cpus = vr_num_cpus; + /* we only have space for 64 stats block max when encoding */ + for (i = 0; ((i < vr_num_cpus) && (i < 64)); i++) { + resp->fr_hold_stat[i] = + router->vr_flow_table_info->vfti_hold_count[i]; + hold_count += resp->fr_hold_stat[i]; + } + + resp->fr_created = hold_count; + break; case FLOW_OP_FLOW_SET: diff --git a/dp-core/vr_sandesh.c b/dp-core/vr_sandesh.c index 7c79ea12d..a088abeb6 100644 --- a/dp-core/vr_sandesh.c +++ b/dp-core/vr_sandesh.c @@ -34,7 +34,8 @@ struct sandesh_object_md sandesh_md[] = { .obj_type_string = "vr_mirror_req", }, [VR_FLOW_OBJECT_ID] = { - .obj_len = 4 * sizeof(vr_flow_req), + .obj_len = ((4 * sizeof(vr_flow_req)) + + (64 * sizeof(unsigned int))), .obj_type_string = "vr_flow_req", }, [VR_VRF_ASSIGN_OBJECT_ID] = { diff --git a/include/vr_flow.h b/include/vr_flow.h index 4b1fcb06f..b360615c6 100644 --- a/include/vr_flow.h +++ b/include/vr_flow.h @@ -123,6 +123,8 @@ struct vr_flow { */ struct vr_flow_table_info { uint64_t vfti_action_count; + uint64_t vfti_added; + uint32_t vfti_oflows; uint32_t vfti_hold_count[0]; }; diff --git a/include/vrouter.h b/include/vrouter.h index dd8f71d5e..e59b96a81 100644 --- a/include/vrouter.h +++ b/include/vrouter.h @@ -35,6 +35,7 @@ extern int vr_perfq1, vr_perfq2, vr_perfq3; extern int vr_from_vm_mss_adj; extern int vr_to_vm_mss_adj; extern int vr_udp_coff; +extern unsigned int vr_flow_hold_limit; extern int vr_use_linux_br; extern int hashrnd_inited; extern uint32_t vr_hashrnd; diff --git a/linux/vrouter_mod.c b/linux/vrouter_mod.c index 050f9a235..927dcdb7f 100644 --- a/linux/vrouter_mod.c +++ b/linux/vrouter_mod.c @@ -2514,6 +2514,13 @@ static struct ctl_table vrouter_table[] = .mode = 0644, .proc_handler = proc_dointvec, }, + { + .procname = "flow_hold_limit", + .data = &vr_flow_hold_limit, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = proc_dointvec, + }, {} }; diff --git a/sandesh/vr.sandesh b/sandesh/vr.sandesh index 6987593fa..2107765a1 100644 --- a/sandesh/vr.sandesh +++ b/sandesh/vr.sandesh @@ -150,6 +150,12 @@ buffer sandesh vr_flow_req { 24: i16 fr_flow_nh_id; 25: i16 fr_drop_reason; 26: string fr_file_path; + 27: i64 fr_processed; + 28: i64 fr_created; + 29: i64 fr_added; + 30: i32 fr_cpus; + 31: i32 fr_hold_oflows; + 32: list fr_hold_stat; } buffer sandesh vr_vrf_assign_req { diff --git a/utils/flow.c b/utils/flow.c index aee33375d..da73684c0 100644 --- a/utils/flow.c +++ b/utils/flow.c @@ -59,8 +59,14 @@ struct flow_table { struct vr_flow_entry *ft_entries; u_int64_t ft_entries_p; u_int64_t ft_span; + u_int64_t ft_processed; + u_int64_t ft_created; + u_int64_t ft_added; unsigned int ft_num_entries; unsigned int ft_flags; + unsigned int ft_cpus; + unsigned int ft_hold_oflows; + u_int32_t ft_hold_stat[64]; char flow_table_path[256]; } main_table; @@ -145,8 +151,20 @@ dump_table(struct flow_table *ft) const char *drop_reason = NULL; struct in_addr in_src, in_dest; - printf("Flow table\n\n"); + printf("Flow table(size %lu, entries %u)\n\n", ft->ft_span, + ft->ft_num_entries); + printf("Entries: Created %lu Added %lu Processed %lu\n", + ft->ft_created, ft->ft_added, ft->ft_processed); + printf("(Created Flows/CPU: "); + for (i = 0; i < ft->ft_cpus; i++) { + printf("%u", ft->ft_hold_stat[i]); + if (i != (ft->ft_cpus - 1)) + printf(" "); + } + printf(")(oflows %u)\n\n", ft->ft_hold_oflows); + dump_legend(); + printf(" Index Source:Port Destination:Port \tProto(V)\n"); printf("-----------------------------------------------------------------"); printf("--------\n"); @@ -458,6 +476,7 @@ int flow_table_map(vr_flow_req *req) { int ret; + unsigned int i; struct flow_table *ft = &main_table; const char *flow_path; @@ -495,6 +514,18 @@ flow_table_map(vr_flow_req *req) ft->ft_span = req->fr_ftable_size; ft->ft_num_entries = ft->ft_span / sizeof(struct vr_flow_entry); + ft->ft_processed = req->fr_processed; + ft->ft_created = req->fr_created; + ft->ft_hold_oflows = req->fr_hold_oflows; + ft->ft_added = req->fr_added; + ft->ft_cpus = req->fr_cpus; + + if (req->fr_hold_stat && req->fr_hold_stat_size) { + for (i = 0; i < ft->ft_cpus; i++) { + ft->ft_hold_stat[i] = req->fr_hold_stat[i]; + } + } + return ft->ft_num_entries; }