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