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? 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)
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;
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);
return ep;
}
> > + 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
