On Sat, Jul 15, 2017 at 09:33:09PM +0800, Hangbin Liu wrote: > When bond active slave interface changed. We will receive multi rtnl > messages, i.e. bond and slave interfaces status changing information. > > No matter what's the iface index of the rtnl messages, it will trigger > FD_RTNL event. Since the bond interface link status is keeping on. We > will return EV_FAULT_DETECTED every time. At the same time transport_recv > will keep return fail. Then it become port_dispatch message flood. e.g.
I don't understand the problem, but ... > Fix this by moving the port_dispatch event to port_link_status() and return > EV_NONE in port_event(). > > At the same time, when ts_inface info changed, the port link status will not > change. So remove the port link status check in port_link_status(). > > Now we will record status_changed with two scenarios. 1) link status changed. > 2) ts_iface changed. Others we will do nothing. The fix here is too ugly. If there are two scenarios, then the code should clearly reflect this. For example, using two sub-functions. > Signed-off-by: Hangbin Liu <liuhang...@gmail.com> > --- > port.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/port.c b/port.c > index 3c4e39f..05e79e7 100644 > --- a/port.c > +++ b/port.c > @@ -2225,12 +2225,16 @@ static void port_link_status(void *ctx, int index, > int linkup, char *ts_iface) > { > struct port *p = ctx; > int required_modes = 0; > + int status_changed = 0; > > - if (index != if_nametoindex(p->name) || p->link_status == linkup) > + if (index != if_nametoindex(p->name)) > return; > > - p->link_status = linkup; > - pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : "down"); > + if (p->link_status != linkup) { > + p->link_status = linkup; > + pr_notice("port %hu: link %s", portnum(p), linkup ? "up" : > "down"); > + status_changed = 1; > + } > > /* ts_iface changed */ > if (ts_iface[0] != '\0' && strcmp(p->iface->ts_iface, ts_iface)) { > @@ -2249,14 +2253,22 @@ static void port_link_status(void *ctx, int index, > int linkup, char *ts_iface) > clock_switch_phc(p->clock, p->phc_index); > } > } > + > + status_changed = 1; > } > > - /* > - * A port going down can affect the BMCA result. > - * Force a state decision event. > - */ > - if (!p->link_status) > - clock_set_sde(p->clock, 1); > + if (status_changed == 1) { > + if (p->link_status) { This testing of two Booleans (status_changed, link_status) smells bad. Please refactor this into something that makes sense. > + port_dispatch(p, EV_FAULT_CLEARED, 0); > + } else { > + port_dispatch(p, EV_FAULT_DETECTED, 0); > + /* > + * A port going down can affect the BMCA result. > + * Force a state decision event. > + */ > + clock_set_sde(p->clock, 1); > + } > + } > } > > enum fsm_event port_event(struct port *p, int fd_index) > @@ -2301,7 +2313,7 @@ enum fsm_event port_event(struct port *p, int fd_index) > case FD_RTNL: > pr_debug("port %hu: received link status notification", > portnum(p)); > rtnl_link_status(fd, port_link_status, p); > - return port_link_status_get(p) ? EV_FAULT_CLEARED : > EV_FAULT_DETECTED; > + return EV_NONE; Maybe we can let rtnl_link_status() return the EV_ value from port_link_status(), in order to keep a functional pattern and avoid the hidden port_dispatch(). > } > > msg = msg_allocate(); > -- > 2.5.5 Thanks, Richard ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel