Christopher Leech wrote:
> Vasu Dev wrote:
>> This will ensure exch will get freed if exch ref dropped to zero
>> in fc_exch_mgr_delete_ep().
>
> That's ... interesting. The count will never be different at the end of
> fc_exch_mgr_delete_ep() than it is at the beginning. The hold and
> release you add here are working around the fact that
> fc_exch_done_locked() decrements the count without checking to see if it
> became zero.
>
> I was looking at the same problem yesterday, and I think a better
> solution is to never decrement the count without testing, and to protect
> against it going to 0 in the middle of fc_exch_done_locked() by adding a
> hold and release in fc_exch_done().
>
> A hold and release in fc_exch_done() is essentially the same fix as what
> you put in fc_exch_mgr_delete_ep(), but it makes a little more sense to
> me while reading the code. Plus, it makes it safe to do a real release
> in fc_exch_done_locked() instead of just the atomic_dec(). I'll send
> out my patch.
>
> Also I tried converting direct use of an atomic_t for the refcount to a
> kref. Looks good to me so far, but I'm not sure the code is correct
> everywhere.
krefs could work, but it bothers me that the fast paths of kref_get
and kref_put aren't inlined. I see that with the WARN_ONs they do,
it might not be such a good idea to inline them. It is nice that they
check for underflow and overflow.
With FCP in software, these are in a critical path. Although I'll admit
the performance difference isn't much. It might make code smaller.
I'd vote for leaving the hold/release functions in.
Joe
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
