[clearview-discuss] code review request for the UV fastpath project

[Eric Cheng]

Hi Cathy,

Here are my comments.

1.
dld_active_clear() is called many times with the 2nd arg B_FALSE but
only once with the 2nd arg B_TRUE. it might be simpler if you do
something like:
-rename dls_active_clear() to dls_active_clear_common().
-add dls_active_clear(dld_str_t *dsp) which calls
 dld_active_clear_common(dsp, B_FALSE)
-change dls_close() to call dls_active_clear_common(dsp, B_TRUE).


2.
from what I understand, ml_active_set/clear are needed to prevent the
fastpath bind req from succeeding in case aggr has marked the link
exclusive.
instead of using ml_active_set/clear, can we:
disable fastpath when adding a port to an aggr. if the port
is later plumbed, softmac_wput() will go through dld_wput()->
dls_active_set() which will correctly fail because aggr already
owns the port.


3.
if vlan uses the fast path, then you would no longer have per-vlan
statistics. also, since you disable fastpath in i_mac_set_resources
but not in mac_client_set_resources(), vlan properties would be
ignored if they are passed at vlan creation time.
it might be simpler to just disable fastpath for both vlans and vnics.


4.
DLD_CAPAB and related structures are meant to be a private interface
between IP and dld. I noticed this is now used in softmac as well.
How much extra performance does this give you? If the gain is not
significant, I would prefer if this capability is not used in softmac
because we don't want to have to consider softmac everytime we
change this capability (which could be quite often).


5.
I noticed that you've added calls to mac_fastpath_disable()/enable()
in a few places; and I understand the intention for doing so is to
allow fastpath to be automatically disabled when gldv3 is needed and
re-enabled when not. but this convenience (for only a very small
percentage of users) has the drawback of requiring future projects
touching gldv3 to add their own fastpath disable/enable logic.
This may result in the fastpath being deeper ingrained into the
framework, which seems like a wrong direction to head towards.
I am not asking for a solution now. I am just wondering if this
problem was considered and if so what options are available.



eric

On Tue, Jan 20, 2009 at 03:16:44PM -0800, Cathy Zhou wrote:
> Hi,
> 
> I'd like to start the code review for the UV fastpath project. The timer 
> is set to two weeks. You can send the review comments to the 
> Clearview-discuss alias or directly send it to me.
> 
> The webrev is at:
> 
> http://cr.opensolaris.org/~yun/webrev_fastpath/
> 
> The fasttrack is at:
> 
> http://opensolaris.org/os/project/clearview/docs/softmac_fast_path
> 
> and you can find the cscope in
> 
> /net/lido.sfbay/export/home/cathy/clearview-fastpath-review/usr/src
> 
> Thanks
> - Cathy
> 
> _________________________________
> clearview-discuss mailing list
> clearview-discuss at opensolaris.org