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