Sowmini.Varadhan at Sun.COM wrote:
> the idea is that if the driver can't set the property because it is
> BUSY (because it has been inflexibly written to handle the property
> change at attach only), it can return EBUSY. In this case we want
> to store the property setting in the repository, so that, if the
> driver were to be restarted (e.g., unplumb/plumb pair of operations),
> the new setting would be restored when the driver contacts the daemon
> at attach. (See thread at
> http://mail.opensolaris.org/pipermail/brussels-dev/2007-June/000253.html
> for some history on this discussion)
>
> So the behavior (after persistence via the daemon is implemented)
> will be to echo a warning that the property was not set, but will
> be picked up at the next driver restart. The model being followed
> here is similar to update_drv (which echoes "cannot unload module: <foo> \n
> will be unloaded upon reboot", when attempted on a busy module)
Okay. As long as something meaningful is printed to the user of dladm,
that's fine.
>>>> usr/src/lib/libdladm/common/linkprop.c
>>>>
>>>> * 218-241: Why are these needed? Are there actual alignment issues, or
>>>> are these used to quiet lint? Could memcpy() not be used instead?
>>>> * 223: Why htonl()? Since we're dealing with values that don't show up
>>>> on the wire, intuitively, we should be dealing with values in
>>>> host-byte-order. Am I missing something?
>>>> * 1927: I don't see the need for GETINT64() here. Why not a simple
>>>> memcpy()?
>>> These were used to quiet lint errors- I was reluctant to suppress the lint
>>> message by other means, since the layout of the structure
>>> can change in the future, and we would have to deal with the alignment
>>> problem at that time anyway. The htonl itself was used
>>> to deal with byte-ordering differences (memcpy will run into
>>> byte-ordering differences, and doing the byte-swap is messy for
>>> int64's). I'll comment this to make it clearer.
>> What properties are stored or returned in network byte-order?
>
> e.g., the mtu is 64 bits. if we do a memcpy, we'd get the byte order wrong
> on little-endian platforms like intel.
But the MTU value itself doesn't end up on the network; it's in host byte
order in all of the software I know of. Maybe I'm being dense; where
exactly is the MTU stored in network byte-order?
Regardless, it would seem simpler to me to document that the driver API
expects that all property values are set in host byte-order, and must be
returned (for get operations) in host byte-order. You then don't need
these quirky macros, and you won't need to guess which values are in
network byte-order and which are in host byte-order. This is an API
after all, and not a networking protocol. The drivers and the API both
speak "host byte-order"...
>>>> * 584: The semantics should be that if DLADM_OPT_ACTIVE isn't set, then
>>>> the caller doesn't want the changes applied to the running kernel.
>>>> While dladm currently doesn't have a command-line flag to do
>>>> persistent-only operations, it doesn't seem too hard to implement that
>>>> in the API.
>>> need clarification: this would also be something that applies
>>> to the parent clearview-uv gate, right? i.e., your comment is that
>>> i_dladm_set_linkprop should only be invoked from dladm_set_linkprop
>>> if DLADM_OPT_ACTIVE is set, correct?
>> Not quite; My comment as it relates to this specific piece of code is that
>> i_dladm_set_linkprop() should not be called if DLADM_OPT_ACTIVE isn't set.
>> The higher-level issue with the entire libdladm API is that the active
>> configuration should be modified if and only if DLADM_OPT_ACTIVE is set,
>> and the persistent configuration should be modified if and only if
>> DLADM_OPT_PERSIST is set. The API is inconsistent, as there are some cases
>> where the active configuration is unconditionally modified regardless of
>> the flags (as is the case here).
>>
>> You're right that the UV gate has this problem; and this inconsistency
>> wasn't introduced by Brussels. Feel free to defer this to Clearview and
>> I'll see if we can fix this in the UV gate.
>
> Ok, I may file a CR for this, so that one of the two projects can fix this.
That's fine with me.
>
> < discussion about passing linkid instead of link_name in
> dld_ioc_prop_val_t>
>
>>>> Such a change would ripple up to the ioctl handling in dld, obviously.
>>> Defer: this will imply some changes to the ARC'ed interfaces, and is a
>>> little complex to define, without the supporting UV defintions.
>>> We'll do this part as part of the Property Persistence component of
>>> Brussels.
>> I'm not sure I see the relationship with persistence. Do you mean that
>> you'd rather not change the interfaces because they're currently defined in
>> a way that is independent of the putback order of UV/Brussels?
>
> Yes, I'm referring to independance of putback order... you're right,
> it's not directly related to persistence, but since we expect the persistence
> case to go in soon after cv-uv, we are proposing that this comment
> be deferred to that point. Does that sound reasonable?
Okay.
>
>>>> * 3000: The mac_info_t must be comprised of immutable values (see the
>>>> comment in mac.h.) The idea is that mac_info_t is copiedThe min and
>>>> max sdu need to be moved out of mac_info_t, and directly into
>>>> mac_impl_t.
>>> This is actually something that I imported from clearview (which I just
>>> noticed has been updated).
>> I'm not sure what you mean. The mac_info_t in the clearview gate has not
>> contained the min and max sdu since the MAC-level MTU interfaces have
>> existed in there.
>>
>>> BTW, we had discussed this earlier, and I
>>> believe we concurred that we'd need a fast-track for this interface-
>>> can the clearview-iptun help me with that part by fast-tracking this
>>> into Nevada?
>> Which interfaces are you proposing be deferred?
>
> I'm referring to mac_maxsdu_update() and the changes that go with it.
>
> However, your specific comment was about the location of mi_sdu_max
> and mi_sdu_min fields themselves. In onnv these are in the mac_info_t,
> whereas you correctly point out that they belong in mac_impl_t.
> When I tried cscoping this out, I found several references to
> the mi_sdu_{min,max} in aggr/xnbo/vnic files (in addition to the expected
> ones in dld/mac modules). I could fix all of these with my putback
> or (since this is something that cv-iptun is planning to ARC this) we could
> fast-track this piece and put it back to nevada so that I could
> import it. Either method is fine with me- please let me know what you
> prefer.
I planned on having this be part of Clearview iptun anyway, so I don't
have a problem with doing this as part of that case. My main concern at
this point, however, is making sure that the Brussels interfaces make
sense. Without a mac_maxsdu_update(), is the Brussels MTU property at
all useful?
-Seb
