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