From 45776e57a15166e56c0ddfa96740eeb6ef938a58 Mon Sep 17 00:00:00 2001 From: Divakar Date: Mon, 25 Jul 2016 15:33:01 +0530 Subject: [PATCH] Removing ref counting for Mirror entry Flow and VIF hold Mirroring index rather reference to mirroring entry. Though only index is used, the ref counting is used to delete the mirroring entry when Agent deletes it. This is unncecessary as no other data structure is holding a reference to this. Also the current ref counting is not atomic and is leading to wrong ref counting, leading to mirroring entry's wrong deletion. As a fix, the ref counting is remvoed for mirroring. When Agent deletes the entry, it is removed from Mirroring database. While mirroring if mirroring entry is not found the packet just does not get mirrored. closes-bug: #1605874 Change-Id: Id6bce2425465402a456a989a72a5f142cf829e83 --- dp-core/vr_flow.c | 4 -- dp-core/vr_mirror.c | 123 +++++++++++++++++--------------------------- include/vr_mirror.h | 7 ++- utils/mirror.c | 7 +-- 4 files changed, 52 insertions(+), 89 deletions(-) diff --git a/dp-core/vr_flow.c b/dp-core/vr_flow.c index 0a1f2f17d..bbdc99fd1 100644 --- a/dp-core/vr_flow.c +++ b/dp-core/vr_flow.c @@ -176,9 +176,7 @@ vr_flow_reset_mirror(struct vrouter *router, struct vr_flow_entry *fe, unsigned int index) { if (fe->fe_flags & VR_FLOW_FLAG_MIRROR) { - vrouter_put_mirror(router, fe->fe_mirror_id); fe->fe_mirror_id = VR_MAX_MIRROR_INDICES; - vrouter_put_mirror(router, fe->fe_sec_mirror_id); fe->fe_sec_mirror_id = VR_MAX_MIRROR_INDICES; vr_mirror_meta_entry_del(router, index); } @@ -1556,7 +1554,6 @@ vr_flow_set_mirror(struct vrouter *router, vr_flow_req *req, if (fe->fe_mirror_id != req->fr_mir_id) { if (fe->fe_mirror_id < router->vr_max_mirror_indices) { - vrouter_put_mirror(router, fe->fe_mirror_id); fe->fe_mirror_id = router->vr_max_mirror_indices; } @@ -1575,7 +1572,6 @@ vr_flow_set_mirror(struct vrouter *router, vr_flow_req *req, if (fe->fe_sec_mirror_id != req->fr_sec_mir_id) { if (fe->fe_sec_mirror_id < router->vr_max_mirror_indices) { - vrouter_put_mirror(router, fe->fe_sec_mirror_id); fe->fe_sec_mirror_id = router->vr_max_mirror_indices; } diff --git a/dp-core/vr_mirror.c b/dp-core/vr_mirror.c index bf2048c8b..2aafc4a1d 100644 --- a/dp-core/vr_mirror.c +++ b/dp-core/vr_mirror.c @@ -10,11 +10,8 @@ #include "vr_message.h" #include "vr_mirror.h" -int vr_mirror_add(vr_mirror_req *); -int vr_mirror_del(vr_mirror_req *); - -static struct vr_mirror_entry * -__vrouter_get_mirror(unsigned int rid, unsigned int index) +struct vr_mirror_entry * +vrouter_get_mirror(unsigned int rid, unsigned int index) { struct vrouter *router = vrouter_get(rid); @@ -24,22 +21,22 @@ __vrouter_get_mirror(unsigned int rid, unsigned int index) return router->vr_mirrors[index]; } -struct vr_mirror_entry * -vrouter_get_mirror(unsigned int rid, unsigned int index) +static void +vr_mirror_defer_delete(struct vrouter *router, void *arg) { - struct vr_mirror_entry *mirror; + struct vr_defer_data *defer = (struct vr_defer_data *)arg; - mirror = __vrouter_get_mirror(rid, index); - if (mirror) - mirror->mir_users++; + vr_free(defer->vdd_data, VR_MIRROR_OBJECT); - return mirror; + return; } int -vrouter_put_mirror(struct vrouter *router, unsigned int index) +__vr_mirror_del(struct vrouter *router, unsigned int index) { + struct vr_nexthop *nh; struct vr_mirror_entry *mirror; + struct vr_defer_data *defer; if (index >= router->vr_max_mirror_indices) return -EINVAL; @@ -48,83 +45,48 @@ vrouter_put_mirror(struct vrouter *router, unsigned int index) if (!mirror) return -EINVAL; - if (!--mirror->mir_users) { - router->vr_mirrors[index] = NULL; - - if (!vr_not_ready) + nh = mirror->mir_nh; + router->vr_mirrors[index] = NULL; + mirror->mir_nh = NULL; + + if (!vr_not_ready) { + defer = vr_get_defer_data(sizeof(*defer)); + if (defer) { + defer->vdd_data = (void *)mirror; + vr_defer(router, vr_mirror_defer_delete, (void *)defer); + } else { vr_delay_op(); - - vrouter_put_nexthop(mirror->mir_nh); + vr_free(mirror, VR_MIRROR_OBJECT); + } + } else { vr_free(mirror, VR_MIRROR_OBJECT); } + vrouter_put_nexthop(nh); return 0; } - int vr_mirror_del(vr_mirror_req *req) { - int ret = 0; + int ret = -EINVAL; struct vrouter *router; - struct vr_mirror_entry *mirror; - struct vr_nexthop *nh; router = vrouter_get(req->mirr_rid); - if (!router) { - ret = -EINVAL; - goto generate_resp; - } - - mirror = __vrouter_get_mirror(req->mirr_rid, req->mirr_index); - if (!mirror) { - ret = -EINVAL; - goto generate_resp; - } - - mirror->mir_flags |= VR_MIRROR_FLAG_MARKED_DELETE; - nh = mirror->mir_nh; - mirror->mir_nh = vrouter_get_nexthop(req->mirr_rid, NH_DISCARD_ID); - /* release the old nexthop */ - vrouter_put_nexthop(nh); + if (router) + ret = __vr_mirror_del(router, req->mirr_index); - /* ...and finally try to release the mirror entry */ - vrouter_put_mirror(router, req->mirr_index); - -generate_resp: vr_send_response(ret); return ret; } -static int -vr_mirror_change(struct vr_mirror_entry *mirror, vr_mirror_req *req, - struct vr_nexthop *nh_new) -{ - struct vr_nexthop *nh_old = mirror->mir_nh; - - if (mirror->mir_flags & VR_MIRROR_FLAG_MARKED_DELETE) - mirror->mir_users++; - - mirror->mir_flags = req->mirr_flags; - mirror->mir_flags &= ~VR_MIRROR_FLAG_MARKED_DELETE; - mirror->mir_rid = req->mirr_rid; - mirror->mir_vni = req->mirr_vni; - if (nh_old != nh_new) { - mirror->mir_nh = nh_new; - if (nh_old) - vrouter_put_nexthop(nh_old); - } - - return 0; -} - int vr_mirror_add(vr_mirror_req *req) { - struct vrouter *router; - struct vr_nexthop *nh; int ret = 0; + struct vrouter *router; + struct vr_nexthop *nh, *old_nh = NULL; struct vr_mirror_entry *mirror; router = vrouter_get(req->mirr_rid); @@ -138,22 +100,33 @@ vr_mirror_add(vr_mirror_req *req) goto generate_resp; } - req->mirr_flags &= ~VR_MIRROR_FLAG_MARKED_DELETE; - nh = vrouter_get_nexthop(req->mirr_rid, req->mirr_nhid); if (!nh) { ret = -EINVAL; goto generate_resp; } - mirror = __vrouter_get_mirror(req->mirr_rid, req->mirr_index); + mirror = router->vr_mirrors[req->mirr_index]; if (!mirror) { mirror = vr_zalloc(sizeof(*mirror), VR_MIRROR_OBJECT); - mirror->mir_users++; + if (!mirror) { + ret = -ENOMEM; + vrouter_put_nexthop(nh); + goto generate_resp; + } + } else { + old_nh = mirror->mir_nh; } - ret = vr_mirror_change(mirror, req, nh); + + mirror->mir_nh = nh; + mirror->mir_rid = req->mirr_rid; + mirror->mir_flags = req->mirr_flags; + mirror->mir_vni = req->mirr_vni; router->vr_mirrors[req->mirr_index] = mirror; + if (old_nh) + vrouter_put_nexthop(old_nh); + generate_resp: vr_send_response(ret); @@ -169,7 +142,6 @@ vr_mirror_make_req(vr_mirror_req *req, struct vr_mirror_entry *mirror, if (mirror->mir_nh) req->mirr_nhid = mirror->mir_nh->nh_id; - req->mirr_users = mirror->mir_users; req->mirr_flags = mirror->mir_flags; req->mirr_rid = mirror->mir_rid; req->mirr_vni = mirror->mir_vni; @@ -226,12 +198,11 @@ vr_mirror_get(vr_mirror_req *req) (unsigned int)req->mirr_index >= router->vr_max_mirror_indices) { ret = -ENODEV; } else { - mirror = __vrouter_get_mirror(req->mirr_rid, req->mirr_index); + mirror = router->vr_mirrors[req->mirr_index]; if (!mirror) ret = -ENOENT; } - if (mirror) { vr_mirror_make_req(req, mirror, req->mirr_index); } else @@ -541,7 +512,7 @@ vr_mirror_exit(struct vrouter *router, bool soft_reset) if (router->vr_mirrors) for (i = 0; i < router->vr_max_mirror_indices; i++) if (router->vr_mirrors[i]) - vrouter_put_mirror(router, i); + __vr_mirror_del(router, i); if (router->vr_mirror_md) { vr_itable_delete(router->vr_mirror_md, diff --git a/include/vr_mirror.h b/include/vr_mirror.h index c6402184a..e95a50ce5 100644 --- a/include/vr_mirror.h +++ b/include/vr_mirror.h @@ -5,8 +5,8 @@ #define __VR_MIRROR_H__ #define VR_MAX_MIRROR_INDICES 255 -#define VR_MIRROR_FLAG_MARKED_DELETE 0x1 -#define VR_MIRROR_FLAG_DYNAMIC 0x2 + +#define VR_MIRROR_FLAG_DYNAMIC 0x1 struct vrouter; @@ -29,10 +29,9 @@ typedef enum { struct vr_mirror_entry { - unsigned int mir_users:20; - unsigned int mir_flags:12; unsigned int mir_rid; int mir_vni; + unsigned int mir_flags; struct vr_nexthop *mir_nh; }; diff --git a/utils/mirror.c b/utils/mirror.c index e3d05aa3f..72bfa9fd2 100644 --- a/utils/mirror.c +++ b/utils/mirror.c @@ -43,12 +43,9 @@ vr_mirror_req_process(void *s_req) memset(flags, 0, sizeof(flags)); if (req->mirr_flags & VR_MIRROR_FLAG_DYNAMIC) strcat(flags, "D"); - if (req->mirr_flags & VR_MIRROR_FLAG_MARKED_DELETE) - strcat(flags, "Md"); printf("%5d %7d", req->mirr_index, req->mirr_nhid); printf(" %4s", flags); - printf(" %7u", req->mirr_users); if (req->mirr_vni != -1) printf(" %7d", req->mirr_vni); printf("\n"); @@ -311,8 +308,8 @@ int main(int argc, char *argv[]) if ((mirror_op == SANDESH_OP_DUMP) || (mirror_op == SANDESH_OP_GET)) { printf("Mirror Table\n\n"); - printf("Flags:D=Dynamic Mirroring, Dm:Delete Marked\n\n"); - printf("Index NextHop Flags References VNI\n"); + printf("Flags:D=Dynamic Mirroring \n\n"); + printf("Index NextHop Flags VNI\n"); printf("------------------------------------------------\n"); }