Nicolas, Thank you very much for your comments. Please see my replies inline. You can also refer to the webrev to see the changes I made based on your comments:
/net/aquila.prc/export/home/cathy/clearview/webrev_11_06 > > 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. > Fixed. > 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? > This could fail because of several reasons: a) The VNIC is deleted by another process. b) The dlmgmtd daemon is not available somehow. This would be a fatal error, but could be recovered when dlmgmtd is restarted, so an assertion is not preferred. The administrator should try the operation again if this fails. > ND-03/178: Do you mean "link name"? > Fixed. >> usr/src/lib/libdladm/common/libdlvnic.c > > ND-04/57-58: Could use "_id" for both field names to be consistent. > Accept. > ND-05/128-129: The comment no longer matches the code > Fixed. > ND-06/140-142: Why is this check needed in this particular function? > dladm_vnic_info() could take either DLADM_OPT_PERSIST or DLADM_OPT_ACTIVE to get the persistent or active configuration information of a VNIC. But for now, there is no persistent VNICs, so that we need this check. > 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) > Sure. >> usr/src/lib/libdladm/common/libdlvnic.h > > ND-08/41-42: Could you "_id" for both field names to be consistent. > Sure. >> usr/src/uts/intel/vnic/Makefile > > ND-09/general: see ND-12 > See ND-12 >> usr/src/uts/sparc/vnic/Makefile > > ND-10/general: see ND-12 > See ND-12 >> usr/src/uts/common/sys/vnic.h > > ND-11/58-59,68-69,94-95: see ND-04 > Fixed. >> 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. > If I understand correctly, to make the changes as you suggested, we will need to change mac_open() to take the link name (or linkid) as the argument. This is actually what I had but it was pointed out by another reviewer, that the mac module should not have any knowledge of either link name or linkid, and one should call dls functions to convert link name, linkid to the mac name, and call mac_open() with the returned mac name. In another word, MAC clients like aggr, vnic does not rely on DLS streams, but relies on dls module which manages dls_vlan_t (the representation of a data-link). If one needs to convert link name (or linkid) to mac name, it has to call into the dls module. The only change that I can make is to make dls_hold_device(), dls_get_macname() and dls_rele_device() to be in the modstub, so that other drivers do not need to have explicit dependency on dls. But I don't think even that is correct, as they do have dependencies. >> usr/src/uts/i86xpv/xnbo/Makefile > > ND-13/general: see ND-12 > See ND-12. >> usr/src/uts/common/io/vnic/vnic_dev.c > > ND-14/219-235: see ND-12 > 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. > Sure. > 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. > This has been changed - as we felt that the creation/deletion of data-link should be independent from the creation/deletion of a mac, so that we moved dls_create()/dls_destroy() from mac_register()/mac_unregister(): creation/deletion of the dls_vlan_t for physical links will be taken care of by the framework (specifically, they will be called in softmac during the post-attachment/pre-detach routine of each physical device), but each pseudo driver (aggr, iptun, vnic) will have to call dls_create()/dls_destory() for virtual links. >> 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"? > Accepted. > ND-18/428: It seems that this could be done from within dls_vlan_hold() > itself to avoid code duplication > Accept. > ND-19/482-483: Fix the grammar > Fixed. > 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? > No. A removed physical link is a link that is DRed out or is removed during system shutdown. Its configuration will not be lost unless a reconfiguration boot is done or its configuration is explicitly deleted by a "dladm delete-phys". If we rename an "active" link to "removed" link, say: # dladm rename-link <link1> <link2> where link1 is currently active, and link2 is currently DRed out, this command could be ran when you DRed link2 out, and then DRed link1 in, and would like link1 to have all all configuration of link2 (for example, autopush, zone, aggregation and VLANs created over it). As a result, link1 itself won't be existent. I've rephrased the comments. >> 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. > This is again because of Nemo unification and the perstream fast-path. Some legacy driver simply does not support the special-tagged packets (VID=0 but pri!=0), and if we use per-stream fast-path for those drivers, they won't support QoS for non-VLAN streams. > ND-22/477: typo "prossessing" > Fixed. >> 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. > Okay. > ND-24/417: Please cleanup the XXX, also you must not refer to "Crossbow" > in the source code. > Sure. Actually we discussed within the team and we think that I should leave all the "XXX" comments, to get more attention from the reviewer. > 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". > One is for per-stream mode and another is for non-perstream mode. I've made it clear. > 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. > Okay. I've added at the top of dld_str.c >> 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"? > Changed. > ND-28/229: it seems DLS_CHANNEL_LEGACY no longer exists. > Removed. > 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? > It should always work. The impact of MAC_CAPAB_NO_NATIVEVLAN is 1. per-stream fast-path cannot be used for VLANs. 2. hardware-checksum should not be advertised for VLAN streams, even the driver claimed that they are able to do hardware-checksum. > ND-31/345-346: is there any case where DLS_VLAN_EXPLICIT > and DLS_VLAN_FORCE could be set without the other? > DLS_VLAN_FORCE can only be set when DLS_VLAN_EXPLICIT is set, but the reverse is not true. DLS_VLAN_EXPLICIT can be set without DLS_VLAN_FORCE is set. I've changed the comments. > ND-32/356: typo: s/hold/held/ > As I move dls_hold_device() into dls_vlan_hold(), this comment is not any more needed. > 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. > Accept. >> usr/src/uts/common/io/dls/dls_link.c > > ND-34/1316: why is a separate hold of softmac needed? >> This is to make sure that the devinfo is hold and not detached after dls_hold_device() is called, so that its mac is always registered, and its dls_vlan_t always exists. >> 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" > Changed to "If this happens, simply assume this is a invalid door call request and return failure." >> usr/src/uts/common/io/dls/dls_soft_ring.c > > ND-36/629: why is this needed? > It is not needed. >> usr/src/uts/common/io/dls/dls_vlan.c > > ND-37/145: s/Hole/Hold/ > Accepted. >> 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. > This was done the way you suggested, but we found it loses a tail-call which cause more performance regression. > 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. > As we already started the discussion about the per-stream fast-path in another thread, let's continue the discussion there. > 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? > I agree. I've changed this to another two new MAC functions: mac_hold_exclusive() and mac_rele_exclusive(), which check whether we can hold the mac exclusively. I think the better option is to change the signature of mac_open() to take an additional exclusive argument, and see whether exclusively open the mac could succeed, but that requires to change all the mac clients (when they call mac_open()). What do you think? > ND-41/898,972: These functions shouldn't be impacted by the per-stream > fast-path. > Again, let's discuss this separately in other per-stream related thread, as whether or not we use the current fast-path scheme is not decided. In fact, I did a prototype to prove that the current per-stream fast-path can be disabled dynamically, and in that fast-path, I changed the implementation so that these functions are not 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. > I merged MAC_CAPAB_NONONVLANQOS, MAC_CAPAB_NOLINK_UPDATE, MAC_CAPAB_MDT, MAC_CAPAB_TX_LOOPBACK and MAC_CAPAB_PERSTREAM into a single capability MAC_CAPAB_LEGACY. But I left MAC_CAPAB_NOZCOPY, MAC_CAPAB_NO_NATIVEVLAN and MAC_CAPAB_PUTNEXT_TX, because the aggr driver (which is not legacy) also needs to advertise the first two, and MAC_CAPAB_PUTNEXT_TX is associated with a queue pointer as the data, and this the queue pointer won't be known when mac_register() is called. As a result, we need several new mac client functions: boolean_t mac_nonvlanqos_capable(mac_handle_t); mac_mdt_capab_t *mac_mdt_capab(mac_handle_t); uint32_t mac_no_notification(mac_handle_t); and they will be used by the dld module. >> usr/src/uts/common/sys/Makefile > > ND-43/general: these changes don't seem to be needed, only used by files > in uts > Okay. >> 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? > This should be removed. >> usr/src/uts/common/sys/dls_impl.h > > ND-45/85: "SPA"? Should move some of the comment at 100-103 here. > Accepted. >> usr/src/uts/common/sys/mac.h > > ND-57/35. A MAC interface shouldn't depend on DLPI. > Removed. > ND-68/116-124: If so, they should be in mac_impl.h. Explain why they are > here. > Maybe I should change the comments. I think we discussed before, that we added MAC_STAT_LINK_STATE to the dls_vlan_t kstats. Or maybe I should just move MAC_STAT_LINK_STATE to mac_driver_stat? > 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. > See ND-42. > 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. > See ND-42. > ND-70/290-294, 316-317, 332-338: This is softmac specific stuff and it > shouldn't be made common for all MAC interfaces. > Although it is softmac specific, but they need to be used by other modules like dld (for example, the mac_mdt_capab_t structure is used by dld_proto.c to advertise the MDT capability). Also, mc_open() and mc_close() is implemented in a way that can be used by other MACs other than softmac, it will be called as a result of a mac_open()/mac_close() call. > ND-71/452-554: More per-stream stuff in a common header file. Does not > belong here. > What's your suggestion? Note that mac_perstream_open_arg_t is used by mac functions too. >> usr/src/uts/common/sys/mac_impl.h > > ND-72/127-130: What's the purpose of moving these fields around? > I've mentioned this in the comments: "Below fields are set in mac_register() and will not be changed until mac_unregister(). No lock is needed to access these fields." >> usr/src/uts/intel/aggr/Makefile > > ND-73/58: Shouldn't be needed, see ND-12 > see ND-12 >> usr/src/uts/intel/ia32/ml/modstubs.s > > ND-74/1209: s/depend/depends/ > Accepted. >> usr/src/uts/sparc/aggr/Makefile > > ND-75/58: see ND-12 > see ND-12 >> usr/src/uts/sparc/ml/modstubs.s > > ND-76/1176: s/depend/depends/ > Accepted. >> 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. > Please see PSARC/2007/527. The GLDv2 changes are discussed there. >> usr/src/uts/common/io/aggr/aggr_grp.c > > ND-78/general: aggr shouldn't have to depend on dls, see also ND-12. > see ND-12 > ND-79/154: s/lacp protocal/LACP protocol/ > Accepted. > ND-80/155: s/the key to be a 16 bit/a 16 bits key/ > Accepted. > ND-81/155: s/identfication/identification/ > Accepted. > ND-82/184: "usually"? Should say when that's not the case. > Removed. > ND-83/835-836: comment no longer valid > Changed. > ND-84/1371-1376,1453-1470,1487-1501: should not be separate > capabilities, see ND-69 > aggr is not a legacy device, so that I cannot use MAC_CAPAB_LEGACY. But I realized that MAC_CAPAB_NONONVLANQOS actually should not be advertised by the aggr driver, as we won't use the per-stream path for streams over an aggr, so that QoS over non-VLAN streams would always be supported. > ND-85/1553: why is this spelled all-caps > Changed to lower case. > ND-86: cleanup XXX in comments > I changed this and this is not TBD anymore, hence XXX is also removed. >> 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. > Accepted. >> usr/src/uts/common/io/aggr/aggr_port.c > > ND-88/145-154: see ND-12 > 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. > Accepted. > ND-90/302-303: So what's the impact when the cable actually goes down? > Is the administrator left alone with a broken aggregation? > Yes. The aggregation won't work, even "dladm show-aggr" shows the port is attached. That's why "-f" is requested. > ND-91/general: more instances of per-stream related changes making their > way throughout unrelated Nemo code. > Again, let's talk about the per-stream implementation separately. Thanks - Cathy
