Alexander Kolbasov
2008-Oct-02 05:28 UTC
[crossbow-discuss] Comments for squeue.c; part 2.
1.
566 squeue_enter(squeue_t *sqp, mblk_t *mp, mblk_t *tail, uint32_t cnt,
Why cnt is declared as uint32_t (fixed size?) Why not just int or uint_t?
554 * squeue_enter() - enter squeue sqp with mblk mp (which can be
555 * a chain), while tail points to the end and cnt in number of
556 * mblks in the chain.
It is not quite clear what is a tail (and why it is needed as well)
558 * For a chain of single packet (i.e. mp == tail), go through the
559 * fast path if no one is processing the squeue and nothing is queued.
Is single packet case cnt == 1 or mp == tail or both or one of these? Or is it
chain_size == 1? What is the diferece between the cnt and chain_size?
Also, there is a lot of mentioning of ''fast-path'' without any
explanation of
what it actually means.
Also, the comment for the function should mentioned that for any processed
messages the corresponding connp looses one reference (and explain why this is
done this way).
2.
581 * ''interrupt'' is set when the packet is coming
from below
582 * either through a soft ring worker thread that directly
583 * called ip_input() (tag == SQTAG_IP_INPUT_RX_RING) or
584 * when we are being invoked from a NIC interrupt context.
This seems like a hack. First of all, why is processing by poll thread should be
treated in the same way as the real interrupt handler (which is quite different
beast)? Second, I don''t like the use of tags in this context. Can an
explicit
flag be passed to squeue_enter when there is a need to do something special?
3.
593 * Try to process the packet if SQ_FILL flag is not set and
594 * we are allowed to process the squeue. The SQ_NODRAIN is
595 * ignored if the packet chain consists of more than 1 packet.
596 */
597 if (!(sqp->sq_state & SQS_PROC) &&
598 ((process_flag == SQ_PROCESS) || (process_flag == SQ_NODRAIN
&&
599 sqp->sq_first == NULL))) {
The comment and the code do not agree with each other.
4.
614 * We are the chain of 1 packet so
615 * go through this fast path.
The comment is confusing (especially since there is no explanation of fast
path). Can you say something like "request for processing a single packet
can be
completed by the caller if there are no other messages on the squeue".
5.
617 ASSERT(mp->b_prev != NULL);
618 ASSERT(mp->b_queue != NULL);
Should these ASSERTS be satisfied for every message in a chain? Should the check
be done at the beginning of the function rather than here?
6.
630 if (connp->conn_sqp == sqp) {
...
642 } else {
643 SQUEUE_ENTER_ONE(connp->conn_sqp, mp,
proc,
644 connp, chain_size, SQ_FILL,
645 SQTAG_SQUEUE_CHANGE);
646 }
Why clear all fields in mp just to set them back here? The check for conn_sqp
could have been done earlier eliminating the need for calling SQUEUE_ENTER_ONE -
a plain squeue_enter could be called instead.
Also it seems that changing the tag here is wrong. What matters is who placed
the message on the squeue - which was indicated by the original tag. The fact
that it was replaced to another squeue is much less interesting for debugging
the source of the message.
7.
647 ASSERT(MUTEX_NOT_HELD(&sqp->sq_lock));
What is the point of this ASSERT? Is it to verify that the callback function
didn''t grab sq_lock and forgot to release it? Even this is so, the next
line
will cause recursive mutex enter panic anyway.
8.
651 if (sqp->sq_first == NULL ||
652 process_flag == SQ_NODRAIN) {
653 if (sqp->sq_first != NULL) {
654 squeue_worker_wakeup(sqp);
655 } else {
661 mutex_exit(&sqp->sq_lock);
662 }
663 return;
664 }
The sqp->sq_first is needlessly checked two times.
if (sqp->sq_first == NULL) {
mutex_exit(sq_lock);
return;
} else if (process_flag == SQ_NODRAIN) {
squeue_worker_wakeup(sqp); /* drops sq_lock */
return;
}
is both simpler and more efficient.
8.
681 sqp->sq_isintr = interrupt;
Do we want to distinguish here between the real interrupt and the case of
polling thread calling squeue_enter()?
9.
683 now = gethrtime();
684 if (interrupt) {
685 sqp->sq_drain(sqp, SQS_ENTER, now +
686 squeue_intrdrain_ns);
Since squeue_intrdrain_ns is a constant and squeue_drain can get the current
time by itself and squeue_drain wants to know whether it is called in interrupt
context or non-interrupt context, wouldn''t it be more straightforward
to pass
''interrupt'' flag instead of the time to squeue_drain?
10.
698 sqp->sq_run = NULL;
This was already done in line 650.
11.
703 * We let a thread processing a squeue reenter only
704 * once. This helps the case of incoming connection
705 * where a SYN-ACK-ACK that triggers the conn_ind
706 * doesn''t have to queue the packet if listener
and
707 * eager are on the same squeue. Also helps the
708 * loopback connection where the two ends are bound
709 * to the same squeue (which is typical on single
710 * CPU machines).
711 * We let the thread reenter only once for the fear
712 * of stack getting blown with multiple traversal.
This whole comment is pretty bad in explaining what it tries to explain. Th
whole reenter thing is quite a hack that at least requires clear explanation.
12.
715 if (!(sqp->sq_state & SQS_REENTER) &&
716 (process_flag != SQ_FILL) && (sqp->sq_first
== NULL) &&
717 (sqp->sq_run == curthread) && (cnt == 1)
&&
718 (connp->conn_on_sqp == B_FALSE)) {
How can it happen that sq_run != NULL && connp->conn_on_sqp ==
B_FALSE?
13.
630 if (connp->conn_sqp == sqp) {
631 SQUEUE_DBG_SET(sqp, interrupt, mp, proc,
connp,
632 tag);
...
This big chunk of code is repeated several times in the code. Can it become a
function or a macro?
14.
756 * one or more paquets on the queue. Enqueue the
spelling in paquets.
Apparently Analagous Threads
Wisdom of the Ancients
