From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id 179505F13 for ; Tue, 4 Sep 2018 02:21:06 +0200 (CEST) Received: by mail-wr1-f65.google.com with SMTP id u12-v6so2090805wrr.4 for ; Mon, 03 Sep 2018 17:21:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=1yUvBytYgqAJewpoqccHGHLDMPSX1qx+L3YIQh3ZiDw=; b=VFavOovKQy+V6LhBz55bRg8pSz5bz5mh+UZdgai65y1bQJkUgyhov1Vhsyu/Gfa1wc ECj5eA/kVbVP17hosc8vIiV1rnN9V5IS4xr+kHtswRvbwvLxmCDDOdbRfMmppPg9hfbi uAa1ZqyjQh7cBiRB46y8haFlAVUv5XjFHvf3HwkLAUTpTOaERwKuoZ4I5Vmc40oD+sBS ZZuE4St4awPYpsiUx0Rf8ed8EWcE7mcZtF9I1meBUFvngmf5BiXxrtjtK/kphAkRmyIs DybH9u+qDGBNgByqZsHxgGGZoVnEMiDpqnvyMvUtoD6FGFooNqIlHc6CnbKFKQSQ9AAq k+bg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=1yUvBytYgqAJewpoqccHGHLDMPSX1qx+L3YIQh3ZiDw=; b=kYe7oMAtNuM/2IjQtoXLZftgLn1hKGhjvolJxhyM8VV90Th7NeXVhEY8DnD6ieyi77 SENh2CdXGAK+pcy6nICop5gqqRhb27Hbu3ieKg1I5R0JpcTJSG6QwKOInQWclXID7kWH ahnuw8cDSl8Be2UQB1rJDILHLTehaEvwwS2n1bzepaiz1IU/dp0N/Q5Hd8Ue3Dt+0bfJ 418TNw0hxW8GjP345+Zpp7ldwoxL5uruB/CS10K/dzSNOkSeLm+Glyl+qpwOm6rpZnqD w23nFrnH4b7uc5eYnrS3cCW7+bMFvjAQnqG7cZcG+RjHalS9gGq7jL1EQS2H1dEISY3T KMKw== X-Gm-Message-State: APzg51DKR/rVaObirKIk0Mke7xUCqOqZ7cGjNwY43A3LH7C8b7KFd4oe bculG0B+tFQDfWl/n2XZ1JaN5EiKMaWXLflLfRE= X-Google-Smtp-Source: ANB0Vda46kKQtnIaNlbL/Sgh6GRJLu3j/oJ6KcXVlX8TFsHZ13MeL9EQIedUGeel6dWBY9gffT4dBsRtTIZm9+0wwb8= X-Received: by 2002:adf:e1c5:: with SMTP id l5-v6mr20827373wri.36.1536020465745; Mon, 03 Sep 2018 17:21:05 -0700 (PDT) MIME-Version: 1.0 Sender: dan.gora@gmail.com Received: by 2002:adf:fbc1:0:0:0:0:0 with HTTP; Mon, 3 Sep 2018 17:20:25 -0700 (PDT) In-Reply-To: <611163de-bef7-488b-a77b-0e1ff190f1fb@intel.com> References: <20180628224513.18391-1-dg@adax.com> <20180629015508.26599-1-dg@adax.com> <20180629015508.26599-3-dg@adax.com> <611163de-bef7-488b-a77b-0e1ff190f1fb@intel.com> From: Dan Gora Date: Mon, 3 Sep 2018 21:20:25 -0300 X-Google-Sender-Auth: d2zRxu-FQG1opoFqr6EQnChhCZA Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v2 02/10] kni: separate releasing netdev from freeing KNI interface X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Sep 2018 00:21:08 -0000 Hi Ferruh, thanks for your feedback.. I apologize for the delay getting back to you on this.. On Wed, Aug 29, 2018 at 7:59 AM, Ferruh Yigit wrote: > You are right, that problem exits. > Although I don't see problem related to holding the kni_list_lock, polling > thread terminated before unregister interface cause the problem. Actually no, the polling thread is not terminated when the deadlock occurs.. The code path is this: Note that this only occurs when the interface is in the UP state! DPDK app calls rte_kni_release()-> ioctl (RTE_KNI_IOCTL_RELEASE); Down in the kernel in the KNI driver: kni_ioctl (RTE_KNI_IOCTL_RELEASE) -> kni_ioctl_release()-> down_write(kni_list_lock); -- This prevents the polling thread from running because it does a -- "down_read(kni_list_lock)". kni_dev_remove()-> unregister_netdev() - This generates the callback into the DPDK application. ndo_stop - kni_net_release() -> kni_net_process_request() -> wait_event_interruptible_timeout(3 seconds); Back in the DPDK app (Note this has to be in a different thread than the one which called rte_kni_release()!) rte_kni_handle_request() - RTE_KNI_REQ_CFG_NETWORK_IF -> ** Application callback for config_network_if ** kni_fifo_put the response back to the kernel in the resp_q fifo. This is the deadlock... The polling thread cannot run because kni_ioctl_release() is still holding the kni_list_lock, so kni_net_process_request() will not see the response back from the DPDK app in the resp_q fifo. Only when wait_event_interruptible_timeout times out does kni_net_process_request see the response back from the DPDK app, which causes kni_dev_remove() to return, allowing kni_ioctl_release to drop the kni_list_lock. > And it has a reason to terminate polling thread first, because it uses device > resources. > > Separating unregister and free steps looks good, but I am not sure if this > should be reflected to the user, with a new ioctl and API. > When user done with interface it calls rte_kni_release() to release them, does > user really need a rte_kni_free() API or need to know the difference of two, is > there any action to take in userspace between these two APIs? I think no. > > What about keeping single rte_kni_release() API and solve the issue internally > in KNI? > > Previously it was doing: > - Stop threads (also there is another single/multi thread error [1]) > - kni_dev_remove() > - unregister and free netdev() [2] > - kni_net_release_fifo_phy() [3] > > Instead internally can we do: > a- Unregister kernel interfaces, rte_kni_unregister()? > b- stop threads > c- kni_net_release_fifo_phy > d- free netdev > > The challenge I can see is some time required between a) and b) to let userspace > app to response, we need a way to know response received before stopping the thread. > > Another thing is there are two release path, kni_release() and > kni_ioctl_release() both should be fixed. > > > > [1] > If multi thread enabled they have been stopped, but if single thread used it has > not been stopped that is why you don't see the 3 seconds delay for default > single thread case, but not stopping the polling thread but removing the > interface is wrong. You see the problem in the single thread case as well if the interface is in the 'UP' state when it is removed. That is what triggers the callback to the DPDK application to mark the interface 'down', as shown above... > > [2] > unregistering netdev will trigger a userspace request but response won't be read > because polling thread also polls the response queue, and that thread is already > stopped at this stage. > Not exactly.. The polling thread is not dead, just locked out because kni_ioctl_release() is holding the kni_list_lock semaphore. > [3] > This is also wrong as you have pointed in later patch in your series, > kni_net_release_fifo_phy() moves packets from rxq/alloq queue to free queue, > queues are still allocated but the references kept in kernel may be invalid at > this stage because of free netdev() It's worse than that actually because you cannot touch 'dev' at all after the free_netdev() because the 'struct kni_dev' is embedded in the 'struct net_dev' allocated with alloc_netdev(), so you cannot call kni_net_release_fifo_phy() after free_netdev: kni_dev_remove() { if (dev->net_dev) { unregister_netdev(dev->net_dev); free_netdev(dev->net_dev); } kni_net_release_fifo_phy(dev); } Because 'dev' has already been freed. Similarly in kni_ioctl_release() you cannot do: kni_dev_remove(dev); list_del(&dev->list); Because currently kni_dev_remove() calls free_netdevk(), which frees the memory pointed to by 'dev'. Maybe this all can be resolved more easily by allocating the 'struct kni_dev' in separate memory rather than embedding it in the net_dev, that way we can drop the 'kni_list_lock' before calling kni_dev_remove() and we can still touch 'dev' after calling free_netdev(), so we can then grab the lock again and remove it from the list... I'll noodle this a bit more and see if I can get it all to work.. I just wanted to get back to you about how to reproduce the lockup. thanks dan