Page MenuHomeFreeBSD

mtod macro: Drop the type argument
Needs ReviewPublic

Authored by cem on Sep 16 2019, 4:59 AM.

Details

Summary

In the past we've moved away from macros with explicit types as parameters
in favor of casting void* results as needed (e.g., MALLOC()). Here, do the
same with mtod()*.

Obviously, this requires changing a lot of C files. To do so, a Coccinelle
semantic patch was used. It is provided in tools for consumers that want to
convert their code to the 1 argument variant.

*: For out of tree consumers, such as ports, a 2-argument variant of the
mtod macro is retained with identical API to the prior version.

The collection of preprocessor macro hacks used to construct a variadic
mtod() is shamelessly appropriated from Gregory Pakosz:
https://stackoverflow.com/a/1872506/218830

Test Plan

The real patch will mechanically convert all files that reference the
macro, but that does not seem especially useful to post on Phabricator. So
instead I have converted a single file for example: netinet/ip_options.c.
No hand-editing was performed.

TODO:

  • Bump __FreeBSD_version
  • Update the manual page mtod is in
  • Fix minor unused variable in cocci script

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 26664
Build 25038: arc lint + arc unit

Event Timeline

cem created this revision.Sep 16 2019, 4:59 AM
cem edited the test plan for this revision. (Show Details)Sep 16 2019, 5:01 AM
cem added inline comments.Sep 16 2019, 5:04 AM
convert_mtod.cocci
24 ↗(On Diff #62142)

This identifier i line is unused and can be dropped. I forgot to remove it when I separated out r1 from r2.

sys/netinet/ip_options.c
104

This and others like it are good examples of nicely eliminated duplication.

456

This is about as legible as it was before, I think.

markj added a comment.Sep 17 2019, 3:31 PM

I have no objections to the change in principle, it's a nice cleanup. It would cause a massive amount of churn: we have about 2000 uses of mtod() in the tree, and downstream trees will have many thousands more. I think such a change deserves a wider discussion on -arch or so.

I agree with mark - this will be a nice cleanup and I think should be done, but will take some careful coordination with downstreams, and ports.

I would suggest that we add convert_mtod.cocci to the tree (somewhere under tools/ perhaps) and give an example of its use to help downstream consumers.

cem added a comment.Sep 17 2019, 8:27 PM

Both remarks seem reasonable to me. I wasn’t going to drop this in the tree without circulating the idea past net@ at a minimum 😀. But I wanted some feedback from a smaller audience first— thank you both.

One additional possibility would be to define mtod in such a way that either 1 or 2 argument versions continue to compile, as an easier onramp for out of tree consumers. (I believe this is possible but haven’t yet confirmed so.) It would be an ugly macro hack, but importantly the ugliness would just be in one place. If that is possible, do either of you have an opinion on whether that seems worse or better than requiring a flag day for out of tree consumers?

markj added a comment.Sep 18 2019, 6:25 PM
In D21669#473159, @cem wrote:

Both remarks seem reasonable to me. I wasn’t going to drop this in the tree without circulating the idea past net@ at a minimum 😀. But I wanted some feedback from a smaller audience first— thank you both.
One additional possibility would be to define mtod in such a way that either 1 or 2 argument versions continue to compile, as an easier onramp for out of tree consumers. (I believe this is possible but haven’t yet confirmed so.) It would be an ugly macro hack, but importantly the ugliness would just be in one place. If that is possible, do either of you have an opinion on whether that seems worse or better than requiring a flag day for out of tree consumers?

I think having a macro compatible with both uses would be much easier to adopt. The only real downsides are that the macro is uglier (not a big deal), and that inconsistencies in its use would be confusing (not really an issue if we can just convert in-tree code using coccinelle).

cem added a comment.Sep 24 2019, 2:36 AM

Here's what the 1 or 2-argument macro looks like (warning: highly ugly, but it works): https://godbolt.org/z/4qBp7t . Will update review shortly.

Shamelessly ripped off from Gregory Pakosz on Stackoverflow: https://stackoverflow.com/questions/1872220/is-it-possible-to-iterate-over-arguments-in-variadic-macros

cem updated this revision to Diff 62493.Sep 24 2019, 3:22 AM
  • Add mtod variant that takes prior two arguments.
  • Place coccinelle script in the tree under tools.

The kernel actually compiles as-is (i.e., ip_options.c converted and the rest
left using the 2-argument variant). I think this demonstrates the variadic
macro is functional.

cem edited the summary of this revision. (Show Details)Sep 24 2019, 3:24 AM
cem edited the test plan for this revision. (Show Details)

Ugly, but amazing. I would suggest committing the macro change first followed by the refactoring.

And mbuf.9 change along with mbuf.h.

cem added a reviewer: bz.Sep 26 2019, 6:01 PM
cem added a subscriber: bz.

CC @bz

Suggestions of the right mailing list to circulate this on welcome.

CCing some networking folks, names kindly provided by Ed.

I would suggest describing this plan on freebsd-net@ and freebsd-arch@ mailing lists

rrs added a comment.Sep 28 2019, 9:27 AM

Well I am going to voice a different view. I do *not* see this as a nice cleanup. I see it
has a lessening of information.

Has a code reader if you see

i = mtod(m, struct ip *)

You *know* that i is a struct ip *.

Now if I can just do

i = mtod(m)

You have less information at that point in the code. Yes in a lot of
places in the network stack we do the

ip = mtod(m, struct ip *)

somewhere at the top so its obvious. But the problem is that
whats in the inbound ip changes and down the road in the code
I may do

p = mtod(m, struct sctphdr *);

or

p = mtod(m, struct tcphdr *);

After doing some sort of m_adj of course...

To me this makes the mtod macro provide *less* information. Yes I can go back in the code
and lookup what a p was, But thats a detriment not a plus in my thinking.

cem added a comment.Sep 28 2019, 2:45 PM
In D21669#476596, @rrs wrote:

Well I am going to voice a different view. I do *not* see this as a nice cleanup. I see it
has a lessening of information.
Has a code reader if you see
i = mtod(m, struct ip *)
You *know* that i is a struct ip *.

Hi Randall,

The automated semantic patch converts instances of i = mtod(m, struct ip *) to i = (struct ip *)mtod(m), generally. Is it not just as clear that i is a struct ip * afterwards?

There are limited exceptions where the cast is dropped automatically — at declaration, where the type of i is already present; and when type is void *. It would be possible to make the same macro change without one or both of these exceptions, if you see either of them as helpful.

I think it would be perfectly fine for the network stack to retain a style convention of explicit casts of mtod() (which you effectively have with the 2-argument version, today) indefinitely. I had no plans to drop the explicit casts in any existing code after this conversion.

Best,
Conrad

cem added a comment.Sep 28 2019, 3:12 PM

I would suggest describing this plan on freebsd-net@ and freebsd-arch@ mailing lists

Sent; I'm in the moderation queue on net@, though.

I'm contemplating what we gain by this change... We get a much more complex definition of mtod() and, for example, in the case of the SCTP source, which are shared with other BSD like platforms which don't make that change, we have to provide a way to do it still the old way. I guess, I have to replace mtod() by SCTP_MTOD() with the 2 argument semantic in the SCTP sources and then make platform specific definitions: On FreeBSD, call your version and do the explicit cast, on the other platforms just call mtod(). Another point is that these mechanical global changes might make MFCing code harder...

So in my view, there is a price to pay. So I would like to understand what we gain.