14: GETTING THE CODE RIGHT 2 3While there is much to be said for a solid and community-oriented design 4process, the proof of any kernel development project is in the resulting 5code. It is the code which will be examined by other developers and merged 6(or not) into the mainline tree. So it is the quality of this code which 7will determine the ultimate success of the project. 8 9This section will examine the coding process. We'll start with a look at a 10number of ways in which kernel developers can go wrong. Then the focus 11will shift toward doing things right and the tools which can help in that 12quest. 13 14 154.1: PITFALLS 16 17* Coding style 18 19The kernel has long had a standard coding style, described in 20Documentation/CodingStyle. For much of that time, the policies described 21in that file were taken as being, at most, advisory. As a result, there is 22a substantial amount of code in the kernel which does not meet the coding 23style guidelines. The presence of that code leads to two independent 24hazards for kernel developers. 25 26The first of these is to believe that the kernel coding standards do not 27matter and are not enforced. The truth of the matter is that adding new 28code to the kernel is very difficult if that code is not coded according to 29the standard; many developers will request that the code be reformatted 30before they will even review it. A code base as large as the kernel 31requires some uniformity of code to make it possible for developers to 32quickly understand any part of it. So there is no longer room for 33strangely-formatted code. 34 35Occasionally, the kernel's coding style will run into conflict with an 36employer's mandated style. In such cases, the kernel's style will have to 37win before the code can be merged. Putting code into the kernel means 38giving up a degree of control in a number of ways - including control over 39how the code is formatted. 40 41The other trap is to assume that code which is already in the kernel is 42urgently in need of coding style fixes. Developers may start to generate 43reformatting patches as a way of gaining familiarity with the process, or 44as a way of getting their name into the kernel changelogs - or both. But 45pure coding style fixes are seen as noise by the development community; 46they tend to get a chilly reception. So this type of patch is best 47avoided. It is natural to fix the style of a piece of code while working 48on it for other reasons, but coding style changes should not be made for 49their own sake. 50 51The coding style document also should not be read as an absolute law which 52can never be transgressed. If there is a good reason to go against the 53style (a line which becomes far less readable if split to fit within the 5480-column limit, for example), just do it. 55 56 57* Abstraction layers 58 59Computer Science professors teach students to make extensive use of 60abstraction layers in the name of flexibility and information hiding. 61Certainly the kernel makes extensive use of abstraction; no project 62involving several million lines of code could do otherwise and survive. 63But experience has shown that excessive or premature abstraction can be 64just as harmful as premature optimization. Abstraction should be used to 65the level required and no further. 66 67At a simple level, consider a function which has an argument which is 68always passed as zero by all callers. One could retain that argument just 69in case somebody eventually needs to use the extra flexibility that it 70provides. By that time, though, chances are good that the code which 71implements this extra argument has been broken in some subtle way which was 72never noticed - because it has never been used. Or, when the need for 73extra flexibility arises, it does not do so in a way which matches the 74programmer's early expectation. Kernel developers will routinely submit 75patches to remove unused arguments; they should, in general, not be added 76in the first place. 77 78Abstraction layers which hide access to hardware - often to allow the bulk 79of a driver to be used with multiple operating systems - are especially 80frowned upon. Such layers obscure the code and may impose a performance 81penalty; they do not belong in the Linux kernel. 82 83On the other hand, if you find yourself copying significant amounts of code 84from another kernel subsystem, it is time to ask whether it would, in fact, 85make sense to pull out some of that code into a separate library or to 86implement that functionality at a higher level. There is no value in 87replicating the same code throughout the kernel. 88 89 90* #ifdef and preprocessor use in general 91 92The C preprocessor seems to present a powerful temptation to some C 93programmers, who see it as a way to efficiently encode a great deal of 94flexibility into a source file. But the preprocessor is not C, and heavy 95use of it results in code which is much harder for others to read and 96harder for the compiler to check for correctness. Heavy preprocessor use 97is almost always a sign of code which needs some cleanup work. 98 99Conditional compilation with #ifdef is, indeed, a powerful feature, and it 100is used within the kernel. But there is little desire to see code which is 101sprinkled liberally with #ifdef blocks. As a general rule, #ifdef use 102should be confined to header files whenever possible. 103Conditionally-compiled code can be confined to functions which, if the code 104is not to be present, simply become empty. The compiler will then quietly 105optimize out the call to the empty function. The result is far cleaner 106code which is easier to follow. 107 108C preprocessor macros present a number of hazards, including possible 109multiple evaluation of expressions with side effects and no type safety. 110If you are tempted to define a macro, consider creating an inline function 111instead. The code which results will be the same, but inline functions are 112easier to read, do not evaluate their arguments multiple times, and allow 113the compiler to perform type checking on the arguments and return value. 114 115 116* Inline functions 117 118Inline functions present a hazard of their own, though. Programmers can 119become enamored of the perceived efficiency inherent in avoiding a function 120call and fill a source file with inline functions. Those functions, 121however, can actually reduce performance. Since their code is replicated 122at each call site, they end up bloating the size of the compiled kernel. 123That, in turn, creates pressure on the processor's memory caches, which can 124slow execution dramatically. Inline functions, as a rule, should be quite 125small and relatively rare. The cost of a function call, after all, is not 126that high; the creation of large numbers of inline functions is a classic 127example of premature optimization. 128 129In general, kernel programmers ignore cache effects at their peril. The 130classic time/space tradeoff taught in beginning data structures classes 131often does not apply to contemporary hardware. Space *is* time, in that a 132larger program will run slower than one which is more compact. 133 134More recent compilers take an increasingly active role in deciding whether 135a given function should actually be inlined or not. So the liberal 136placement of "inline" keywords may not just be excessive; it could also be 137irrelevant. 138 139 140* Locking 141 142In May, 2006, the "Devicescape" networking stack was, with great 143fanfare, released under the GPL and made available for inclusion in the 144mainline kernel. This donation was welcome news; support for wireless 145networking in Linux was considered substandard at best, and the Devicescape 146stack offered the promise of fixing that situation. Yet, this code did not 147actually make it into the mainline until June, 2007 (2.6.22). What 148happened? 149 150This code showed a number of signs of having been developed behind 151corporate doors. But one large problem in particular was that it was not 152designed to work on multiprocessor systems. Before this networking stack 153(now called mac80211) could be merged, a locking scheme needed to be 154retrofitted onto it. 155 156Once upon a time, Linux kernel code could be developed without thinking 157about the concurrency issues presented by multiprocessor systems. Now, 158however, this document is being written on a dual-core laptop. Even on 159single-processor systems, work being done to improve responsiveness will 160raise the level of concurrency within the kernel. The days when kernel 161code could be written without thinking about locking are long past. 162 163Any resource (data structures, hardware registers, etc.) which could be 164accessed concurrently by more than one thread must be protected by a lock. 165New code should be written with this requirement in mind; retrofitting 166locking after the fact is a rather more difficult task. Kernel developers 167should take the time to understand the available locking primitives well 168enough to pick the right tool for the job. Code which shows a lack of 169attention to concurrency will have a difficult path into the mainline. 170 171 172* Regressions 173 174One final hazard worth mentioning is this: it can be tempting to make a 175change (which may bring big improvements) which causes something to break 176for existing users. This kind of change is called a "regression," and 177regressions have become most unwelcome in the mainline kernel. With few 178exceptions, changes which cause regressions will be backed out if the 179regression cannot be fixed in a timely manner. Far better to avoid the 180regression in the first place. 181 182It is often argued that a regression can be justified if it causes things 183to work for more people than it creates problems for. Why not make a 184change if it brings new functionality to ten systems for each one it 185breaks? The best answer to this question was expressed by Linus in July, 1862007: 187 188 So we don't fix bugs by introducing new problems. That way lies 189 madness, and nobody ever knows if you actually make any real 190 progress at all. Is it two steps forwards, one step back, or one 191 step forward and two steps back? 192 193(http://lwn.net/Articles/243460/). 194 195An especially unwelcome type of regression is any sort of change to the 196user-space ABI. Once an interface has been exported to user space, it must 197be supported indefinitely. This fact makes the creation of user-space 198interfaces particularly challenging: since they cannot be changed in 199incompatible ways, they must be done right the first time. For this 200reason, a great deal of thought, clear documentation, and wide review for 201user-space interfaces is always required. 202 203 204 2054.2: CODE CHECKING TOOLS 206 207For now, at least, the writing of error-free code remains an ideal that few 208of us can reach. What we can hope to do, though, is to catch and fix as 209many of those errors as possible before our code goes into the mainline 210kernel. To that end, the kernel developers have put together an impressive 211array of tools which can catch a wide variety of obscure problems in an 212automated way. Any problem caught by the computer is a problem which will 213not afflict a user later on, so it stands to reason that the automated 214tools should be used whenever possible. 215 216The first step is simply to heed the warnings produced by the compiler. 217Contemporary versions of gcc can detect (and warn about) a large number of 218potential errors. Quite often, these warnings point to real problems. 219Code submitted for review should, as a rule, not produce any compiler 220warnings. When silencing warnings, take care to understand the real cause 221and try to avoid "fixes" which make the warning go away without addressing 222its cause. 223 224Note that not all compiler warnings are enabled by default. Build the 225kernel with "make EXTRA_CFLAGS=-W" to get the full set. 226 227The kernel provides several configuration options which turn on debugging 228features; most of these are found in the "kernel hacking" submenu. Several 229of these options should be turned on for any kernel used for development or 230testing purposes. In particular, you should turn on: 231 232 - ENABLE_WARN_DEPRECATED, ENABLE_MUST_CHECK, and FRAME_WARN to get an 233 extra set of warnings for problems like the use of deprecated interfaces 234 or ignoring an important return value from a function. The output 235 generated by these warnings can be verbose, but one need not worry about 236 warnings from other parts of the kernel. 237 238 - DEBUG_OBJECTS will add code to track the lifetime of various objects 239 created by the kernel and warn when things are done out of order. If 240 you are adding a subsystem which creates (and exports) complex objects 241 of its own, consider adding support for the object debugging 242 infrastructure. 243 244 - DEBUG_SLAB can find a variety of memory allocation and use errors; it 245 should be used on most development kernels. 246 247 - DEBUG_SPINLOCK, DEBUG_ATOMIC_SLEEP, and DEBUG_MUTEXES will find a 248 number of common locking errors. 249 250There are quite a few other debugging options, some of which will be 251discussed below. Some of them have a significant performance impact and 252should not be used all of the time. But some time spent learning the 253available options will likely be paid back many times over in short order. 254 255One of the heavier debugging tools is the locking checker, or "lockdep." 256This tool will track the acquisition and release of every lock (spinlock or 257mutex) in the system, the order in which locks are acquired relative to 258each other, the current interrupt environment, and more. It can then 259ensure that locks are always acquired in the same order, that the same 260interrupt assumptions apply in all situations, and so on. In other words, 261lockdep can find a number of scenarios in which the system could, on rare 262occasion, deadlock. This kind of problem can be painful (for both 263developers and users) in a deployed system; lockdep allows them to be found 264in an automated manner ahead of time. Code with any sort of non-trivial 265locking should be run with lockdep enabled before being submitted for 266inclusion. 267 268As a diligent kernel programmer, you will, beyond doubt, check the return 269status of any operation (such as a memory allocation) which can fail. The 270fact of the matter, though, is that the resulting failure recovery paths 271are, probably, completely untested. Untested code tends to be broken code; 272you could be much more confident of your code if all those error-handling 273paths had been exercised a few times. 274 275The kernel provides a fault injection framework which can do exactly that, 276especially where memory allocations are involved. With fault injection 277enabled, a configurable percentage of memory allocations will be made to 278fail; these failures can be restricted to a specific range of code. 279Running with fault injection enabled allows the programmer to see how the 280code responds when things go badly. See 281Documentation/fault-injection/fault-injection.txt for more information on 282how to use this facility. 283 284Other kinds of errors can be found with the "sparse" static analysis tool. 285With sparse, the programmer can be warned about confusion between 286user-space and kernel-space addresses, mixture of big-endian and 287small-endian quantities, the passing of integer values where a set of bit 288flags is expected, and so on. Sparse must be installed separately (it can 289be found at https://sparse.wiki.kernel.org/index.php/Main_Page if your 290distributor does not package it); it can then be run on the code by adding 291"C=1" to your make command. 292 293The "Coccinelle" tool (http://coccinelle.lip6.fr/) is able to find a wide 294variety of potential coding problems; it can also propose fixes for those 295problems. Quite a few "semantic patches" for the kernel have been packaged 296under the scripts/coccinelle directory; running "make coccicheck" will run 297through those semantic patches and report on any problems found. See 298Documentation/coccinelle.txt for more information. 299 300Other kinds of portability errors are best found by compiling your code for 301other architectures. If you do not happen to have an S/390 system or a 302Blackfin development board handy, you can still perform the compilation 303step. A large set of cross compilers for x86 systems can be found at 304 305 http://www.kernel.org/pub/tools/crosstool/ 306 307Some time spent installing and using these compilers will help avoid 308embarrassment later. 309 310 3114.3: DOCUMENTATION 312 313Documentation has often been more the exception than the rule with kernel 314development. Even so, adequate documentation will help to ease the merging 315of new code into the kernel, make life easier for other developers, and 316will be helpful for your users. In many cases, the addition of 317documentation has become essentially mandatory. 318 319The first piece of documentation for any patch is its associated 320changelog. Log entries should describe the problem being solved, the form 321of the solution, the people who worked on the patch, any relevant 322effects on performance, and anything else that might be needed to 323understand the patch. Be sure that the changelog says *why* the patch is 324worth applying; a surprising number of developers fail to provide that 325information. 326 327Any code which adds a new user-space interface - including new sysfs or 328/proc files - should include documentation of that interface which enables 329user-space developers to know what they are working with. See 330Documentation/ABI/README for a description of how this documentation should 331be formatted and what information needs to be provided. 332 333The file Documentation/kernel-parameters.txt describes all of the kernel's 334boot-time parameters. Any patch which adds new parameters should add the 335appropriate entries to this file. 336 337Any new configuration options must be accompanied by help text which 338clearly explains the options and when the user might want to select them. 339 340Internal API information for many subsystems is documented by way of 341specially-formatted comments; these comments can be extracted and formatted 342in a number of ways by the "kernel-doc" script. If you are working within 343a subsystem which has kerneldoc comments, you should maintain them and add 344them, as appropriate, for externally-available functions. Even in areas 345which have not been so documented, there is no harm in adding kerneldoc 346comments for the future; indeed, this can be a useful activity for 347beginning kernel developers. The format of these comments, along with some 348information on how to create kerneldoc templates can be found in the file 349Documentation/kernel-doc-nano-HOWTO.txt. 350 351Anybody who reads through a significant amount of existing kernel code will 352note that, often, comments are most notable by their absence. Once again, 353the expectations for new code are higher than they were in the past; 354merging uncommented code will be harder. That said, there is little desire 355for verbosely-commented code. The code should, itself, be readable, with 356comments explaining the more subtle aspects. 357 358Certain things should always be commented. Uses of memory barriers should 359be accompanied by a line explaining why the barrier is necessary. The 360locking rules for data structures generally need to be explained somewhere. 361Major data structures need comprehensive documentation in general. 362Non-obvious dependencies between separate bits of code should be pointed 363out. Anything which might tempt a code janitor to make an incorrect 364"cleanup" needs a comment saying why it is done the way it is. And so on. 365 366 3674.4: INTERNAL API CHANGES 368 369The binary interface provided by the kernel to user space cannot be broken 370except under the most severe circumstances. The kernel's internal 371programming interfaces, instead, are highly fluid and can be changed when 372the need arises. If you find yourself having to work around a kernel API, 373or simply not using a specific functionality because it does not meet your 374needs, that may be a sign that the API needs to change. As a kernel 375developer, you are empowered to make such changes. 376 377There are, of course, some catches. API changes can be made, but they need 378to be well justified. So any patch making an internal API change should be 379accompanied by a description of what the change is and why it is 380necessary. This kind of change should also be broken out into a separate 381patch, rather than buried within a larger patch. 382 383The other catch is that a developer who changes an internal API is 384generally charged with the task of fixing any code within the kernel tree 385which is broken by the change. For a widely-used function, this duty can 386lead to literally hundreds or thousands of changes - many of which are 387likely to conflict with work being done by other developers. Needless to 388say, this can be a large job, so it is best to be sure that the 389justification is solid. Note that the Coccinelle tool can help with 390wide-ranging API changes. 391 392When making an incompatible API change, one should, whenever possible, 393ensure that code which has not been updated is caught by the compiler. 394This will help you to be sure that you have found all in-tree uses of that 395interface. It will also alert developers of out-of-tree code that there is 396a change that they need to respond to. Supporting out-of-tree code is not 397something that kernel developers need to be worried about, but we also do 398not have to make life harder for out-of-tree developers than it needs to 399be. 400