Leech, Christopher wrote:
> Mike Christie wrote:
>> Dev, Vasu wrote:
>>>> -----Original Message-----
>>>> From: Mike Christie [mailto:[EMAIL PROTECTED]
>>>> Sent: Wednesday, August 27, 2008 2:17 PM
>>>> To: Dev, Vasu
>>>> Cc: [email protected]
>>>> Subject: Re: [Open-FCoE] [PATCH 3/4] libfc: Added exch release
>>>> fc_exch_mgr_delete_ep()
>>>>
>>>> Dev, Vasu wrote:
>>>>>> -----Original Message-----
>>>>>> From: Mike Christie [mailto:[EMAIL PROTECTED]
>>>>>>
>>>>>> Ok so now I am really lost :) See the attached patch. It looks
>>>>>> like if fc_seq_lookup_recip returns FC_RJT_NONE then from the
>>>>>> fc_exch_find call it returns from fc_seq_lookup_recip with a hold
>>>>>> on the ep.
>>>>> Correct, the ep returned from fc_seq_lookup_recip() will have one
>>>>> exch hold and that will be used by upper layer as explained below
>>>>> in detail.
>>>>>
>>>>>> So in
>>>>>> fc_exch_recv_req we need to just release it?
>>>>> The exch hold from fc_seq_lookup_recip() will be released by upper
>>>>> layer response handler by calling fc_exch_done() but since
>>>>> fc_exch_done()
>>> will
>>>> But shouldn't there be a hold from when the ep was allocated and
>>>> added to the mp->exches array?
>>> Yes and I was looking at that case only when fc_exch_resp() allocates
>>> new exchange in fc_seq_lookup_recip() and that will be most likely
>>> cod
>> Ah I see. And I was only looking at the find paths :)
>
> Should fc_seq_lookup_recip be split into two functions? It does two
> distinctly different things; Lookup an exchange and return an error if
> it doesn't exist, or create an exchange returing an error if it already
> exists. And callers already need to know which one to expect in order
> to get the reference counting correct.
>
Callers of fc_seq_lookup_recip do not need to know this. I thought with
Vasu's other fix fc_seq_lookup_recip will always return with a hold
(when successful), that the caller must drop.
But splitting it is not a bad idea.
I think a more drastic step that might simplify some things or maybe
make it more complicated or at least make the code prettier is
establishing a locking order for the ex_lock and the mp em_lock. Right
now all this dancing seems due to:
1. On abort completion, we are telling fc_fcp that it can releases its
resrouces, but fc_exch.c might still need the ep to send the rrq.
2. We never hold the ex_lock and em_lock at the same time and only want
to remove the ep from mp->exches if we do not have to do a rrq so we end
up with code like fc_exch_done.
3. I goofed and got overly paranoid and that it was best that when we
call the resp handler (which would call exch_done) we had a hold, so
that if the mgr reset function got run at the same time both would have
proper holds and the resp handler would not suddenly see the ep get
freed from under it because the mgr reset function did the last release.
Vaus figured out that should not happen becuase the upper layer will
always sync up the completion with their own locking (he also has
patches to make sure we do not free the upper layer object while
multiple threads are calling the resp handler).
So instead of the caller doing the extra hold we could do:
done()
{
hold ep
lock ex_lock
if already called done
goto done
if ESB_ST_REC_QUAL
goto done;
lock mp em_lock
do the cleanup currently in fc_exch_mgr_delete_ep
unlock mp em_lock
done:
unlock ex_lock
release ep
return
Note that for fc_exch.c completing the ep then it would still need the
hold from the find/lookup if it did not hold a lock (there would be a
done_locked variant like there is today) to sync up resets and normal
completions.
So actually it might make the code prettier probably because we do need
code like this:
spin_lock_bh(&ep->ex_lock);
rc = fc_exch_done_locked(ep);
spin_unlock_bh(&ep->ex_lock);
if (!rc)
fc_exch_mgr_delete_ep(ep);
But since in many cases we need the hold because fc_exch.c is doing the
completion then it might not buy very much as far as refcounting
simplification goes. I mean we would not special case grabbing a hold or
not based on the upper layer because that would make it ugly again.
Ok so everything after the comment about breaking up the function is
rambling :) I am not sure what would be prettier or easier to understand.
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
