Joe Eykholt wrote: > Robert Love wrote: >> On Tue, 2010-05-18 at 10:32 -0700, Joe Eykholt wrote: >>> Joe Eykholt wrote: >>>> Robert Love wrote: >>>>> On Fri, 2010-05-07 at 16:26 -0700, Joe Eykholt wrote: >>>>>> The following series implements the proposed-to-become-standard >>>>>> FIP point-to-multipoint support. >>>>>> >>>>>> The T11 FC-BB-6 committee is proposing a new FIP usage model called >>>>>> VN_port to VN_port mode. It allows VN_ports to discover each other >>>>>> over a loss-free L2 Ethernet without any FCF or Fibre-channel fabric >>>>>> services. This is point-to-multipoint. There is also a mode >>>>>> of this called point-to-point (not implemented here) which provides >>>>>> for making sure there is just one pair of ports operating over >>>>>> the Ethernet fabric. >>>>>> >>>>>> The point-to-multipoint mode will work over direct links and >>>>>> eventually, DCB switches. Cisco switches require turning off >>>>>> the FCoE feature to allow this, however. One shouldn't take >>>>>> this as an indication that switches will or will not more fully >>>>>> support VN2VN mode. >>>>>> >>>>>> See the spec at http://www.t11.org/ftp/t11/pub/fc/bb-6/10-019v2.pdf >>>>>> for more details. That's close, but I know there are updates coming. >>>>>> >>>>>> This part of the BB-6 proposals may become more solid after the >>>>>> next T11 meeting in June or the one after that. The BB-6 standards >>>>>> as a whole may not be ratified until April 2011 or so. I'd be >>>>>> interested to hear about whether these are useful enough >>>>>> to integrate before the standard is completely done. >>>>>> >>>>>> Some of these patches are obviously applicable without the VN2VN >>>>>> changes. >>>>>> >>>>> Hey Joe, >>>>> >>>>> Can you help me understand where the fip->ctlr_src_addr gets >>>>> updated >>>>> after we have claimed a FCID? As far as I can tell we're setting it in >>>>> fcoe_interface_setup and fcoe_interface_setup, but not anywhere around >>>>> the time we call update_mac in the fcoe_ctlr_vn_timeout work thread. >>>> ctl_src_addr never gets updated, it's always the native MAC address >>>> of the >>>> interface. When we get an FC_ID in VN2VN mode, we use update_mac to >>>> give >>>> the address to fcoe, which it calls data_src_addr. >>>> >>>>> We're using the fip->ctlr_src_addr to send the CLAIM after >>>>> update_mac, >>>>> but I'm not sure it's being updated. >>>> That's correct. The Claim comes from ctl_src_addr. The contents >>>> show the >>>> FC_ID and the MAC we're claiming, not the Ethernet header. >>> To add to this explanation, the MAC descriptor in the claim has the >>> control MAC (what the spec calls ENode MAC), and the Vx_Port_ID >>> descriptor >>> has the FPMA data mac as inserted by this code: >>> >>> frame->vn.fd_desc.fip_dtype = FIP_DT_VN_ID; >>> frame->vn.fd_desc.fip_dlen = sizeof(frame->vn) / FIP_BPW; >>> hton24(frame->vn.fd_mac, FIP_VN_FC_MAP); >>> hton24(frame->vn.fd_mac + 3, fip->port_id); >>> hton24(frame->vn.fd_fc_id, fip->port_id); >>> >>> I'll work on updating the patches to apply on top of the other recent >>> patches. >>> >> >> Thanks for the explanation Joe. I've reviewed all of these patches and >> they look pretty good. Some can probably be consolidated, but I >> appreciate you separating them for review. >> >> I have two quips. >> >> 1) I'm a bit reluctant to have a 4th rport object (the fcoe_vn_port). >> Would it make sense to add some private space at the end of the >> fc_rport_priv so that the allocations aren't separate between libfc and >> libfcoe's rports? >> >> Is there ever a time when you'd have a VN2VN rport, but not a libfc >> rport? > > I was also reluctant to add another rport struct, but it seemed necessary. > Even if we combine the allocations, we need the separate structure. > There is a short time where we track the vn_port after receiving a claim > notification from it until we receive a beacon from it where we don't > create the rdata, but we could create the rdata and just not log it in > to eliminate that gap. > > There's a locking/lookup issue that makes it better to allocate and > look it up separately, although it may be possible to change fc_rport_priv > to behave appropriately. The issue is that when sending an ELS that needs > to be FIP encapsulated, we need to lookup the vnport to find the correct > destination MAC address. The ELS send is often called while holding the > lport and rport locks, and sometimes the disc_mutex as well, so the > FIP code can't lock the rport list. I made the vnport lookup use > an RCU read lock to handle that. > > If we make the fc_rport_lookup() callers hold either the READ read lock > or the disc_mutex, that would be OK. With just the RCU read lock, > they couldn't do a mutex_enter on the rport, but could do a > kref_get(), then drop the RCU lock and then do the mutex_enter(), > but there's no need to change existing callers if they just continue > to hold the disc_mutex, which prevents changes to the list. > It makes some of the rport code a little more tricky. > > Overall I don't think there's much to save by changing the rport > lookup to use RCU and have extra space for the vnport structure. > It would save some space, though that wouldn't matter until we > get to big VN2VN configurations. None of this is in the fast path, > and it doesn't save much code. What it saves in FIP, it adds to libfc. > > I'm willing to do it, though, if you and others think it's a good idea.
I haven't heard anything more about this but I was thinking about going ahead and changing rdata to provide some space for libfcoe based on a size in the lport template, use to RCU lookups. The extra size would be set in the template only when running in FIP VN2VN mode. This won't take too much work and will simplify things a bit in libfcoe at least. Another issue is the patch for prandom32. It is headed upstream, I think, but it isn't in Linus's tree yet, so I'm not sure what's going on there. I'll change use random32() for now, even though it won't be repeatable across boots as required by FIP, and change to prandom32() once that patch has made it all the way down to fcoe-next.git. >> 2) I think the fcoe_vn_port name might lead to some confusion. It sounds >> like it's for NPIV. Also, it doesn't indicate that it's associated with >> rports, i.e. that it's a "remote" entity. What about something like >> fcoe_vn_rport or fcoe_vnvn_rport? (fcoe_vnvn_rport doesn't match your >> naming convention though) > > The specs call all FCoE N_ports VN_Ports. I don't like the term much > either. How about just fcoe_rport and the local variable frport. Sound > OK? > This is less specific which might be good if we combine the rport info > later or use it for another type of remote port. > I'm also OK with fcoe_vn_rport if you like that better, but would leave > the local variable named vnport in that case. > >> BTW: I'm fine with splitting out the libfcoe.c file into multiple files >> if that helps. > > Thanks. I think I'll leave it as one piece for now. It's still a bit > smaller than fcoe.c. > >> Thanks, //Rob > > Regards, > Joe > > _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
