Page MenuHomeFreeBSD

ndp: Remove goto and unused condition in prelist_update
Needs ReviewPublic

Authored by pouria on Sat, Mar 28, 8:14 PM.

Details

Reviewers
markj
glebius
bz
Group Reviewers
network
Summary

Also, use predict_false on a rare condition.
While here, style it.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71784
Build 68667: arc lint + arc unit

Event Timeline

sys/netinet6/nd6_rtr.c
1737–1742

I moved this comment about the condition to make it easier to read, considering below comment.

1753–1758

This one is removed.

zlei added inline comments.
sys/netinet6/nd6_rtr.c
1751

I'd recommend not to use __predict_false() and __predict_true() for the slow protocols. It will not save noticeable CPU cycles.

A typical valid usage is iflib.c . Every packet ( regardless of the protocol ) will hit the path and it is valuable to shine __predict_false() and __predict_true().

sys/netinet6/nd6_rtr.c
1751

But it may save some, I don't understand why it could be a problem.
Could you explain, please?
I can remove it, I just want to understand the reason.

sys/netinet6/nd6_rtr.c
1751

For performance critical path, generally you want either,
a) Benchmark, and find out it save cycles, or reduce cache miss, or reduce false prediction,
b) By intuition, you can conclude it is no doubt that the path is hot.

Otherwise ( if it is not a performance critical path ), it wastes your time to hint the compiler to do optimization.

Quick question: Is prelist_update() a performance critical path ? How frequently will a host receive prefix information ?

1753

I was about to support you to re-prototype in6_if2idlen() to return unsigned int, until I see the comment in default branch,

/*
 * Unknown link type:
 * It might be controversial to use the today's common constant
 * of 64 for these cases unconditionally.  For full compliance,
 * we should return an error in this case.  On the other hand,
 * if we simply miss the standard for the link type or a new
 * standard is defined for a new link type, the IFID length
 * is very likely to be the common constant.  As a compromise,
 * we always use the constant, but make an explicit notice
 * indicating the "unknown" case.

So I think it is better to have an assert here.

KASSERT(ifidlen >= 0, ("IFID undefined (%s)", if_name(ifp)));