Vasu Dev wrote:
> On Thu, 2009-05-28 at 10:56 -0700, Love, Robert W wrote:
>> Vasu Dev wrote:
>>> 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
>>
>> This is implemented in the patch set I sent you before I went on
>> vacation (not on list as there were still items that needed to be
>> addressed).
>
> This is not implemented as discussed in above link to use this in
> fc_exch_mgr_reset to reset only exches corresponding to a lport,
> instead you moved more functionality into fcoe.ko with added
> fcoe_em_reset in most recent attached patch given to me, pasting this
> func below in case attachment doesn't go through mail list.
>
Right, I think the generic ordered list (with match function) is the
correct aproach, but I hadn't implemented it yet.
> +/**
> + * fcoe_em_reset() - resets allocated exchanges for an lport
> + * @lport: The lport whose exchanges should be freed
> + * @sid: The source ID of an exchange to be freed
> + * @did: The destination ID of an exchange to be freed
> + *
> + * This function should only be called when there is an OEM.
> + * If there is no OEM then the libfc function will be used.
> + */
> +void fcoe_em_reset(struct fc_lport *lport, u32 sid, u32 did)
> +{
> + struct fcoe_softc *fc = lport_priv(lport);
> + u16 xid = FC_XID_UNKNOWN;
> + if (sid != FC_XID_UNKNOWN)
> + xid = sid;
> + else if (did != FC_XID_UNKNOWN)
> + xid = did;
> + if (xid == FC_XID_UNKNOWN) {
> + lport->tt.exch_mgr_reset(fc->oem, lport, sid, did);
> + lport->tt.exch_mgr_reset(lport->emp, lport, sid, did);
> + } else if (fc->oem && !fc_exch_mgr_owns_xid(fc->oem, xid))
> + lport->tt.exch_mgr_reset(fc->oem, lport, sid, did);
> + else
> + lport->tt.exch_mgr_reset(lport->emp, lport, sid, did);
> +}
> +
>
This function is a mess as the covermail to the series I sent you states.
The biggest problem is that it is confusing XIDs and FC-IDs. But yes,
I removed the EM list from the lport so I had to call down to fcoe since
libfc no longer knew about the OEM.
> Instead having single fc_exch_mgr_reset in libfc would be more clean
> taking care of resetting all EMs of a lport. Your latest approach with
> added fcoe_em_reset required so many additional checks in this func
> beside more APIs from libfc as you mentioned below,
> exch_mgr_reset(lport, em) and lport_exch_mgr_reset(lport).
>
This boils down to if the lport has a list of EMs or not. If it does
have a list then we should be using that list when allocating exchanges.
This is what I was talking about in my previous reply with the "match"
routine.
If you don't do that, then you need to have fcoe override the
lport_exch_mgr_reset() routine (with fcoe_em_reset) because otherwise
libfc doesn't know about the OEM.
I agree with you that the fcoe_em_reset() isn't the best aproach which is
why I'm asking for the generic EM list with the "match" routine. What I
don't like is having a half-and-half solution. The orignal series was
using the list for reset, but not for allocation.
This must be very hard for others to follow since we're talking about
internal patches. :)
> This is easy to fix and it is almost taken care based on my initial
> series post, the hardest part for me in resuming work from most recent
> series that you moved all code from my initial post to this 4/4
> followed by three new patches from you doing prep-work per CPU XID
> work. As I said I'll get to per CPU XID once shared OEM work done,
> therefore resuming work from initial post worked best for me, anyway
> as you said below you don't care how I get this done, so we are good
> to go. I'm almost done with changes working for RFC after some more
> testing and updating patch description of initial post.
>
>> Your original patch set did not decouple the 1 to 1 pairing of the EM
>> and lport enough, so my revised patch set does that. Specifically,
>> the code after your patch set was still using the lport pointer from
>> the lport emp (hate the 'p' on the end) when assigning an lport to
>> an exchange.
>>
>
>> This is a problem because when two lports share the EM the second
>> lport will overwrite this em->lport pointer which causes every
>> exchange allocated from that EM to be associated with the second
>> lport. This is a
>> big problem when handling reset since we want to check the lport
>> pointer
>> of the exchange to ensure we don't reset exchanges from other lports.
>>
>
> Yes this was the feedback on my initial OEM series post, complete fix
> needed just lp check in fc_exch_mgr_reset after above described not
> using lp pointer in emp to tear down 1o1 em and lp pairing, your
> changes for de-pairing were fine but reset became complicated in
> latest patch set as I described above.
>
How is reset complicated? It may not have been optimal, but it's a much
cleaner design than having a list for reset and exch_get() overloaded
by fcoe.
Anyway, please implement the list method since that seems to be both
of our preferences.
>> To have an lport pointer per exchange we now need to pass the lport
>> around every time we allocate an exchange. It also required me to
>> split the EM
>> reset routines, I now have-
>>
>> exch_mgr_reset(lport, em)
>> - resets all exchanges on em associated with lport
>>
>> lport_exch_mgr_reset(lport)
>> - resets all exchanges on all EMs associated with lport
>>
>> Both are members of the libfc template so I had fcoe overload
>> lport_exch_mgr_reset() when lro_enabled was set (just like
>> exch_get()).
>> This allowed fcoe to call exch_mgr_reset() on both the EM and OEM and
>> solved the problem.
>>
>> I still don't like the bouncing between libfc and fcoe though. It
>> seems
>> that we might want to have an ordered list of EMs per lport. We
>> could have
>> a small match() function pointer in the EM that is used to determine
>> if the EM should be used or not.
>>
>> For LRO we would put the OEM as list entry 0 with a match function
>> that only returns true for read requests and the default EM as entry
>> 1 with a match function that always returns true. When allocating
>> exchanges we'd walk the list and call match to see if we're supposed
>> to use that EM or move on.
>> The default EM would always be the last element in the list and
>> should catch all exchanges not picked up by a custom EM (i.e. OEM).
>>
>> I hadn't implemented this yet, but it is the cleanest solution I can
>> think
>> of as it just lets fcoe allocate the OEM but then all other
>> interactions
>> with this OEM are just in libfc (with the exception of the match
>> function which would be exported from fcoe).
>>
>
> This would be like generic exch_get inside libfc, sounds good to me
> but I think in current implementation this would likely change EM
> call flow and APIs substantially, let us discuss this more as we get
> to multiple EMs and per CPU XID allocation in fcoe.ko to reduce
> locking cost.
>
No, I will require this for acceptance otherwise we don't have a clean
design. This doesn't have anything to do with CPU XID allocation.
>>> 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(-).
>>
>> The number of lines changed alone isn't a strong reason that a patch
>> isn't Reviewable. If a patch is doing too many things then that
>> makes it hard to review.
>>
>
> I think mentioned large patch did too many things which made it
> difficult to review.
>
It would be more constructive to list the things the patch does instead of
just saying it's too big.
>>> 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.
>>
>> I don't care if it's in this this set or another one. It seems that
>> the later approach just has you submitting a single patch, later. I
>> don't really see why it wouldn't just be the last patch in this set-
>> the work is done. It's up
>> to you.
>
> As far as OEM work goes the latest patch still got issues as I
> mentioned above on exch reset and locking issue still exist which was
> not there in initial post. Your latest patch still does OEM alloc and
> fcoe_hostlist_add separately which is still issue.
>
This was another TODO item that I called out in the covermail I sent you,
it's #10-
10) hostlist_lock in fcoe_if_create() needs to unlock only after we've
added the the new softc to the hostlist. This is becuase we don't want
another VN_Port to be created and not find the origninal N_Ports OEM
in the list.
All you need to do is move the write_unlock() down so it's done after
the hostlist_add().
>
> + /* lport exch manager allocation */
> + write_lock(&fcoe_hostlist_lock);
> + rc = fcoe_em_alloc(lp);
> + write_unlock(&fcoe_hostlist_lock);
> + if (rc) {
> + FC_DBG("Could not configure the EM for the "
> + "interface\n");
> + goto out_lp_destroy;
> + }
> +
> /* add to lports list */
> fcoe_hostlist_add(lp);
>
>
>>
>>> 3. The initial series already took care of issue around offload EM
>>> addition deletion in re-organized this code series.
>>>
>> It accomplished this by breaking offload in the first patch and then
>> fixing it in the last patch. This was my primary motivation for
>> reorganizing the
>> set.
>>
>
> As you agreed below this could have got fixed simply by not removing
> call to offload setup than patch set reorg.
>
I said that might work, I don't know for sure I haven't tried it.
>>> 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
>>
>> Patch count wasn't a problem. The main problems were-
>>
>> - That the set was not bisectable with offload.
>
> Taken care by simply not removing calling offload setup in first
> patch.
>
>> - The em_config() routine shouldn't be adding to the hostlist.
>
> This is required, otherwise your latest patch still got locking issue
> on using OEM across lport.
>
I don't agree with this. The lock should be held before the em_config()
and released after the hostlist_add(), we don't need to tuck hostlist_add()
into em_config(). The name em_config() doesn't imply that it's adding
to the hostlist, so let's not hide it in there.
>> - There was both a list in the lport of all the EMs as well as
>> pointers from lport and softc to their EMs.
>
> That is still the case in latest patch beside now moving more code in
> fcoe.ko to fcoe_em_reset with fc->oem.
>
This is no good because it is not a good design as described above.
>> - The EM and lport were still one to one (lport pointer in EM).
>> - exch_mgr_reset() was broken with multiple lports per EM.
>
> Yes this was issue as got discussed in initial series feedback and as
> I said above partly taken care so I'll collect some of your changes on
> this.
>
>> - Patch descriptions were hard to follow.
>
> I agree, I'll try do better job on this.
>
>> - Some patches needed to be consolidated.
>>
>>> 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.
>>>
>> This may be simpler, I chose a different approach.
>>
>>> 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.
>>>
>> I think the revised series gets us closer to the correct patch set,
>> but
>> as long as you get good code out I don't care which set you start
>> with.
>>
>> Just keep in mind that most of the issues I encountered, I fixed. So,
>> you would likely need to duplicate that work if you were starting
>> where
>> you last left off.
>
> I've already collected discussed fcoe_exch_recv() or fcoe_exch_get()
> improvements on mail list and I'll collect your lp separation code
> from EM which was quite easy to follow and locking issue around OEM
> additional/deletion is already fixed in initial post, so all quick and
> easy to go from initial post. I'll try to get out its RFC soon later
> today so not much duplicate work in going with initial series post for
> OEM work and it was quick.
>
> Per CPU XID is another items you started but I'll get to that once OEM
> done.
>
Sure.
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
