On (11/26/07 16:33), Sebastien Roy wrote: > > Sowmini.Varadhan at Sun.COM wrote: >> dladm.c: >>> * 4517: What are the error semantics of DLADM_STATUS_BUSY? It looks >>> like if DLADM_STATUS_BUSY is returned, we silently don't do anything >>> and pretend that the command succeeded. >> linkprop.c >>> * 595: If you got DLADM_STATUS_BUSY at 584, the caller will never know, >>> and you might return DLADM_STATU_OK, thus pretending that the property >>> was successfully set when it wasn't. Honestly, I don't understand why >>> DLADM_STATUS_BUSY isn't a regular error like any other error. >> Accept (both of above are related). Regarding the semantics of >> DLADM_STATUS_BUSY: >> The intention is that if the driver cannot set the property because it >> is busy, then it will return the DLADM_STATUS_BUSY error code to the >> caller, but the setting is still stored in the dladm persistent >> repository (to be attempted at the next driver attach, via the >> daemon). The processing of DLADM_STATUS_BUSY is more integral to the >> persistence component, so it will be treated as any other error for the >> framework. > > I still don't understand what this will look like from the perspective of > the person typing "dladm". Will an error now get printed stating that the > property wasn't set? If so, how does the administrator know that the > property will somehow get set later via some other mechanism, and how do > they know when that will happen? Why wouldn't failing the entire operation > be better?
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) >>> 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. >>> * 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. < 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? >>> * 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. --Sowmini > >> When I tried updating as you suggest, >> I found this change impacts several files that access mi_sdu_max >> such as xnbo.c etc. so I'd like to defer this update for cv-iptun. > > If Brussels is proposing to have a dynamic MTU property, then my preference > would be that the stack needs to be able to handle it with the Brussels > putback. That said, I don't think I fully understand which interfaces you > propose be left in Brussels, and which be deferred until Clearview iptun. > Can you clarify? >
