On Sep 28, 2007, at 7:37 AM, Cathy Zhou wrote: > http://cr.grommit.com/~yun/webrev_uv_09_28 > http://jurassic.sfbay/net/forwar.prc/builds/yz147064/clearview-uv- > review/webrev_review/
Cathy, Here are my comments on the changes related to GLD, VNICs, xVM, and aggr. > Cscope: > > /net/forwar.prc/builds/yz147064/clearview-uv-review/ > > xVM related changes > ------------------------------- > usr/src/cmd/vna/vna.c ND-01/110-111: The comment you added doesn't match the functionality provided by this function. ND-02/163-170: What can cause this error? If the link exists because it was found while walking the devices, why would this fail? How is a user supposed to interpret this error, and what is the expected corrective action? ND-03/178: Do you mean "link name"? > usr/src/lib/libdladm/common/libdlvnic.c ND-04/57-58: Could use "_id" for both field names to be consistent. ND-05/128-129: The comment no longer matches the code ND-06/140-142: Why is this check needed in this particular function? ND-07/267: Note to self: this will have to be changed with anchor VNICs (it's possible to create a VNIC on top of an anchor VNIC, but only anchor VNICs) > usr/src/lib/libdladm/common/libdlvnic.h ND-08/41-42: Could you "_id" for both field names to be consistent. > usr/src/uts/intel/vnic/Makefile ND-09/general: see ND-12 > usr/src/uts/sparc/vnic/Makefile ND-10/general: see ND-12 > usr/src/uts/common/sys/vnic.h ND-11/58-59,68-69,94-95: see ND-04 > usr/src/uts/common/sys/vnic_impl.h NC (No Comment) > usr/src/uts/common/xen/io/xnbo.c ND-12/general: This applies to other files as well. The fact that every MAC client now depends on dls to open a device is crude. A MAC client shouldn't have to rely on another MAC client. The fact that DLS is way to get to that information, and all fact that the device needs to be held through dls_hold_device() shouldn't be exposed to the MAC clients. This should be done instead via a generic function exported to kernel modules via modstubs. This way you avoid explicit dependencies on dls from both the modules points of view as well as the API point of view, and avoid all the clients to have to deal with that additional logic. > usr/src/uts/i86xpv/xnbo/Makefile ND-13/general: see ND-12 > usr/src/uts/common/io/vnic/vnic_ctl.c NC > usr/src/uts/common/io/vnic/vnic_dev.c ND-14/219-235: see ND-12 ND-15/241: This should be hidden behind a new mac_is_legacy() call. VNIC shouldn't have to know about this capability which is really used only between softmac and mac. ND-16/821-824,951-954,967: The VNIC shouldn't have to do that, it's a MAC provider and client and shouldn't have to invoke dls directly. > GLD changes > ------------------------------ > usr/src/uts/common/io/dld/dld_drv.c ND-17/197: Shouldn't the hash name start with a "dld" instead of "dls"? ND-18/428: It seems that this could be done from within dls_vlan_hold () itself to avoid code duplication ND-19/482-483: Fix the grammar ND-20/620-623: I'm not sure what you're trying to say here. Are you describing a failure case? "its own configuration would become invalid" is not clear to me, do you mean that the configuration of the link being removed is lost? Where's the "inherit" part? Isn't the configuration of the "removed physical link" gone when that link is removed? > usr/src/uts/common/io/dld/dld_proto.c ND-21/276-281: Which bug does this correspond to? It doesn't seem to be related to the list of bugs associated with that file. ND-22/477: typo "prossessing" > usr/src/uts/common/io/dld/dld_str.c ND-23/390: that local variable seems overkill for this block of code. Simply use dsp->ds_perstream below. ND-24/417: Please cleanup the XXX, also you must not refer to "Crossbow" in the source code. ND-25/980-1051: This is quite confusing. dld_wput_perstream_callback ()'s comment says "The poll callback function", while dld_wput_poll_callback()'s comment says "Also the poll callback function". ND-26/general: It would be useful to have somewhere a comment in the code which summarizes the different transmit and receive code paths and when they are invoked depending on the various features that are enabled and conditions, i.e. polling mode vs raw vs per-stream, flow controlled vs non-flow controlled. Currently it's hard to see the big picture since bits of that logic are scattered throughout dld/dls/mac. > usr/src/uts/common/io/dls/dls.c ND-27/207: typo: "openning". Also shouldn't it be "current non-global zone" instead of "given local zone"? ND-28/229: it seems DLS_CHANNEL_LEGACY no longer exists. ND-29/253-254: per-stream stuff, see ND-39. ND-30/307: what's the risk of forcing a VLAN creation if MAC_CAPAB_NO_NATIVEVLAN is advertised? Will the VLAN work always? only once in a while? How can the administrator know for sure? ND-31/345-346: is there any case where DLS_VLAN_EXPLICIT and DLS_VLAN_FORCE could be set without the other? ND-32/356: typo: s/hold/held/ ND-33/371: it would be nice if this function could be prefixed with a dls_ or i_dls_ to be consistent with the rest of the file. > usr/src/uts/common/io/dls/dls_link.c ND-34/1316: why is a separate hold of softmac needed? > > usr/src/uts/common/io/dls/dls_mgmt.c ND-35/175: I can't parse this: "In this case, simply think something wrong with the door call" > usr/src/uts/common/io/dls/dls_mod.c NC > usr/src/uts/common/io/dls/dls_soft_ring.c ND-36/629: why is this needed? > usr/src/uts/common/io/dls/dls_stat.c NC > usr/src/uts/common/io/dls/dls_vlan.c ND-37/145: s/Hole/Hold/ > usr/src/uts/common/io/mac/mac.c ND-38/2061-2094, 2184: why does mac need to implement a special case and do the putnext down to the softmac. The putnext should be done from softmac not MAC. ND-39/general: We talked about this offline, and other Crossbow team members will send their own comments, but in general the per-stream optimization is incompatible with the Nemo changes that are being introduced by Crossbow. So the fast-path needs to be done either outside of Nemo completely, or not done at all. ND-40/620-627: How can this ever be called safely? How do you guarantee that the status returned won't be invalid as soon as it is used by the caller? ND-41/898,972: These functions shouldn't be impacted by the per- stream fast-path. ND-42/1658-1696: PUTNEXT_TX, TX_LOOPBACK, and MDT, PERSTREAM are set specifically for softmac, and they should be merged into a single capability for softmac. See also my comments for mac.h. > usr/src/uts/common/io/mac/mac_stat.c NC > usr/src/uts/common/sys/Makefile ND-43/general: these changes don't seem to be needed, only used by files in uts > usr/src/uts/common/sys/aggr.h NC > usr/src/uts/common/sys/aggr_impl.h ND-44/64-67: two questions 1) why should aggr care about some softmac queue, and 2) why is this comment here in this file? > usr/src/uts/common/sys/dld.h NC > usr/src/uts/common/sys/dld_impl.h NC > usr/src/uts/common/sys/dlpi.h NC > usr/src/uts/common/sys/dls.h NC > usr/src/uts/common/sys/dls_impl.h ND-45/85: "SPA"? Should move some of the comment at 100-103 here. > usr/src/uts/common/sys/dls_soft_ring.h NC > usr/src/uts/common/sys/gld.h NC > usr/src/uts/common/sys/mac.h ND-57/35. A MAC interface shouldn't depend on DLPI. ND-68/116-124: If so, they should be in mac_impl.h. Explain why they are here. ND-69/222-236.: So many capabilities only for softmac. Why do you need a separate capabilities for MAC_CAPAB_PUTNEXT_TX and MAC_CAPAB_PERSTREAM? These capabilities are related to STREAMs processing under MAC which is the case only by softmac, and should be therefore covered by a common MAC_CAPAB_SOFTMAC capability to be used exclusively by softmac. The other capabilities introduced here MAC_CAPAB_NOZCOPY, MAC_CAPAB_NO_NATIVEVLAN also seem to be used only by softmac, and always supported by softmac, and should be merged as well. ND-69/234: MDT was made obsolete by LSO, yet you are *adding* support for it. This is a step backward. Since this is for softmac only, it should be folded into the singe softmac capability, see previous comment. ND-70/290-294, 316-317, 332-338: This is softmac specific stuff and it shouldn't be made common for all MAC interfaces. ND-71/452-554: More per-stream stuff in a common header file. Does not belong here. > usr/src/uts/common/sys/mac_impl.h ND-72/127-130: What's the purpose of moving these fields around? > usr/src/uts/intel/Makefile.intel.shared NC > usr/src/uts/intel/aggr/Makefile ND-73/58: Shouldn't be needed, see ND-12 > usr/src/uts/intel/ia32/ml/modstubs.s ND-74/1209: s/depend/depends/ > usr/src/uts/intel/strplumb/Makefile NC > usr/src/uts/sparc/Makefile.sparc.shared NC > usr/src/uts/sparc/aggr/Makefile ND-75/58: see ND-12 > usr/src/uts/sparc/ml/modstubs.s ND-76/1176: s/depend/depends/ > usr/src/uts/common/io/gld.c ND-77/general: why are these changes needed for a legacy framework which sits under softmac? Since softmac is introducing because of legacy drivers, it should handle appropriate defaults for new properties not supported by these drivers. > usr/src/uts/common/io/strplumb.c NC > usr/src/uts/common/os/streamio.c NC > usr/src/cmd/truss/codes.c NC > usr/src/uts/common/Makefile.files NC > the aggr driver changes > ---------------------------- > usr/src/uts/common/io/aggr/aggr_ctl.c NC > usr/src/uts/common/io/aggr/aggr_dev.c NC > usr/src/uts/common/io/aggr/aggr_grp.c ND-78/general: aggr shouldn't have to depend on dls, see also ND-12. ND-79/154: s/lacp protocal/LACP protocol/ ND-80/155: s/the key to be a 16 bit/a 16 bits key/ ND-81/155: s/identfication/identification/ ND-82/184: "usually"? Should say when that's not the case. ND-83/835-836: comment no longer valid ND-84/1371-1376,1453-1470,1487-1501: should not be separate capabilities, see ND-69 ND-85/1553: why is this spelled all-caps ND-86: cleanup XXX in comments > usr/src/uts/common/io/aggr/aggr_lacp.c ND-87/1037: how easily can the administrator make sense of "aggr <some number> port <some number> etc"? I think this should display link names. > usr/src/uts/common/io/aggr/aggr_port.c ND-88/145-154: see ND-12 ND-89/169: it's weird to talk about "-f" from here. "-f" of what? Instead, I'd suggest talking about the "force" flag and that it is set if the user forces the port to be aggregated. ND-90/302-303: So what's the impact when the cable actually goes down? Is the administrator left alone with a broken aggregation? ND-91/general: more instances of per-stream related changes making their way throughout unrelated Nemo code. > usr/src/uts/common/io/aggr/aggr_recv.c NC -- Nicolas Droux - Solaris Core OS - Sun Microsystems, Inc. droux at sun.com - http://blogs.sun.com/droux -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/clearview-discuss/attachments/20071030/273b75b7/attachment.html>
