Voodoo vs. Engineering

Voodoo vs. Engineering

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.
After our first pass, things are starting to look more comprehensible -- and at no loss of functionality:

     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:

  1. If the two vnodes are the same (pipe), then the single stream head has its write-side pointed to its read-side.
  2. If the two vnodes are different (FIFO), then each stream head's write-side is pointed to the other's read-side.
In the second case, the SETMATED() macro is also called in order to link the two stream heads (via sd_mate) and to set the STRMATE sd_flag on each stream head[3]. Since str_mate() is the only routine that establishes mates, it is also the only user of the SETMATED() macro. Since the macro neither adds semantic value nor reduces code duplication, our final version inlines the macro (whose definition can then be removed), preferring clarity to brevity. In addition, str_mate() has been renamed to strmate() for consistency with other STREAMS functions:
     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:
Technorati Tag:

Comments:

Post a Comment:
  • HTML Syntax: NOT allowed
About

meem

Search

Categories
Archives
« July 2014
SunMonTueWedThuFriSat
  
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
  
       
Today
News

No bookmarks in folder

Blogroll

No bookmarks in folder