Vasu Dev wrote:
> 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.
It could just look for other users like it does at create(), and
don't use refcounts.
> 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.
I forgot fc_exch_mgr() was private, and I think it's nice to keep it that
way. If you want to use the refcounts, let's put them in fc_exch.c only,
and add get/set ops.
> Thanks for review comments and glad to see someone on the list reviewing
> patches carefully.
You're welcome. I have a large set of patches I'll send out soon. Hopefully
someone will look at those, too!
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
