Seb, Thanks for your comments! See inline:
> Cathy Zhou wrote: >> External webrev: >> >> http://cr.grommit.com/~yun/webrev_uv_09_28 >> > usr/src/cmd/dladm/dladm.c > ------------------------- > > * 2209,2245: It doesn't look like the head and the printed information > quite line up. e.g.: > > bash-3.2# dladm show-vlan > LINK VID OVER FLAGS > bge2001 2 bge1 -i--- > > Testing should run through the output of all sub-commands (including > parseable mode) to make sure everything looks good. > > Okay. > usr/src/cmd/dlmgmtd/dlmgmt_util.c > --------------------------------- > > * 97,108,122: These functions don't do the finding, but compare two > values. Naming them cmp_link_by_*() would be more appropriate. > Accept. > usr/src/uts/common/io/dls/dls_link.c > ------------------------------------ > > * 901-906: I thought that this kind of thing would generate lint errors > for non-debug builds, as you end up with an empty block. In any case, > this can also be written as: > > ASSERT(dlp->dl_mh == NULL || dlp->dl_mh == mh); > if (dlp->dl_mh == NULL) { > dlp->dl_mh = mh; > dlp->dl_mip = mac_info(mp); > } > Accept. While I can do what you suggest. Is this (lint error for non-debug builds) really matters? > usr/src/uts/common/io/dls/dls_mgmt.c > ------------------------------------ > > * 73: Out of curiosity, I'd like to know if there's an inverse of this > operation. In order to allow libdladm in a non-global zone to > operate, it needs a file-descriptor associated with the dlmgmtd door. > An ioctl which returns the fd would do the trick, but I'm not clear on > the details of how to obtain that fd. > By looking up the NV code, I don't think there is a function which can convert door_handle_t to a fd. But I don't see any problem if dls keeps the fd information if needed. > * 169: I don't feel comfortable with the lack of a ceiling on the amount > of time we spend attempting (or the number of times we attempt) to do > this upcall. > Okay. I will make it also managed by the retry number (so that the total maximum retry attempt would be 3), that includes retry attempts caused by REVOKED door and EAGAIN errno. > * 206: The description of the "dev" argument is misleading. I believe > this must only be non-zero for devices under softmac. Is that the > case? > No. It is non-zero for all physical links. Including of those GLDv3 devices. > * 210: Correct me if I'm wrong, but no link-id mapping needs to persist > unless something else which is persistent refers to it (like a > persistent property, or the link being part of a persistent > aggregation, or a persistent VLAN being created over the data-link in > question.) If so, then the "persist" argument is an architectural > hiccup, and the logic to determine whether or not to persist a linkid > mapping really belongs in libdladm. > Not really. We will need to persist the physical dev_t of a physical link when a physical link is created. This physical dev_t is used to open the device (and probably force the attachment of this physical device so assure the existence of the MAC). > At the same time, information for implicitly created VLANs must never > be persisted, so those semantics are slightly different. In any case, > I'm fine with what you have, but I find it strange that we persist the > link-id mapping for physical data-links when we don't have to. > > * 217: I find the "nosupp" argument confusing, but I also noticed that > it was removed from the UV gate a few days ago. Someone else must > have also thought the same. :-) > It is moved from dls_mgmt_create() to a new dls_mgmt_update() function (because of bug 6615700. If you prefer, I can change this argument to "novanity" to indicate it means this physical link does not support vanity naming (e.g., DL_IB links). > usr/src/uts/common/io/mac/mac.c > ------------------------------- > > * 500: s/MACs/MAC/, and the parentheses needs closing in the comment. > Accept. > * 505-510: This comment block needs some grammatical fixups. We can > work out the specifics offline. > Sure. I will contact you offline. > * 547: Checking for the same MAC_CAPAB_PERSTREAM capability on every > open doesn't seem optimal, since you'll be getting the same result > everytime. The open function to call shouldn't change from call to > call, so wouldn't it be better to obtain the open function pointer > once, perhaps during mac_register()? MAC_CAPAB_PERSTREAM isn't > applicable at all unless MAC_CAPAB_PUTNEXT_TX is also supported, > right? If so, it might make sense to simply return the open function > as part of MAC_CAPAB_PUTNEXT_TX and get rid of MAC_CAPAB_PERSTREAM. > the MACs of legacy devices (and only them) would have below capabilities: MAC_CAPAB_TX_LOOPBACK, MAC_CAPAB_PUTNEXT_TX, MAC_CAPAB_PERSTREAM Do you prefer me to merge all three together - to be something like MAC_CAPAB_LEGACY or such. > * 973: I don't understand the "set" argument. Some intro. comment is > needed to explain it, or alternatively, it should be removed in favor > of some way for the mac module to determine the semantics based on the > state of the mac_impl_t. Snaking up the call chain, I cannot > determine any logic to when B_TRUE is passed in or B_FALSE is passed > in. > We cannot remove it. Because whether "set" is true of false relies on whether the caller is a per-stream type DLS clients. Note that the per-stream type DLS client would have an single lower stream associated to it, and the promiscuous mode would be directly through that lower stream. In that case, "set" is set to false, and mac_promisc_set() is not responsible to set the promiscuous mode of the underlying device (through the shared lower stream), neither it causes the update of the MAC's tx callback pointer (mi_txinfo and mi_txloopinfo). The only thing mac_promisc_set() does is to see whether it needs to call i_mac_notify(mip, MAC_NOTE_DEVPROMISC). This helps me to find two problems though: a. mi_legacy_promisc is not needed. mi_promisc is only incremented and decremented when "set" is true in mac_promisc_set(), and that might cause th e update of the tx callback pointer. b. mac_promisc_get() should be changed to: ... if (ptype == MAC_DEVPROMISC) return (mip->mi_devpromisc + mip->mi_legacy_devpromisc != 0); The same as mac_stat_get(MAC_STAT_PROMISC). > Reading how it's used, it would seem to represent the need for the > caller to have us tweak the underlying device's promiscuous mode. I > don't understand how the caller is the authority on this. How does > the caller know what's below the mac module? > See above. I will add some comments. > * 1551: ASSERT() is too severe here, as it allows a misbehaving driver > to panic the machine, while failing the mac_register() would > gracefully handle the error in a visible way for both debug and > non-debug. > Accept. > > usr/src/uts/common/io/net_dacf/net_dacf.c > ----------------------------------------- > > * 29: For those not intimatley familiar with the dacf framework, it > would be useful to expand a bit on this comment. For example, explain > which devices these operations apply to. Is it for all DDI_NT_NET > devices? How does the dacf framework know to apply net_config_op for > these devices? > Accept. > * 62: With the move to Mercurial, we are removing the use of keywords in > the source. > Okay. I changed the string to "net DACF" > > usr/src/uts/common/io/softmac/softmac_dev.c > ------------------------------------------- > > * 460: Instead of calling dld_wput() here, could we setup q_qinfo in > softmac_open() put's on this queue call dld_wput() instead of > softmac_wput()? I did something like that in iptun (see iptun_dev.c > in the clearview gate). > Accept. > > usr/src/uts/common/io/softmac/softmac_pkt.c > ------------------------------------------- > > * 42: Why soes softmac need a to define softmac_m_resources() and a > blanking function? > This is unfortunate, that in order to make use of the POLL capability, we have to have those functions. > > usr/src/uts/common/io/softmac/softmac_main.c > -------------------------------------------- > > * General: How does softmac_create() end up being called for network > devices which are not supported by softmac and not GLDv3 (such as IB > and PPP for example)? > I think PPP would be handled in this case. As they don't create DDI_NT_NET device node. But every DDI_NT_NET device (including the IB device) would be associated to a softmac_head_t. Then softmac determine whether it needs to call mac_register() and dls_create() based on different attribute of this MAC. For example, whether it is a native GLDv3 device, and whether the media type have a GLDv3 MAC plugin. > * 177: An intro. comment is needed here. Given the name of this > function, it's not at all evident that it's called by net_dacf for > _all_ network device nodes. It also performs a different function for > different types of devices (GLDv3 vs. non-GLDv3 for example). > Sure. I will add some comments. > * 215-220: I don't understand this comment. What is its purpose? What > is the impact of sh_cnt not being accurate? > I think it might not be needed any more after we move dls_destroy() as part of softmac_destroy(). I will test it out. > * 256: Could you add a comment explaining why a taskq is needed here? I > can see why a taskq would be needed for calling mac_register(), but > softmac_mac_register() does a whole lot more than that. It's not > clear why all of what softmac_mac_register() does needs to be done by > a taskq. One concern is error semantics for GLDv3 devices. If > dls_create() fails in softmac_mac_register(), for example, shouldn't > we fail the driver's attach? > I've added the comments to explain the reason: /* * Note that this function could be called as a result of a open() * system call, and spec_open() already locked the snode (SLOCKED * is set). Thefore, we can only start a taskq to finish the rest * of work, as the potential ldi_open_by_dev() call would again * try to hold the same lock. Before that, inform dlmgmtd that * this link is up so that softmac_hold_link() is able to know * the correct state of the link. */ In general, taskq is only needed for non-GLDv3 drivers. I will try to restructure this function. > * 319: I believe this is equivalent to: > > *notifications = ~softmac->smac_notifications & > (DL_NOTE_LINK_UP | DL_NOTE_LINK_DOWN | DL_NOTE_SPEED); > Accept. > * 355: Won't this ASSERT() trigger if (for example) I've named some > existing interface "ce0", and the "ce0" softmac tries to register? > In that case, EEXIST would not be returned. Instead, we will assign a "net<N>" name to the ce device. > * 368: My impression of softmac_mac_register() is that some code > refactoring is needed to separate the GLDv3 vs. non-GLDv3 code paths, > and perhaps to put some of the code that doesn't have anything to do > with registering with the mac module back into softmac_create(). The > function's name implies that its function is to call mac_register() > for softmac devices, but it does much more than that. Specifically, > could the GLDv3-specific code-paths be up-leveled to softmac_create()? > See above. > * 427: Is it really necessary to restrict the use of softmac in this > way? If some 3rd party wants to provide a DL_FDDI MAC-Type plugin, > for example, could softmac work with legacy DLPI FDDI drivers if this > check didn't exist? > How to determine the m_type_ident of the mac_register_t? If that can be derived from the media type, we can remove this check. > * 762: Intro. block comment needed for softmac_mac_recreate(). > Added. > * 834: Intro. block comment needed for softmac_lower_setup(). > Added. > * 873: s/nodes/node/ > Accept. > * 879: Where do we do the attach for a style-2 non-VLAN device? > 878 if (((softmac->smac_style == DL_STYLE2) || (vid != VLAN_ID_NONE)) && 879 ((err = dl_attach(lh, vid * 1000 + softmac->smac_ppa)) != 0)) { 880 goto done; 881 } 882 > * 960: eri is now a GLDv3 driver. > Changed to "old legacy eri driver" > > usr/src/uts/common/sys/softmac_impl.h > ------------------------------------- > > * This file could stay in usr/src/uts/common/io/softmac/. > I could change this. But I'd like to understand the common practice. Why dls_impl.h, dld_impl.h are put in usr/src/uts/common/sys? Thanks - Cathy
