From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Dan Gora <dg@adax.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2 02/10] kni: separate releasing netdev from freeing KNI interface
Date: Wed, 29 Aug 2018 11:59:15 +0100 [thread overview]
Message-ID: <611163de-bef7-488b-a77b-0e1ff190f1fb@intel.com> (raw)
In-Reply-To: <20180629015508.26599-3-dg@adax.com>
On 6/29/2018 2:55 AM, Dan Gora wrote:
> Currently the rte_kni kernel driver suffers from a problem where
> when the interface is released, it generates a callback to the DPDK
> application to change the interface state to Down. However, after the
> DPDK application handles the callback and generates a response back to
> the kernel, the rte_kni driver cannot wake the thread which is asleep
> waiting for the response, because it is holding the kni_link_lock
> semaphore and it has already removed the 'struct kni_dev' from the
> list of interfaces to poll for responses.
>
> This means that if the KNI interface is in the Up state when
> rte_kni_release() is called, it will always sleep for three seconds
> until kni_net_release gives up waiting for a response from the DPDK
> application.
>
> To fix this, we must separate the step to release the kernel network
> interface from the steps to remove the KNI interface from the list
> of interfaces to poll.
>
> When the kernel network interface is removed with unregister_netdev(),
> if the interface is up, it will generate a callback to mark the
> interface down, which calls kni_net_release(). kni_net_release() will
> block waiting for the DPDK application to call rte_kni_handle_request()
> to handle the callback, but it also needs the thread in the KNI driver
> (either the per-dev thread for multi-thread or the per-driver thread)
> to call kni_net_poll_resp() in order to wake the thread sleeping in
> kni_net_release (actually kni_net_process_request()).
>
> So now, KNI interfaces should be removed as such:
>
> 1) The user calls rte_kni_release(). This only unregisters the
> netdev in the kernel, but touches nothing else. This allows all the
> threads to run which are necessary to handle the callback into the
> DPDK application to mark the interface down.
>
> 2) The user stops the thread running rte_kni_handle_request().
> After rte_kni_release() has been called, there will be no more
> callbacks for that interface so it is not necessary. It cannot be
> running at the same time that rte_kni_free() frees all of the FIFOs
> and DPDK memory for that KNI interface.
>
> 3) The user calls rte_kni_free(). This performs the RTE_KNI_IOCTL_FREE
> ioctl which calls kni_ioctl_free(). This function removes the struct
> kni_dev from the list of interfaces to poll (and kills the per-dev
> kthread, if configured for multi-thread), then frees the memory in
> the FIFOs.
>
> Signed-off-by: Dan Gora <dg@adax.com>
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.
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.
[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.
[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()
next prev parent reply other threads:[~2018-08-29 10:59 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-28 22:45 [dpdk-dev] [PATCH 00/10] kni: Interface detach and link status fixes Dan Gora
2018-06-29 1:54 ` Dan Gora
2018-06-29 1:54 ` [dpdk-dev] [PATCH v2 01/10] kni: remove unused variables from struct kni_dev Dan Gora
2018-08-29 10:29 ` Ferruh Yigit
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 02/10] kni: separate releasing netdev from freeing KNI interface Dan Gora
2018-08-29 10:59 ` Ferruh Yigit [this message]
2018-09-04 0:20 ` Dan Gora
2018-09-04 0:36 ` Dan Gora
2018-10-10 17:24 ` Ferruh Yigit
2018-10-10 18:18 ` Dan Gora
2018-10-10 22:51 ` Ferruh Yigit
2018-10-10 23:38 ` Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 03/10] kni: don't touch struct kni_dev after freeing Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 04/10] kni: add rte_kni_free to KNI library Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 05/10] kni: don't run rte_kni_handle_request after interface release Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 06/10] kni: increase length of timeout for KNI responses Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 07/10] kni: update kni test for rte_kni_free Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 08/10] kni: add rte_kni_free to KNI example app Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 09/10] kni: add rte_kni_free to KNI vdev driver Dan Gora
2018-06-29 1:55 ` [dpdk-dev] [PATCH v2 10/10] kni: add API to set link status on kernel interface Dan Gora
2018-08-29 11:48 ` Ferruh Yigit
2018-08-29 21:10 ` Dan Gora
2018-08-29 22:01 ` Stephen Hemminger
2018-08-29 15:54 ` Stephen Hemminger
2018-08-29 21:02 ` Dan Gora
2018-08-29 22:00 ` Stephen Hemminger
2018-08-29 22:12 ` Dan Gora
2018-08-29 22:41 ` Dan Gora
2018-08-29 23:10 ` Stephen Hemminger
2018-08-30 9:49 ` Igor Ryzhov
2018-08-30 10:32 ` Igor Ryzhov
2018-08-30 21:41 ` Dan Gora
2018-08-30 22:09 ` Stephen Hemminger
2018-08-30 22:11 ` Dan Gora
2018-09-04 0:47 ` Dan Gora
2018-09-05 12:57 ` Stephen Hemminger
2018-09-11 21:45 ` Dan Gora
2018-09-11 21:52 ` Stephen Hemminger
2018-09-11 22:07 ` Dan Gora
2018-09-11 23:14 ` Stephen Hemminger
2018-09-12 4:02 ` Jason Wang
2018-09-11 23:14 ` [dpdk-dev] [PATCH 0/2] " Dan Gora
2018-09-11 23:14 ` [dpdk-dev] [PATCH 1/2] " Dan Gora
2018-09-11 23:18 ` Dan Gora
2018-07-20 11:36 ` [dpdk-dev] [PATCH 00/10] kni: Interface detach and link status fixes Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=611163de-bef7-488b-a77b-0e1ff190f1fb@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=dg@adax.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).