From 5578590086b49586c7967ad75da0e58e28dcf368 Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Tue, 9 May 2017 11:30:43 -0700 Subject: [PATCH 1/5] pimd: Avoid deleting SGRpt entry from PP->P state -Upon Receving SGRpt Prune message, transitioning from Prune Pending state to NOINFO state, ifchannel entry was getting deleted in prune pending timer expiry. This can result in SGRpt ifhchannel deleted and recreated upon receving triggered or periodic SGRpt received from downstream. The automation test failed as it expected (check) SGRpt entry at RP after it triggers SPT switchover. - While transitioning from Prune-Pending state to NOINFO(Pruned) state, Trigger SGRpt message towards RP. - Add/del some of the debug traces Ticket:CM-16057 Reviewed By:CCR-6198 Testing Done: Rerun test08 multiple times and observed passing it. Pim-smoke with hardnode Ran 95 tests in 11219.420s FAILED (SKIP=10, failures=4) Signed-off-by: Chirag Shah --- pimd/pim_ifchannel.c | 49 +++++++++++++++++------ pimd/pim_nht.c | 6 +-- pimd/pim_upstream.c | 95 +++++++++++++++++++++++++------------------- pimd/pim_upstream.h | 2 +- 4 files changed, 96 insertions(+), 56 deletions(-) diff --git a/pimd/pim_ifchannel.c b/pimd/pim_ifchannel.c index f4fe609605..04f8a22c6b 100644 --- a/pimd/pim_ifchannel.c +++ b/pimd/pim_ifchannel.c @@ -148,7 +148,6 @@ void pim_ifchannel_delete(struct pim_ifchannel *ch) /* SGRpt entry could have empty oil */ if (ch->upstream->channel_oil) pim_channel_del_oif (ch->upstream->channel_oil, ch->interface, mask); - pim_channel_del_oif (ch->upstream->channel_oil, ch->interface, mask); /* * Do we have any S,G's that are inheriting? * Nuke from on high too. @@ -179,6 +178,10 @@ void pim_ifchannel_delete(struct pim_ifchannel *ch) pim_upstream_update_join_desired(ch->upstream); } + /* upstream is common across ifchannels, check if upstream's + ifchannel list is empty before deleting upstream_del + ref count will take care of it. + */ pim_upstream_del(ch->upstream, __PRETTY_FUNCTION__); ch->upstream = NULL; @@ -200,6 +203,9 @@ void pim_ifchannel_delete(struct pim_ifchannel *ch) hash_release(pim_ifp->pim_ifchannel_hash, ch); listnode_delete(pim_ifchannel_list, ch); + if (PIM_DEBUG_PIM_TRACE) + zlog_debug ("%s: ifchannel entry %s is deleted ", __PRETTY_FUNCTION__, ch->sg_str); + pim_ifchannel_free(ch); } @@ -571,14 +577,18 @@ pim_ifchannel_add(struct interface *ifp, listnode_add_sort(up->ifchannels, ch); + if (PIM_DEBUG_PIM_TRACE) + zlog_debug ("%s: ifchannel %s is created ", __PRETTY_FUNCTION__, ch->sg_str); + return ch; } -static void ifjoin_to_noinfo(struct pim_ifchannel *ch) +static void ifjoin_to_noinfo(struct pim_ifchannel *ch, uint8_t ch_del) { pim_forward_stop(ch); pim_ifchannel_ifjoin_switch(__PRETTY_FUNCTION__, ch, PIM_IFJOIN_NOINFO); - delete_on_noinfo(ch); + if (ch_del) + delete_on_noinfo(ch); } static int on_ifjoin_expiry_timer(struct thread *t) @@ -589,7 +599,7 @@ static int on_ifjoin_expiry_timer(struct thread *t) ch->t_ifjoin_expiry_timer = NULL; - ifjoin_to_noinfo(ch); + ifjoin_to_noinfo(ch, 1); /* ch may have been deleted */ return 0; @@ -613,10 +623,6 @@ static int on_ifjoin_prune_pending_timer(struct thread *t) pim_ifp = ifp->info; send_prune_echo = (listcount(pim_ifp->pim_neighbor_list) > 1); - //ch->ifjoin_state transition to NOINFO - ifjoin_to_noinfo(ch); - /* from here ch may have been deleted */ - if (send_prune_echo) { struct pim_rpf rpf; @@ -625,6 +631,23 @@ static int on_ifjoin_prune_pending_timer(struct thread *t) rpf.rpf_addr.u.prefix4 = pim_ifp->primary_address; pim_jp_agg_single_upstream_send(&rpf, ch->upstream, 0); } + /* If SGRpt flag is set on ifchannel, Trigger SGRpt + message on RP path upon prune timer expiry. + */ + if (PIM_IF_FLAG_TEST_S_G_RPT (ch->flags)) + { + if (ch->upstream) + pim_upstream_update_join_desired(ch->upstream); + /* + ch->ifjoin_state transition to NOINFO state + ch_del is set to 0 for not deleteing from here. + Holdtime expiry (ch_del set to 1) delete the entry. + */ + ifjoin_to_noinfo(ch, 0); + } + else + ifjoin_to_noinfo(ch, 1); + /* from here ch may have been deleted */ } else { @@ -801,7 +824,7 @@ void pim_ifchannel_join_add(struct interface *ifp, (ch->upstream->parent->flags & PIM_UPSTREAM_FLAG_MASK_SRC_IGMP) && !(ch->upstream->flags & PIM_UPSTREAM_FLAG_MASK_SRC_LHR)) { - pim_upstream_ref (ch->upstream, PIM_UPSTREAM_FLAG_MASK_SRC_LHR); + pim_upstream_ref (ch->upstream, PIM_UPSTREAM_FLAG_MASK_SRC_LHR, __PRETTY_FUNCTION__); pim_upstream_keep_alive_timer_start (ch->upstream, qpim_keep_alive_time); } break; @@ -895,8 +918,10 @@ void pim_ifchannel_prune(struct interface *ifp, case PIM_IFJOIN_NOINFO: if (source_flags & PIM_ENCODE_RPT_BIT) { - PIM_IF_FLAG_SET_S_G_RPT(ch->flags); - ch->ifjoin_state = PIM_IFJOIN_PRUNE_PENDING; + if (!(source_flags & PIM_ENCODE_WC_BIT)) + PIM_IF_FLAG_SET_S_G_RPT(ch->flags); + + ch->ifjoin_state = PIM_IFJOIN_PRUNE_PENDING; if (listcount(pim_ifp->pim_neighbor_list) > 1) jp_override_interval_msec = pim_if_jp_override_interval_msec(ifp); else @@ -1306,7 +1331,7 @@ pim_ifchannel_set_star_g_join_state (struct pim_ifchannel *ch, int eom, uint8_t if (up) { if (PIM_DEBUG_TRACE) - zlog_debug ("%s: del inherit oif from up %s", __PRETTY_FUNCTION__, up->sg_str); + zlog_debug ("%s: SGRpt Set, del inherit oif from up %s", __PRETTY_FUNCTION__, up->sg_str); pim_channel_del_oif (up->channel_oil, ch->interface, PIM_OIF_FLAG_PROTO_STAR); } } diff --git a/pimd/pim_nht.c b/pimd/pim_nht.c index c5f8d1d826..9165bef566 100644 --- a/pimd/pim_nht.c +++ b/pimd/pim_nht.c @@ -567,7 +567,7 @@ pim_ecmp_nexthop_search (struct pim_nexthop_cache *pnc, //PIM ECMP flag is enable then choose ECMP path. hash_val = pim_compute_ecmp_hash (src, grp); mod_val = hash_val % pnc->nexthop_num; - if (PIM_DEBUG_TRACE) + if (PIM_DEBUG_PIM_TRACE_DETAIL) zlog_debug ("%s: hash_val %u mod_val %u ", __PRETTY_FUNCTION__, hash_val, mod_val); } @@ -914,7 +914,7 @@ pim_ecmp_nexthop_lookup (struct pim_nexthop *nexthop, struct in_addr addr, { hash_val = pim_compute_ecmp_hash (src, grp); mod_val = hash_val % num_ifindex; - if (PIM_DEBUG_TRACE) + if (PIM_DEBUG_PIM_TRACE_DETAIL) zlog_debug ("%s: hash_val %u mod_val %u", __PRETTY_FUNCTION__, hash_val, mod_val); } @@ -1037,7 +1037,7 @@ int pim_ecmp_fib_lookup_if_vif_index(struct in_addr addr, { hash_val = pim_compute_ecmp_hash (src, grp); mod_val = hash_val % num_ifindex; - if (PIM_DEBUG_TRACE) + if (PIM_DEBUG_PIM_TRACE_DETAIL) zlog_debug ("%s: hash_val %u mod_val %u", __PRETTY_FUNCTION__, hash_val, mod_val); } diff --git a/pimd/pim_upstream.c b/pimd/pim_upstream.c index dd6eab9cfe..6fadfc2f29 100644 --- a/pimd/pim_upstream.c +++ b/pimd/pim_upstream.c @@ -78,9 +78,17 @@ pim_upstream_remove_children (struct pim_upstream *up) while (!list_isempty (up->sources)) { child = listnode_head (up->sources); - child->parent = NULL; listnode_delete (up->sources, child); + if (PIM_UPSTREAM_FLAG_TEST_SRC_LHR(child->flags)) + { + PIM_UPSTREAM_FLAG_UNSET_SRC_LHR(child->flags); + child = pim_upstream_del(child, __PRETTY_FUNCTION__); + } + if (child) + child->parent = NULL; } + list_delete(up->sources); + up->sources = NULL; } /* @@ -149,10 +157,14 @@ void pim_upstream_free(struct pim_upstream *up) static void upstream_channel_oil_detach(struct pim_upstream *up) { - if (up->channel_oil) { - pim_channel_oil_del(up->channel_oil); - up->channel_oil = NULL; - } + if (up->channel_oil) + { + /* Detaching from channel_oil, channel_oil may exist post del, + but upstream would not keep reference of it + */ + pim_channel_oil_del(up->channel_oil); + up->channel_oil = NULL; + } } struct pim_upstream * @@ -163,7 +175,7 @@ pim_upstream_del(struct pim_upstream *up, const char *name) if (PIM_DEBUG_TRACE) zlog_debug ("%s(%s): Delete %s ref count: %d , flags: %d c_oil ref count %d (Pre decrement)", - __PRETTY_FUNCTION__, name, up->sg_str, up->ref_count, up->flags, + __PRETTY_FUNCTION__, name, up->sg_str, up->ref_count, up->flags, up->channel_oil->oil_ref_count); --up->ref_count; @@ -195,26 +207,12 @@ pim_upstream_del(struct pim_upstream *up, const char *name) } pim_upstream_remove_children (up); + if (up->sources) + list_delete (up->sources); + up->sources = NULL; pim_mroute_del (up->channel_oil, __PRETTY_FUNCTION__); upstream_channel_oil_detach(up); - if (up->sources) - { - struct listnode *node, *nnode; - struct pim_upstream *child; - for (ALL_LIST_ELEMENTS (up->sources, node, nnode, child)) - { - if (PIM_UPSTREAM_FLAG_TEST_SRC_LHR(child->flags)) - { - PIM_UPSTREAM_FLAG_UNSET_SRC_LHR(child->flags); - pim_upstream_del(child, __PRETTY_FUNCTION__); - } - } - - list_delete (up->sources); - } - up->sources = NULL; - list_delete (up->ifchannels); up->ifchannels = NULL; @@ -223,11 +221,10 @@ pim_upstream_del(struct pim_upstream *up, const char *name) into pim_upstream_free() because the later is called by list_delete_all_node() */ - if (up->parent) - { - listnode_delete (up->parent->sources, up); - up->parent = NULL; - } + if (up->parent && up->parent->sources) + listnode_delete (up->parent->sources, up); + up->parent = NULL; + listnode_delete (pim_upstream_list, up); hash_release (pim_upstream_hash, up); @@ -538,13 +535,14 @@ pim_upstream_switch(struct pim_upstream *up, { enum pim_upstream_state old_state = up->join_state; - if (PIM_DEBUG_PIM_EVENTS) { - zlog_debug("%s: PIM_UPSTREAM_%s: (S,G) old: %s new: %s", + if (PIM_DEBUG_PIM_EVENTS) + { + zlog_debug ("%s: PIM_UPSTREAM_%s: (S,G) old: %s new: %s", __PRETTY_FUNCTION__, up->sg_str, pim_upstream_state2str (up->join_state), pim_upstream_state2str (new_state)); - } + } up->join_state = new_state; if (old_state != new_state) @@ -584,7 +582,17 @@ pim_upstream_switch(struct pim_upstream *up, if (old_state == PIM_UPSTREAM_JOINED) pim_msdp_up_join_state_changed(up); - pim_jp_agg_single_upstream_send(&up->rpf, up, 0 /* prune */); + /* IHR, Trigger SGRpt on *,G IIF to prune S,G from RPT */ + if (pim_upstream_is_sg_rpt(up) && up->parent) + { + if (PIM_DEBUG_PIM_TRACE_DETAIL) + zlog_debug ("%s: *,G IIF %s S,G IIF %s ", __PRETTY_FUNCTION__, + up->parent->rpf.source_nexthop.interface->name, + up->rpf.source_nexthop.interface->name); + pim_jp_agg_single_upstream_send(&up->parent->rpf, up->parent, 1 /* (W,G) Join */); + } + else + pim_jp_agg_single_upstream_send(&up->rpf, up, 0 /* prune */); join_timer_stop(up); } } @@ -717,9 +725,9 @@ pim_upstream_new (struct prefix_sg *sg, if (PIM_DEBUG_TRACE) { - zlog_debug ("%s: Created Upstream %s upstream_addr %s", + zlog_debug ("%s: Created Upstream %s upstream_addr %s ref count %d increment", __PRETTY_FUNCTION__, up->sg_str, - inet_ntoa (up->upstream_addr)); + inet_ntoa (up->upstream_addr), up->ref_count); } return up; @@ -750,6 +758,9 @@ pim_upstream_find_or_add(struct prefix_sg *sg, { up->flags |= flags; up->ref_count++; + if (PIM_DEBUG_TRACE) + zlog_debug ("%s(%s): upstream %s ref count %d increment", + __PRETTY_FUNCTION__, name, up->sg_str, up->ref_count); } } else @@ -759,10 +770,13 @@ pim_upstream_find_or_add(struct prefix_sg *sg, } void -pim_upstream_ref(struct pim_upstream *up, int flags) +pim_upstream_ref(struct pim_upstream *up, int flags, const char *name) { up->flags |= flags; ++up->ref_count; + if (PIM_DEBUG_TRACE) + zlog_debug ("%s(%s): upstream %s ref count %d increment", + __PRETTY_FUNCTION__, name, up->sg_str, up->ref_count); } struct pim_upstream *pim_upstream_add(struct prefix_sg *sg, @@ -773,7 +787,7 @@ struct pim_upstream *pim_upstream_add(struct prefix_sg *sg, int found = 0; up = pim_upstream_find(sg); if (up) { - pim_upstream_ref(up, flags); + pim_upstream_ref(up, flags, name); found = 1; } else { @@ -786,10 +800,11 @@ struct pim_upstream *pim_upstream_add(struct prefix_sg *sg, { char buf[PREFIX2STR_BUFFER]; prefix2str (&up->rpf.rpf_addr, buf, sizeof (buf)); - zlog_debug("%s(%s): %s, iif %s found: %d: ref_count: %d", + zlog_debug("%s(%s): %s, iif %s (%s) found: %d: ref_count: %d", __PRETTY_FUNCTION__, name, - up->sg_str, buf, found, - up->ref_count); + up->sg_str, buf, up->rpf.source_nexthop.interface ? + up->rpf.source_nexthop.interface->name : "NIL" , + found, up->ref_count); } else zlog_debug("%s(%s): (%s) failure to create", @@ -1660,7 +1675,7 @@ pim_upstream_sg_running (void *arg) if (PIM_DEBUG_TRACE) zlog_debug ("source reference created on kat restart %s", up->sg_str); - pim_upstream_ref(up, PIM_UPSTREAM_FLAG_MASK_SRC_STREAM); + pim_upstream_ref(up, PIM_UPSTREAM_FLAG_MASK_SRC_STREAM, __PRETTY_FUNCTION__); PIM_UPSTREAM_FLAG_SET_SRC_STREAM(up->flags); pim_upstream_fhr_kat_start(up); } diff --git a/pimd/pim_upstream.h b/pimd/pim_upstream.h index f1c8df35b1..d728c6c01f 100644 --- a/pimd/pim_upstream.h +++ b/pimd/pim_upstream.h @@ -145,7 +145,7 @@ struct pim_upstream *pim_upstream_find_or_add (struct prefix_sg *sg, struct pim_upstream *pim_upstream_add (struct prefix_sg *sg, struct interface *ifp, int flags, const char *name); -void pim_upstream_ref (struct pim_upstream *up, int flags); +void pim_upstream_ref (struct pim_upstream *up, int flags, const char *name); struct pim_upstream *pim_upstream_del(struct pim_upstream *up, const char *name); int pim_upstream_evaluate_join_desired(struct pim_upstream *up); From a91ec03e11c779bb8dcd21882bcecb0a7a8a68e5 Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Sat, 6 May 2017 11:18:24 -0700 Subject: [PATCH 2/5] pimd: IGMP Query Tx when Oth. Querier exp Send IGMP General Query and Get Gen. Query timer, once Other Querier timer expired. Ticket:CM-13152 Reviewed By:CCR-6189 Testing Done: tor-4# show ip igmp interface Interface State Address V Querier Query Timer Uptime swp2 up 70.1.1.2 3 other --:--:-- 00:06:45 swp3 up 80.1.1.2 3 local 00:00:35 00:22:52 Upon disabling IGMP on remote igmp interface, after other querier expiry sends a query and elects as Querier tor-4# show ip igmp interface Interface State Address V Querier Query Timer Uptime swp2 up 70.1.1.2 3 local 00:01:29 00:10:16 swp3 up 80.1.1.2 3 local 00:01:14 00:26:23 Signed-off-by: Chirag Shah --- pimd/pim_igmp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pimd/pim_igmp.c b/pimd/pim_igmp.c index ee88e7d8ea..dd5f7e77e9 100644 --- a/pimd/pim_igmp.c +++ b/pimd/pim_igmp.c @@ -39,6 +39,7 @@ #include "pim_zebra.h" static void group_timer_off(struct igmp_group *group); +static int pim_igmp_general_query(struct thread *t); /* This socket is used for TXing IGMP packets only, IGMP RX happens * in pim_mroute_msg() @@ -172,8 +173,11 @@ static int pim_igmp_other_querier_expire(struct thread *t) /* We are the current querier, then re-start sending general queries. + RFC 2236 - sec 7 Other Querier + present timer expired (Send General + Query, Set Gen. Query. timer) */ - pim_igmp_general_query_on(igmp); + pim_igmp_general_query(t); return 0; } @@ -497,8 +501,6 @@ int pim_igmp_packet(struct igmp_sock *igmp, char *buf, size_t len) return -1; } -static int pim_igmp_general_query(struct thread *t); - void pim_igmp_general_query_on(struct igmp_sock *igmp) { struct pim_interface *pim_ifp; From 30cfe9a2910297f1114c3da44d97e0fdcacaecd0 Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Mon, 23 Jan 2017 17:39:54 -0800 Subject: [PATCH 3/5] pimd: fix ip pim hello option does not take in effect If frr.conf file has pim hello option setup prior to pim sm under an interface, upon frr start/restart hello option cli replayed prior to sm command from vtysh. Added a code in pim hello option cli handler to create pim vif if it does not exist. Testing Done: configure pim hello options before pim sm in frr.conf file and restart frr Signed-off-by: Chirag Shah --- pimd/pim_cmd.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pimd/pim_cmd.c b/pimd/pim_cmd.c index 1224bc5fc8..0f4e8fdcbf 100644 --- a/pimd/pim_cmd.c +++ b/pimd/pim_cmd.c @@ -5075,11 +5075,16 @@ DEFUN (interface_ip_pim_hello, pim_ifp = ifp->info; - if (!pim_ifp) { - vty_out(vty, "Pim not enabled on this interface%s", VTY_NEWLINE); - return CMD_WARNING; - } + if (!pim_ifp) + { + if (!pim_cmd_interface_add(ifp)) + { + vty_out(vty, "Could not enable PIM SM on interface%s", VTY_NEWLINE); + return CMD_WARNING; + } + } + pim_ifp = ifp->info; pim_ifp->pim_hello_period = strtol(argv[idx_time]->arg, NULL, 10); if (argc == idx_hold + 1) From 8dc60b69b582728866ec485c77a3e296123aac5a Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Tue, 24 Jan 2017 14:58:59 -0800 Subject: [PATCH 4/5] pimd: fix pim reg processing return 1 upon success pim register_recv api returns 1 instead of 0 upon succesfully processing REG message Testing Done: Verified At RP via receiving PIM (Data/Null) Register messages and checked show ip pim interface < > Received errors under Hello Signed-off-by: Chirag Shah --- pimd/pim_register.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pimd/pim_register.c b/pimd/pim_register.c index 8dc179c144..e6145a142e 100644 --- a/pimd/pim_register.c +++ b/pimd/pim_register.c @@ -346,7 +346,7 @@ pim_register_recv (struct interface *ifp, zlog_debug("%s: Sending register-Stop to %s and dropping mr. packet", __func__, "Sender"); /* Drop Packet Silently */ - return 1; + return 0; } } @@ -408,5 +408,5 @@ pim_register_recv (struct interface *ifp, pim_register_stop_send (ifp, &sg, dest_addr, src_addr); } - return 1; + return 0; } From 95d3f5011b509acbbc3a257c74c23bc5b57148af Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Tue, 16 May 2017 21:57:25 -0700 Subject: [PATCH 5/5] pimd: Fix input value to bool Signed-off-by: Chirag Shah --- pimd/pim_ifchannel.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pimd/pim_ifchannel.c b/pimd/pim_ifchannel.c index 04f8a22c6b..c5953e560c 100644 --- a/pimd/pim_ifchannel.c +++ b/pimd/pim_ifchannel.c @@ -583,7 +583,7 @@ pim_ifchannel_add(struct interface *ifp, return ch; } -static void ifjoin_to_noinfo(struct pim_ifchannel *ch, uint8_t ch_del) +static void ifjoin_to_noinfo(struct pim_ifchannel *ch, bool ch_del) { pim_forward_stop(ch); pim_ifchannel_ifjoin_switch(__PRETTY_FUNCTION__, ch, PIM_IFJOIN_NOINFO); @@ -599,7 +599,7 @@ static int on_ifjoin_expiry_timer(struct thread *t) ch->t_ifjoin_expiry_timer = NULL; - ifjoin_to_noinfo(ch, 1); + ifjoin_to_noinfo(ch, true); /* ch may have been deleted */ return 0; @@ -643,10 +643,10 @@ static int on_ifjoin_prune_pending_timer(struct thread *t) ch_del is set to 0 for not deleteing from here. Holdtime expiry (ch_del set to 1) delete the entry. */ - ifjoin_to_noinfo(ch, 0); + ifjoin_to_noinfo(ch, false); } else - ifjoin_to_noinfo(ch, 1); + ifjoin_to_noinfo(ch, true); /* from here ch may have been deleted */ } else