sowmini.varadhan at sun.com wrote:
> 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?
>
Lowering the MTU is done at the IP layer. There is little reason to
change the SDU size reported by the driver. dladm shouldn't concern
itself with that.
The driver's default_mtu is really the value reported via the maximum
SDU in Nemo. I'd just leave it "default_mtu" for now.
Do we have a way for the upper layers of the stack (IP, TCP, VLAN, aggr)
to react to a change in the max_sdu? I don't think so. Its likely that
changing this tunable will require replumbing various layers. And,
quite frankly, that's probably okay, because when switching to a larger
MTU it requires other administrative changes as well. (I.e. you have to
make sure all other hosts on the same subnet *also* can cope with large
frames.)
Engineering a solution around this is likely to be "non-trivial". So it
may be simpler to just have a way for brussels to report back to the
user that the change won't take effect until the upper layers are replumbed.
-- Garrett
>
>> 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
>
>
> _______________________________________________
> networking-discuss mailing list
> networking-discuss at opensolaris.org
>
