net*: use inlined m_pullup()
Needs ReviewPublic
Actions

Authored by glebius on Mon, Oct 12, 11:26 PM.

Details

Reviewers

pouria
gallatin
zlei

Group Reviewers

network
transport

Summary

Leverage the fact that m_pullup() in an inline function in mbuf.h and let
compiler to do all optimizations.

Mostly mechanical changes, but in few cases surrounding lines are brought
to style(9). In netinet6, a few input functions changed to set the
input/output struct mbuf ** pointer to NULL at a single place.

Looks like pfsync(4) got a bug fixed, where a result from a successful
m_pullup() was not stored.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 78234
Build 75117: arc lint + arc unit

Event Timeline

glebius created this revision.Mon, Oct 12, 11:26 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptMon, Oct 12, 11:26 PM

Herald added subscribers: ivy, vegeta_tuxpowered.net, donner and 4 others. · View Herald Transcript

glebius requested review of this revision.Mon, Oct 12, 11:26 PM

Harbormaster completed remote builds in B78111: Diff 189685.Mon, Oct 12, 11:26 PM

glebius added a parent revision: D60688: mbuf: make m_pullup() an inline over most expected case.Mon, Oct 12, 11:26 PM

I've reviewed the code under sys/net/

I also reviewed the code under sys/netinet6.

sys/netinet6/icmp6.c
453	Nice catch!
sys/netinet6/route6.c
65–67	Yup, it was not defined at all. `fgrep -r ip6aux sys/`

pouria added a child revision: D60552: ip_encap: Recompute flowid for packets received over tunnel.Tue, Oct 13, 7:08 PM

pouria mentioned this in D60552: ip_encap: Recompute flowid for packets received over tunnel.Tue, Oct 13, 7:34 PM

pouria removed a child revision: D60552: ip_encap: Recompute flowid for packets received over tunnel.Tue, Oct 13, 8:55 PM

evan_uninitialized-memory.com added a subscriber: evan_uninitialized-memory.com.Wed, Oct 14, 7:12 AM

Generally looks good to me. Well I'd recommend fixing existing bugs prior to committing this change.

sys/net/if_bridge.c
4137	This shall never happen if the mbuf is correctly constructed.
sys/net/if_geneve.c
3099	if_geneve has not been yet MFCed to stable branches. I'd recommend to commit this part separately. That will reduce conflicts if someone would like to MFC this.
sys/netgraph/ng_pppoe.c
1457	m->m_len >= 2 && (m = m_pullup(m, 2)) The above logic appears wrong to me. I would expect to pullup only when `m->m_len < 2` . CC @glebius to confirm.
sys/netinet/tcp_subr.c
597	I'd keep this untouched, to reduce diff.
sys/netinet6/route6.c
69–71	I personally like this, - if ((m = m_pullup(m, off + sizeof(rh))) == NULL) { + if ((m = mp = m_pullup(mp, off + sizeof(rh))) == NULL) { That will clearly show the intention.
sys/netpfil/pf/if_pfsync.c
982	Yes it looks like a bug. CC @kp to confirm. If that is true then we shall fix it firstly.
1085	Ditto.

rcm added a subscriber: rcm.Wed, Oct 14, 3:43 PM

Rebase over fixed bug in pfsync and over caddr_t cleanup D60738

Harbormaster completed remote builds in B78234: Diff 189951.Thu, Oct 15, 4:54 AM

glebius marked 3 inline comments as done.Thu, Oct 15, 4:56 AM

glebius added inline comments.

sys/netgraph/ng_pppoe.c
1457	Yes, looks like another bug accidentally fixed.

Looks good to me.

sys/netgraph/ng_pppoe.c
1457	`V_ng_pppoe_lcp_pcp` is 0 by default. I think that is why this code is not widely tested and no bugs reported. This code was introduced via 28903f396af4b . Also CC the author @eugen_grosbein.net . If that is indeed a bug, I think it should be fixed separately and MFCed to stable branches.