> I'd like to start the code review for the UV fastpath project. The timer
> is set to two weeks. You can send the review comments to the
> Clearview-discuss alias or directly send it to me.
>
> The webrev is at:
>
> http://cr.opensolaris.org/~yun/webrev_fastpath/
I've only looked at the IP and ARP changes so far. Things look good; most
of my comments are nits.
arp_impl.h:
* 69: This comment line seems unnecessary.
arp.c:
[ Cathy and I spoke offline regarding some issues with the
current structure of the ar_dlpi_*() functions and in particular
some issues with a missing ar_cmd_done() call that can cause the
ARP command queue to hang. I will review the updated webrev
once she has tested the changes. ]
ip.h:
* 1941: Following the convention for other field comments, prefer
"replumb mp from ill_replumb()"
* 1942: Remove unnecessary blank line.
ip.c:
* 15707: Remove unnecessary blank line.
ip_if.c:
* 1607: No need for `ill'; can just directly pass q->q_ptr on
line 1610.
* 3021: This seems like a separate bugfix (?)
* 8049: s/the IPIF_CHANGING/IPIF_CHANGING/
* 8061: s/specical/special/
* 8064-8067: This comment appears unrelated to the current code.
* 14064-14069: Most of this comment is now stale.
* 14110: Would be nice to update this comment to cover `logical'.
* 14120: The code no longer modifies ipif_state_flags, so this
comment is stale.
* 14483-14501: I'm confused by the `waitack' handling here. It
seems like the existing special-case logic covered by the
comment at line 14483 doesn't work because queued messages will
never be drained (since we won't process the ACKs), except that
(as per the DLPI spec) almost all ACK messages are actually
M_PCPROTO -- so I think the old logic is doing nothing aside
from adding complexity.
* 16225-16233: It'd be simpler to use the GRAB_CONN_LOCK()
and RELEASE_CONN_LOCK() macros.
* 20156: Tabbing out of `*ill' looks a little silly here.
* 20173: Seem to be more parentheses here than are really needed;
`((dl_notify_conf_t *)mp->b_rptr)->dl_notification' suffices.
* 20183: What is the impact of ill_replumb_mp being NULL?
ip_multi.c:
* 678: A comment above this function would be useful.
* 679: Why pass `ip_addr' as a pointer?
* 701, 704: Code would be cleaner if it used hw_addrlen here.
* 706-710: Assuming ip_addr wasn't a pointer, seems like it'd
be simpler to do:
ip_addr &= proto_extract_mask;
from = (uint8_t *)&ip_addr;
while (len-- > 0)
hw_addr[hw_extract_start + len] |= from[len];
* 736, 961: s/dl_*multi_req/DL_*MULTI_REQ/
* 740, 965: Remove unnecessary blank line.
* 968: Restore blank line.
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
