4.Coding.rst 21 KB

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