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

Reply via email to