From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754078AbcKUOvt (ORCPT ); Mon, 21 Nov 2016 09:51:49 -0500 Received: from mout.kundenserver.de ([212.227.17.10]:57773 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752054AbcKUOvq (ORCPT ); Mon, 21 Nov 2016 09:51:46 -0500 From: Arnd Bergmann To: Sagi Grimberg Cc: Binoy Jayan , Doug Ledford , Sean Hefty , Hal Rosenstock , Leon Romanovsky , Sagi Grimberg , Bart Van Assche , Nicholas Bellinger , Jenny Derzhavetz , Ira Weiny , Steve Wise , Mark Bloch , Tatyana E Nikolova , Matan Barak , Lijun Ou , "Wei Hu(Xavier)" , Faisal Latif , Mustafa Ismail , Mark Brown , linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, target-devel@vger.kernel.org Subject: Re: [PATCH v5 5/9] IB/isert: Replace semaphore sem with completion Date: Mon, 21 Nov 2016 15:50:59 +0100 Message-ID: <31503532.zqWpFShUDU@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <5489f4ef-02fb-5207-6261-c08ee805acae@grimberg.me> References: <1479708496-9828-1-git-send-email-binoy.jayan@linaro.org> <2625189.HB0S3sfnY7@wuerfel> <5489f4ef-02fb-5207-6261-c08ee805acae@grimberg.me> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:ZcuAGkeYjiFp0JaONE1ZZwz3hXnlx+eWiWN4iGo7Dyz3gaRKdx0 SMaE6HgiuZjCc96/ru0bflP7NWQyd3v83W1WDTaDnG3mAFocSsCpJhk5u2oQbhSPl3pF5sB 3nywSdHtDY0J2jP4EjBJoKXhT1Tq/2w9h1W3VRu6t3i8e8rXnaaQhOzavrlZYwe2S/0rfKD OE9qEKNKsKaoMLC+nDrZA== X-UI-Out-Filterresults: notjunk:1;V01:K0:MSjV5I8Z8zI=:vXUMYUTMOx+kwSRoDNNTmf EM6gBJj/0uOtpB9EsDcJpZkrwfmKRovsR9vddMAr8mcI0X01nqKc+r95fXJgGp+ctzmVIkN+w eJ6bnc+8kNnby0N6D6JW3Bmf8rUG7Dar3q09V7NXSMB9uUNa2FXNOQJejBp8JLjnGgtlAzK9G b7TVXb13qNiwXAK/F7FpH0QAF8dZlm4ZyrzDn4a086kO1ZJRUu4TDOEEVfm6uyTpPOjJLAWCK dxcR6cyPSSKohm7v6nU70teLAAsihgJgt/dq27NS3prmECWehGYdvb3rAj+2aIbFBhKFosk69 ee893IUYgHXHE1HpVHJKOjbBTEqNfXluUQzbzSg85lpFZm+38Zs+brEymMiRbCXXdDegdRI/I sjt3kb5JbF28sXQlArJPTEP19HTlruyNCTQNfd+ORnj5bRq2lS9F28YkosNzHSjW4BlJhH2FI 0+CL9/y7nb8PlwdMZ4qXwJeBahJi5gwZatcksURfoWMpfPFf08P+wGUCKbD7sZp2AjFyZOTjX brnSpAXN+7qs0rhput/Wvit4TLqnK3cC+hvWOjhQqjzUaL6KxXkQgMMbLKFDapsduvnH3CEgg fL7RGALA43DYBpDw3P6wRFVRPpJA5cMZxD46lI6SacOux35S3YnnoWQzYSYPDo14xmaola3Jq 2nrfKKOX52NDTz5+SyYwumUBSC5tPYkmPtKkJPXQT9AZzjXKu0k5oKg7nd8G0wyjbGbjmmGho a511QmX9iDmovyeQ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, November 21, 2016 2:33:02 PM CET Sagi Grimberg wrote: > >>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c > >>> index 6dd43f6..de80f56 100644 > >>> --- a/drivers/infiniband/ulp/isert/ib_isert.c > >>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c > >>> @@ -619,7 +619,7 @@ > >>> mutex_unlock(&isert_np->mutex); > >>> > >>> isert_info("np %p: Allow accept_np to continue\n", isert_np); > >>> - up(&isert_np->sem); > >>> + complete(&isert_np->comp); > >>> } > >>> > >>> static void > >>> @@ -2311,7 +2311,7 @@ struct rdma_cm_id * > >>> isert_err("Unable to allocate struct isert_np\n"); > >>> return -ENOMEM; > >>> } > >>> - sema_init(&isert_np->sem, 0); > >>> + init_completion(&isert_np->comp); > >> > >> This is still racy, a connect event can complete just before we > >> init the completion and *will* get lost... > >> > >> This code started off with a waitqueue which exposes the same > >> problem, see: > >> 531b7bf4bd79 Target/iser: Fix iscsit_accept_np and rdma_cm racy flow > >> > >> So, still NAK from me... > > > > I don't see what you mean here. The code using a waitqueue had no > > counter but the completion does. > > The problem here is that init_completion sets the counter to zero > and between this thread wakes up and until it initializes comp->done > we might have another connect event completing it and I fail to > see how this is handled correctly. But the sema_init()/init_completion() is done right after the structure is allocated, so nothing should have a pointer to it at this time. Later when we do the down()/wait_for_completion(), that will just decrement the counter rather than initialize, and converting from one to the other doesn't change anything here. Arnd