Page MenuHomeFreeBSD

Use fib4_lookup_nh_ext() in ip_output().
ClosedPublic

Authored by glebius on Apr 2 2019, 9:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 7, 10:54 PM
Unknown Object (File)
Sun, Apr 7, 9:30 AM
Unknown Object (File)
Fri, Mar 29, 11:36 PM
Unknown Object (File)
Fri, Mar 29, 11:36 PM
Unknown Object (File)
Fri, Mar 29, 11:36 PM
Unknown Object (File)
Fri, Mar 29, 11:36 PM
Unknown Object (File)
Fri, Mar 29, 11:36 PM
Unknown Object (File)
Fri, Mar 29, 11:36 PM
Subscribers

Details

Summary

Existense of PCB route cachine doesn't allow us to use new fast route
lookup KPI in ip_output() like it is already used in ip_forward().

However, when there is no PCB provided we can use it. Typical scenario
is a DNS server that would use sendto(2) over a not connected UDP socket.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

melifaro added inline comments.
sys/netinet/ip_output.c
306 ↗(On Diff #55755)

Nitpick: if we're revisiting this fancy condition, would it be possible to consider replace its occurrence with something like

if (RTE_IS_INVALID(ro->ro_rt) || dst->sin_family != AF_INET ...)

?

sys/netinet/ip_output.c
306 ↗(On Diff #55755)

Do we have RTE_IS_INVALID? But we always first need to check that ro->ro_rt isn't NULL anyway.

More comments & fix a bug with broadcast.

  • Copy RTF_HOST to NHF_HOST.
  • Export in_ifaddr as nh_ia. This allows to fix ifa packets/bytes accounting
sys/netinet/in_fib.h
46 ↗(On Diff #55789)

Would it be possible to move it to a spare field?

sys/netinet/ip_output.c
443 ↗(On Diff #55789)

Seem to always fallback to the in_ifaddr_broadcast() check.

Originally RTF_BROADCAST flag was added 22 years ago in r15652 to avoid calling in_ifaddr_broadcast(). We had route cloning back then, so it was a fair assumption - if the lookup result was a host route (and it almost always was), then we could trust its broadcast flag, as it was set in the in_addroute() after verificaton.

Currently we don't have route cloning and the only way of having RTF_BROADCAST set on the route is to explicitly add host route to the broadcast address:

route add -host 10.0.0.255 -iface vtnet0

assuming 10.0.0.0/24 belongs to vtnet0

Given that, the check here and in line 400 (if (ro->ro_rt->rt_flags & RTF_HOST)) will aways failback to in_ifaddr_broadcast().

Maybe it's worth just eliminating this condition and always check for in_ifaddr_broadcast().

306 ↗(On Diff #55755)

No, we don't. I was suggesting adding it as we have these checks repeated at least twice in the ip_output()

sys/netinet/ip_output.c
443 ↗(On Diff #55789)

But as long as we allow adding such routes manually, we should support them, shouldn't we?

  • Remove spare, since nh_ia was added.

LGTM. The last one: are you planning to implement the same functionality in ip6_output()? :-)

This revision is now accepted and ready to land.Apr 8 2019, 3:22 PM
This revision was automatically updated to reflect the committed changes.