Reduce device-driver and protocol-stack use of MLEN, MHLEN, and MCLBYTES

[rwatson]

Summary

Replace many instances of MLEN, MHLEN, and MCLBYTES with calls to M_SIZE()
in the device-driver and protocol stacks. This substantially reduces the
extent to which these consumers of mbufs need be aware of low-level layout
decisions in the mbuf data structure, which should make it easier to
change those layout choices in the future.

Test Plan

Almost impossible to test as so many drivers are touched; reasonable smokescreen testing seems to pass but this is going to rely heavily on review.

[rwatson]

Regenerated patch to include 'head/' to see if that helps phabricator provide full context.

[rwatson]

Third try to generate with more context: this time, manually generated via svn diff.

[rpaulo]

sys/dev/ixgbe/ixv.c

2718

I agree it's redundant. It's probably safe to remove it.

sys/dev/ixl/ixl_txrx.c

944

Looks like copy & paste code at this point...

[rwatson]

Some notes from an IRC conversation with emaste@ last night:

<rwatson> emaste: fwiw, I basically have three reservations about this patch
<rwatson> emaste: (1) it takes something that was a statically compiled constant and makes it dynamic — necessary, however, if we plan to start changing mbuf behaviours out from under drivers; (2) it’s a potentially error-prone patch: although I’ve not yet spotted any mistakes, it’s the kind of semi-mechanical change that likely has errors in it; and (3) many drivers embed assumptions about MLEN, MHLEN, MCLBYTES that are deeper than initialisation of length fields, and I’ve not changed those assumptions, nor documented them.
<rwatson> emaste: on (1), I think this is actually quite a necessary change, as if we want to start supporting things like runtime-configurable in-mbuf-header metadata, then the existence of MLEN and MHLEN really can’t stick around
<rwatson> emaste: (in mbuf consumers, that is)
<rwatson> emaste: (3) is the architecturally worrying concern — lots of drivers have low-level assumptions in their DMA models, but also in the hardware-software interfaces, regarding something cluster-like in size, and they have not unreasonably picked MCLBYTES as the constant of choice
<rwatson> emaste: and I’ve made no attempt to modify those assumptions — primarily, I’ve chased up any instance of mbuf/cluster parts being allocated and then MLEN, MHLEN, and MCLBYTES being immediately assigned to their length fields
<rwatson> I think the patch is a substantial net win, despite these concerns, however
<emaste> rwatson: I agree with #1, there's not much choice for us in the matter

Notes from an IRC conversation with bz@ last night:

• bz notes that D1454 needs a lot more time to review as there are semantic changes implied which require device driver specific context; ... I'll look this afternoon into that diff sometime hopefully

<rwatson> bz: yes, this was a key worry for me — #3 if you saw the last I gave emaste on irc last night
<rwatson> bz: although in practice, the actual value stored in (len) as a result of the changes should pretty much always be the same
<rwatson> bz: it just comes from a different place
<rwatson> bz: the longer-term, harder issue is device drivers that embed assumptions such as “MHLEN will always be enough for X” or worse “MHLEN with a TCP ACK will always have enough room for this other thing in its trailer space"
<bz> rwatson: yeah one other intersting fact is that M_SIZE check for M_EXT first and not for pkthdr

[trasz]

Sorry, I have a deadline approaching, and this looks _very_ hard to review properly.

[bz]

I am only part way through but I don't want to lose all the comments by accident; more to follow.

sys/arm/allwinner/if_emac.c

359

that should be = M_SIZE();

As a matter of fact that line looks like a NOP actually.

377

There's a whole lot I don't understand with the driver in the following:
(1) why are we moving by ETHER_HDR_LEN rather than by ETHER_ALIGN if it's all just fixing the uint32_t alignment enforced by the HW to then get the equivalent of m_adj(m, ETHER_ALIGN) after receiving the data?
(2) Why in the old world there was a check for MHLEN on first if w/o making sure there's no external storage, despite we know we have a cluster from m_getcl?
(3) it only uses l2hlen later but the comment says moving ethernet + IP header.
(4) no one ever initialises l2hlen in this driver, so it's 0, right? Which is probably good as otherwise the first else if() condition would be bogus.
(5) that effectively makes parts of the else if condition now a bit redundant as we already have a cluster from m_getcl() and thus we are really only asking if (ETHER_HDR_LEN /* or ETHER_ALIGN */ <= MHLEN)) so we can move the ethernet header to the internal storage of the new pkthdr mbuf with proper alignment (or not as we don't care in that case as the L3 header would be moved to properly align, but isn't either leaving the L3 probably at a %2 and not a %4 offset?)

I am just confused here.

sys/dev/bce/if_bce.c

5563

I wonder if that wants to be m_new->m_len aka. M_SIZE(n_new)?

The other cases of MCLBYTES and JUMBO sizes are blah to fix w/o restructuring the driver or having at least accessor functions/macros hiding the actual values so the magic could be changed under the hood in the future. I guess in a future iteration.

sys/dev/cm/smc90cx6.c

543

I keep wondering why we can't first calculate the "len" and then ask for the mbuf size we need? Probably not part of this change.

sys/dev/cs/if_cs.c

719

I keep wondering why we can't just ask for the appropriate size depending on "length"? Probably not part of this change.

sys/dev/e1000/if_igb.c

4452

This line should also move up before the m_adj call; otherwise lens are not initialised and m_adj probably does funky things.

That's an independent fix and should probably go in it's own commit afterwards.

sys/dev/e1000/if_lem.c

3241

Lines 3721 and following; there are more MCLBYTES that could be changed based on adapter->fmp or mp.
Line 3853 has another one (in addition to more c&p stupidity discussed similar to emac it seems)

sys/dev/ed/if_ed.c

1321

The +2 should probably all be ETHER_ALIGN; untreated to this change ;-)

sys/dev/en/midway.c

843

Why not at least check m->m_len first to see what size you want?
Probably exposes another constant again:( But common theme it seems.

857

Line 2928 is another MLEN that wants changing.
Line 1951 looks spookily like it could be M_SIZE() as well instead of MCLBYTES but it is weird.

sys/dev/ex/if_ex.c

783

I gave up on the logic, but the changes seem fine. I wonder if this driver wants to be de-orbited ;-)

sys/dev/fatm/if_fatm.c

1768

Line 1199 looks like it can be M_SIZE() as well

sys/dev/fxp/if_fxp.c

2660

That one can be changed as well, I'd say.

2673

Line 2744 is like 2660 and should get the same treatment.

sys/dev/hifn/hifn7751.c

1898

This is another M_SIZE(m0) given the follow-up usage, right?

sys/dev/ie/if_ie.c

732

In theory that could be changed to M_SIZE as well.

736

You touched the driver, you own it :-p /SCNR :D

sys/dev/iscsi_initiator/isc_soc.c

137

I keep wondering about this one. That's a lookahead :-(
That code should be slightly rewritten I guess.

sys/dev/ixgb/if_ixgb.c

1801

Unrelated, but do I have to understand this one?

sys/dev/ixgbe/ixv.c

2940

This line should go before m_adj()

sys/dev/ixl/ixl_txrx.c

1150

This line should move before m_adj()

sys/dev/jme/if_jme.c

3200

There's plenty of MCLBYTES some of which look remotely resolvable but maybe in a next pass.

sys/dev/le/lance.c

422

Looks like a missed one.

[bz]

Ok, that was less than the first half :-)
There's a lot of *whistle* code out there that wants to be improved though.

sys/dev/netfpga10g/nf10bmac/if_nf10bmac.c

383

This following block is (as pointed out before) fairly common logic. It'll soon be irrelevant hopefully but otherwise fold it into something.

sys/dev/safe/safe.c

1353

Looks like a missed one

sys/dev/sbni/if_sbni.c

876

The 2 should probably be ETHER_ALIGN but unrelated to this change.

sys/dev/vx/if_vx.c

871

This is an actual bugfix, as the previous if() check was wrong.

sys/netgraph/atm/ccatm/ng_ccatm.c

975

These two lines could be M_SIZE() if we would check that m is valid.

sys/netgraph/bluetooth/socket/ng_btsocket_rfcomm.c

1854

Not sure about this one; m0 could have an EXT in which case this might flip to a big length?

[trasz]

sys/dev/iscsi_initiator/isc_soc.c

112

I don't think the old initiator (or the new initiator, fwiw) actually using AHS at all. So this is probably dead code already.

(I've tested the old initiator with the already committed patches and it seems to work.)

[kmacy]

Could you please update this with respect to the latest HEAD? This would be valuable for some iflib performance work.

Thanks in advance.
-M