Kais Belgaied
2008-Sep-27 00:54 UTC
[crossbow-discuss] [Fwd: Re: Crossbow Code Review Phase II ready]
-------- Original Message --------
Subject: Re: [crossbow-discuss] Crossbow Code Review Phase II ready
Date: Fri, 26 Sep 2008 17:27:41 -0700
From: Alexander Kolbasov <akolb at eng.sun.com>
To: Kais.Belgaied at Sun.COM, crossbow-discuss at opensolaris.org,
erik.nordmark at Sun.COM, peter.memishian at Sun.COM
General comment for the squeue code.
These are for include files only. I have more comments for squeue.c file
itself.
The comments are really bad. A lot of them are obsolete and do not describe
what is going on now. New ones fail to describe what is going on and why and
critical comments are missing. The code was quite complicated before and new
additions make it even more convoluted, so having good comments is essential.
Comments for usr/src/uts/common/sys/squeue.h
1.
51 #define SQ_FILL 0x0001
52 #define SQ_NODRAIN 0x0002
53 #define SQ_PROCESS 0x0004
Are these mutually exclusive? In this case may be this should be an enum?
Are there any other flags you may need to pass (e.g. this is processed by
interrupt thread)?
It would be nice to have a comment describing the semantics of these. Previously
these were separate functions - why have you decided to move it to the flags?
2.
What is the point of having sq_enter and sq_drain be function pointers in the
squeue structure instead of direct function calls? I do not see any consumers of
such indirection.
3.
55 #define SQUEUE_ENTER(sqp, head, tail, cnt, size, flag, tag) { \
56 sqp->sq_enter(sqp, head, tail, cnt, size, flag, tag); \
57 }
58
59 #define SQUEUE_ENTER_ONE(sqp, mp, proc, arg, size, flag, tag) { \
60 ASSERT(mp->b_next == NULL); \
61 ASSERT(mp->b_prev == NULL); \
62 SET_SQUEUE(mp, proc, arg); \
63 SQUEUE_ENTER(sqp, mp, mp, 1, size, flag, tag); \
64 }
Is this done to avoid filling mp->b_next/b_prev in some cases? When I looked
at
squeue code it seems that it clears these before calling the callback function.
Can you explain when these stay intact and can be reused?
4.
69 typedef enum {
70 SQPRIVATE_TCP,
71 SQPRIVATE_MAX
72 } sqprivate_t;
This was done at the time where there was one squeue per CPU to provide some
per-CPU storage for TCP. Now with many squeues per CPU is this still the right
thing to do? Do you really want to distribute some of the TCP state per squeue
or per CPU or may be both?
Comments for usr/src/uts/common/sys/squeue_impl.h
1. (Style)
106 kcondvar_t sq_async; /* async thread blocks on */
107 kcondvar_t sq_cv; /* cond variable poll_thr waits on
*/
108 kcondvar_t sq_quiesce_done_cv;
Previously there was only one conditional variable sq_async, now there are
three and their naming is very obscure. Can you invent more descriptive names
for those? Reading the code it is really hard to understand when you should
signal to sq_async and when to sq_cv.
2. (Maintaining)
124 #ifdef DEBUG
125 int sq_isintr; /* serviced by interrupt */
...
This was always a debug-only facility but I guess that without it debugging
problems with messages stuck on squeues on production systems is going to be
tough. Have you considered moving them to the "always on status"?
3) (Maintaining)
The squeue state is way overloaded with tons of flags describing not only
state but state change requests. I would consider splitting the state into
more fields to make the distinction more clear (and adding the comment
describing these in more detail as well).
More over I would propose introducing special functions for things like
squeue quiesce requests so that others do not need to know intricate details
of squeue thread interaction.
4)
170 #define SQUEUE_SWITCH(connp, new_sqp) \
171 (connp)->conn_sqp = new_sqp;
As I understand it, squeue.h describes squeue *interface* while squeue_impl.h
deals with squeue implementation. The macro above is definitely an interface
since it is only used by TCP.
5) (Maintaining)
The squeue_impl.h is private implementation file and as such should not be
included by anything other than squeue.c (and, possibly, ip_squeue.c). Any
real interfaces should be provided by squeue.h. Otherwise it is not clear at
all what exact interface is provided by squeues.
6)
What is this strange thing?
90 extern int ip_squeue_flag;
- akolb
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/crossbow-discuss/attachments/20080926/ce7aaeea/attachment.html>
Alexander Kolbasov
2008-Oct-01 05:59 UTC
[crossbow-discuss] Crossbow Code Review squeue.c, part I.
Here is the first part of comments fr squeue.c
1.
The old block comment on lines 29-46 is stale and should be updated
2.
58 * 2. Only a thread executing in the squeue can change the squeue of the
59 * connection. It does so by calling a squeue framework function to do
this.
Please describe what framework function it should call.
60 * After changing the squeue, the thread must leave the squeue. It must
not
61 * continue to execute any code that needs squeue protection.
It is not the thread who must leave the squeue, but the callback function that
should return, right?
62 * 3. The squeue framework, after entering the squeue, checks if the current
63 * squeue matches the conn_sqp. If the check fails, the packet is
SQFILL''ed
64 * to the right squeue.
Prootion of SQFILL to a verb is a bit too much :-). Can you explain
what''s going
on without using such jargon?
3.
Please add block comment explaining the interaction between worker and
polling thread.
4.
The interface part of the comment starting from line 85 is completely out of
date.
5.
144 * Each squeue keeps small amount of private data space available for
various
145 * consumers. Current consumers include TCP and NCA. Other consumers
need to
146 * add their private tag to the sqprivate_t enum. The private
information is
147 * limited to an uintptr_t value. The squeue has no knowledge of its
content
148 * and does not manage it in any way.
149 *
150 * The typical use may be a breakdown of data structures per CPU (since
151 * squeues are usually per CPU). See NCA for examples of use.
152 * Currently ''p'' may have one legal value
SQPRIVATE_TCP.
NCA doesn''t use it any longer. And, as I mentioned in the squeue.h
discussion,
the use squeue_getprivate() should be re-evaluated.
6.
234 /*
235 * Workaround for the tcp_timercache which creates a
236 * pseudo mblk which has wptr as NULL and b_datap as NULL.
237 * msgdsize needs both to be not NULL.
238 */
239 #define SQUEUE_MSG_SIZE(mp, count) { \
240 mblk_t *bp = (mp); \
...
This is quite ugly. Can you modify mp in the timer ache to avoid this? At a
minimum, can you just call msgdsize() when you know that this is a normal mblk?
Something like this would be much nicer:
#define SQUEUE_MSG_SIZE(mp) ((mp)->b_wptr == NULL ? 0 : msgdsize (mp))
7.
248 #define ENQUEUE_CHAIN(sqp, mp, tail, cnt, sz) { \
This macro is rather non-trivial and deserves a comment, which should define
what is a chain and describe how all this is supposed to work and what is mp and
tail.
How is tail related to mp? Does mp>b_next point to the tail or to something
else?
Isn''t it rather expensive to keep track of message sizes on squeue?
8.
262 DTRACE_PROBE4(squeue__enqueuechain, squeue_t *, sqp, \
263 mblk_t *, mp, mblk_t *, tail, int, cnt); \
Previously squeues had squeue-enqueue probe which fired whenever anything was
enqueued. This probe complicates things so that the consumer needs to know
various types of enqueuing. Is it possible to keep a generic squeue-enqueue
probe that will work in all the cases?
9.
267 /*
268 * Used by squeue_drain to requeue the mblks it dequeued back into the
squeue,
269 * when it is unable to send them due to driver device queue flow control
270 * condition. Currently used only by non-tcp squeues.
271 */
What part is used only by non-tcp squeues? Is i the flow control part or
squeue_drain part?
All my comments for the ENQUEUE_CHAIN apply here as well.
10.
290 #define SQS_POLLING_ON(sqp, rx_ring, interrupt) {
What is the SQS prefix?
11
297 rx_ring->rr_intr_disable(rx_ring->rr_intr_handle);
I would consider hiding details of interrupt disabling by some function which
takes rx_ring as an argument. Something like
rr_intr_disable(rx_ring);
Same goes for the enabling.
12
312 #define SQS_POLL_CHECK_NOW(sqp) { \
The name is quite misleading. The macro doesn''t check for anything, it
just
wakes up some other threads (in fact, the commet should explain what it wakes
up).
13
324 #define SQS_WORKER_THR_QUIESCED \
325 (SQS_POLL_QUIESCE_DONE | SQS_PROC | SQS_WORKER)
The macro above and its use are quite confusing. Just using the expansion may
indicate what is going on with more clarity.
14.
330 #ifdef DEBUG
331 #define SQUEUE_DBG_SET(sqp, interrupt, mp, proc, connp, tag) { \
332 (sqp)->sq_isintr = (interrupt); \
333 (sqp)->sq_curmp = (mp); \
334 (sqp)->sq_curproc = (proc); \
335 (sqp)->sq_connp = (connp); \
336 (mp)->b_tag = (sqp)->sq_tag = (tag); \
337 }
Perhaps some (or all) of these should be non-debug to assist diagnosing problems
on production systems?
15.
351 static void
352 squeue_size_verify(squeue_t *sqp, mblk_t *bp, size_t chain_sz, boolean_t
flag)
What is this function? Is it DEBUG only or is it also used on non-DEBUG kernels?
Should it use CE_PANIC below or just ASSERTs? What is the
''flag'' argument?
Aren''t the messages in CE_PANIC rather confusing?
Also this function seems like a concatenation of two different functions - one
used when flag is set and another when flag is not set. Can it be split into two
separate functions?
16.
407 sqp->sq_wait = MSEC_TO_TICK(wait);
Wha is the purpose of this? Do you keep separate waits per squeue? May be it
should become a global squeue tuneable?
17.
414 sqp->sq_enter = squeue_enter;
415 sqp->sq_drain = squeue_drain;
I mentioned in squeue_impl.h comments that the purpose of these is a mystery for
me.
18.
420 /*
421 * If bind is -1, then re-bind to prior bound CPU.
422 * If bind is != -1, then bind to that CPU
423 * If already bound, will first unbind and then bind to new CPU
424 */
This comment misses the explanation of what the function is actually doing. It
should state something like this:
Bind squeue worker thread to the specified CPU, given by CPU id. If the CPU id
value is -1, bind the worker thread to the value specified in sq_bind field.
If a thread is already bound to a different CPU, unbind it from the old CPU
and bind to the new one.
426 void
427 squeue_bind(squeue_t *sqp, processorid_t bind)
428 {
In the function below, why have you decided to hold sq_lock across calls to
thread_affinity_xxx? It was intentionally dropped in the original code.
I also think that if SQS_BOUND is not kept in sq_state but in a separate field
you can get away without holding sq_lock since this field may be safely
protected by cpu_lock.
429 mutex_enter(&sqp->sq_lock);
430 ASSERT(sqp->sq_bind != -1 || bind != -1);
Can PBIND_NONE be used instead of -1?
431 ASSERT(MUTEX_HELD(&cpu_lock));
432
433 if (sqp->sq_state & SQS_BOUND) {
434 if (sqp->sq_bind == bind) {
435 mutex_exit(&sqp->sq_lock);
436 return;
437 }
438 thread_affinity_clear(sqp->sq_worker);
439 } else
440 sqp->sq_state |= SQS_BOUND;
Please use {} for the else part as well.
Same comments apply to squeue_unbind() below.
--
- akolb