On Tue, 2009-05-19 at 12:38 -0700, Joe Eykholt wrote: > Robert Love wrote: > > On Mon, 2009-05-18 at 15:16 -0700, Joe Eykholt wrote: > >> Robert Love wrote: > >>> From: Vasu Dev <[email protected]> > >>> > >>> Previously when a netdev supported offloaded Rx it was > >>> indicating the range of XIDs for offload to the singular > >>> EM that was associated with the lport on the port. > >>> > >>> Previous patches have made it so an EM can be shared across > >>> multiple lports. This patch adds an offload EM to the fcoe_softc > >>> and allows the fcoe module to override the exch_get() function > >>> poiner in libfc's template so that requests for exchanges are > >>> handled by fcoe. fcoe can then choose from either the default > >>> EM associated with the lport the exchange is on or the offload > >>> EM associated with the lport's netdev. > >>> > >>> To accomplish this the patch does the following: > >>> > >>> - Adds oem (offload EM) pointer to the fcoe_softc > >>> > >>> - Reorganize EM freeing to consider the offload EM > >>> > >>> - Reorganize fcoe_em_alloc() (previously fcoe_em_config()) to > >>> assign the same EMs to any VN_Ports (lports) associated with > >>> the same netdev->phys_dev. > >>> > >>> - Implements fcoe_exch_recv() to handle incoming frames and > >>> determine with EM should handle the inbound exchange. > >>> > >>> - Overrides the exch_get() routine of the libfc template > >>> so that fcoe can use offload EM when offload criteria is met > >>> (i.e. exchange will be for a read request). > >>> > >>> Signed-off-by: Vasu Dev <[email protected]> > >>> Signed-off-by: Robert Love <[email protected]> > >>> --- > >>> > >>> drivers/scsi/fcoe/fcoe.c | 134 > >>> +++++++++++++++++++++++++++++++++++++++--- > >>> drivers/scsi/fcoe/fcoe.h | 1 > >>> drivers/scsi/libfc/fc_exch.c | 92 +++++------------------------ > >>> include/scsi/libfc.h | 5 -- > >>> 4 files changed, 141 insertions(+), 91 deletions(-) > >>> > >>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c > >>> index e4ba631..f589a47 100644 > >>> --- a/drivers/scsi/fcoe/fcoe.c > >>> +++ b/drivers/scsi/fcoe/fcoe.c > >>> @@ -407,20 +407,78 @@ static int fcoe_shost_config(struct fc_lport *lp, > >>> struct Scsi_Host *shost, > >>> } > >>> > >>> /** > >>> - * fcoe_em_config() - allocates em for this lport > >>> + * fcoe_em_alloc() - allocates em for this lport > >>> * @lp: the port that em is to allocated for > >>> * > >>> * Returns : 0 on success > >>> */ > >>> -static inline int fcoe_em_config(struct fc_lport *lp) > >>> +static inline int fcoe_em_alloc(struct fc_lport *lp) > >>> { > >>> + struct fcoe_softc *fc = lport_priv(lp); > >>> + struct fcoe_softc *oldfc = NULL; > >>> + int rc = 0; > >>> + > >>> BUG_ON(lp->emp); > >>> > >>> + /* > >>> + * Check if need to allocate an em instance for > >>> + * offload exchange ids to be shared across all VN_PORTs/lport. > >>> + */ > >>> + if (!lp->lro_enabled || !lp->lro_xid || (lp->lro_xid >= FCOE_MAX_XID)) { > >>> + lp->lro_xid = 0; > >>> + goto skip_oem; > >>> + } > >>> + > >>> + /* > >>> + * Reuse exitsing offload em instance in case > >> typo: existing > >> > >>> + * it is already allocated on phys_dev > >>> + */ > >>> + write_lock(&fcoe_hostlist_lock); > >>> + list_for_each_entry(oldfc, &fcoe_hostlist, list) { > >>> + if (oldfc->phys_dev == fc->phys_dev) { > >>> + fc->oem = oldfc->oem; > >> Take a reference here? > >> > >> What if the other other instance gets deleted, including the EM > >> before we start using it? Maybe do the exch_mgr_alloc inside the lock? > >> > > Thanks for the review Joe! All the spelling mistakes are fixed in my > > local patches. > > > > What is the "other instance" are you referring to the softc or the EM? > > Both. If oldfc goes away, along with its > oem, then the oem will be freed before we take a reference on it. > I'll wait for your new version. > > BTW, I think the exch_mgr_reset functionality still has problems, unless > the OEM isn't in the list. Maybe fcoe.ko should implement exch_mgr_reset > and call the oem and then call the normal exch_mgr_reset function to do > the non-offloaded EMs. >
Joe you pointed this issue before in response to initial this patch series post at http://www.open-fcoe.org/pipermail/devel/2009-May/002329.html. I think earlier suggested lp check in exch_mgr_reset to solve this issue will be better solution since that will be more generic being in libfc than having each libfc user like fcoe.ko implement reset to handle resetting of shared OEM and also it will make libfc fully capable of allowing shared em across lport/VN_PORTs. I'm working on this fix. For this fix, I'm working on originally posted series from me instead fixing this re-organized patches series for these reasons:- 1. Some of the combined patches too big to review like this patch 3/4 of this series, 141 insertions(+), 91 deletions(-). 2. Keep EM exchange ID allocation patch 4/4 separate from my initial series doing only work for dedicate shared EM for offload xids, I want to focus on this first. 3. The initial series already took care of issue around offload EM addition deletion in re-organized this code series. However Rob has done good documentation improvements in this series and I'll try same for my initial post while also collecting applicable all feedback discussed in this series to my initial series. I'm not sure what prompted Rob to re-org initial series post than addressing discussed comments from http://www.open-fcoe.org/pipermail/devel/2009-May/002330.html, I guess it was not due to patch count since initial post had moderate logical breakup for average approx 60 lines code change per patch in 6 patch series. It could be discussed fc_fcp_ddp_setup during last code walk through with Rob, the initial series dropped using fc_fcp_ddp_setup in first patch and then used it in last patch once dedicated OEM setup done but that could be fixed by simply not dropping use of fc_fcp_ddp_setup in first patch of initial patch series. I'll do that in rework so that DDP will be in use for all patches of the series in case DDP functionality needs git-bisect in future. However Rob if you insist to fix from revised series then I'm fine with that as well but for reasons mentioned above I'll prefer to work from initial patch series. Vasu > > I > > guess both need to be considered. I think this may have been something > > that Vasu solved, but I removed when I was messing with his patches, not > > knowing what it was for. He had the hostlist_add within the em_alloc() > > function and had the hostlist_lock held for the entirety of the > > em_alloc() routine. I didn't like the em_alloc() function doing the > > hostlist_add(), so I removed it, but I think it was there for good > > reason. > > > > I've taken a different approach though. I've pulled out all of the > > lock/unlocking from the hostlist_add/lookup/remove functions and moved > > it out to the calling functions. This allows me to make the em_alloc() > > and hostlist_add() occur under the same lock. > > > > When destroying a softc we don't need to hold the lock so broadly > > though, only over the hostlist_remove() (and not the EM free). This is > > because once the softc is removed from the list it will never be found > > by another em_alloc(), so we can free the EM without having it > > protected. The reference counting within fc_exch_mgr_alloc() should > > ensure that an EM in use by another lport/softc won't be freed. > > > > This might be too confusing without code- I'll post new code soon. > > > >>> + break; > >>> + } > >>> + } > >> > >>> + write_unlock(&fcoe_hostlist_lock); > >>> + > >>> + fc->oem = fc_exch_mgr_alloc(lp, fc->oem, FC_CLASS_3, > >>> + FCOE_MIN_XID, lp->lro_xid); > >>> + > >>> + if (!fc->oem) { > >>> + printk(KERN_ERR "fcoe_em_alloc: failed to " > >>> + "allocate em for offload%s\n", fc->real_dev->name); > >>> + rc = -ENOMEM; > >>> + goto out; > >>> + } > >>> + > >>> +skip_oem: > >>> lp->emp = fc_exch_mgr_alloc(lp, NULL, FC_CLASS_3, > >>> - FCOE_MIN_XID, FCOE_MAX_XID); > >>> - if (!lp->emp) > >>> - return -ENOMEM; > >>> + FCOE_MIN_XID + lp->lro_xid, > >>> + FCOE_MAX_XID); > >>> + > >>> + if (!lp->emp) { > >>> + printk(KERN_ERR "Failed to allocate an Exchange " > >>> + "Manager for %s\n", fc->real_dev->name); > >>> + rc = -ENOMEM; > >>> + } > >>> +out: > >>> + return rc; > >>> +} > >>> > >>> +/** > >>> + * fcoe_em_free() - frees allocated em for a lport > >>> + * @lp: the port that em is to allocated for > >>> + * > >>> + * Returns : 0 on success > >>> + */ > >>> +static inline int fcoe_em_free(struct fc_lport *lp) > >>> +{ > >>> + struct fcoe_softc *fc = lport_priv(lp); > >>> + > >>> + if (fc->oem) > >>> + fc_exch_mgr_free(fc->oem); > >>> + fc_exch_mgr_free(lp->emp); > >>> return 0; > >>> } > >>> > >>> @@ -468,7 +526,7 @@ static int fcoe_if_destroy(struct net_device *netdev) > >>> > >>> /* There are no more rports or I/O, free the EM */ > >>> if (lp->emp) > >>> - fc_exch_mgr_free(lp->emp); > >>> + fcoe_em_free(lp); > >>> > >>> /* Free the per-CPU revieve threads */ > >>> fcoe_percpu_clean(lp); > >>> @@ -486,6 +544,59 @@ static int fcoe_if_destroy(struct net_device *netdev) > >>> return 0; > >>> } > >>> > >>> +/** > >>> + * fcoe_exch_recv() - receive a incoming frame > >>> + * @lp: the associated local port > >>> + * @fp: received fc_frame > >>> + * > >>> + * Locates exchange manager instance for received frame > >>> + * and pass it up to libfc using fc_exch_recv api. > >>> + */ > >>> +static void fcoe_exch_recv(struct fc_lport *lp, struct fc_frame *fp) > >>> +{ > >>> + struct fcoe_softc *fc = lport_priv(lp); > >>> + struct fc_frame_header *fh = fc_frame_header_get(fp); > >>> + u16 oxid = ntohs(fh->fh_ox_id); > >>> + u32 ctx; > >>> + > >>> + if (lp->lro_enabled && (oxid <= lp->lro_xid)) { > >> Should this be < instead of <= ? > >> If I'm correct, the first XID belonging to the non-offload EM is lro_xid. > >> BTW, parens are not needed around the comparison. > >> Since lro_xid is 0 when lro is disabled, we don't need to check > >> lro_enabled. > >> Just: > >> if (oxid < lp->lro_xid) { > >> > > Yes, you're right. This is changed. > > > >>> + ctx = ntoh24(fh->fh_f_ctl) & (FC_FC_EX_CTX | FC_FC_SEQ_CTX); > >>> + if (ctx == FC_FC_EX_CTX) { > >> Maybe this should allow SEQ_CTX to be 1 or 0, not just 0. > >> That would permit class 2 action someday. Unless class 2 is supported, > >> it isn't necessary to check SEQ_CTX here. OK if you want to leave it as > >> is. > >> > > I need to look into this more. > > > >>> + fc_exch_recv(lp, fc->oem, fp); > >>> + return; > >>> + } > >>> + } > >>> + fc_exch_recv(lp, lp->emp, fp); > >>> +} > >>> + > >>> +/** > >>> + * fcoe_exch_get() - allocates an exchange > >>> + * @lp: the associated local port > >>> + * @fp: the fc_frame to be transmitted > >>> + * > >>> + * Allocates exchange from fc->oem for offload eligible frames > >>> + * otherwise allocates from exhcange from the default EM instance. > >> typo: exchange > >> > >>> + */ > >>> +static struct fc_exch *fcoe_exch_get(struct fc_lport *lp, struct > >>> fc_frame *fp) > >>> +{ > >>> + struct fcoe_softc *fc = lport_priv(lp); > >>> + struct fc_exch *ep = NULL; > >> Don't initialize ep here. It won't be needed if we > >> rearrange to do ddp setup in the read case only. > >> > > This has to be initialized to avoid a compile WARNING. (see next > > comment) > > A valid warning, too. I hoped to avoid the need to initialize > by recoding the rest. > > > drivers/scsi/fcoe/fcoe.c: In function ‘fcoe_exch_get’: > > drivers/scsi/fcoe/fcoe.c:597: warning: ‘ep’ may be used uninitialized in > > this function > > > >>> + > >>> + if (fc_fcp_is_read(fr_fsp(fp))) > >> { > >>> + ep = fc_exch_alloc(fc->oem); > >>> + > >>> + /* > >>> + * if valid ep allocated from fc->ofemp > >>> + * then set up ddp here, else allocate > >>> + * ep from other em. > >>> + */ > >>> + if (ep) > >> This should be part of the above if-statement. > >> Test this only in the case of a read. > >> > > I've re-worked this to be the following, but I still need to do the > > fc_exch_alloc() outside of the if{} because the exch_alloc() on the OEM > > may fail. I could add a return within the if{} if there is a exchange > > allocated by the OEM, I don't know which is preferable. I think I'd > > prefer less return statements, but I don't have a technical reason. > > > > > > static struct fc_exch *fcoe_exch_get(struct fc_lport *lp, struct > > fc_frame *fp) > > { > > struct fcoe_softc *fc = lport_priv(lp); > > struct fc_exch *ep; > > But in this version, you need to pre-set ep to NULL. > > > if (fc_fcp_is_read(fr_fsp(fp))) { > > ep = fc_exch_alloc(fc->oem); > > > > /* > > > > * if valid ep allocated from > > fc->ofemp > > * then set up ddp here, else > > allocate > > * ep from other > > em. > > */ > > if (ep) > > fc_fcp_ddp_setup(fr_fsp(fp), ep->xid); > > } > > > > if (!ep) > > ep = fc_exch_alloc(lp->emp); > > This is an extra check in the non-offloaded case, but > maybe the compiler optimizes around it. > > > > > return ep; > > } > > > How about > if (fc_fcp_is_read(fr_fsp(fp))) && > (ep = fc_exch_alloc(fc->oem)) != NULL) > fc_fcp_ddp_setup(fr_fsp(fp), ep->xid); > else > ep = fc_exch_alloc(lp->emp); > return ep; > > In this version, ep doesn't need to be pre-initialized. > > > >>> + fc_fcp_ddp_setup(fr_fsp(fp), ep->xid); > >> > >>> + else > >>> + ep = fc_exch_alloc(lp->emp); > >>> + return ep; > >>> +} > >>> + > > > > _______________________________________________ > devel mailing list > [email protected] > http://www.open-fcoe.org/mailman/listinfo/devel _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
