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. >>>> 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. >>>> 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. >> 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. >>>> 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. > Sure. >>> 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. > As most of them (mc_open, mc_close and MDT related stuff) are only for per-stream fast-path, and it will be changed soon. I'd prefer to leave it as is for now. >>>> 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? > First, if a GLDv2 driver registers its gldm_send_tagged() callback, then the GLDv2 framework will update its margin value to 4 (if the driver itself didn't register a value). Second, if the legacy driver used to support VLAN ppa hack, then softmac will automatically change the margin value to 4 (if they didn't register its own margin value). So, the answer for your question is that "-f" is not needed if the legacy device used to support VLAN. >>>> 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. But when the link is back on, then the aggregation will start working. The problem you mentioned above is the exact reason why "-f" is required, and it is also easy to diagnose such problem if there is any call generation, "dladm show-aggr" would show that this aggregation is forcibly created. > 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. > For legacy devices that do not support link notification, I think the link state would be reported as "unknown". Thanks - Cathy
