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
