From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============5220445854494579008==" MIME-Version: 1.0 From: Sasha Kotchubievsky Subject: Re: [SPDK] Some thoughts for the code in rdma.c Date: Sun, 23 Dec 2018 16:09:25 +0200 Message-ID: In-Reply-To: FA6C2217B01E9D48A581BB48660210143E0A3D14@shsmsx102.ccr.corp.intel.com List-ID: To: spdk@lists.01.org --===============5220445854494579008== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Ziye, Which problem do you solve: performance, stability or remove code = complexity? Can you elaborate? If you ask me, the refcout, removed by your patch, is needed because qp = can be disconnected in response for different events served in different = threads. I believe, some synchronization is required. For example, NVME-OF initiator closes QP in the middle of = RDMA_READ/RDMA_WRITE from target side and only then generate disconnect = event in librdmacm. Target gets 2 events: completion with error code and = disconnect event (doesn't matter in which order). In that case, qp = disconnect in target can be triggered by two events coming from = different sources and possible handled in different threads. Best regards Sasha On 12/21/2018 3:35 AM, Yang, Ziye wrote: > Hi Ben, > > According to my knowledge, I think that the inc/dec_refcnt is still not n= eeded. I know that the qpair management related with the group (there is a = thread which manages the rdma_cm event and another is for polling). The rea= son that caused this issue is that: we use the spdk_send_msg to add the qpa= ir to the group during the connection, but we return the connection ready t= o the host side early. So there is a window, that the host sends the discon= nect, and there will be no group binding to the qpair. My idea is that we d= o this checks in spdk_nvmf_qpair_disconnect. If the qpair is not adding to = the group, there will be two reasons: > > 1 In the group adding operation, there is an issue, so the qpair is freed= . So we do not need to handle the disconnect and free the qpair again. > 2 The qpair is still not added to the group. > > So I suggest if the qpair is not added in the case 2, we should deny the = disconnect operation or delay such operation. (This is related with excepti= onal case, I think this handling is still reasonable. For normal operation,= it will not do the connect, and disconnect with nothing operation. So deny= the disconnect request this time is OK, and the initiator can do the disco= nnect later, and we can make sure the disconnect will only be accepted that= the qpair is binding to the group. And I do not think that this strategy t= o handle the exceptional case will have any side effect.) > > BTW, only checking the qpair->group is not sufficient, but we can have ot= her state of the qpair (which is already defined in struct spdk_nvmf_qpair_= state). > > I would like to still post some patches to remove the inc/def_refcnt (may= be as test), and let those patches tested by our validation team to see wh= ether this really can solve this issue. > > > > > Best Regards > Ziye Yang > > -----Original Message----- > From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, Benj= amin > Sent: Friday, December 21, 2018 1:16 AM > To: spdk(a)lists.01.org > Subject: Re: [SPDK] Some thoughts for the code in rdma.c > > 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 necess= ary. >> 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 and accessed only from >> * the thread that created the group. Here, we're not on >> that 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 just 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 in= dependent 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 not= ifies 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 discon= nect the queue pair, but there are scenarios where we could legitimately ge= t both events for the same qpair. The processing of both of these events in= volves passing a message to the poll group thread, which is an asynchronous= process and may take an arbitrarily long time. The refcnt coordinates betw= een those two events, such that the first event does not destroy the qpair = while there is another event in- flight still. > > The qpair itself, after the initial set up phase, is also managed by the = poll group thread. If the user destroys a poll group, or manipulates the po= ll group to add or remove a different qpair, or the qpair gets a completion= entry that it begins processing, then the qpair data structure is being mo= dified by the poll group thread. You can't disconnect it on the management = thread while this is happening or you risk corrupting some of the data stru= ctures - in particular the poll group's qpair list. > > Checking that the qpair hasn't been assigned to a poll group yet (by look= ing for qpair.group set to NULL) isn't a sufficient protection because a me= ssage to add the qpair to the poll group may already be in-flight. I made s= ome suggestions to use the refcnt to deal with this scenario on your most r= ecent 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 >> directly 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 > _______________________________________________ > SPDK mailing list > SPDK(a)lists.01.org > https://lists.01.org/mailman/listinfo/spdk > _______________________________________________ > SPDK mailing list > SPDK(a)lists.01.org > https://lists.01.org/mailman/listinfo/spdk --===============5220445854494579008==--