Sowmini.Varadhan at Sun.COM wrote:
> http://opensolaris.org/os/project/brussels/files/brussels.pdf
Excellent document. My comments are based on version 0.2.
General:
* It would be good to note in the document title that this is a design
document.
Section 2.1:
* The description of "Well-known" property is very confusing to me. How
can such properties (you list link speed and auto-neg as examples) be
expected to occur in every driver if the properties are only inherent to
specific media types? Also, how can they be expected to occur in every
driver if the callbacks described in section 3.1 are optional (see my
comments regarding section 3.1)? Maybe the word "expected" threw me off,
as I interpret that as meaning that any driver that doesn't implement
public properties is not behaving as "expected", which is clearly not true.
Section 3:
* Some details are needed regarding when it is valid to set a property's
value. We've had discussions before about potential problem with
modifying some properties after a driver has attached. Have these issues
been worked out? If so, is there nothing to note regarding that in the
design document?
* I don't see any interface documented here providing support for kernel
components to get or set link properties. Is one being provided? I hint
at the need for one in my comments on section A.1.2.
Section 3.1:
* Although it's implied by the existence of the MC_SETPROP and MC_GETPROP
flags, it would be good to explicitly point out that these two callbacks
are optional. I'm guessing that this was the intent anyway.
* I don't see any details here regarding how the framework discovers the
list of supported properties or how the driver supplies its initial
values for properties during attach. Maybe none is needed?
Section 3.2:
* In the first PP, the phrase, "the {set, reset, show}-linkprop commands
will be extended as described [8] for devices." is ambiguous in my mind.
What is a "device"? Do you mean "physical device"? Does this mean
that one can't use this framework for aggregation MACs, or VLANs, or IP
tunnel interfaces (as will be introduced by Clearview)? Please clarify
in the document.
* The strange difference in naming convention between the GET/SET ioctls
isn't explained.
Section 3.2.1:
* The set of property attributes listed (1-5) here seem like they're
somewhat in a vacuum. Where do these attributes live? Are these things
that each driver tells the framework about using some other mac module
function? Some clarification is needed.
* Is the dld_ioc_prop_val_t used for both the GET and the SET ioctls?
* I'm confused by the description of DLD_PROP_PRIVATE here. You say that
dladm passes down that value for "dld_pr_num", but so far, there's no
description of how "dld_pr_num" belongs to any data structure that gets
passed around between user and kernel space. Do you mean "pr_num"?
What's the relationship between "dld_pr_num" and "pr_num"?
Section 3.2.3:
* I think this sub-section needs to be expanded a bit. Are all public
properties listed in "show-linkprop" output for every class of link?
Are some of these properties' values printed in the "show-link" output as
well? What should dladm "show-linkprop" and "show-link" output look like
for links that don't support all or some public properties?
Section 3.2.4:
* Nit: s/colescing/coalescing/
Section A.1:
* All of these (except for MTU) are very Ethernet specific. How can the
MAC-type plugin architecture be leveraged to provide a separate set of
"public" properties for different link types?
Section A.1.2:
* A lot more detail is needed for the MTU property. What happens when
this property is modified on the fly? A DL_NOTE_SDU_SIZE needs to make
its way up to interested DLPI consumers, and that's not detailed here.
Also, the max and min sdu are currently immutable values in mac_info_t.
If MTU is now a variable thing, then max sdu (and maybe also min sdu for
consistency) needs to be yanked out of mac_info_t. Other layers
interested in obtaining the max and min sdu would now need to stop
peeking at the mac_info_t, and instead use some kernel API in mac or dls.
I don't see such an interface proposed in this doc.
Another issue is; if all of these properties are optional from the
driver's point of view, how is an administrator going to react to a link
which does not appear to have an "mtu" property in its show-linkprop
output (for example)? Clearly, every link has an MTU regardless of which
properties the driver wishes to implement, if any.
The concept of MTU (or max SDU) exists in the GLDv3 framework regardless
of whether the driver supports having its "default" MTU modified. Going
a step further, it might be possible to lower the MTU of a given link
within the framework without involvement of the driver. In any case,
even if modification of MTU isn't implemented, it doesn't seem to me that
reporting the MTU of a link should depend on an underlying driver's
capability to implement the get/setprop callbacks. It's possible that
there are other properties like that which are inherent to datalink
interfaces and not to any particular driver.
Section A.2:
* Project scoping nit: It might be worth noting here that these
properties will be implemented by the Clearview IP tunneling device
driver, which is still under development, and that they're not part of
the Brussels project.
Section A.3:
* Same here with reference to the Crossbow project.
-Seb
