[Linux] Listen to and correctly handle IPv6 address change announcements.

[Linux] Listen to and correctly handle IPv6 address change announcements.

BUG=36591
TEST=manually test that network change notifications happen at the right time.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55171

[jar (doing other things), 2010-07-22 18:47:09]

I'm not at all expert in this area... so please humor my comments and help me
understand the code.

thanks,

Jim

http://codereview.chromium.org/2881027/diff/1/2
File net/base/network_change_notifier_netlink_linux.cc (right):

http://codereview.chromium.org/2881027/diff/1/2#newcode33
net/base/network_change_notifier_netlink_linux.cc:33: return false;
I think the function name confused me here.  When I saw "return false" I said
"hmm... that means it is not stale,"

I think this might be more readable if had a separate IsIPv6Update() test,
called from the switch statement, and then (and only then) called a test for
IsStaleIPv6Update (where you merely assert it is IPv6).

http://codereview.chromium.org/2881027/diff/1/2#newcode45
net/base/network_change_notifier_netlink_linux.cc:45: if (cache_info->cstamp !=
cache_info->tstamp)
I would have expected this to be a test for equality, to indicate that this is a
new (non-stale) time.  ...and then, when we discover a new, non-stale item, we
would probably return false.

Your comment above seems to support that we "ignore ... broadcasts where time
stamps [are different]" (and presumably continue, rather than return).... what
am I confused about??

http://codereview.chromium.org/2881027/diff/1/2#newcode74
net/base/network_change_notifier_netlink_linux.cc:74: RTMGRP_NOTIFY;
Will this dramatically increase the number of notifications?  When Will Chan
played around in this area, we saw a LOT of notifications, to the point at which
we began to wonder if it would impact performance.  Comments?

http://codereview.chromium.org/2881027/diff/1/2#newcode109
net/base/network_change_notifier_netlink_linux.cc:109: // Fall through.
Fall through seems to only reduce readability, unless you can anticipate that
DELADDR is going to have a more complex evaluation in the future.

Suggest:
  return !IsStale...();


or even an explicit return in both branches of the conditional (rather than a
comment).

[vandebo (ex-Chrome), 2010-07-22 20:52:59]

On 2010/07/22 18:47:09, jar wrote:
> I'm not at all expert in this area... so please humor my comments and help me
> understand the code.

After replying to your comments I realize that you may not know the underlying
cause of the problem you were seeing.  IPv6 has a DHCP type mechanism built in,
called Router Advertisements.  Instead of being request response like DHCP, it's
broadcast.  The RA server sends out periodic RAs, telling hosts the information
they need in order to auto-configure (it broadcasts a /64 netblock, the host
uses its MAC address to generate a unique IP in that block).  It seems that the
linux netlink code sends an update every time an RA is received, regardless of
it changing the configured addresses or not.  So this code ignores RAs that are
not new.

http://codereview.chromium.org/2881027/diff/1/2
File net/base/network_change_notifier_netlink_linux.cc (right):

http://codereview.chromium.org/2881027/diff/1/2#newcode33
net/base/network_change_notifier_netlink_linux.cc:33: return false;
On 2010/07/22 18:47:09, jar wrote:
> I think the function name confused me here.  When I saw "return false" I said
> "hmm... that means it is not stale,"
> 
> I think this might be more readable if had a separate IsIPv6Update() test,
> called from the switch statement, and then (and only then) called a test for
> IsStaleIPv6Update (where you merely assert it is IPv6).

Done.

http://codereview.chromium.org/2881027/diff/1/2#newcode45
net/base/network_change_notifier_netlink_linux.cc:45: if (cache_info->cstamp !=
cache_info->tstamp)
On 2010/07/22 18:47:09, jar wrote:
> I would have expected this to be a test for equality, to indicate that this is
a
> new (non-stale) time.  ...and then, when we discover a new, non-stale item, we
> would probably return false.
> 
> Your comment above seems to support that we "ignore ... broadcasts where time
> stamps [are different]" (and presumably continue, rather than return).... what
> am I confused about?? 

Only netlink broadcasts triggered by Router Advertisements have the cacheinfo
attribute.  If the machine hasn't seen the RA before, cache_info->cstamp ==
cache_info->tstamp.  However, if it is a stale RA, i.e. we aren't getting a new
addresses, just an announcement for an address we already have, then we should
return true.  Suggestions to make this more obvious?

http://codereview.chromium.org/2881027/diff/1/2#newcode74
net/base/network_change_notifier_netlink_linux.cc:74: RTMGRP_NOTIFY;
On 2010/07/22 18:47:09, jar wrote:
> Will this dramatically increase the number of notifications?  When Will Chan
> played around in this area, we saw a LOT of notifications, to the point at
which
> we began to wonder if it would impact performance.  Comments?  

How many notifies did you see?  With RTMGRP_IPV6_IFADDR, I get a netlink message
every minute or two (each time the machine gets an RA).  However, using the
above function, we ignore RA's that don't add new addresses.

http://codereview.chromium.org/2881027/diff/1/2#newcode109
net/base/network_change_notifier_netlink_linux.cc:109: // Fall through.
On 2010/07/22 18:47:09, jar wrote:
> Fall through seems to only reduce readability, unless you can anticipate that
> DELADDR is going to have a more complex evaluation in the future.
> 
> Suggest:
>   return !IsStale...();
> 
> 
> or even an explicit return in both branches of the conditional (rather than a
> comment).

Done.

[jar (doing other things), 2010-07-29 00:51:55]

http://codereview.chromium.org/2881027/diff/1/2
File net/base/network_change_notifier_netlink_linux.cc (right):

http://codereview.chromium.org/2881027/diff/1/2#newcode45
net/base/network_change_notifier_netlink_linux.cc:45: if (cache_info->cstamp !=
cache_info->tstamp)
On 2010/07/22 20:53:00, vandebo wrote:
> On 2010/07/22 18:47:09, jar wrote:
> > I would have expected this to be a test for equality, to indicate that this
is
> a
> > new (non-stale) time.  ...and then, when we discover a new, non-stale item,
we
> > would probably return false.
> > 
> > Your comment above seems to support that we "ignore ... broadcasts where
time
> > stamps [are different]" (and presumably continue, rather than return)....
what
> > am I confused about?? 
> 
> Only netlink broadcasts triggered by Router Advertisements have the cacheinfo
> attribute.  If the machine hasn't seen the RA before, cache_info->cstamp ==
> cache_info->tstamp.  However, if it is a stale RA, i.e. we aren't getting a
new
> addresses, just an announcement for an address we already have, then we should
> return true.  Suggestions to make this more obvious?

Looking at the code, it would appear that there are (potentially) multiple
Router Advertisements.  I would have expected you to look at all Router
Advertisements, and (as per your comment) ignore ones that are not equal, and
only return if you found one that was equal (where the computer "hasn't seen the
RA before.").

If after scanning all of them, you conclude they are all stale, then I'd expect
to return true to indicate that they are all stale.

In contrast, the code appears to return as soon as a stale value is observed. 
Is there never more than one value??

Perhaps you should educate me off-line if I'm still confused (and you feel your
code is correct).

http://codereview.chromium.org/2881027/diff/1/2#newcode74
net/base/network_change_notifier_netlink_linux.cc:74: RTMGRP_NOTIFY;
My vague recollection was well in excess of 6 a minute.  It may have been closer
to 30.  This is my recollection when running inside google, so YMMV.

As I (again, vaguely) recall, the server was sending inconsequential updates
about IPv6 addresses, and we then ended up sending notifications of a need to
re-probe much more often than "when the network connection really changed."

Perhaps we should temporarilly histogram the interarrival times of these
notifcations, just to be sure that they are indeed mostly "every few minutes"
and not "almost every second."  

WDYT?  ...or am I confusing this approach with some other more problematic
approach??

On 2010/07/22 20:53:00, vandebo wrote:
> On 2010/07/22 18:47:09, jar wrote:
> > Will this dramatically increase the number of notifications?  When Will Chan
> > played around in this area, we saw a LOT of notifications, to the point at
> which
> > we began to wonder if it would impact performance.  Comments?  
> 
> How many notifies did you see?  With RTMGRP_IPV6_IFADDR, I get a netlink
message
> every minute or two (each time the machine gets an RA).  However, using the
> above function, we ignore RA's that don't add new addresses.

[jar (doing other things), 2010-08-04 16:18:22]

LGTM