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.
+/**
+ * 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);
+}
+
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 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.
> 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.
> > 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.
> > 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.
+ /* 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.
> > 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.
> - 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.
> - 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.
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
