From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============6072347107823440657==" MIME-Version: 1.0 From: Walker, Benjamin Subject: Re: [SPDK] Some thoughts for the code in rdma.c Date: Thu, 20 Dec 2018 17:15:57 +0000 Message-ID: <7e50eaacb6fdd055c5f5ee0d1f13ce92bdc635ed.camel@intel.com> In-Reply-To: FA6C2217B01E9D48A581BB48660210143E0A3963@shsmsx102.ccr.corp.intel.com List-ID: To: spdk@lists.01.org --===============6072347107823440657== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Thu, 2018-12-20 at 02:59 +0000, Yang, Ziye wrote: > Hi all, > = > I would like to discuss the following functions in /lib/nvmf/rdma.c > = > 1 I do not think that the _nvmf_rdma_disconnect_retry function is necessa= ry. > This function is used in nvmf_rdma_disconnect. And there is a description= as > follows: > = > /* Read the group out of the qpair. This is normally set a= nd > accessed only from > * the thread that created the group. Here, we're not on th= at > thread necessarily. > * The data member qpair->group begins it's life as NULL and > then is assigned to > * a pointer and never changes. So fortunately reading this= and > checking for > * non-NULL is thread safe in the x86_64 memory model. */ > = > But for group adding for the qpair, it is related with this > function: spdk_nvmf_poll_group_add, if the group is not ready, we can j= ust > call spdk_nvmf_qpair_disconnect in that function. And the > spdk_nvmf_qpair_disconnect should have the strategy to prevent the second= time > entering if the qpair is not destroyed due to async behavior. There are two threads involved here typically - a management thread that is polling the RDMA CM Event channel and for IBV async events (which are independent things), and the poll group thread that owns the qpair. The RDMA CM Event channel notifies the target of two relevant events for a = qpair - connect and disconnect. The IBV async event mechanism independently notif= ies the target of IBV_EVENT_QP_FATAL conditions. The target's response to both = an RDMA_CM_EVENT_DISCONNECT and to an IBV_EVENT_QP_FATAL is to disconnect the = queue pair, but there are scenarios where we could legitimately get both events f= or the same qpair. The processing of both of these events involves passing a message to the poll group thread, which is an asynchronous process and may = take an arbitrarily long time. The refcnt coordinates between those two events, = such that the first event does not destroy the qpair while there is another even= t in- flight still. The qpair itself, after the initial set up phase, is also managed by the po= ll group thread. If the user destroys a poll group, or manipulates the poll gr= oup to add or remove a different qpair, or the qpair gets a completion entry th= at it begins processing, then the qpair data structure is being modified by the p= oll group thread. You can't disconnect it on the management thread while this is happening or you risk corrupting some of the data structures - in particula= r the poll group's qpair list. Checking that the qpair hasn't been assigned to a poll group yet (by lookin= g for qpair.group set to NULL) isn't a sufficient protection because a message to= add the qpair to the poll group may already be in-flight. I made some suggestio= ns to use the refcnt to deal with this scenario on your most recent patch: https://review.gerrithub.io/#/c/spdk/spdk/+/437232/ > = > 2 If that, the following two functions in rdma.c are also unnecessary: > = > spdk_nvmf_rdma_qpair_inc_refcnt > spdk_nvmf_rdma_qpair_dec_refcnt > = > = > Generally, my idea is that, we should use spdk_nvmf_qpair_disconnect dire= ctly > in every transport, and do not use two many async messaging for doing this > because I do not think those are necessary. > = > Thanks. > = > = > = > = > Best Regards > Ziye Yang > = > _______________________________________________ > SPDK mailing list > SPDK(a)lists.01.org > https://lists.01.org/mailman/listinfo/spdk --===============6072347107823440657==--