Page MenuHomeFreeBSD

Update the MTU in affected routes when IPv6 RA changes the MTU
ClosedPublic

Authored by vangyzen on Feb 7 2018, 9:26 PM.

Details

Summary

ip6_calcmtu() only looks at the interface MTU if neither the TCP hostcache nor the route provides an MTU. Update the routes so they do not provide stale MTUs.

Test Plan

This fixes UNH IPv6 conformance test cases v6LC_4_1_08 and v6LC_4_1_09,
which use a RA to reduce the link MTU from 1500 to 1280.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vangyzen created this revision.Feb 7 2018, 9:26 PM

I do not plan to commit this change. We made this change to demonstrate that it fixes two conformance tests. However, I imagine there is a more correct change, since this one simply reverts part of rS274611. Perhaps rt_updatemtu() needs to be called in the nd_opts_mtu case in nd6_ra_input()?

Note to self: SCOS-47425

vangyzen added inline comments.Feb 8 2018, 5:49 PM
sys/netinet6/ip6_output.c
1402 ↗(On Diff #39027)

After further review, I think we should either do

mtu = min(mtu, ifmtu);

here, as was done prior to rS274611, or call rt_updatemtu() in nd6_ra_input(). Which is more correct?

vangyzen added inline comments.Feb 8 2018, 6:22 PM
sys/netinet6/ip6_output.c
1402 ↗(On Diff #39027)

Since RTM_ADD and RTM_CHANGE limit the route MTU to the IPv6 RA MTU, the intent seems to be that the route is always updated to reflect the actual MTU. If that is correct, I think we should call rt_updatemtu() in nd6_ra_input(). What do you think, @ae and @melifaro?

vangyzen updated this revision to Diff 39059.Feb 8 2018, 8:08 PM
  • Revert "IPv6: consider interface MTU in path MTU calculation"
  • IPv6: consider interface MTU in path MTU calculation
vangyzen added inline comments.Feb 8 2018, 8:09 PM
sys/netinet6/ip6_output.c
1402 ↗(On Diff #39027)

The submitter tried adding rt_updatemtu() in nd6_ra_input(), but it did not work, so I now suggest this change.

vangyzen added inline comments.Feb 8 2018, 9:51 PM
sys/netinet6/ip6_output.c
1402 ↗(On Diff #39027)

Correction: Adding rt_updatemtu() in nd6_ra_input() does work. Sorry for the confusion. So, the question is still open: Which change is preferred?

ae added a comment.Feb 9 2018, 9:36 AM

Invoking of rt_updatemtu() looks right to me. But I don't like how rt_updatemtu() works.
Recently we found that in6_mtutimo() on systems with many IPv6 routes produces noticeable delay for packets processing.
It holds RIB_WLOCK while all routes are processed, and thus normal packets processing is blocked on RIB_RLOCK for this time.
Also in6_mtutimo looks like dead code, that is doing nothing useful.

In D14257#299355, @ae wrote:

Invoking of rt_updatemtu() looks right to me. But I don't like how rt_updatemtu() works.
Recently we found that in6_mtutimo() on systems with many IPv6 routes produces noticeable delay for packets processing.
It holds RIB_WLOCK while all routes are processed, and thus normal packets processing is blocked on RIB_RLOCK for this time.

There are other rt_foreach_fib() users as well. Probably we can change iteration logic the following way:

retry = 1
while (retry && retry_count < N) {
retry = 0
RADIX_RLOCK
generation = rnh_gen
foreach_route()
  if_match()
    if linked_list_add(route)
      refcount(route)
    else
     retry++; // allocation failure, retry
RADIX_RUNLOCK

if linked_list_notempty() {
RADIX_WLOCK
if rnh_gen != generation
  retry++;
foreach_list_item() {
  unref(route)
  call_function(route)
}
RADIX_WUNLOCK
}

}
sys/netinet6/ip6_output.c
1402 ↗(On Diff #39027)

In general, updating mtu is control plane change. We don't need to make data plane checks - this seem wrong. We should be able to maintain valid values in our dataplane structures, to optimise the data path. So, calling rt_updatemtu() should be preferred here.

vangyzen updated this revision to Diff 39087.Feb 9 2018, 3:15 PM
  • Revert "IPv6: consider interface MTU in path MTU calculation"
  • Update the MTU in affected routes when IPv6 RA changes the MTU
vangyzen updated this revision to Diff 39088.Feb 9 2018, 3:20 PM
  • Pass the ifp...duh.
vangyzen retitled this revision from IPv6: consider interface MTU in path MTU calculation to Update the MTU in affected routes when IPv6 RA changes the MTU.Feb 9 2018, 3:42 PM
vangyzen edited the summary of this revision. (Show Details)

I would like to commit this change and leave improvements in rt_updatemtu() for another day. MTU changes by RA should be a very rare event, so the inefficiency of rt_updatemtu() is not so significant for this change.

dab accepted this revision.Feb 9 2018, 5:21 PM
This revision is now accepted and ready to land.Feb 9 2018, 5:21 PM
melifaro accepted this revision.Feb 9 2018, 5:34 PM

I would like to commit this change and leave improvements in rt_updatemtu() for another day.

Sure, it's totally not the scope of this CR.

This revision was automatically updated to reflect the committed changes.