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