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