nd6: Remove goto and unused condition in prelist_update

[pouria]

Summary

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

[pouria]

sys/netinet6/nd6_rtr.c

1751–1756

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

1767–1772

This one is removed.

[zlei]

sys/netinet6/nd6_rtr.c

1766

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().

[pouria]

sys/netinet6/nd6_rtr.c

1766

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.

[zlei]

sys/netinet6/nd6_rtr.c

1766

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 ?

1767

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)));

[markj]

sys/netinet6/nd6_rtr.c

1752

1766

In general, if a function isn't getting called, say, thousands of times a second, then it is not worth annotating branches. prelist_update() probably doesn't satisfy this heuristic, and I'm pretty sure that this particular case does not either.

1768

Why not change this to else if ((ia6 = ...))? Then you don't need the duplicate rele either.

[pouria]

Address @zlei and @markj comments.

[pouria]

sys/netinet6/nd6_rtr.c

1767–1772

Per RFC, we should ignore the PI option if this happen, I don't think assertion is a good idea here.

[zlei]

Looks good to me.