Mike Christie wrote:
> Dev, Vasu wrote:
>>> -----Original Message-----
>>> From: Mike Christie [mailto:[EMAIL PROTECTED]
>>> Sent: Tuesday, August 26, 2008 6:24 PM
>>> To: Dev, Vasu
>>> Cc: [email protected]
>>> Subject: Re: [Open-FCoE] [PATCH 3/4] libfc: Added exch release
>>> fc_exch_mgr_delete_ep()
>>>
>>> Vasu Dev wrote:
>>>> This will ensure exch will get freed if exch ref dropped to zero
>>>> in fc_exch_mgr_delete_ep().
>>>> struct fc_exch_mgr *mp;
>>>>
>>> If we can call fc_exch_mgr_delete_ep() while the last refcount is
>>> dropped then if the last fc_exch_release is done here before the hold
>>> then it will be freed when we call fc_exch_hold and we could be doing
>>> the hold on freed memory.
>>>
>>> I do not think we should be able to get to this point.
>> Correct, otherwise we are already accessing wrong memory in
>> fc_exch_mgr_delete_ep().
>>
>>> Everyone calling
>>> it should have a valid ref (or if called from a resp handler calling a
>>> exch_done then the caller of the resp handler should have a refcount to
>>> the ep). If not then the ref counting is not right.
>>>
>> The exch ref is mostly correct except in fc_exch_recv_req(), the
>> fc_exch_recv_req() passed new exchage to upper layer without any
>> additional exch hold and release. So without this patch ep was not
>> freed from fc_exch_recv_req() when upper layer called fc_exc_done().
>>
>> I'm okay either way on this patch applied or not but now it is kind
>> of weired that ep is deleted from mp and along with mp->total_exches
>> is also updated, that means now mp->total_exches could be zero
>> while some ep are still around till their final fc_exch_release()
>
>
> Ah, I was thinking of them as two issues like you mentioned below. The
> refcounting is for just the lifetime management of the ep struct memory.
> Then calling exch_done/fc_exch_mgr_delete_ep manages the mp resources
> like the id reuse.
>
>> is called. I guess it may be okay since mp->total_exches is keeping
>> track of only exches in mp. So may be now we need another ep tracking
>> for its memory since ep memory was not freed in fc_exch_recv_req()
>> code paths and it went undetected though my unit test was hitting
>> fc_exch_recv_req() code paths.
>
> Do you mean you were running tests on that code path and then in some
> tests you ended up getting to fc_exch_mgr_free?
>
> I have hit the WARN_ON in fc_exch_mgr_free and errors in mempool_destroy
> or kmem_cache_destroy errors indicating there were still eps around
> somewhere.
>
>
>> I fixed exch ref in fc_exch_recv_req() path instead depending on this
>> patch in attached fixed-exch-rel.patch and similarly we always need to
>> ensure correct exch ref in other code paths without this patch. (no
>> intentional new line in this para, to test my ongoing mail server issue
>> causing paragraph appearing as long line without explicit new line)
>
> Path looked ok.
I just had one question on that code path.
fr_seq(fp) = NULL;
If we set fr_seq to null here
reject = fc_seq_lookup_recip(mp, fp);
if (reject == FC_RJT_NONE) {
sp = fr_seq(fp); /* sequence will be held */
Then here sp is NULL, right?
ep = fc_seq_exch(sp);
So if we do a container_of on a NULL pointer what do we get? Is the ep
that is returned valid? Is ep->resp ever set in the bottom of the function?
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
