426 lines · plain
1.. _development_coding:2 3Getting the code right4======================5 6While there is much to be said for a solid and community-oriented design7process, the proof of any kernel development project is in the resulting8code. It is the code which will be examined by other developers and merged9(or not) into the mainline tree. So it is the quality of this code which10will determine the ultimate success of the project.11 12This section will examine the coding process. We'll start with a look at a13number of ways in which kernel developers can go wrong. Then the focus14will shift toward doing things right and the tools which can help in that15quest.16 17 18Pitfalls19---------20 21Coding style22************23 24The kernel has long had a standard coding style, described in25:ref:`Documentation/process/coding-style.rst <codingstyle>`. For much of26that 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 kernel28which does not meet the coding style guidelines. The presence of that code29leads to two independent hazards for kernel developers.30 31The first of these is to believe that the kernel coding standards do not32matter and are not enforced. The truth of the matter is that adding new33code to the kernel is very difficult if that code is not coded according to34the standard; many developers will request that the code be reformatted35before they will even review it. A code base as large as the kernel36requires some uniformity of code to make it possible for developers to37quickly understand any part of it. So there is no longer room for38strangely-formatted code.39 40Occasionally, the kernel's coding style will run into conflict with an41employer's mandated style. In such cases, the kernel's style will have to42win before the code can be merged. Putting code into the kernel means43giving up a degree of control in a number of ways - including control over44how the code is formatted.45 46The other trap is to assume that code which is already in the kernel is47urgently in need of coding style fixes. Developers may start to generate48reformatting patches as a way of gaining familiarity with the process, or49as a way of getting their name into the kernel changelogs - or both. But50pure coding style fixes are seen as noise by the development community;51they tend to get a chilly reception. So this type of patch is best52avoided. It is natural to fix the style of a piece of code while working53on it for other reasons, but coding style changes should not be made for54their own sake.55 56The coding style document also should not be read as an absolute law which57can never be transgressed. If there is a good reason to go against the58style (a line which becomes far less readable if split to fit within the5980-column limit, for example), just do it.60 61Note that you can also use the ``clang-format`` tool to help you with62these 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/dev-tools/clang-format.rst <clangformat>`67for more details.68 69Some basic editor settings, such as indentation and line endings, will be70set automatically if you are using an editor that is compatible with71EditorConfig. See the official EditorConfig website for more information:72https://editorconfig.org/73 74Abstraction layers75******************76 77Computer Science professors teach students to make extensive use of78abstraction layers in the name of flexibility and information hiding.79Certainly the kernel makes extensive use of abstraction; no project80involving several million lines of code could do otherwise and survive.81But experience has shown that excessive or premature abstraction can be82just as harmful as premature optimization. Abstraction should be used to83the level required and no further.84 85At a simple level, consider a function which has an argument which is86always passed as zero by all callers. One could retain that argument just87in case somebody eventually needs to use the extra flexibility that it88provides. By that time, though, chances are good that the code which89implements this extra argument has been broken in some subtle way which was90never noticed - because it has never been used. Or, when the need for91extra flexibility arises, it does not do so in a way which matches the92programmer's early expectation. Kernel developers will routinely submit93patches to remove unused arguments; they should, in general, not be added94in the first place.95 96Abstraction layers which hide access to hardware - often to allow the bulk97of a driver to be used with multiple operating systems - are especially98frowned upon. Such layers obscure the code and may impose a performance99penalty; they do not belong in the Linux kernel.100 101On the other hand, if you find yourself copying significant amounts of code102from another kernel subsystem, it is time to ask whether it would, in fact,103make sense to pull out some of that code into a separate library or to104implement that functionality at a higher level. There is no value in105replicating the same code throughout the kernel.106 107 108#ifdef and preprocessor use in general109**************************************110 111The C preprocessor seems to present a powerful temptation to some C112programmers, who see it as a way to efficiently encode a great deal of113flexibility into a source file. But the preprocessor is not C, and heavy114use of it results in code which is much harder for others to read and115harder for the compiler to check for correctness. Heavy preprocessor use116is almost always a sign of code which needs some cleanup work.117 118Conditional compilation with #ifdef is, indeed, a powerful feature, and it119is used within the kernel. But there is little desire to see code which is120sprinkled liberally with #ifdef blocks. As a general rule, #ifdef use121should be confined to header files whenever possible.122Conditionally-compiled code can be confined to functions which, if the code123is not to be present, simply become empty. The compiler will then quietly124optimize out the call to the empty function. The result is far cleaner125code which is easier to follow.126 127C preprocessor macros present a number of hazards, including possible128multiple evaluation of expressions with side effects and no type safety.129If you are tempted to define a macro, consider creating an inline function130instead. The code which results will be the same, but inline functions are131easier to read, do not evaluate their arguments multiple times, and allow132the compiler to perform type checking on the arguments and return value.133 134 135Inline functions136****************137 138Inline functions present a hazard of their own, though. Programmers can139become enamored of the perceived efficiency inherent in avoiding a function140call and fill a source file with inline functions. Those functions,141however, can actually reduce performance. Since their code is replicated142at each call site, they end up bloating the size of the compiled kernel.143That, in turn, creates pressure on the processor's memory caches, which can144slow execution dramatically. Inline functions, as a rule, should be quite145small and relatively rare. The cost of a function call, after all, is not146that high; the creation of large numbers of inline functions is a classic147example of premature optimization.148 149In general, kernel programmers ignore cache effects at their peril. The150classic time/space tradeoff taught in beginning data structures classes151often does not apply to contemporary hardware. Space *is* time, in that a152larger program will run slower than one which is more compact.153 154More recent compilers take an increasingly active role in deciding whether155a given function should actually be inlined or not. So the liberal156placement of "inline" keywords may not just be excessive; it could also be157irrelevant.158 159 160Locking161*******162 163In May, 2006, the "Devicescape" networking stack was, with great164fanfare, released under the GPL and made available for inclusion in the165mainline kernel. This donation was welcome news; support for wireless166networking in Linux was considered substandard at best, and the Devicescape167stack offered the promise of fixing that situation. Yet, this code did not168actually make it into the mainline until June, 2007 (2.6.22). What169happened?170 171This code showed a number of signs of having been developed behind172corporate doors. But one large problem in particular was that it was not173designed to work on multiprocessor systems. Before this networking stack174(now called mac80211) could be merged, a locking scheme needed to be175retrofitted onto it.176 177Once upon a time, Linux kernel code could be developed without thinking178about the concurrency issues presented by multiprocessor systems. Now,179however, this document is being written on a dual-core laptop. Even on180single-processor systems, work being done to improve responsiveness will181raise the level of concurrency within the kernel. The days when kernel182code could be written without thinking about locking are long past.183 184Any resource (data structures, hardware registers, etc.) which could be185accessed concurrently by more than one thread must be protected by a lock.186New code should be written with this requirement in mind; retrofitting187locking after the fact is a rather more difficult task. Kernel developers188should take the time to understand the available locking primitives well189enough to pick the right tool for the job. Code which shows a lack of190attention to concurrency will have a difficult path into the mainline.191 192 193Regressions194***********195 196One final hazard worth mentioning is this: it can be tempting to make a197change (which may bring big improvements) which causes something to break198for existing users. This kind of change is called a "regression," and199regressions have become most unwelcome in the mainline kernel. With few200exceptions, changes which cause regressions will be backed out if the201regression cannot be fixed in a timely manner. Far better to avoid the202regression in the first place.203 204It is often argued that a regression can be justified if it causes things205to work for more people than it creates problems for. Why not make a206change if it brings new functionality to ten systems for each one it207breaks? The best answer to this question was expressed by Linus in July,2082007:209 210::211 212 So we don't fix bugs by introducing new problems. That way lies213 madness, and nobody ever knows if you actually make any real214 progress at all. Is it two steps forwards, one step back, or one215 step forward and two steps back?216 217(https://lwn.net/Articles/243460/).218 219An especially unwelcome type of regression is any sort of change to the220user-space ABI. Once an interface has been exported to user space, it must221be supported indefinitely. This fact makes the creation of user-space222interfaces particularly challenging: since they cannot be changed in223incompatible ways, they must be done right the first time. For this224reason, a great deal of thought, clear documentation, and wide review for225user-space interfaces is always required.226 227 228Code checking tools229-------------------230 231For now, at least, the writing of error-free code remains an ideal that few232of us can reach. What we can hope to do, though, is to catch and fix as233many of those errors as possible before our code goes into the mainline234kernel. To that end, the kernel developers have put together an impressive235array of tools which can catch a wide variety of obscure problems in an236automated way. Any problem caught by the computer is a problem which will237not afflict a user later on, so it stands to reason that the automated238tools should be used whenever possible.239 240The first step is simply to heed the warnings produced by the compiler.241Contemporary versions of gcc can detect (and warn about) a large number of242potential errors. Quite often, these warnings point to real problems.243Code submitted for review should, as a rule, not produce any compiler244warnings. When silencing warnings, take care to understand the real cause245and try to avoid "fixes" which make the warning go away without addressing246its cause.247 248Note that not all compiler warnings are enabled by default. Build the249kernel with "make KCFLAGS=-W" to get the full set.250 251The kernel provides several configuration options which turn on debugging252features; most of these are found in the "kernel hacking" submenu. Several253of these options should be turned on for any kernel used for development or254testing purposes. In particular, you should turn on:255 256 - FRAME_WARN to get warnings for stack frames larger than a given amount.257 The output generated can be verbose, but one need not worry about258 warnings from other parts of the kernel.259 260 - DEBUG_OBJECTS will add code to track the lifetime of various objects261 created by the kernel and warn when things are done out of order. If262 you are adding a subsystem which creates (and exports) complex objects263 of its own, consider adding support for the object debugging264 infrastructure.265 266 - DEBUG_SLAB can find a variety of memory allocation and use errors; it267 should be used on most development kernels.268 269 - DEBUG_SPINLOCK, DEBUG_ATOMIC_SLEEP, and DEBUG_MUTEXES will find a270 number of common locking errors.271 272There are quite a few other debugging options, some of which will be273discussed below. Some of them have a significant performance impact and274should not be used all of the time. But some time spent learning the275available options will likely be paid back many times over in short order.276 277One of the heavier debugging tools is the locking checker, or "lockdep."278This tool will track the acquisition and release of every lock (spinlock or279mutex) in the system, the order in which locks are acquired relative to280each other, the current interrupt environment, and more. It can then281ensure that locks are always acquired in the same order, that the same282interrupt assumptions apply in all situations, and so on. In other words,283lockdep can find a number of scenarios in which the system could, on rare284occasion, deadlock. This kind of problem can be painful (for both285developers and users) in a deployed system; lockdep allows them to be found286in an automated manner ahead of time. Code with any sort of non-trivial287locking should be run with lockdep enabled before being submitted for288inclusion.289 290As a diligent kernel programmer, you will, beyond doubt, check the return291status of any operation (such as a memory allocation) which can fail. The292fact of the matter, though, is that the resulting failure recovery paths293are, probably, completely untested. Untested code tends to be broken code;294you could be much more confident of your code if all those error-handling295paths had been exercised a few times.296 297The kernel provides a fault injection framework which can do exactly that,298especially where memory allocations are involved. With fault injection299enabled, a configurable percentage of memory allocations will be made to300fail; these failures can be restricted to a specific range of code.301Running with fault injection enabled allows the programmer to see how the302code responds when things go badly. See303Documentation/fault-injection/fault-injection.rst for more information on304how to use this facility.305 306Other kinds of errors can be found with the "sparse" static analysis tool.307With sparse, the programmer can be warned about confusion between308user-space and kernel-space addresses, mixture of big-endian and309small-endian quantities, the passing of integer values where a set of bit310flags is expected, and so on. Sparse must be installed separately (it can311be found at https://sparse.wiki.kernel.org/index.php/Main_Page if your312distributor does not package it); it can then be run on the code by adding313"C=1" to your make command.314 315The "Coccinelle" tool (http://coccinelle.lip6.fr/) is able to find a wide316variety of potential coding problems; it can also propose fixes for those317problems. Quite a few "semantic patches" for the kernel have been packaged318under the scripts/coccinelle directory; running "make coccicheck" will run319through those semantic patches and report on any problems found. See320:ref:`Documentation/dev-tools/coccinelle.rst <devtools_coccinelle>`321for more information.322 323Other kinds of portability errors are best found by compiling your code for324other architectures. If you do not happen to have an S/390 system or a325Blackfin development board handy, you can still perform the compilation326step. A large set of cross compilers for x86 systems can be found at327 328 https://www.kernel.org/pub/tools/crosstool/329 330Some time spent installing and using these compilers will help avoid331embarrassment later.332 333 334Documentation335-------------336 337Documentation has often been more the exception than the rule with kernel338development. Even so, adequate documentation will help to ease the merging339of new code into the kernel, make life easier for other developers, and340will be helpful for your users. In many cases, the addition of341documentation has become essentially mandatory.342 343The first piece of documentation for any patch is its associated344changelog. Log entries should describe the problem being solved, the form345of the solution, the people who worked on the patch, any relevant346effects on performance, and anything else that might be needed to347understand the patch. Be sure that the changelog says *why* the patch is348worth applying; a surprising number of developers fail to provide that349information.350 351Any code which adds a new user-space interface - including new sysfs or352/proc files - should include documentation of that interface which enables353user-space developers to know what they are working with. See354Documentation/ABI/README for a description of how this documentation should355be formatted and what information needs to be provided.356 357The file :ref:`Documentation/admin-guide/kernel-parameters.rst358<kernelparameters>` describes all of the kernel's boot-time parameters.359Any patch which adds new parameters should add the appropriate entries to360this file.361 362Any new configuration options must be accompanied by help text which363clearly explains the options and when the user might want to select them.364 365Internal API information for many subsystems is documented by way of366specially-formatted comments; these comments can be extracted and formatted367in a number of ways by the "kernel-doc" script. If you are working within368a subsystem which has kerneldoc comments, you should maintain them and add369them, as appropriate, for externally-available functions. Even in areas370which have not been so documented, there is no harm in adding kerneldoc371comments for the future; indeed, this can be a useful activity for372beginning kernel developers. The format of these comments, along with some373information on how to create kerneldoc templates can be found at374:ref:`Documentation/doc-guide/ <doc_guide>`.375 376Anybody who reads through a significant amount of existing kernel code will377note that, often, comments are most notable by their absence. Once again,378the expectations for new code are higher than they were in the past;379merging uncommented code will be harder. That said, there is little desire380for verbosely-commented code. The code should, itself, be readable, with381comments explaining the more subtle aspects.382 383Certain things should always be commented. Uses of memory barriers should384be accompanied by a line explaining why the barrier is necessary. The385locking rules for data structures generally need to be explained somewhere.386Major data structures need comprehensive documentation in general.387Non-obvious dependencies between separate bits of code should be pointed388out. Anything which might tempt a code janitor to make an incorrect389"cleanup" needs a comment saying why it is done the way it is. And so on.390 391 392Internal API changes393--------------------394 395The binary interface provided by the kernel to user space cannot be broken396except under the most severe circumstances. The kernel's internal397programming interfaces, instead, are highly fluid and can be changed when398the need arises. If you find yourself having to work around a kernel API,399or simply not using a specific functionality because it does not meet your400needs, that may be a sign that the API needs to change. As a kernel401developer, you are empowered to make such changes.402 403There are, of course, some catches. API changes can be made, but they need404to be well justified. So any patch making an internal API change should be405accompanied by a description of what the change is and why it is406necessary. This kind of change should also be broken out into a separate407patch, rather than buried within a larger patch.408 409The other catch is that a developer who changes an internal API is410generally charged with the task of fixing any code within the kernel tree411which is broken by the change. For a widely-used function, this duty can412lead to literally hundreds or thousands of changes - many of which are413likely to conflict with work being done by other developers. Needless to414say, this can be a large job, so it is best to be sure that the415justification is solid. Note that the Coccinelle tool can help with416wide-ranging API changes.417 418When making an incompatible API change, one should, whenever possible,419ensure that code which has not been updated is caught by the compiler.420This will help you to be sure that you have found all in-tree uses of that421interface. It will also alert developers of out-of-tree code that there is422a change that they need to respond to. Supporting out-of-tree code is not423something that kernel developers need to be worried about, but we also do424not have to make life harder for out-of-tree developers than it needs to425be.426