Page MenuHomeFreeBSD

netinet6: Remove PULLDOWN_TESTs.
ClosedPublic

Authored by bz on Nov 12 2019, 5:44 PM.

Details

Summary

Remove the KAME introduced PULLDOWN_TESTs which did not even
have a compile-time option in sys/conf to turn them on for a
custom kernel build. They made the code a lot harder to read
or more complicated in a few cases.

Convert the IP6_EXTHDR_CHECK() calls into FreeBSD looking code.
Rather than throwing the packet away if it would not fit the
KAME mbuf expectations, convert the macros to m_pullup() calls.
Do not do any extra manual conditional checks upfront as to
whether the m_len would suffice (*), simply let m_pullup() do
its work.

Remove extra m_pullup() calls where earlier in the function or
the only caller has already done the pullup.

Discussed with: rwatson (*)

Test Plan

Very basic test case(s) exercising some extension headers with
UDP and TCP upper layer protocols are in tree already.

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

bz created this revision.Nov 12 2019, 5:44 PM
ae added a comment.Nov 13 2019, 10:32 AM

The most of code that uses m_pullup() does check first, that the call is needed. Do you think that now it is considered as light enough, to do not check this?

bz added a comment.Nov 13 2019, 11:17 AM
In D22334#488613, @ae wrote:

The most of code that uses m_pullup() does check first, that the call is needed. Do you think that now it is considered as light enough, to do not check this?

As I tried to mention in the comment, I was wondering about the same.
I also emailed gnn and rwatson, and @rwatson thought the if () was probably for historic reason.

I went and checked and it came from 4.4BSD Lite sources at least ( I stopped digging there ). That was a totally different time.

Having looked at the checks that m_pullup() does before any (expensive) data movements, I was thinking that (a) having the checks in 1 place is probably better, and (b) if the checks in the stack and the beginning of m_pullup() always do the same in all cases, and (c) on a way to a future of variable sized mbufs (a) is a lot better.

I guess the question boils down to how good is the branch predictor vs. how expensive is the function call.

ae accepted this revision.Nov 14 2019, 1:53 PM
ae added inline comments.
sys/netinet6/icmp6.c
897 ↗(On Diff #64243)

It seems it is not needed, since icmp6_notify_error() called from one place and does this on error.

This revision is now accepted and ready to land.Nov 14 2019, 1:53 PM
bz added a comment.Nov 15 2019, 9:37 PM

Thanks a lot for reviewing! Much appreciated!

sys/netinet6/icmp6.c
897 ↗(On Diff #64243)

True for here and most of below.
Given this is an error path I am less concerned currently.
I am a lot more worried about consistency at the moment given did have valid cases where we did not update *mp and have continued on the wrong (freed) mbuf.

This revision was automatically updated to reflect the committed changes.