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). 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. 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). > 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. > 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. > 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. > 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. - The em_config() routine shouldn't be adding to the hostlist. - There was both a list in the lport of all the EMs as well as pointers from lport and softc to their EMs. - The EM and lport were still one to one (lport pointer in EM). - exch_mgr_reset() was broken with multiple lports per EM. - Patch descriptions were hard to follow. - 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. _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
