Even at my stately pace for blog entries, it's been a while. Truthfully, the last few months have been a blur, as I've been completely consumed by two projects: Clearview, and the future of WiFi on Solaris (more on that in a future blog entry). On the Clearview front, the IPMP design review on OpenSolaris has wrapped up, the IP Tunnel Rearchitecture and IP Observability Device design reviews are nearing completion, and Cathy's formidable "Nemo Unification and Vanity Naming" design is about to get underway. Thanks again to everyone who has contributed -- we really appreciate the feedback -- and keep it coming!
Each time I go through the design phase[1] of a project, I'm reminded of just how hard a process it is do with rigor and integrity. Performed as intended, the design phase forces us to confront and address the critical flaws at the start of a project, rather than right before integration -- or in the case of the technologies Clearview's rearchitecting, long after shipment. With Clearview, we've spent many months carefully working through the design process, constructing crisp, precise, and thorough design materials that collectively top 200 pages.
Along the way, I occasionally find it necessary to seek more real-world examples of why we are willing to endure such pain. The most entertaining method is to don the hazmat suit, wade into the toxic anarchy of subsystems from years gone by, and bear witness to the nightmare that results from inadequate design.
Though under-designed (or just plain never-designed) subsystems make themselves known in many capacities, one of their hallmarks is the presence of voodoo coding. Specifically, because the forces shaping the design space were never really understood, the developer must iterate until something finally "works". Code from previous iterations or stillborn ideas of how to solve unexpected problems are left behind as the logic is debugged into existence. Eventually, persistence prevails over common sense, and the code is checked in, complete with ritual sacrifices. As the years go by, subsequent developers become convinced that there is something tricky going on that they simply aren't capable of understanding, and the subsystem in question becomes steeped in mysticism.
As I've mentioned before, one subsystem that has more than its fair share of arcana is STREAMS -- which is undoubtedly why I find it so fascinating. About once a release, I like to go in and haul out a few thousand lines[2] of nonsense. While I started on the periphery, at this point I've plucked the low-hanging fruit and must instead target the wizened roots that comprise the core paths. Once such routine is str_mate(), which "twists" two streams together so that they can act as pipe or a FIFO. Since its introduction more than a decade ago, it has looked like this (line numbers added for subsequent discussion):
1 /*
2 * No activity allowed on streams
3 * Input: 2 write driver end write queues
4 * Return 0 if error
5 * If wrq1 == wrq2 then we are doing a loop back,
6 * otherwise just mate two queues.
7 * If these queues are not the stream head they must have
8 * a service procedure
9 * It is up to caller to ensure that neither queue goes away.
10 * XXX str_mate() and str_unmate() should be moved to new file kstream.c
11 * XXX these routines need to be a little more general
12 */
13 int
14 str_mate(queue_t *wrq1, queue_t *wrq2)
15 {
16 /*
17 * Loop back?
18 */
19 if (wrq2 == 0 || wrq1 == wrq2) {
20 /*
21 * driver end of stream?
22 */
23 if (! (wrq1->q_flag & QEND))
24 return (EINVAL);
25
26 ASSERT(wrq1->q_next == 0); /* sanity */
27
28 /*
29 * connect write queue to read queue
30 */
31 wrq1->q_next = _RD(wrq1);
32
33 /*
34 * If write queue does not have a service routine,
35 * assign the forward service procedure from the
36 * read queue
37 */
38
39 if (! wrq1->q_qinfo->qi_srvp)
40 wrq1->q_nfsrv = _RD(wrq1)->q_nfsrv;
41 /*
42 * set back service procedure..
43 * XXX - note back service procedure is not implemented
44 * this may cause a race condition, breaking it
45 * a bit more.
46 */
47 _RD(wrq1)->q_nbsrv = wrq1;
48 } else {
49 /*
50 * driver end of stream?
51 */
52 if (! (wrq1->q_flag & QEND))
53 return (EINVAL);
54 if (! (wrq2->q_flag & QEND))
55 return (EINVAL);
56
57 ASSERT(wrq1->q_next == NULL); /* sanity */
58 ASSERT(wrq2->q_next == NULL); /* sanity */
59
60
61 /*
62 * if first queue is a stream head, second must
63 * must also be one
64 */
65 if (! (_RD(wrq1)->q_flag & QEND)) {
66 if (_RD(wrq2)->q_flag & QEND)
67 return (EINVAL);
68 } else if (! (_RD(wrq2)->q_flag & QEND))
69 return (EINVAL);
70 /*
71 * Twist the stream head queues so that the write queue
72 * points to the other stream's read queue.
73 */
74 wrq1->q_next = _RD(wrq2);
75 wrq2->q_next = _RD(wrq1);
76
77 if (! wrq1->q_qinfo->qi_srvp)
78 wrq1->q_nfsrv = _RD(wrq2)->q_nfsrv;
79 if (! wrq2->q_qinfo->qi_srvp)
80 wrq2->q_nfsrv = _RD(wrq1)->q_nfsrv;
81
82 /*
83 * Nothing really uses the back service routines,
84 * but fill them in for completeness
85 */
86
87 _RD(wrq1)->q_nbsrv = wrq2;
88 _RD(wrq2)->q_nbsrv = wrq1;
89
90 SETMATED(STREAM(wrq1), STREAM(wrq2));
91 }
92 return (0);
93 }
As a developer, this is the kind of ghetto you hope you never have to enhance or debug: gnarly logic, unsettling comments, and suspect codepaths abound. Further, as a code minimalist, it's a personal affront to everything I hold dear.
The function's biggest problem is that it is pointlessly general-purpose (despite the contrary claim on line 11), undoubtedly because no one ever bolted down its requirements. Specifically, the function signature and comments suggest that it can mate any two STREAMS driver queues together at any time. However, because of the limitations expressed on lines 2, 7, 8, and 9, this was never possible in practice -- instead, the function was only used to mate stream head queues before they were put into use. In fact, all of its callers go through extra work to accommodate this needless generality:
if (dotwist && firstopen) {
queue_t *wq = strvp2wq(*vpp);
(void) str_mate(wq, wq);
}
Adding insult to injury, str_mate() can never fail when passed stream head queues -- so, as shown above, all callers simply discard the return value. Indeed, by simply changing str_mate() to accept two vnode pointers and letting it derive the stream heads to mate, we can (a) simplify its callers, (b) simplify its interface, and (c) simplify its implementation. With this change, callers will just do:
if (dotwist && firstopen)
str_mate(*vpp, *vpp);
Regarding (c):
- Because the queues are guaranteed to be stream heads, lines 61-69 can be removed
- Because stream heads always have QEND set, lines 20-23 and 49-55 can be removed
- Because stream heads always have a service procedure, lines 33-40 and 77-81 can be removed. However, since one day this may not be true, an ASSERT() should be added to avoid debugging headaches.
1 /*
2 * Mate the stream heads of two vnodes together. If the two vnodes are the
3 * same, we just make the write-side point at the read-side -- otherwise,
4 * we do a full mate. Only works on vnodes associated with streams that
5 * are still being built and thus have only a stream head.
6 */
7 void
8 str_mate(vnode_t *vp1, vnode_t *vp2)
9 {
10 queue_t *wrq1 = strvp2wq(vp1);
11 queue_t *wrq2 = strvp2wq(vp2);
12
13 /*
14 * We rely on the stream head always having a service procedure
15 * to avoid tweaking q_nfsrv.
16 */
17 ASSERT(wrq1->q_qinfo->qi_srvp != NULL);
18 ASSERT(wrq2->q_qinfo->qi_srvp != NULL);
19
20 /*
21 * Loop back?
22 */
23 if (wrq2 == 0 || wrq1 == wrq2) {
24 ASSERT(wrq1->q_next == 0); /* sanity */
25
26 /*
27 * connect write queue to read queue
28 */
29 wrq1->q_next = _RD(wrq1);
30
31 /*
32 * set back service procedure..
33 * XXX - note back service procedure is not implemented
34 * this may cause a race condition, breaking it
35 * a bit more.
36 */
37 _RD(wrq1)->q_nbsrv = wrq1;
38 } else {
39 ASSERT(wrq1->q_next == NULL); /* sanity */
40 ASSERT(wrq2->q_next == NULL); /* sanity */
41
42 /*
43 * Twist the stream head queues so that the write queue
44 * points to the other stream's read queue.
45 */
46 wrq1->q_next = _RD(wrq2);
47 wrq2->q_next = _RD(wrq1);
48
49 /*
50 * Nothing really uses the back service routines,
51 * but fill them in for completeness
52 */
53
54 _RD(wrq1)->q_nbsrv = wrq2;
55 _RD(wrq2)->q_nbsrv = wrq1;
56
57 SETMATED(STREAM(wrq1), STREAM(wrq2));
58 }
59 }
That hauled out a third of it, but there's plenty more to go. First, since wrq2 is no longer passed in, there is no risk that it can be NULL, so the check on 23 can be disposed of. Next, as boldly proclaimed in the comments on lines 32-35 and 50-51, q_nbsrv is indeed unused (a weed which I hauled out as part of 6267823) -- so it can be torched as well. Finally, the "sanity" checks on lines 24, 39, and 40 can be hoisted to neighbor the qi_nfsrv ASSERT()'s at minimal cost (one extra compare on DEBUG systems). Our result:
1 /*
2 * Mate the stream heads of two vnodes together. If the two vnodes are the
3 * same, we just make the write-side point at the read-side -- otherwise,
4 * we do a full mate. Only works on vnodes associated with streams that
5 * are still being built and thus have only a stream head.
6 */
7 void
8 str_mate(vnode_t *vp1, vnode_t *vp2)
9 {
10 queue_t *wrq1 = strvp2wq(vp1);
11 queue_t *wrq2 = strvp2wq(vp2);
12
13 /*
14 * Verify that there are no modules on the stream yet. We also
15 * rely on the stream head always having a service procedure to
16 * avoid tweaking q_nfsrv.
17 */
18 ASSERT(wrq1->q_next == NULL && wrq2->q_next == NULL);
19 ASSERT(wrq1->q_qinfo->qi_srvp != NULL);
20 ASSERT(wrq2->q_qinfo->qi_srvp != NULL);
21
22 /*
23 * If the queues are the same, just twist; else do a full mate.
24 */
25 if (wrq1 == wrq2) {
26 wrq1->q_next = _RD(wrq1);
27 } else {
28 wrq1->q_next = _RD(wrq2);
29 wrq2->q_next = _RD(wrq1);
30 SETMATED(STREAM(wrq1), STREAM(wrq2));
31 }
32 }
Behold, under all that cargo-cult programming: a scraggly, unintimidating function -- but functionally identical to the original 92-line thug. Now, we can trivially follow the logic:
- If the two vnodes are the same (pipe), then the single stream head has its write-side pointed to its read-side.
- If the two vnodes are different (FIFO), then each stream head's write-side is pointed to the other's read-side.
1 /*
2 * Mate the stream heads of two vnodes together. If the two vnodes are the
3 * same, we just make the write-side point at the read-side -- otherwise,
4 * we do a full mate. Only works on vnodes associated with streams that
5 * are still being built and thus have only a stream head.
6 */
7 void
8 strmate(vnode_t *vp1, vnode_t *vp2)
9 {
10 queue_t *wrq1 = strvp2wq(vp1);
11 queue_t *wrq2 = strvp2wq(vp2);
12
13 /*
14 * Verify that there are no modules on the stream yet. We also
15 * rely on the stream head always having a service procedure to
16 * avoid tweaking q_nfsrv.
17 */
18 ASSERT(wrq1->q_next == NULL && wrq2->q_next == NULL);
19 ASSERT(wrq1->q_qinfo->qi_srvp != NULL);
20 ASSERT(wrq2->q_qinfo->qi_srvp != NULL);
21
22 /*
23 * If the queues are the same, just twist; else do a full mate.
24 */
25 if (wrq1 == wrq2) {
26 wrq1->q_next = _RD(wrq1);
27 } else {
28 wrq1->q_next = _RD(wrq2);
29 wrq2->q_next = _RD(wrq1);
30 STREAM(wrq1)->sd_mate = STREAM(wrq2);
31 STREAM(wrq1)->sd_flag |= STRMATE;
32 STREAM(wrq2)->sd_mate = STREAM(wrq1);
33 STREAM(wrq2)->sd_flag |= STRMATE;
34 }
35 }
The above is identical to the version in OpenSolaris.
Footnotes
[1]
Of course, no one who has actually done design believes in a pure
waterfall
model -- instead, designs must be revised over the lifetime of the
project. Regardless, the more rigorous the initial design is, the fewer
changes that will have to be made later in the project.
[2]
While most developers pride themselves on the "lines of code" they've
*produced*, I'm quite the opposite: I feel a profound sense of failure
if the codebase is larger after I've integrated a bugfix or feature.
[3]
STRMATE isn't strictly needed, but since sd_flag is often checked to
accept or reject certain STREAMS operations, it's more convenient than
constantly checking whether sd_mate != NULL.
Technorati Tag:
OpenSolaris
Technorati Tag:
Solaris