About Kernel Documentation Linux Kernel Contact Linux Resources Linux Blog

Documentation / development-process / 4.Coding




Custom Search

Based on kernel version 3.13. Page generated on 2014-01-20 22:00 EST.

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	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	
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	
247	 - DEBUG_SPINLOCK, DEBUG_ATOMIC_SLEEP, and DEBUG_MUTEXES will find a
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
281	Documentation/fault-injection/fault-injection.txt for more information on
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
289	be found at https://sparse.wiki.kernel.org/index.php/Main_Page if your
290	distributor does not package it); it can then be run on the code by adding
291	"C=1" to your make command.
292	
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	
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
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.
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
338	clearly explains the options and when the user might want to select them.
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
389	justification is solid.  Note that the Coccinelle tool can help with
390	wide-ranging API changes.
391	
392	When making an incompatible API change, one should, whenever possible,
393	ensure that code which has not been updated is caught by the compiler.
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
398	not have to make life harder for out-of-tree developers than it needs to
399	be.
Hide Line Numbers
About Kernel Documentation Linux Kernel Contact Linux Resources Linux Blog

Information is copyright its respective author. All material is available from the Linux Kernel Source distributed under a GPL License. This page is provided as a free service by mjmwired.net.