On Tue, 2009-06-02 at 10:18 -0700, Joe Eykholt wrote:
> Vasu Dev wrote:
> > Adds pointer lp->oemp to lport to allow multiple lport to share
> > offload em instance using this pointer.
> >
> > Modifies fc_exch_mgr_reset to reset both lp->emp and lp->oemp.
> >
> > I used p suffix in naming to have new names aligned with existing
> > names e.g. lp->emp, lp, mp etc.
> >
> > Adds APIs fc_exch_mgr_inc and fc_exch_mgr_dec to update added
> > em usage counter em_user.
> >
> > The fc_exch_mgr_free decrements em_user counter and frees em when
> > em_user counter reaches to zero.
> >
> > New em allocation increments em_user counter and this will be
> > incremented for shared em instance use across lports on a eth
> > a device in later patches of this series.
> >
> > Signed-off-by: Vasu Dev <[email protected]>
> > ---
> >
> > drivers/scsi/fcoe/fcoe.c | 1 +
> > drivers/scsi/libfc/fc_exch.c | 45
> > +++++++++++++++++++++++++++++++++++-------
> > include/scsi/libfc.h | 12 +++++++++++
> > 3 files changed, 50 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index 8870948..ab238fb 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -427,6 +427,7 @@ static inline int fcoe_em_config(struct fc_lport *lp)
> > {
> > BUG_ON(lp->emp);
> >
> > + lp->oemp = NULL;
> > lp->emp = fc_exch_mgr_alloc(lp, FC_CLASS_3,
> > FCOE_MIN_XID, FCOE_MAX_XID);
> > if (!lp->emp)
> > diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
> > index bbabd2c..bd6836b 100644
> > --- a/drivers/scsi/libfc/fc_exch.c
> > +++ b/drivers/scsi/libfc/fc_exch.c
> > @@ -57,6 +57,7 @@ struct fc_exch_mgr {
> > enum fc_class class; /* default class for sequences */
> > spinlock_t em_lock; /* exchange manager lock,
> > must be taken before ex_lock */
> > + u16 em_user; /* exchange manager user count */
> > u16 last_xid; /* last allocated exchange ID */
> > u16 min_xid; /* min exchange ID */
> > u16 max_xid; /* max exchange ID */
> > @@ -1404,21 +1405,17 @@ static void fc_exch_reset(struct fc_exch *ep)
> > resp(sp, ERR_PTR(-FC_EX_CLOSED), arg);
> > }
> >
> > -/*
> > - * Reset an exchange manager, releasing all sequences and exchanges.
> > - * If sid is non-zero, reset only exchanges we source from that FID.
> > - * If did is non-zero, reset only exchanges destined to that FID.
> > - */
> > -void fc_exch_mgr_reset(struct fc_lport *lp, u32 sid, u32 did)
> > +static void fc_exch_em_reset(struct fc_lport *lp, struct fc_exch_mgr *mp,
> > + u32 sid, u32 did)
> > {
> > struct fc_exch *ep;
> > struct fc_exch *next;
> > - struct fc_exch_mgr *mp = lp->emp;
> >
> > spin_lock_bh(&mp->em_lock);
> > restart:
> > list_for_each_entry_safe(ep, next, &mp->ex_list, ex_list) {
> > - if ((sid == 0 || sid == ep->sid) &&
> > + if ((lp == ep->lp) &&
> > + (sid == 0 || sid == ep->sid) &&
> > (did == 0 || did == ep->did)) {
>
>
> BTW, I was also thinking that passing both sid and did to the
> fc_exch_mgr_reset should be eliminated. We only need to pass one
> fc_id, and the local port fc_id is implied. We reset all
> exchanges that match the local port and the specified fc_id (or, if
> zero is specified, any at all). I can submit a separate patch for
> that later.
>
Make sense to improve this API by having one less arg but how are we
going to reset all exchanges when a rport going away since currently in
that case we call this API twice to reset all exchanges for a going away
rport.
lport->tt.exch_mgr_reset(lport, 0, port_id);
lport->tt.exch_mgr_reset(lport, port_id, 0);
As I understand this could be fixed by fixing fc_exch_em_reset() to
check sid and did along with exchange esb_state checking and then call
this API only once with lport and port_id. This might require moving
exch_lock before making check for exch selection in fc_exch_em_reset to
check exch context in esb_stat.
This would change API and will also require changes to fnic also, so
this can be done in another patch as you said above.
> > fc_exch_hold(ep);
> > spin_unlock_bh(&mp->em_lock);
> > @@ -1437,6 +1434,18 @@ restart:
> > }
> > spin_unlock_bh(&mp->em_lock);
> > }
> > +
> > +/*
> > + * Reset exchange managers, releasing all sequences and exchanges.
> > + * If sid is non-zero, reset only exchanges we source from that FID.
> > + * If did is non-zero, reset only exchanges destined to that FID.
> > + */
> > +void fc_exch_mgr_reset(struct fc_lport *lp, u32 sid, u32 did)
> > +{
> > + fc_exch_em_reset(lp, lp->emp, sid, did);
> > + if (lp->oemp)
> > + fc_exch_em_reset(lp, lp->oemp, sid, did);
> > +}
> > EXPORT_SYMBOL(fc_exch_mgr_reset);
>
> This is OK, but another way to do this is to have the fcoe LLD implement
> the transport pointer for fc_exch_mgr_reset() and call fc_exch_mgr_reset()
> for both EMs.
>
> That way, other LLDs that don't offload, or offload differently,
> wouldn't be exposed to oemp. Oemp could just be in fcoe_softc, and not
> in the local port.
>
Make sense to redo this another way suggested above for reasons
mentioned by you above, I started with em_list in my initial post with
intent to have just one common reset for shared oemp or non-shared
lp->emp but later realized that won't work for shared oemp as oemp
cannot be put on two lists with one list_head, so we can't have common
reset code that way for oemp. So I'll move above oemp reset code to
fcoe.ko.
> Also, since the fcoe driver knows how many users have the same oemp, you
> don't need a refcount at all (although that's not so bad). The refcount
> could be entirely managed in fcoe, eliminating the incr/decr interfaces.
>
I think having reference count in libfc fc_exch_mgr will be more helpful
for fcoe.ko to get hold of same oemp across fcoe_softc and free the same
EM by reference count tracking, otherwise fcoe.ko has to allocate
another shared struct around oemp to easily keep track of fc_exch_mgr
reference count.
I'll do that using kref in fc_exch_mgr as you suggested in other
response.
However to avoid more APIs to just hold or release EM, I could move
fc_exch_mgr to libfc.h and then have fcoe.ko directly update reference
count.
Thanks for review comments and glad to see someone on the list reviewing
patches carefully.
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
