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 | ||
134 | ||
135 | * Locking | |
136 | ||
137 | In May, 2006, the "Devicescape" networking stack was, with great | |
138 | fanfare, released under the GPL and made available for inclusion in the | |
139 | mainline kernel. This donation was welcome news; support for wireless | |
140 | networking in Linux was considered substandard at best, and the Devicescape | |
141 | stack offered the promise of fixing that situation. Yet, this code did not | |
142 | actually make it into the mainline until June, 2007 (2.6.22). What | |
143 | happened? | |
144 | ||
145 | This code showed a number of signs of having been developed behind | |
146 | corporate doors. But one large problem in particular was that it was not | |
147 | designed to work on multiprocessor systems. Before this networking stack | |
148 | (now called mac80211) could be merged, a locking scheme needed to be | |
149 | retrofitted onto it. | |
150 | ||
151 | Once upon a time, Linux kernel code could be developed without thinking | |
152 | about the concurrency issues presented by multiprocessor systems. Now, | |
153 | however, this document is being written on a dual-core laptop. Even on | |
154 | single-processor systems, work being done to improve responsiveness will | |
155 | raise the level of concurrency within the kernel. The days when kernel | |
156 | code could be written without thinking about locking are long past. | |
157 | ||
158 | Any resource (data structures, hardware registers, etc.) which could be | |
159 | accessed concurrently by more than one thread must be protected by a lock. | |
160 | New code should be written with this requirement in mind; retrofitting | |
161 | locking after the fact is a rather more difficult task. Kernel developers | |
162 | should take the time to understand the available locking primitives well | |
163 | enough to pick the right tool for the job. Code which shows a lack of | |
164 | attention to concurrency will have a difficult path into the mainline. | |
165 | ||
166 | ||
167 | * Regressions | |
168 | ||
169 | One final hazard worth mentioning is this: it can be tempting to make a | |
170 | change (which may bring big improvements) which causes something to break | |
171 | for existing users. This kind of change is called a "regression," and | |
172 | regressions have become most unwelcome in the mainline kernel. With few | |
173 | exceptions, changes which cause regressions will be backed out if the | |
174 | regression cannot be fixed in a timely manner. Far better to avoid the | |
175 | regression in the first place. | |
176 | ||
177 | It is often argued that a regression can be justified if it causes things | |
178 | to work for more people than it creates problems for. Why not make a | |
179 | change if it brings new functionality to ten systems for each one it | |
180 | breaks? The best answer to this question was expressed by Linus in July, | |
181 | 2007: | |
182 | ||
183 | So we don't fix bugs by introducing new problems. That way lies | |
184 | madness, and nobody ever knows if you actually make any real | |
185 | progress at all. Is it two steps forwards, one step back, or one | |
186 | step forward and two steps back? | |
187 | ||
188 | (http://lwn.net/Articles/243460/). | |
189 | ||
190 | An especially unwelcome type of regression is any sort of change to the | |
191 | user-space ABI. Once an interface has been exported to user space, it must | |
192 | be supported indefinitely. This fact makes the creation of user-space | |
193 | interfaces particularly challenging: since they cannot be changed in | |
194 | incompatible ways, they must be done right the first time. For this | |
195 | reason, a great deal of thought, clear documentation, and wide review for | |
196 | user-space interfaces is always required. | |
197 | ||
198 | ||
199 | ||
200 | 4.2: CODE CHECKING TOOLS | |
201 | ||
202 | For now, at least, the writing of error-free code remains an ideal that few | |
203 | of us can reach. What we can hope to do, though, is to catch and fix as | |
204 | many of those errors as possible before our code goes into the mainline | |
205 | kernel. To that end, the kernel developers have put together an impressive | |
206 | array of tools which can catch a wide variety of obscure problems in an | |
207 | automated way. Any problem caught by the computer is a problem which will | |
208 | not afflict a user later on, so it stands to reason that the automated | |
209 | tools should be used whenever possible. | |
210 | ||
211 | The first step is simply to heed the warnings produced by the compiler. | |
212 | Contemporary versions of gcc can detect (and warn about) a large number of | |
213 | potential errors. Quite often, these warnings point to real problems. | |
214 | Code submitted for review should, as a rule, not produce any compiler | |
215 | warnings. When silencing warnings, take care to understand the real cause | |
216 | and try to avoid "fixes" which make the warning go away without addressing | |
217 | its cause. | |
218 | ||
219 | Note that not all compiler warnings are enabled by default. Build the | |
220 | kernel with "make EXTRA_CFLAGS=-W" to get the full set. | |
221 | ||
222 | The kernel provides several configuration options which turn on debugging | |
223 | features; most of these are found in the "kernel hacking" submenu. Several | |
224 | of these options should be turned on for any kernel used for development or | |
225 | testing purposes. In particular, you should turn on: | |
226 | ||
227 | - ENABLE_WARN_DEPRECATED, ENABLE_MUST_CHECK, and FRAME_WARN to get an | |
228 | extra set of warnings for problems like the use of deprecated interfaces | |
229 | or ignoring an important return value from a function. The output | |
230 | generated by these warnings can be verbose, but one need not worry about | |
231 | warnings from other parts of the kernel. | |
232 | ||
233 | - DEBUG_OBJECTS will add code to track the lifetime of various objects | |
234 | created by the kernel and warn when things are done out of order. If | |
235 | you are adding a subsystem which creates (and exports) complex objects | |
236 | of its own, consider adding support for the object debugging | |
237 | infrastructure. | |
238 | ||
239 | - DEBUG_SLAB can find a variety of memory allocation and use errors; it | |
240 | should be used on most development kernels. | |
241 | ||
242 | - DEBUG_SPINLOCK, DEBUG_SPINLOCK_SLEEP, and DEBUG_MUTEXES will find a | |
243 | number of common locking errors. | |
244 | ||
245 | There are quite a few other debugging options, some of which will be | |
246 | discussed below. Some of them have a significant performance impact and | |
247 | should not be used all of the time. But some time spent learning the | |
248 | available options will likely be paid back many times over in short order. | |
249 | ||
250 | One of the heavier debugging tools is the locking checker, or "lockdep." | |
251 | This tool will track the acquisition and release of every lock (spinlock or | |
252 | mutex) in the system, the order in which locks are acquired relative to | |
253 | each other, the current interrupt environment, and more. It can then | |
254 | ensure that locks are always acquired in the same order, that the same | |
255 | interrupt assumptions apply in all situations, and so on. In other words, | |
256 | lockdep can find a number of scenarios in which the system could, on rare | |
257 | occasion, deadlock. This kind of problem can be painful (for both | |
258 | developers and users) in a deployed system; lockdep allows them to be found | |
259 | in an automated manner ahead of time. Code with any sort of non-trivial | |
260 | locking should be run with lockdep enabled before being submitted for | |
261 | inclusion. | |
262 | ||
263 | As a diligent kernel programmer, you will, beyond doubt, check the return | |
264 | status of any operation (such as a memory allocation) which can fail. The | |
265 | fact of the matter, though, is that the resulting failure recovery paths | |
266 | are, probably, completely untested. Untested code tends to be broken code; | |
267 | you could be much more confident of your code if all those error-handling | |
268 | paths had been exercised a few times. | |
269 | ||
270 | The kernel provides a fault injection framework which can do exactly that, | |
271 | especially where memory allocations are involved. With fault injection | |
272 | enabled, a configurable percentage of memory allocations will be made to | |
273 | fail; these failures can be restricted to a specific range of code. | |
274 | Running with fault injection enabled allows the programmer to see how the | |
275 | code responds when things go badly. See | |
276 | Documentation/fault-injection/fault-injection.text for more information on | |
277 | how to use this facility. | |
278 | ||
279 | Other kinds of errors can be found with the "sparse" static analysis tool. | |
280 | With sparse, the programmer can be warned about confusion between | |
281 | user-space and kernel-space addresses, mixture of big-endian and | |
282 | small-endian quantities, the passing of integer values where a set of bit | |
283 | flags is expected, and so on. Sparse must be installed separately (it can | |
284 | be found at http://www.kernel.org/pub/software/devel/sparse/ if your | |
285 | distributor does not package it); it can then be run on the code by adding | |
286 | "C=1" to your make command. | |
287 | ||
288 | Other kinds of portability errors are best found by compiling your code for | |
289 | other architectures. If you do not happen to have an S/390 system or a | |
290 | Blackfin development board handy, you can still perform the compilation | |
291 | step. A large set of cross compilers for x86 systems can be found at | |
292 | ||
293 | http://www.kernel.org/pub/tools/crosstool/ | |
294 | ||
295 | Some time spent installing and using these compilers will help avoid | |
296 | embarrassment later. | |
297 | ||
298 | ||
299 | 4.3: DOCUMENTATION | |
300 | ||
301 | Documentation has often been more the exception than the rule with kernel | |
302 | development. Even so, adequate documentation will help to ease the merging | |
303 | of new code into the kernel, make life easier for other developers, and | |
304 | will be helpful for your users. In many cases, the addition of | |
305 | documentation has become essentially mandatory. | |
306 | ||
307 | The first piece of documentation for any patch is its associated | |
308 | changelog. Log entries should describe the problem being solved, the form | |
309 | of the solution, the people who worked on the patch, any relevant | |
310 | effects on performance, and anything else that might be needed to | |
311 | understand the patch. | |
312 | ||
313 | Any code which adds a new user-space interface - including new sysfs or | |
314 | /proc files - should include documentation of that interface which enables | |
315 | user-space developers to know what they are working with. See | |
316 | Documentation/ABI/README for a description of how this documentation should | |
317 | be formatted and what information needs to be provided. | |
318 | ||
319 | The file Documentation/kernel-parameters.txt describes all of the kernel's | |
320 | boot-time parameters. Any patch which adds new parameters should add the | |
321 | appropriate entries to this file. | |
322 | ||
323 | Any new configuration options must be accompanied by help text which | |
324 | clearly explains the options and when the user might want to select them. | |
325 | ||
326 | Internal API information for many subsystems is documented by way of | |
327 | specially-formatted comments; these comments can be extracted and formatted | |
328 | in a number of ways by the "kernel-doc" script. If you are working within | |
329 | a subsystem which has kerneldoc comments, you should maintain them and add | |
330 | them, as appropriate, for externally-available functions. Even in areas | |
331 | which have not been so documented, there is no harm in adding kerneldoc | |
332 | comments for the future; indeed, this can be a useful activity for | |
333 | beginning kernel developers. The format of these comments, along with some | |
334 | information on how to create kerneldoc templates can be found in the file | |
335 | Documentation/kernel-doc-nano-HOWTO.txt. | |
336 | ||
337 | Anybody who reads through a significant amount of existing kernel code will | |
338 | note that, often, comments are most notable by their absence. Once again, | |
339 | the expectations for new code are higher than they were in the past; | |
340 | merging uncommented code will be harder. That said, there is little desire | |
341 | for verbosely-commented code. The code should, itself, be readable, with | |
342 | comments explaining the more subtle aspects. | |
343 | ||
344 | Certain things should always be commented. Uses of memory barriers should | |
345 | be accompanied by a line explaining why the barrier is necessary. The | |
346 | locking rules for data structures generally need to be explained somewhere. | |
347 | Major data structures need comprehensive documentation in general. | |
348 | Non-obvious dependencies between separate bits of code should be pointed | |
349 | out. Anything which might tempt a code janitor to make an incorrect | |
350 | "cleanup" needs a comment saying why it is done the way it is. And so on. | |
351 | ||
352 | ||
353 | 4.4: INTERNAL API CHANGES | |
354 | ||
355 | The binary interface provided by the kernel to user space cannot be broken | |
356 | except under the most severe circumstances. The kernel's internal | |
357 | programming interfaces, instead, are highly fluid and can be changed when | |
358 | the need arises. If you find yourself having to work around a kernel API, | |
359 | or simply not using a specific functionality because it does not meet your | |
360 | needs, that may be a sign that the API needs to change. As a kernel | |
361 | developer, you are empowered to make such changes. | |
362 | ||
363 | There are, of course, some catches. API changes can be made, but they need | |
364 | to be well justified. So any patch making an internal API change should be | |
365 | accompanied by a description of what the change is and why it is | |
366 | necessary. This kind of change should also be broken out into a separate | |
367 | patch, rather than buried within a larger patch. | |
368 | ||
369 | The other catch is that a developer who changes an internal API is | |
370 | generally charged with the task of fixing any code within the kernel tree | |
371 | which is broken by the change. For a widely-used function, this duty can | |
372 | lead to literally hundreds or thousands of changes - many of which are | |
373 | likely to conflict with work being done by other developers. Needless to | |
374 | say, this can be a large job, so it is best to be sure that the | |
375 | justification is solid. | |
376 | ||
377 | When making an incompatible API change, one should, whenever possible, | |
378 | ensure that code which has not been updated is caught by the compiler. | |
379 | This will help you to be sure that you have found all in-tree uses of that | |
380 | interface. It will also alert developers of out-of-tree code that there is | |
381 | a change that they need to respond to. Supporting out-of-tree code is not | |
382 | something that kernel developers need to be worried about, but we also do | |
383 | not have to make life harder for out-of-tree developers than it it needs to | |
384 | be. |