Sowmini.Varadhan at Sun.COM wrote:
>> * 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."
Okay.
>> 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?
I don't see it. Maybe I'm not looking hard enough. I'm looking for a
discussion detailing the implications of setting properties when there
are active DLS clients utilizing a link. Is it okay to set all kinds of
properties at any time, and are there no implications of doing so? Can
some properties only be set when there are no DLS clients? Can other be
set at anytime but only take effect when MAC drivers re-registered?
> 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,
I'm not at all familiar with how diskless boot works, but why would there
be no daemon in that case?
> 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?
Okay.
>> * 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.
I'm less interested in asynchronous notification mechanisms than in a
MAC-layer interface to get or set a property. I'm going to need that for
the Clearview IP tunneling device driver.
Right now, I implement a mac_sdu_get() interface to do what I want. The
iptun driver notifies the mac module of max SDU changes by using a
mac_maxsdu_update() function, which caches the result in mac_impl_t (so
that mac_sdu_get() can have access to the appropriate value), and also
triggers i_mac_notify(mip, MAC_NOTE_SDU_SIZE), which in turn results in
DL_NOTE_SDU_SIZE for all interested upper layers.
>> 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).
Okay.
> 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"
Okay.
>
>> * 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.
Got it.
>
>> 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.
Alright.
>
>> * Is the dld_ioc_prop_val_t used for both the GET and the SET ioctls?
>
> Yes.
Okay.
>
>> * 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.
Okay.
>
>> 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.
I agree.
> 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.
Right.
> 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.
Okay. This can be worked out over time. I'm not really concerned about it.
>
>> 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.
Yes, this is what I was thinking. Each MAC-Type plugin tells the
framework, "MACs for this type should support this set of properties".
> 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.
Not trivial. Maybe this points out an hole in the Nemo architecture, in
that we have a generic framework with pluggable MAC-types in the kernel,
but no such equivalent for dladm/libdladm.
> 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?
No, you addressed what I was curious about. The issues here are not
trivial to solve indeed.
>
>> 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.
This is required by the Clearview iptun driver, and have this
functionality working for max SDU (using DL_NOTE_SDU_SIZE upon max SDU
changes). I basically detailed what I needed to do to get this working
in the Clearview gate. The issues were not really that difficult to
resolve for MTU.
> 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 could either help you make the few changes necessary to get a dynamic
MTU property working for Brussels, or I will need to modify what you've
done with Brussels to get this working for Clearview. Either way, a
static MTU is a show-stopper for IP tunnel, as they adjust their MTU on
the fly in response to path MTU changes to the tunnel destination.
>
> I'll make sure I add all this detail in the design doc.
Okay, great.
>> 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?
I'm not really as concerned about which error codes are returned than
about what the administrator sees in show-link or show-linkprop output.
>> 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?
No, I don't think it would. My point isn't that there are two different
MTUs (one at the driver layer and another in the framework) that can be
modified independently of one another. It's that the link MTU exists
regardless of whether the underlying driver implements the property API
and properties documented here.
The driver tells the framework what its max SDU is when it registers in
mac_register(), so the framework knows what the "default_mtu" is
regardless of whether that driver implements Brussels APIs. My (perhaps
useless) thought was that if a driver is incapable of responding to
mc_getprop(deafult_mtu) calls, the framework could respond on its behalf
given that it knows what the answer must be... Maybe this isn't worth
implementing.
>> 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?
Yes, but I think there are inter-project dependencies implied here which
need to be made explicit. In other words, the IP tunnel properties will
only be of use with the Clearview GLDv3 iptun driver. Likewise, VNIC
properties will only be of use when Crossbow delivers VNICs.
Thanks,
-Seb
