eal/interrupts: add function to allow interruptible epoll
Checks
Commit Message
The existing definition of rte_epoll_wait retries if interrupted
by a signal. This behavior makes it hard to use rte_epoll_wait
for applications that want to use signals do do things like
exit polling loop and shutdown.
Since changing existing semantic might break applications, add
a new rte_epoll_wait_interruptible() function that does the
same thing as rte_epoll_wait but will return -1 and errno of EINTR
if it receives a signal.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
.../common/include/rte_eal_interrupts.h | 23 +++++++++
lib/librte_eal/freebsd/eal/eal_interrupts.c | 12 +++++
lib/librte_eal/linux/eal/eal_interrupts.c | 47 ++++++++++++-------
lib/librte_eal/rte_eal_version.map | 1 +
4 files changed, 65 insertions(+), 18 deletions(-)
Comments
On Wed, 24 Jul 2019 10:20:37 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:
> The existing definition of rte_epoll_wait retries if interrupted
> by a signal. This behavior makes it hard to use rte_epoll_wait
> for applications that want to use signals do do things like
> exit polling loop and shutdown.
>
> Since changing existing semantic might break applications, add
> a new rte_epoll_wait_interruptible() function that does the
> same thing as rte_epoll_wait but will return -1 and errno of EINTR
> if it receives a signal.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
This patch has been sitting unreviewed, uncommented, and unmerged for
two releases. It seems only patches from HW vendors get the attention.
Hi Stephen,
Looks good overall, few nits inline.
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Wednesday, July 24, 2019 12:21 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH] eal/interrupts: add function to allow
> interruptible epoll
>
> The existing definition of rte_epoll_wait retries if interrupted by a signal.
> This behavior makes it hard to use rte_epoll_wait for applications that want
> to use signals do do things like exit polling loop and shutdown.
nit ^^ to
>
> Since changing existing semantic might break applications, add a new
> rte_epoll_wait_interruptible() function that does the same thing as
> rte_epoll_wait but will return -1 and errno of EINTR if it receives a signal.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> .../common/include/rte_eal_interrupts.h | 23 +++++++++
> lib/librte_eal/freebsd/eal/eal_interrupts.c | 12 +++++
> lib/librte_eal/linux/eal/eal_interrupts.c | 47 ++++++++++++-------
> lib/librte_eal/rte_eal_version.map | 1 +
Can you rebase with the master? It will help me apply this cleanly.
> 4 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h
> b/lib/librte_eal/common/include/rte_eal_interrupts.h
> index b370c0d26458..ac1c830f37a1 100644
> --- a/lib/librte_eal/common/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
> @@ -87,6 +87,7 @@ struct rte_intr_handle {
>
> /**
> * It waits for events on the epoll instance.
> + * Retries if signal received.
> *
> * @param epfd
> * Epoll instance fd on which the caller wait for events.
> @@ -105,6 +106,28 @@ int
> rte_epoll_wait(int epfd, struct rte_epoll_event *events,
> int maxevents, int timeout);
>
> +/**
> + * It waits for events on the epoll instance.
> + * Does not retry if signal received.
> + *
> + * @param epfd
> + * Epoll instance fd on which the caller wait for events.
> + * @param events
> + * Memory area contains the events that will be available for the caller.
> + * @param maxevents
> + * Up to maxevents are returned, must greater than zero.
> + * @param timeout
It would be good to mention the units here, but might add complexities depending on the functionality supported by the underlying OS.
> + * Specifying a timeout of -1 causes a block indefinitely.
> + * Specifying a timeout equal to zero cause to return immediately.
> + * @return
> + * - On success, returns the number of available event.
> + * - On failure, a negative value.
> + */
> +__rte_experimental
> +int
> +rte_epoll_wait_interruptible(int epfd, struct rte_epoll_event *events,
> + int maxevents, int timeout);
> +
> /**
> * It performs control operations on epoll instance referred by the epfd.
> * It requests that the operation op be performed for the target fd.
> diff --git a/lib/librte_eal/freebsd/eal/eal_interrupts.c
> b/lib/librte_eal/freebsd/eal/eal_interrupts.c
> index f6831b7902c5..d3f8fd78a1ee 100644
> --- a/lib/librte_eal/freebsd/eal/eal_interrupts.c
> +++ b/lib/librte_eal/freebsd/eal/eal_interrupts.c
> @@ -649,6 +649,18 @@ rte_epoll_wait(int epfd, struct rte_epoll_event
> *events,
> return -ENOTSUP;
> }
>
> +int
> +rte_epoll_wait_interruptible(int epfd, struct rte_epoll_event *events,
> + int maxevents, int timeout)
> +{
> + RTE_SET_USED(epfd);
> + RTE_SET_USED(events);
> + RTE_SET_USED(maxevents);
> + RTE_SET_USED(timeout);
> +
> + return -ENOTSUP;
> +}
> +
> int
> rte_epoll_ctl(int epfd, int op, int fd, struct rte_epoll_event *event) { diff --
> git a/lib/librte_eal/linux/eal/eal_interrupts.c
> b/lib/librte_eal/linux/eal/eal_interrupts.c
> index 1955324d3045..b6ae12a282e7 100644
> --- a/lib/librte_eal/linux/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linux/eal/eal_interrupts.c
> @@ -1239,9 +1239,9 @@ rte_intr_tls_epfd(void)
> return RTE_PER_LCORE(_epfd);
> }
>
> -int
> -rte_epoll_wait(int epfd, struct rte_epoll_event *events,
> - int maxevents, int timeout)
> +static int
> +eal_epoll_wait(int epfd, struct rte_epoll_event *events,
> + int maxevents, int timeout, bool interruptible)
> {
> struct epoll_event evs[maxevents];
> int rc;
> @@ -1255,29 +1255,40 @@ rte_epoll_wait(int epfd, struct rte_epoll_event
> *events,
> if (epfd == RTE_EPOLL_PER_THREAD)
> epfd = rte_intr_tls_epfd();
>
> - while (1) {
> - rc = epoll_wait(epfd, evs, maxevents, timeout);
> - if (likely(rc > 0)) {
> - /* epoll_wait has at least one fd ready to read */
> - rc = eal_epoll_process_event(evs, rc, events);
> - break;
> - } else if (rc < 0) {
> - if (errno == EINTR)
> - continue;
> +retry:
I guess it is a personal choice. I personally prefer using goto for error handling.
> + rc = epoll_wait(epfd, evs, maxevents, timeout);
> + if (likely(rc > 0)) {
> + /* epoll_wait has at least one fd ready to read */
> + rc = eal_epoll_process_event(evs, rc, events);
> + } else if (rc < 0) {
> + if (errno == EINTR) {
> + if (!interruptible)
> + goto retry;
> + } else {
> /* epoll_wait fail */
> - RTE_LOG(ERR, EAL, "epoll_wait returns with
> fail %s\n",
> + RTE_LOG(ERR, EAL,
> + "epoll_wait returns with fail %s\n",
> strerror(errno));
> - rc = -1;
> - break;
> - } else {
> - /* rc == 0, epoll_wait timed out */
> - break;
> }
> }
>
> return rc;
> }
>
> +int
> +rte_epoll_wait(int epfd, struct rte_epoll_event *events,
> + int maxevents, int timeout)
> +{
> + return eal_epoll_wait(epfd, events, maxevents, timeout, false); }
> +
> +int
> +rte_epoll_wait_interruptible(int epfd, struct rte_epoll_event *events,
> + int maxevents, int timeout)
> +{
> + return eal_epoll_wait(epfd, events, maxevents, timeout, true); }
> +
> static inline void
> eal_epoll_data_safe_free(struct rte_epoll_event *ev) { diff --git
> a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 234487787994..e1aeb99ba9c8 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -331,6 +331,7 @@ EXPERIMENTAL {
> rte_dev_hotplug_handle_enable;
> rte_dev_iterator_init;
> rte_dev_iterator_next;
> + rte_epoll_wait_interruptible;
> rte_extmem_attach;
> rte_extmem_detach;
> rte_extmem_register;
> --
> 2.20.1
On Mon, Jul 13, 2020 at 10:41:33PM -0700, Stephen Hemminger wrote:
> On Wed, 24 Jul 2019 10:20:37 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> > The existing definition of rte_epoll_wait retries if interrupted
> > by a signal. This behavior makes it hard to use rte_epoll_wait
> > for applications that want to use signals do do things like
> > exit polling loop and shutdown.
> >
> > Since changing existing semantic might break applications, add
> > a new rte_epoll_wait_interruptible() function that does the
> > same thing as rte_epoll_wait but will return -1 and errno of EINTR
> > if it receives a signal.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Harman Kalra <hkalra@marvell.com>
Hi Stephen,
I have reviewed and tested the patch, looks good to me.
Can you please rebase the patch to latest tree.
Thanks
Harman
>
> This patch has been sitting unreviewed, uncommented, and unmerged for
> two releases. It seems only patches from HW vendors get the attention.
@@ -87,6 +87,7 @@ struct rte_intr_handle {
/**
* It waits for events on the epoll instance.
+ * Retries if signal received.
*
* @param epfd
* Epoll instance fd on which the caller wait for events.
@@ -105,6 +106,28 @@ int
rte_epoll_wait(int epfd, struct rte_epoll_event *events,
int maxevents, int timeout);
+/**
+ * It waits for events on the epoll instance.
+ * Does not retry if signal received.
+ *
+ * @param epfd
+ * Epoll instance fd on which the caller wait for events.
+ * @param events
+ * Memory area contains the events that will be available for the caller.
+ * @param maxevents
+ * Up to maxevents are returned, must greater than zero.
+ * @param timeout
+ * Specifying a timeout of -1 causes a block indefinitely.
+ * Specifying a timeout equal to zero cause to return immediately.
+ * @return
+ * - On success, returns the number of available event.
+ * - On failure, a negative value.
+ */
+__rte_experimental
+int
+rte_epoll_wait_interruptible(int epfd, struct rte_epoll_event *events,
+ int maxevents, int timeout);
+
/**
* It performs control operations on epoll instance referred by the epfd.
* It requests that the operation op be performed for the target fd.
@@ -649,6 +649,18 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
return -ENOTSUP;
}
+int
+rte_epoll_wait_interruptible(int epfd, struct rte_epoll_event *events,
+ int maxevents, int timeout)
+{
+ RTE_SET_USED(epfd);
+ RTE_SET_USED(events);
+ RTE_SET_USED(maxevents);
+ RTE_SET_USED(timeout);
+
+ return -ENOTSUP;
+}
+
int
rte_epoll_ctl(int epfd, int op, int fd, struct rte_epoll_event *event)
{
@@ -1239,9 +1239,9 @@ rte_intr_tls_epfd(void)
return RTE_PER_LCORE(_epfd);
}
-int
-rte_epoll_wait(int epfd, struct rte_epoll_event *events,
- int maxevents, int timeout)
+static int
+eal_epoll_wait(int epfd, struct rte_epoll_event *events,
+ int maxevents, int timeout, bool interruptible)
{
struct epoll_event evs[maxevents];
int rc;
@@ -1255,29 +1255,40 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
if (epfd == RTE_EPOLL_PER_THREAD)
epfd = rte_intr_tls_epfd();
- while (1) {
- rc = epoll_wait(epfd, evs, maxevents, timeout);
- if (likely(rc > 0)) {
- /* epoll_wait has at least one fd ready to read */
- rc = eal_epoll_process_event(evs, rc, events);
- break;
- } else if (rc < 0) {
- if (errno == EINTR)
- continue;
+retry:
+ rc = epoll_wait(epfd, evs, maxevents, timeout);
+ if (likely(rc > 0)) {
+ /* epoll_wait has at least one fd ready to read */
+ rc = eal_epoll_process_event(evs, rc, events);
+ } else if (rc < 0) {
+ if (errno == EINTR) {
+ if (!interruptible)
+ goto retry;
+ } else {
/* epoll_wait fail */
- RTE_LOG(ERR, EAL, "epoll_wait returns with fail %s\n",
+ RTE_LOG(ERR, EAL,
+ "epoll_wait returns with fail %s\n",
strerror(errno));
- rc = -1;
- break;
- } else {
- /* rc == 0, epoll_wait timed out */
- break;
}
}
return rc;
}
+int
+rte_epoll_wait(int epfd, struct rte_epoll_event *events,
+ int maxevents, int timeout)
+{
+ return eal_epoll_wait(epfd, events, maxevents, timeout, false);
+}
+
+int
+rte_epoll_wait_interruptible(int epfd, struct rte_epoll_event *events,
+ int maxevents, int timeout)
+{
+ return eal_epoll_wait(epfd, events, maxevents, timeout, true);
+}
+
static inline void
eal_epoll_data_safe_free(struct rte_epoll_event *ev)
{
@@ -331,6 +331,7 @@ EXPERIMENTAL {
rte_dev_hotplug_handle_enable;
rte_dev_iterator_init;
rte_dev_iterator_next;
+ rte_epoll_wait_interruptible;
rte_extmem_attach;
rte_extmem_detach;
rte_extmem_register;