On Sat, Aug 05, 2017 at 10:13:07AM +0200, Richard Cochran wrote:
> 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 ...
The rtnl msg looks like:
interface bond0, index 10 is up, type bond, mode active, slave eth1
interface bond0, index 10 is up, type bond, mode active, slave eth1
interface bond0, index 10 is up, type bond, mode active, slave eth1
When bond interface failover. The bond interface's link status actually
keeping up, only slave interface info changed. So
port_link_status_get(p) ? EV_FAULT_CLEARED : EV_FAULT_DETECTED;
Will always return EV_FAULT_CLEARED.
But recvmsg will fail since network is down.
So we will get following messages repeatedly.
ptp4l[77068.125]: recvmsg failed: Network is down
ptp4l[77068.125]: port 1: recv message failed
ptp4l[77068.125]: port 1: LISTENING to FAULTY on FAULT_DETECTED
(FT_UNSPECIFIED)
ptp4l[77068.152]: port 1: FAULTY to LISTENING on INIT_COMPLETE
>
> > 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.
>
> > - /*
> > - * 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.
Yes, and the logic is not clear. I'm preparing to force set port link_status 0
when ts_iface change. This will make the behavior of link status change and
ts_iface change same.
I will test it first.
Thanks
Hangbin
------------------------------------------------------------------------------
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
