On (06/23/07 11:17), Sebastien Roy wrote:
> > http://opensolaris.org/os/project/brussels/files/brussels.pdf
thanks very much for the review!
> General:
>
> * It would be good to note in the document title that this is a design
> document.
done (will show up in next rev)
> 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.
Public properties are those parameters that are typically
used to characterize the behaviour of the driver for a given media
type. Does the following definition sound less ambiguous/confusing?
"Well-known, or PUBLIC properties that characterize
behavior of drivers of the given media type. Examples of such
properties are link speed, auto-negotiation for ethernet."
> 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 thought I had captured some of the prior discussions in Section 3
(page 9 of Rev 0.2) - is there some specific discussion that you
had in mind that I had omitted to document?
One issue that we are still working out, is property handling in
a diskless boot environment. The issue here is that there's no daemon
available for loading properties into the kernel, and since the attach
takes place well before init, if the property value *must* be known at
the start of attach, driver.conf seems to be the only available way to
obtain this property. We're still looking into this, to see if this can
be avoided, or mitigated somehow, and I was going to document a summary
of it in the next revisions.. would you prefer to see a place-holder
for this in the current doc?
> * 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.
In the current phase, we are not providing any interfaces to other kernel
modules to notify them about modifications to driver properties, or hooks
for them to get/set link properties. Some complex operations (e.g.,
reconfiguring tx-rings on a plumbed interface without reattach) would
require significant readjustments (ring reassignment in the tx-rings example).
Brussels does not provide the infra-structure to send this notification
to other kernel components.
>
> 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.
right. I will make sure this is mentioned explicitly in the next rev.
> * 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?
By itself, none is needed: if an attempt is made to configure
a property that is not supported by the driver, a ENOTSUP will be returned
(see Section 3.2.3).
However, I believe this question is related to your comment on Section
3.2.3 below, and I've added more clarifications further down..
> 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.
sorry for the ambiguity - that was not the intention: "devices" should
have been "MAC-registered network drivers"
> * The strange difference in naming convention between the GET/SET ioctls
> isn't explained.
oops- that was a typo: should have been DLDIOSETPROP and DLDIOCGETPROP.
i.e., the naming convention is the same.
> 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.
All of these attributes (as they are described in Section 3.2.1) are
maintained inside libdladm. In addition for public properties, the
scalar encoding of the property name is shared with the driver through
the <sys/dld.h> file.
> * Is the dld_ioc_prop_val_t used for both the GET and the SET ioctls?
Yes.
> * 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"?
That should have been "pr_num". "dld_pr_num" is the internal name (in
the current libdladm prototype) for the pr_num passed down to the kernel.
>
> 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?
Currently (i.e., in onnv), there is some overlap of output content
between "show-link", "show-dev" and "show-linkprop": show-link
displaces {mtu, link-type}, show-dev displays {link status, speed,
duplex}, and show-linkprop (currently) displays zone property
information.
I'm open to suggestions on what would be most meaningful here, but it seems
to me that "show-linkprop" should only display output that is relevant to the
link class. For example, in the appendix, the Public properties have
been sub-divided based on media type, so it would not make sense to display
MII properties for a tunnel interface. This would imply that
public properties would have to be subdivided based on link-type
(similar to the philosophy used for MAC-type plugin arch for kstats)
and this could be done by having dladm check the link type before
querying for a set of props.
> Section 3.2.4:
>
> * Nit: s/colescing/coalescing/
will be fixed in the next rev.
> 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?
Interesting thought.. but it strikes me that the problem areas are
slightly different. The MAC-type plugin architecture tries to provide
the same set of entry points for different mactypes: that principle
has been extracted into the mc_setprop/mc_getprop callback points in the
mac_impl_t. The actual implementation differences are embedded in the
setprop/getprop function itself.
One related idea is to build on the methods used for kstat management
via the mactype plugins.. if we had each mac provide a list of
{property number, property name, property type} with mactype_register,
the framework could use that to do the string <-> number mapping.
It has the advantage that there's no reliance on hard-coded mapping via
sys/dld.h, and allows dladm to quickly filter out invalid setprop/getprop
attempts (e.g., setting tunnel hop limit on ethernet), but
this would require that libdladm upload the registered properties for
each mac that starts up, which requires additional system-calls for (at
least) the first setprop on the link, and complicates the initial
door_upcall hand-shake with dladmd to load up properties when the link
attaches. Also, since the pd_check/pd_set/pd_get functions are already
a part of libdladm, the registration is primarily useful for mapping
name <-> number, and assesing if the property is supported.
Or did you have something else in mind?
> 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.
As you point out, more detail is needed. For all of the reasons that
you point out, the mtu cannot be changed easily once the mac is plumbed,
and attempting to do so would need notifications and adjustments to all
the layers above the mac. Making all of these adjustments for each property
is not in the scope of Brussels, and in the mtu case, the mtu can only
be changed if the mi_active of the mac_impl_t is quiescent. Attempting
to change the default_mtu on a plumbed mac will result in a log message
indicating that the change will take effect at the next unplumb/plumb
operation (in practice this will happen before the next mac_start,
when dladmd parses the persistent repository and confiures the mtu property).
I'll make sure I add all this detail in the design 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.
If the driver has a buggy/incomplete implementation of Public property
support, and does not respond correctly to the GETPROP ioctl for some
expected property, that should (correctly, imho) be flagged as an
error.
If, otoh, the driver has not chosen to plug into the Brussels framework, no
mc_{s, g}etprop would be registered, and this would be recognized as
the ENOTSUP by dladm. I realize that this causes some conflict because
the same error code (ENOTSUP) is also being used for the case when
the property has no semantics for the driver (e.g., trying to set
adv_autoneg_cap on tunnels). Should we use some other error code
for the latter case? If we use EINVAL to indicate a malformed
set/get property request (i.e., bad input for valid property) would it
make sense to use ENXIO for attempts to set/get a property in a context
where it does not apply?
>
> 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.
For the mtu case: the MTU definitiion targetted in the doc was intended
to address the driver's default_mtu (the tunable typically tweaked
by setting various params in the driver.conf to get Jumbo Frames).
As you point out, there is a related concept of the GLDv3 max SDU which
can be lowered without involvement from the driver. We could split the
mtu into two items, the "max_mtu" (which would be the classical
ethernet driver's "default_mtu") and the "max_sdu" which would be
recognized and handled in the mac layer itself, without attempting to
invoke the mc_*prop functions. Would this make sense?
> 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.
I could make a note, but I believe the idea is that the current
(ongoing) implementation in both cases will eventually plug into the
Brussels framework?
thanks!
--Sowmini
