Cathy, Cathy Zhou wrote: > Nicolas, > > Sorry that I only have time to get to your comments today. Please see > inline: > >>> 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/ >> >> >> 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. >> > I see what you mean. But I do think this worths explicit dependency, as > those pseudo drivers need dls (datalink module) to create link and get > link-specific information.
The MAC clients depend on vanity naming features which happen to be currently implemented in dls. Creating links via dls as a separate step is a new requirement introduced as a side effect of vanity naming as well. The MAC clients do not depend on any other functionality which is provided by dls. Except for the functionality introduced to support vanity naming, dls is "only" another MAC client. So the dependency is really not on the dls module itself, but on the vanity naming functionality, and for this reason, I think that requiring every MAC client to have an explicity dependency on another MAC client is not clean architecturally, and also introduces additional code that could be easily avoided. modstubs can provide a much less intrusive solution to provide access to this vanity naming functionality, and simply the code. >>>>> 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. >> > ... and create the link as part of mac_register()? This could be done > but I thought that was what we want to change. We changed this not only > because of the the need of Nemo unification. As I recall, separating > link creation from mac registration was requested by several person. I don't remember this discussion, maybe I missed it. Why would we want to force each virtual MAC driver to do these things explicitly if we can do them transparently for them from the MAC layer? >>>>> 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? >> > I cannot tell for sure. But as I recall, it was about -1% to -2% to > netperf performance. I can certainly re-measure it when I does the > fast-path working. With the soft-mac fast-path in place we should keep that code out of the MAC layer. >>> 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. >> > I think it is better to expect the same result when querying the same > capability. Also, as you can see, I've marked those softmac-specific > capabilities (with the comments and the high-bit) as a comment from Seb. Why is it better to have an additional separate capability than simply exchanging that information as part of an entry point of the softmac capability? Maybe this should be revisited with the new fast-path implementation in place, since it will make some of these optimizations and capabilities obsolete. Thanks, Nicolas. -- Nicolas Droux - Solaris Core OS - Sun Microsystems, Inc. droux at sun.com - http://blogs.sun.com/droux
