Cathy, On Nov 6, 2007, at 1:42 AM, Cathy Zhou wrote:
> 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 Thanks for your response. I have a few follow-up comments below... > >> >> 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/ >>> 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. That's not what I was asking specifically. > 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. They can have a dependency, but the modstub allows you to avoid making that dependency explict, and decouples the interface from the current implementation of vanity naming. Having modstubs would allow all new explicit dependencies on dls to be removed, and provides an interface which is not tied to the current implementation of dls. Would it also be possible to do the hold from the function which returns the mac name? This way we could have a simpler "resolve/use/ release" cycle. >>> usr/src/uts/common/io/vnic/vnic_dev.c >> >> 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. Is there a way for the MAC layer to transparently determine whether the link needs to be created on behalf of the pseudo drivers? For example you now have the special m_instances value of -1 for these drivers, maybe this could be used by the MAC layer as hint. >>> 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. Was it a measurable performance impact? Is it worth introducing a putnext() in the MAC layer and impacting the common data-path? >> 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? I think the new solution you have looks fine. Since these functions are used only when removing a link, I don't see the need to complicate the common client API for all other clients. > >> 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. Great. > > 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. It seems that the queue pointer needed by PUTNEXT_TX could be exchanged by the new LEGACY capability itself separately from the mac_register(). This would allow you to consolidate that capability as well. >>> usr/src/uts/common/sys/mac.h >> 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? If you can at least change the comment, I think that would be sufficient IMHO. >> 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. For ND-71 and ND-71, I would suggest considering moving as many of the softmac specific definitions to a softmac specific header file or mac-private header file which is kept separate from the common definitions. >>> 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. So what is the default behavior for legacy GLDv2 drivers which are not modified to pass that value during registration? Will their use require the administrator to use the -f flag during VLAN creation and reduce the interface MTU? >>> usr/src/uts/common/io/aggr/aggr_port.c >> 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. So -f will allow the administrators to build aggregations which will stop working as soon as a common condition such as a down data-link occurs. I think there's a big risk of call generation here. Maybe in the dladm show-aggr output, the link shouldn't be reported as up, but somehow marked with a special value (always-up?), so the administrator knows that it should not trust that value. Nicolas. -- Nicolas Droux - Solaris Core OS - Sun Microsystems, Inc. droux at sun.com - http://blogs.sun.com/droux
