Linus and the two youts

Something about make_u32_from_two_u16()

The creator of Linux, Linus Torvalds, is famous for his rants. This latest rant is linked below. As this is my bailiwick, where I am an expert, I thought I’d comment.

https://lore.kernel.org/lkml/CAHk-=wjLCqUUWd8DzG+xsOn-yVL0Q=O35U9D6j6=2DUWX52ghQ@mail.gmail.com/

tl;dr: He’s right on some things, and wrong on other things.

What I bring to this debate are:

1. actually looking at the code, browsing git commits

2. perspective of a very senior programmer who has been through these debates before

Let’s read the code

The pull request added two macros in a core Linux file <linux/include/wordpart.h>, which is in turn included within <linux/include/kernel.h>, one of the most central files within the Linux kernel. The added macros are highlighted in bold below:

#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
#define lower_32_bits(n) ((u32)((n) & 0xffffffff))
#define upper_16_bits(n) ((u16)((n) >> 16))
#define lower_16_bits(n) ((u16)((n) & 0xffff))
#define make_u32_from_two_u16(hi, lo)	(((u32)(hi) << 16) | (u32)(lo))
#define make_u64_from_two_u32(hi, lo)	(((u64)(hi) << 32) | (u32)(lo))
#define REPEAT_BYTE(x)	((~0ul / 0xff) * (x))
#define REPEAT_BYTE_U32(x)	lower_32_bits(REPEAT_BYTE(x))
#ifdef __LITTLE_ENDIAN
#  define aligned_byte_mask(n) ((1UL << 8*(n))-1)
#else
#  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
#endif

The thing to note about this is that their definition of make_u32_from_two_u16() is wrong. It casts the low part to a (u32) instead of a (u16). If the original variable has high-order bits (such as by being signed and negative), it’ll incorrectly put those bits in the upper part. In other words, make_u32_from_two_u16(0,-1) will be 0xFFFFFFFF.

The definition should’ve looked like, making it (u16)lo.

#define make_u32_from_two_u16(hi, lo)	(((u32)(hi) << 16) | (u16)(lo))

This is a pernicious sort of bug. It’ll almost never show up, and is impossible to test for.

But when the bug does appear, it’s impossible to fix. That’s because you can’t be sure that some other code doesn’t depend upon this bad behavior combining two (u32) integers. Its behavior becomes cast in stone.

If you are going to add such a utility function that’s visible from the entire Linux kernel, then you’d damn well get it right. They got it wrong.

In any case, this macro was added purely to support the definitions in <include/linux/mailbox/riscv-rpmi-message.h>.

#define RPMI_VER_MAJOR(__ver)		upper_16_bits(__ver)
#define RPMI_VER_MINOR(__ver)		lower_16_bits(__ver)
#define RPMI_MKVER(__maj, __min)	make_u32_from_two_u16(__maj, __min)

This RPMI_MKVER() macro is then used in the file <drivers/clk/clk-rpmi.c>.

if (msg.attr.value < RPMI_MKVER(1, 0)) {
    return dev_err_probe(dev, -EINVAL,
     "msg protocol version mismatch, expected 0x%x, found 0x%x\n",
				     RPMI_MKVER(1, 0), msg.attr.value);
}

This sort sort of packing version numbers into an integer is incredibly common and you see the same coding pattern across thousands of drivers and libraries. A version number like v1.0 is encoded as 0x00010000. The above snippet of code is one of many common ways of dealing with the problem.

There are other common patterns. An alternate solution might be the following, getting rid of the whole hierarchy of macros.

#define RPMI_VERSION_1_0 0x00010000u
if (msg.attr.value < RPMI_VERSION_1_0) {
    return dev_err_probe(dev, -EINVAL,
     "msg protocol version mismatch, expected 0x%x, found 0x%x\n",
				     RPMI_VERSION_1_0, msg.attr.value);
}

(I like this solution better, by the way.)

What we see here is a common hubris among programmers. When they see there is no standard solution for a common problem, where programmers solve it differently, often poorly, they decide that other programmers need to be told how to do it in one standard manner. Hence, they put their macro in a global file included throughout the kernel instead of confining their changes to just their own code. It’s such a fine, beautiful thing, everyone should use it.

This is where technical debt comes from, namely, programmers designing for future code reuse. There is a toxic culture centered around reusing code, such as “DRY” or “Don’t Repeat Yourself”. Instead of simply solving the problem at hand, programmers try to create general solutions first, then use that generic solution to solve their specific problem.

The cost of this approach is demonstrated above, where a bug becomes cast in stone, where it cannot be fixed without knowing whether this introduces a new bug.

The better way of programming is to just solve the problem at hand as simply and narrowly as you can. That’s why I’d just create a RPMI_VERSION_1_0 constant. It’s even better at communicating intent, without introduce a whole lot of technical debt from multiple layers of macros.

Where Torvalds is wrong

Linus makes some strong points, which I expanded upon in the above section. But he’s wrong in a lot of other ways.

First of all, the use of such macros to hide details is personal preference. There is a constant ying-vs-yang in such situations where sometimes your preference is to see the raw details, and sometime your preference is to see the intent rather than details. The same programmer might have different preferences at different times.

I, too, have cursed programmers for unnecessarily obfuscating trivial details. If voodoo worked, I’d have a shelf of programmer dolls to stick pins into. But at the same time, when writing code, I’ve decided that it’s more appropriate to show intent than details, so am guilty of this myself. (I had a manager once complain about this in my code.)

Linus has more reason to complain about polluting the global namespace here. The <wordpart.h> file is included in pretty much every source file of the Linux kernel, so that’s a huge impact for something that’s only actually used in one place.

On the other hand, this isn’t strictly policed. As we’ll see below, it’s the evolution of the kernel. When people solve a common problem, they make a solution commonly available, and the amount of stuff included through <include/linux/kernel.h> grows over time. Linus should have a stricter police against people letting their shit taint the overall kernel, but he doesn’t.

One of Linus’s complaints is that the name of the macro doesn’t communicate which side is the high portion, and which is the low. Okay boomer. The macro isn’t defined using the parameters (a, b) as Linus suggests, but (hi, lo), clearly communicating what Linus says is missing. Modern editors show macro parameters, such as in the screenshot from VSCode:

What we see in this screenshot is how the editor pops up a display, where not only can we see the order of parameters, but also the implementation of the macro. Which is high and which is low is clear and unambiguous, just not in the name.

It’s unreasonable to expect code to be written as it was back in the days of the original Unix, which was largely written using paper teletypes and the ed program. We have modern IDEs and should use them.

Linus discusses the implementation of the macro, pointing out (as I did above) you need a (u16) cast to prevent “high bits from polluting the result”. But he describes the macro as:

(a << 16) + b

Don’t mix operator families like this, even with parentheses. The correct macro should be consistent. Either the addition + should be changed to an OR |, or the shift << should be changed to multiplication *.

(a << 16) | b
(a * 0x10000) + b

It’s a small thing, and one of personal preference, I guess. But there are a ton of bugs in code that come from mixing operator families. Programmers understand the order of precedent of related operators, but not of unrelated ones. Moreover, it’s a code purity thing here. What’s being done is manipulating bits, not performing arithmetic.

All this minor stuff is really just a warmup to the macro immediately preceding make_u32_from_two_u16() in <wordpart.h>:

#define lower_16_bits(n) ((u16)((n) & 0xffff))

This should blow your mind if you are a programmer. Combining two integers isn’t the most trivial macro imaginable, this extraction of the lower 16 bits is.

This is so incredibly unnecessary it’s hard to imagine it exists, or is even used. Yet, 15 files in the Linux kernel use this macro.

It was added in June of 2021 to <include/linux/kernel.h> in an otherwise simple pull request. Last year, this family of macros was moved to the newly created <wordpart.h> file, which <kernel.h> then included. We can see the evolution over time as each of these macros was added to the kernel, and then the uptake as people started using them.

If such trivial macros already exist, it’s not unreasonable to add more of the same type.

Combining smaller integers into a bigger one is an extraordinary common task. For example, Microsoft has a MAKEDLONG() macro that’s used extensively throughout Windows. As mentioned above, programmers can actually get it wrong by not masking off high bits. It’s not unreasonable for the Linux kernel to have a standard definition of this to reduce people getting it wrong.

Linus is correct that polluting the entire kernel with a bad make_u32_from_two_u16() is bad, and the programmers should feel bad. But at the same time, he’s wrong on other stuff.

Linus is very nice

Rather than substance what offends most people is his tone. Calling somebody’s code “garbage” and “get bent” certainly doesn’t sound like nice language.

But, in fact, he’s extremely nice.

Bluntness is a virtue. There is no polite way to communicate the importance of the issue.

The rudeness here was from programmers who were so impressed by the beauty of their own code that they decided to pollute the entire kernel with it, telling everybody else how they should solve the same problem in their own code.

It’s a hubris that infects all big engineering organizations and big open source projects. There are a lot of relatively junior programmers who want nothing more than to tell everyone else how to code. It’s a constant battle pursued by passive aggressive politeness.

The notable attribute of Linus’s bluntness is that it’s not actually personal. It certainly sounds personal the way he attacks their baby, but it’s not. He makes it clear that he welcomes their further contributions. This code is garbage, not the people who wrote it.

Conflicts in engineering organizations and large projects are usually personal. That Linus makes this not-personal is a big tribute to him.

His issues have substance. While I argue above that it’s more a personal preference, there’s actually a lot of history and complexity behind the issue.

The easiest programmers to deal with are those who are direct, on point, and who don’t hold personal grudges. Even if their language is sometimes offensive, it’s far better than the norm of passive aggression and personal grudges.

Linus Torvalds is God. His time is a lot more valuable than yours. That he takes the time to teach you to be a better programmer is incredibly gracious. If he had to take the time to figure out how to communicate the level of garbage in your code without using impolite terms like “garbage”, he wouldn’t take the time at all. Be thankful.

Conclusion

I find the debate here tedious.

People aren’t discussing the code itself, which is why I went through and linked the files so you can easily see the code and what’s really happening.

People take the story as given, without thinking in terms of the global context. There are larger debates here about coupling, technical debt, and programmers trying to promote their solutions.

For me, the fatal flaw is masking off the high bits, such as with (u16)lo. That’s just bad programming and technical debt that the original programmers should feel real bad.

But I’m easy-going on the rest of it. It’s a constant battle between programmers wanting to communicate intent rather than implementation details, and the desire of programmers (especially senior ones) to see more of the implementation details. There’s a common problem here, and it’s not unreasonable to want a common solution.