Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-13 Thread Richard Cochran 
On Sun, Aug 13, 2017 at 11:21:21PM +0800, Hangbin Liu wrote:
> On Sat, Aug 12, 2017 at 04:41:23PM +0200, Richard Cochran wrote:
> > Looks better to me, but why not place this code at the end of the
> > port_link_status() function?

...

> 2. If We receive one rtnl message with multi RTM_NEWLINK info, the cb will be
>called more than once, then the return value will be replaced by new ones.

Ok, that is clear.  Place the code after rtnl_link_status() like you said.

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

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-13 Thread Hangbin Liu 
On Sat, Aug 12, 2017 at 04:41:23PM +0200, Richard Cochran wrote:
> > Then after rtnl_link_status(fd, p->name, port_link_status, p);
> > 
> > if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
> > return EV_FAULT_CLEARED;
> > else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
> >  (p->link_status & TS_LABEL_CHANGED))
> > return EV_FAULT_DETECTED;
> > else
> > return EV_NONE;
> > 
> > 
> > What do you think?
> 
> Looks better to me, but why not place this code at the end of the
> port_link_status() function?

Hmm... I saw your comments about let rtnl_link_status() return the EV_ value
from port_link_status().

This seems no easy to handle.
1. We need to return both error code and enum fsm_event in rtnl_link_status().
2. If We receive one rtnl message with multi RTM_NEWLINK info, the cb will be
   called more than once, then the return value will be replaced by new ones.

for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
if (cb)
err = cb(ctx, link_up, slave_index);
}

return err;

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

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-12 Thread Richard Cochran 
On Sat, Aug 12, 2017 at 10:16:20PM +0800, Hangbin Liu wrote:
> For example, when the first time we receive a linkup + ts_iface change
> message, we need to return EV_FAULT_DETECTED. But as we know, we may
> recive multi linkup message. For these kind of message, we need to return
> EV_NONE.

Right...
 
> Then after rtnl_link_status(fd, p->name, port_link_status, p);
> 
> if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
> return EV_FAULT_CLEARED;
> else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
>  (p->link_status & TS_LABEL_CHANGED))
> return EV_FAULT_DETECTED;
> else
> return EV_NONE;
> 
> 
> What do you think?

Looks better to me, but why not place this code at the end of the
port_link_status() function?

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

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-12 Thread Hangbin Liu 
Hi Richard,
On Sat, Aug 05, 2017 at 10:13:07AM +0200, Richard Cochran wrote:
> 
> This testing of two Booleans (status_changed, link_status) smells bad.
> Please refactor this into something that makes sense.

At present, the link could be up or down. On an other hand, when the link stay
up or down, the ts_iface could be changed. So the states could be

LINK_UP -> LINK_DOWN(LINK_STATE_CHANGED):   EV_FAULT_DETECTED
LINK_DOWN -> LINK_UP(LINK_STATE_CHANGED):   EV_FAULT_CLEARED
LINK_UP -> LINK_UP, TS_IFACE_CHANGED:   EV_FAULT_DETECTED
LINK_DOWN -> LINK_DOWN, TS_IFACE_CHANGED:   EV_FAULT_DETECTED

For example, when the first time we receive a linkup + ts_iface change
message, we need to return EV_FAULT_DETECTED. But as we know, we may
recive multi linkup message. For these kind of message, we need to return
EV_NONE.

So I want to make the port link_status to enum like
enum link_state {
LINK_DOWN  = (1<<0),
LINK_UP  = (1<<1),
LINK_STATE_CHANGED = (1<<3),
TS_LABEL_CHANGED  = (1<<4),
};

Then after rtnl_link_status(fd, p->name, port_link_status, p);

if (p->link_status == (LINK_UP | LINK_STATE_CHANGED))
return EV_FAULT_CLEARED;
else if ((p->link_status == (LINK_DOWN | LINK_STATE_CHANGED)) ||
 (p->link_status & TS_LABEL_CHANGED))
return EV_FAULT_DETECTED;
else
return EV_NONE;


What do you think?

Thanks
Hangbin


> 
> > +   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

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-09 Thread Hangbin Liu 
On Sat, Aug 05, 2017 at 10:13:07AM +0200, Richard Cochran wrote:
> >  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().

for ( ; NLMSG_OK(nh, len); nh = NLMSG_NEXT(nh, len)) {
if (nh->nlmsg_type == RTM_NEWLINK) {

cb(ctx, index, info->ifi_flags & IFF_RUNNING ? 1 : 0, device);
}
}

One rtnl msg may have multi messages with nh->nlmsg_type == RTM_NEWLINK, which
will call back port_link_status() multi times. So we could not let
rtnl_link_status() return EV_ value.

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

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-09 Thread Hangbin Liu 
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

Re: [Linuxptp-devel] [PATCHv2 7/9] port.c: fix port_dispatch event flood when change ts_iface info

2017-08-05 Thread Richard Cochran 
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 
> ---
>  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