Vasu Dev wrote:
> Updating exch ref count based on timer_pending check can mess
> up exch ref count if timer fires just after this check. Instead count
> on del/mod_timer() return values to update exch ref count.
>
The timer parts look good.
> @@ -288,11 +288,21 @@ static void fc_exch_release(struct fc_exch *ep)
>
> if (atomic_dec_and_test(&ep->ex_refcnt)) {
> WARN_ON(!ep->esb_stat & ESB_ST_COMPLETE);
> - del_timer(&ep->ex_timer);
> + WARN_ON(timer_pending(&ep->ex_timer));
> mp = ep->em;
> + spin_lock_bh(&mp->em_lock);
> + /*
> + * ensure ep->ex_refcnt is still zero
> + * if not then skip freeing exchange
> + * since some new incoming frame took
> + * hold of this, which is unlikely.
> + */
> + if (atomic_read(&ep->ex_refcnt)) {
> + spin_unlock_bh(&mp->em_lock);
> + return;
This looks weird, because it extended the locking around a access to a
new ex->ref_count read. What was racing? Is it supposed to handle if we
got a response for an exchange, but then a response for a ABTS is coming
in right after it so the two are being processed on mutliple recv threads?
If so it seems like thread0 could still do what looks like the final
release so refcount is 0. Then on thread1 the abts resp could have
already passed the fc_exch_find call in fc_exch_recv_bls. Then thread0
could grab the lock above and see refcount is zero. But then thread1
could just call fc_exch_hold and increase the count and think everything
is ok but fc_exch_release is calling the kemem cache free function.
> + }
> if (ep->lp->tt.exch_put)
> ep->lp->tt.exch_put(ep->lp, mp, ep->xid);
I know it is not part of your patch, but what is exch_put for? I could
not find where it is set. Is it going to be removed?
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
