Raymond LI - Sun Microsystems - Beijing China wrote:
> Garrett, pls see my comments inline.
>
> Garrett D'Amore wrote:
>> Sowmini,
>>
>> Thanks for updating this proposal, I think we're nearing convergence
>> on what I'd like to see. ;-)
>>
>> A few general thoughts:
>>
>> * we still need to work out the details on a few items, including
>> kstat snapshot collection (you indicated it needs consideration), and
>> property registration.
>>
>> * mac_list_private_props ... maybe exposing this as a property might
>> not be the best choice. its not clear. i'd love to see this idea
>> more fully explored though.
>>
>> * I'd avoid making notes/comments about the STABILITY level of
>> individual properties at this point in the games. We should do that
>> just before the case goes to ARC.
>>
>> * link_interrupt_coalescing, link_cksum_offload. As a writable
>> value, I don't think this makes sense, at least not initially. The
>> framework is supposed to auto-tune these for the user going forward,
>> so I'd prefer not to expose them. (As observable values, they may
>> have some merit, however.) (Right now a lot of drivers don't make
>> use of this, but this will change with crossbow, etc.)
>>
>>
> For "link_interrupt_coalescing", though the GLD framework will
> automaticly switch interrupt rates, we still have some tunables in
> driver side. For example, bge uses "bge_rx_ticks_norm" and
> "bge_rx_count_norm" as parameters when registering blank func to GLD,
> which is now tunable. Another example is e1000g, which has
> "intr_throttling_rate" tunable for customer. ixgb has "coalesce-num"
> as a tunable.
>
> But I find it is confusing and non-uniform across drivers to set this
> tunable. bge has only "bge_rx_[ticks|count]_norm" as tunable. e1000g
> has much more tunables for this feature. We might want Brussel to do
> the job of a uniform interface across drivers.
No, we want to remove the tunables altogether. The only reason that
these are here, is because the framework support for adjusting the
throttling rate is inadequate, and the drivers have not all been
converted to use the coalescing feature of the framework. (In other
words, the uniform interface needs to be between the driver and the
framework. End-users should not be exposed to it.)
In the near term, if you want to expose something, I guess that's okay.
But I'd advise against documenting them too strongly... we will want to
remove them in the future, and end-users should not be relying on this
kind of tuning.
>
> "link_cksum_offload" controls tx/rx checksum enabling as well.
> Framework seems not having this as tunable so far.
Its because it doesn't need to be tunable. The framework negotiates
this feature with the drivers... it always enables the feature if the
driver can support it. There is never any reason to disable it.
>> * link_speed and link_duplex. It appears that you're
>> overriding/removing autonegotiation here. I'm uncomfortable with
>> this approach. First off, at 1G and higher, autonegotiation is
>> required by the standard. (You can however choose not to advertise
>> certain link/duplex combinations.) I'd prefer to have the
>> adv_cap_xxx and lp_cap_xxx values. (This also is very useful for
>> debug, because it lets one see what the link partner is doing.)
> Agree. I think we should add that in.
>>
>> I would consider gutting the ability to disable autonegotiation
>> altogether (I.e. you have to autoneg., but you can refuse to autoneg.
>> to anything other than a fixed speed/duplex setting... :-) This
>> requires coordination from all the interested parties though, because
>> some sites still refuse to support autonegotiation in their
>> infrastructure. (IMO, this is a sign of incompetence on the part of
>> the network administrators, but that is another rant...)
>>
>> Having link_speed and link_duplex as *observable* values (i.e. the
>> result of autoneg.) is however useful. Please retain that
>> functionality.
>>
>> * i'd like to see more public tunables. E.g. the lance_mode, ipg0,
>> ipg1, ipg2 values exposed by all NSN drivers.
>>
> I wonder if we could add more public properties in 2nd phase's scope.
> In the first stage, it focuses on bge/e1000g/nge, which don't have
> those tunables supported yet. It will be a problem for us to test
> those "stub" func then.
You might also look at eri, which is now a GLDv3 driver. :-) I'd not
worry too much about nge, it has other issues. bge and e1000g are good
candidates though.
>> * we need to have observables for all the current kstats.
>>
>> * for the near term, we might want to expose bcopy thresholds. A lot
>> of drivers use bcopy thresholds to determine when to switch between
>> D(V)MA and just plain copying. (Separate rx and tx thresholds are
>> needed, really.) Eventually I hope to move this logic to the common
>> Nemo layer, but that is a larger, and unfunded, project.
> Agree. We could have this added.
>>
>> * what about WLAN properties? It would be nice to consider the
>> current, separate WIFI configuration mechanism to see if we can
>> integrate it into Brussels. Maybe not initially, but certainly in
>> the mid- term. (Think SSID, link keys, etc.)
>>
>> * what about loopback modes? I've mentioned this a few times. It
>> seems like setting/changing the loopback mode (used by SunVTS) could
>> easily be treated as another property for use with Brussels. I'd
>> like to eliminate the separate ioctls that are currently used to
>> handle this. Perhaps mention this for phase 2 or somesuch?
> Would user set loopback mode for any application other than SunVTS?
Not that I'm aware of, though maybe another test application. But I'm
not entirely sure how relevant that question is ... I'm interested in
reducing/eliminating redundancy in the drivers. (I'd be perfectly happy
with a situation where the loopback tunables were not exposed to
userland applications through dladm/ndd, but were restricted to an ioctl
call. I just think that Nemo can do the ioctl remapping in just the
same way that it will for the ndd ioctls.
-- Garrett
