Mike Christie wrote: > Mike Christie wrote: >> 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? >> > > Oh nevermind I guess fc_seq_lookup_recip will always set a sp when > returning reject. But in that case should fc_seq_lookup_recip be doing a > hold from the fc_exch_find, that the caller of fc_seq_lookup_recip > releases? > > Or if fc_exch_mgr_reset ran between the time fc_seq_lookup_recip drops > the ref from the fc_exch_find and when fc_exch_recv_req runs, can
I meant when fc_exch_recv_req returns from calling fc_seq_lookup_recip. > fc_exch_mgr_reset free the ep we are trying to access before the added > holds in your patch? > > _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
