Robert Love 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.
>>
>> > I
>>> guess both need to be considered. I think this may have been something
>>> that Vasu solved, but I removed when I was messing with his patches, not
>>> knowing what it was for. He had the hostlist_add within the em_alloc()
>>> function and had the hostlist_lock held for the entirety of the
>>> em_alloc() routine. I didn't like the em_alloc() function doing the
>>> hostlist_add(), so I removed it, but I think it was there for good
>>> reason.
>>>
>>> I've taken a different approach though. I've pulled out all of the
>>> lock/unlocking from the hostlist_add/lookup/remove functions and moved
>>> it out to the calling functions. This allows me to make the em_alloc()
>>> and hostlist_add() occur under the same lock.
>>>
>>> When destroying a softc we don't need to hold the lock so broadly
>>> though, only over the hostlist_remove() (and not the EM free). This is
>>> because once the softc is removed from the list it will never be found
>>> by another em_alloc(), so we can free the EM without having it
>>> protected. The reference counting within fc_exch_mgr_alloc() should
>>> ensure that an EM in use by another lport/softc won't be freed.
>>>
>>> This might be too confusing without code- I'll post new code soon.
>>>
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> + write_unlock(&fcoe_hostlist_lock);
>>>>> +
>>>>> + fc->oem = fc_exch_mgr_alloc(lp, fc->oem, FC_CLASS_3,
>>>>> + FCOE_MIN_XID, lp->lro_xid);
>>>>> +
>>>>> + if (!fc->oem) {
>>>>> + printk(KERN_ERR "fcoe_em_alloc: failed to "
>>>>> + "allocate em for offload%s\n", fc->real_dev->name);
>>>>> + rc = -ENOMEM;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> +skip_oem:
>>>>> lp->emp = fc_exch_mgr_alloc(lp, NULL, FC_CLASS_3,
>>>>> - FCOE_MIN_XID, FCOE_MAX_XID);
>>>>> - if (!lp->emp)
>>>>> - return -ENOMEM;
>>>>> + FCOE_MIN_XID + lp->lro_xid,
>>>>> + FCOE_MAX_XID);
>>>>> +
>>>>> + if (!lp->emp) {
>>>>> + printk(KERN_ERR "Failed to allocate an Exchange "
>>>>> + "Manager for %s\n", fc->real_dev->name);
>>>>> + rc = -ENOMEM;
>>>>> + }
>>>>> +out:
>>>>> + return rc;
>>>>> +}
>>>>>
>>>>> +/**
>>>>> + * fcoe_em_free() - frees allocated em for a lport
>>>>> + * @lp: the port that em is to allocated for
>>>>> + *
>>>>> + * Returns : 0 on success
>>>>> + */
>>>>> +static inline int fcoe_em_free(struct fc_lport *lp)
>>>>> +{
>>>>> + struct fcoe_softc *fc = lport_priv(lp);
>>>>> +
>>>>> + if (fc->oem)
>>>>> + fc_exch_mgr_free(fc->oem);
>>>>> + fc_exch_mgr_free(lp->emp);
>>>>> return 0;
>>>>> }
>>>>>
>>>>> @@ -468,7 +526,7 @@ static int fcoe_if_destroy(struct net_device *netdev)
>>>>>
>>>>> /* There are no more rports or I/O, free the EM */
>>>>> if (lp->emp)
>>>>> - fc_exch_mgr_free(lp->emp);
>>>>> + fcoe_em_free(lp);
>>>>>
>>>>> /* Free the per-CPU revieve threads */
>>>>> fcoe_percpu_clean(lp);
>>>>> @@ -486,6 +544,59 @@ static int fcoe_if_destroy(struct net_device *netdev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * fcoe_exch_recv() - receive a incoming frame
>>>>> + * @lp: the associated local port
>>>>> + * @fp: received fc_frame
>>>>> + *
>>>>> + * Locates exchange manager instance for received frame
>>>>> + * and pass it up to libfc using fc_exch_recv api.
>>>>> + */
>>>>> +static void fcoe_exch_recv(struct fc_lport *lp, struct fc_frame *fp)
>>>>> +{
>>>>> + struct fcoe_softc *fc = lport_priv(lp);
>>>>> + struct fc_frame_header *fh = fc_frame_header_get(fp);
>>>>> + u16 oxid = ntohs(fh->fh_ox_id);
>>>>> + u32 ctx;
>>>>> +
>>>>> + if (lp->lro_enabled && (oxid <= lp->lro_xid)) {
>>>> Should this be < instead of <= ?
>>>> If I'm correct, the first XID belonging to the non-offload EM is lro_xid.
>>>> BTW, parens are not needed around the comparison.
>>>> Since lro_xid is 0 when lro is disabled, we don't need to check
>>>> lro_enabled.
>>>> Just:
>>>> if (oxid < lp->lro_xid) {
>>>>
>>> Yes, you're right. This is changed.
>>>
>>>>> + ctx = ntoh24(fh->fh_f_ctl) & (FC_FC_EX_CTX | FC_FC_SEQ_CTX);
>>>>> + if (ctx == FC_FC_EX_CTX) {
>>>> Maybe this should allow SEQ_CTX to be 1 or 0, not just 0.
>>>> That would permit class 2 action someday. Unless class 2 is supported,
>>>> it isn't necessary to check SEQ_CTX here. OK if you want to leave it as
>>>> is.
>>>>
>>> I need to look into this more.
>>>
>>>>> + fc_exch_recv(lp, fc->oem, fp);
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> + fc_exch_recv(lp, lp->emp, fp);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * fcoe_exch_get() - allocates an exchange
>>>>> + * @lp: the associated local port
>>>>> + * @fp: the fc_frame to be transmitted
>>>>> + *
>>>>> + * Allocates exchange from fc->oem for offload eligible frames
>>>>> + * otherwise allocates from exhcange from the default EM instance.
>>>> typo: exchange
>>>>
>>>>> + */
>>>>> +static struct fc_exch *fcoe_exch_get(struct fc_lport *lp, struct
>>>>> fc_frame *fp)
>>>>> +{
>>>>> + struct fcoe_softc *fc = lport_priv(lp);
>>>>> + struct fc_exch *ep = NULL;
>>>> Don't initialize ep here. It won't be needed if we
>>>> rearrange to do ddp setup in the read case only.
>>>>
>>> This has to be initialized to avoid a compile WARNING. (see next
>>> comment)
>> A valid warning, too. I hoped to avoid the need to initialize
>> by recoding the rest.
>>
>>> drivers/scsi/fcoe/fcoe.c: In function ‘fcoe_exch_get’:
>>> drivers/scsi/fcoe/fcoe.c:597: warning: ‘ep’ may be used uninitialized in
>>> this function
>>>
>>>>> +
>>>>> + if (fc_fcp_is_read(fr_fsp(fp)))
>>>> {
>>>>> + ep = fc_exch_alloc(fc->oem);
>>>>> +
>>>>> + /*
>>>>> + * if valid ep allocated from fc->ofemp
>>>>> + * then set up ddp here, else allocate
>>>>> + * ep from other em.
>>>>> + */
>>>>> + if (ep)
>>>> This should be part of the above if-statement.
>>>> Test this only in the case of a read.
>>>>
>>> I've re-worked this to be the following, but I still need to do the
>>> fc_exch_alloc() outside of the if{} because the exch_alloc() on the OEM
>>> may fail. I could add a return within the if{} if there is a exchange
>>> allocated by the OEM, I don't know which is preferable. I think I'd
>>> prefer less return statements, but I don't have a technical reason.
>>>
>>>
>>> static struct fc_exch *fcoe_exch_get(struct fc_lport *lp, struct
>>> fc_frame *fp)
>>> {
>>> struct fcoe_softc *fc = lport_priv(lp);
>>> struct fc_exch *ep;
>> But in this version, you need to pre-set ep to NULL.
>>
>>> if (fc_fcp_is_read(fr_fsp(fp))) {
>>> ep = fc_exch_alloc(fc->oem);
>>>
>>> /*
>>>
>>> * if valid ep allocated from
>>> fc->ofemp
>>> * then set up ddp here, else
>>> allocate
>>> * ep from other
>>> em.
>>> */
>>> if (ep)
>>> fc_fcp_ddp_setup(fr_fsp(fp), ep->xid);
>>> }
>>>
>>> if (!ep)
>>> ep = fc_exch_alloc(lp->emp);
>> This is an extra check in the non-offloaded case, but
>> maybe the compiler optimizes around it.
>>
>>> return ep;
>>> }
>>
>> How about
>> if (fc_fcp_is_read(fr_fsp(fp))) &&
>> (ep = fc_exch_alloc(fc->oem)) != NULL)
>> fc_fcp_ddp_setup(fr_fsp(fp), ep->xid);
>> else
>> ep = fc_exch_alloc(lp->emp);
>> return ep;
>>
>> In this version, ep doesn't need to be pre-initialized.
>>
>
> static struct fc_exch *fcoe_exch_get(struct fc_lport *lp, struct
> fc_frame *fp)
> {
> struct fcoe_softc *fc = lport_priv(lp);
> struct fc_exch *ep;
>
> if (fc_fcp_is_read(fr_fsp(fp)) &&
> (ep = fc_exch_alloc(fc->oem)) != NULL)
> fc_fcp_ddp_setup(fr_fsp(fp), ep->xid);
> else
> ep = fc_exch_alloc(lp->emp);
>
> return ep;
> }
>
> ERROR: do not use assignment in if condition
> #184: FILE: drivers/scsi/fcoe/fcoe.c:585:
> + if (fc_fcp_is_read(fr_fsp(fp)) &&
>
> total: 1 errors, 0 warnings, 286 lines checked
>
> I guess we're not supposed to do this... :|
>
>
I thought it would be OK with the parens, but it has gotten stricter.
In that case I would do this to get the same code path:
static struct fc_exch *fcoe_exch_get(struct fc_lport *lp, struct
fc_frame *fp)
{
struct fcoe_softc *fc = lport_priv(lp);
struct fc_exch *ep;
if (fc_fcp_is_read(fr_fsp(fp)) {
ep = fc_exch_alloc(fc->oem);
if (ep)
fc_fcp_ddp_setup(fr_fsp(fp), ep->xid);
else
ep = fc_exch_alloc(lp->emp);
} else
ep = fc_exch_alloc(lp->emp);
return ep;
}
I guess your original version was OK, too. The compiler can know
when ep is still as initialized and skip any extra check for NULL.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
