On Wed, Jun 3, 2009 at 7:36 PM, Nicolas Droux <[email protected]> wrote: > Jason, > > Thanks for writing this up. > > I'd suggest having an API which can be leveraged by other types of LLDP TLVs > as well, instead of just DCBX. The APIs could be made more generic by having > clients register for specific type of TLV. The registration function would > allow the client to indicate the type of TLV it is interested in, RX > callback pointer and opaque handle.
Probably not a bad idea.. I can tweak the API a bit to allow for it. > > Some other comments: > > - Use opaque types instead of void * for the client handles It's not a handle to any LLDP data, it's there for the clients to be able to pass data through to the callback when it's invoked. dlpi_walk (among a lot of other functions) do the same thing (or rather I'm not familiar with anything similar using anything other than void *, and am not sure what would make sense). > - Shouldn't TX be defined through an entry point called by client instead of > a callback? I'm not sure how easily that'd work. For each port, a PDU is sent ever tx_interval (default: 30) seconds. Except, if something has changed, then a new PDU is sent out immediately (but no more frequently than tx_delay - default: 1 - second to prevent a flood of PDUs if multiple TLV values are changing). >From what I could get from reading the DCBX docs, if a link on an Opensolaris host was setup as an FCOE target, the OS would send out LLDP PDUs (with the DCBX TLVs) at the normal intervals (i.e. every 30s), once it sees it's neighbor also send a PDU with DCBX TLVs (and there aren't any other neighbors on the same link), it switches to a 'fast start' mode where it sends frames once a second to exchange the needed data, the returns to the normal 30sec interval. If multiple PDUs are sent from the same link with different data, the most recent replaces whatever was there. To me, this suggests it'd be better for LLDP to manage when PDU transmission happens, and just ping anything that wants to send extra TLVs to stick them in the mblk. > - Instead of specifying the link name to the start routine, the registration > routine should be taking a MAC handle or MAC client handle instead, which > would allow you to leverage the existing mac_open_*() variants. One thing I've been struggling with (perhaps you can clarify this) is it looks like the recent changes in the MAC api require that each call to mac_open_*() (obtaining a handle in the process) requires it's own unique MAC address. This is a bit of a problem, since LLDP should be using an existing mac addresses, versus getting one of it's own (this is why I was originally looking at using DLS instead of MAC since the same handle could be reused). Am I correct in that understanding, or did I miss something? > > Nicolas. > > On May 29, 2009, at 10:20 PM, Jason King wrote: > >> Here's what I'm thinking for LLDP hooks for DCBX. Any feedback would >> be appreciated. >> >> I'm assuming that at some point there will be an in-kernel API that >> uses mblk_t that can be used for PDU tx/rx (and thus this will depend >> on such an API appearing). >> >> The idea is that the logic for DCBX will reside separate from LLDP >> itself, with LLDP merely providing hooks to allow DCBX to send/recv >> TLVs for each link as needed. That way LLDP doesn't need to know a >> lot about the FCoE code. >> >> /* >> * Called for each DCBX tlv that is received. >> * data - A DCBX private data structure, set by lldp_dcbx_start(). >> If the remote end is initiating, this may be NULL. >> * m - The message block. b_rptr will point to the start of >> the DCBX TLV data. I.e. the type, len, oui, and oui type fields are >> already consumed. >> * len - The length of the TLV (excluding the oui & oui type >> fields). >> * >> * The callback is expected in all circumstances to advance b_rptr to >> the start of the next TLV (or end of PDU). >> * Return: >> * B_TRUE - TLV was formed correctly. >> * B_FALSE - TLV was malformed. The rx_tlv_discarded counter is >> incremented. Processing continues with the next TLV. >> */ >> typedef boolean_t (*lldp_dcbx_rx_t)(void * data, mblk_t *m, size_t len); >> >> /* >> * Called whenever a link has been enabled for DCBX via >> lldp_dcbx_start() and a PDU is in the process of being constructed for >> TX. >> * Callback is expected to append DCBX TLVs to b_wptr. >> * data - A DCBX private data structure as set by lldp_dcbx_start. >> * m - The mblk_t to append to. >> * Return: >> * B_TRUE - TLVs were added successfully. >> * B_FALSE - insufficient room to add all TLVs. Used to signal no >> additional TLVs (except END TLV) should be appended. b_wptr should >> point >> * to the end of the last sucessfully added TLV. >> */ >> typedef boolean_t (*lldp_dcbx_tx_t)(void *data, mblk_t *m); >> >> /* >> * Not specific to DCBX, but probably useful: >> * >> * Append TLV header of type t_type and length t_len to m. >> * m - The mblk_t to append to >> * t_type - The TLV type. For DCBX, this will be LLDP_T_ORG_SPECIFIC. >> * t_len - The TLV length, for DCBX, this should include the 4 byte >> org specific header. >> * reserve_end - If space for the end TLV should be reserved in the >> TLV. This will always be set to B_TRUE for DCBX. >> * >> * Return: >> * B_TRUE - Header appended and sufficient space remains in mblk_t >> for t_len data (including space for END TLV if reserve_end is set). >> * B_FALSE - Insufficient space remains in the mblk_t, the header was >> not appended. Further additions of TLVs should be aborted. >> */ >> boolean_t lldp_make_tlv_header(mblk_t *m, uint16_t t_type, uint16_t >> t_len, boolean_t reserve_end); >> >> /* Register callbacks to LLDP */ >> void lldp_dcbx_init(lldp_dcbx_tx_t, lldp_dcbx_rx_t); >> >> /* Remove callbacks from LLDP */ >> void lldp_dcbx_fini(void); >> >> /* >> * Enable DCBX on linkname. This causes a number of events to occur: >> * - Fast start is enabled on the interface. >> * - Whenever a PDU is being generated for the link, the registered >> callback will be invoked >> * linkname - The name of the link >> * data - DCBX private data, will be passed to callback >> functions when invoked >> * >> * Return: >> * 0 - Success >> * ENOENT - Link does not exist >> * EINVAL - Link is not enabled for LLDP. >> */ >> int lldp_dcbx_start(const char *linkname, void *data); >> >> /* >> * Disable DCBX on linkname. Callbacks will no longer be invoked >> */ >> void lldp_dcbx_stop(const char *linkname); >> _______________________________________________ >> networking-discuss mailing list >> [email protected] > > -- > Nicolas Droux - Solaris Kernel Networking - Sun Microsystems, Inc. > [email protected] - http://blogs.sun.com/droux > > _______________________________________________ networking-discuss mailing list [email protected]
