On 12/02/2020 15:30, Dumitru Ceara wrote:
On 2/12/20 4:17 PM, Anton Ivanov wrote:


On 12/02/2020 13:41, Dumitru Ceara wrote:
On 2/7/20 9:40 AM, [email protected] wrote:
From: Anton Ivanov <[email protected]>

In order to allow switching between poll and epoll we need to
switch from the direct use of POLLXXX macros to OVS_POLLXXX which
can be either POLLXXX or EPOLLXXX depending on the actual backend.

Signed-off-by: Anton Ivanov <[email protected]>

Hi Anton,

Your patches break OVS. Take the following scenario which adds an OVS
bridge and an internal port to it:

Can you send me your ovs-vswitchd  and ovsdb arguments lists to make
sure I am testing under the same conditions.

On a "clean" system, without any pre-existing OVS conf.db I manually
started OVS:

ovs-ctl start --system-id=local


Thanks in advance,


$ ovs-vsctl add-br br-test
$ ovs-vsctl add-port br-test vm1 -- set interface vm1 type=internal
$ ip netns add vm1
$ ip link set vm1 netns vm1
$ ip netns exec vm1 ip link set vm1 address 00:00:00:00:00:01
$ ip netns exec vm1 ip addr add 42.42.42.2/24 dev vm1

# Bring the port up which will trigger ICMPv6 neighbor/router
solicitation
$ ip netns exec vm1 ip link set dev vm1 up

Then I ran the commands above which create a single bridge with "normal"
action and 1 internal port that gets brought up.

That's it.

Thanks, I have it diagnozed.

The epoll fds which are used to build the dpif-netlink handlers are created in 
one thread and used in another. As a result, they cannot be registered for 
persistence upon creation - they end up in the wrong poll loop.

I will sleep on it and fix it tomorrow in a way which does not compromise the 
performance for the other fds which can be registered at creation.

Brgds,

A.


Regards,
Dumitru


2020-02-12T13:28:20.026Z|00036|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 62 (NETLINK_GENERIC<->NETLINK_GENERIC) at lib/netlink-socket.c:234
(69% CPU usage)
2020-02-12T13:28:20.026Z|00037|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 32 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00038|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 33 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00039|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 34 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00040|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 35 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00041|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 36 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00042|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 62 (NETLINK_GENERIC<->NETLINK_GENERIC) at lib/netlink-socket.c:234
(69% CPU usage)
2020-02-12T13:28:20.026Z|00043|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 32 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00044|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 33 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:20.026Z|00045|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 34 (unknown anon_inode:[eventpoll]) at lib/dpif-netlink.c:2299
(69% CPU usage)
2020-02-12T13:28:26.026Z|00046|poll_loop|INFO|Dropped 1530469 log
messages in last 6 seconds (most recently, 0 seconds ago) due to
excessive rate
2020-02-12T13:28:26.026Z|00047|poll_loop|INFO|wakeup due to [OVS_POLLIN]
on fd 62 (NETLINK_GENERIC<->NETLINK_GENERIC) at lib/netlink-socket.c:234
(98% CPU usage)
2020-02-12T13:28:32.026Z|00048|poll_loop|INFO|Dropped 1548848 log
messages in last 6 seconds (most recently, 0 seconds ago) due to
excessive rate


# ovs-vswitchd stays at 100% CPU until the we bring the bridge down:
$ ovs-vsctl del-br br-test

Also, your patches generate build warnings:
lib/poll-loop.c: In function 'poll_block':
lib/poll-loop.c:383:9: error: unused variable 'counter'
[-Werror=unused-variable]
       int counter;

Regards,
Dumitru

---
   include/openvswitch/poll-loop.h |  6 ++++++
   lib/fatal-signal.c              |  2 +-
   lib/latch-unix.c                |  4 ++--
   lib/netdev-afxdp.c              |  2 +-
   lib/netdev-bsd.c                |  4 ++--
   lib/netdev-linux.c              |  4 ++--
   lib/netlink-notifier.c          |  4 +++-
   lib/poll-loop.c                 | 30 +++++++++++++++---------------
   lib/process.c                   |  2 +-
   lib/route-table-bsd.c           |  2 +-
   lib/rtbsd.c                     |  2 +-
   lib/socket-util.c               |  8 ++++----
   lib/stream-fd.c                 |  6 +++---
   lib/stream-ssl.c                |  8 ++++----
   manpages.mk                     |  3 ---
   tests/test-netflow.c            |  2 +-
   tests/test-sflow.c              |  2 +-
   utilities/nlmon.c               |  2 +-
   18 files changed, 49 insertions(+), 44 deletions(-)

diff --git a/include/openvswitch/poll-loop.h
b/include/openvswitch/poll-loop.h
index 532640497..532d9caa6 100644
--- a/include/openvswitch/poll-loop.h
+++ b/include/openvswitch/poll-loop.h
@@ -41,6 +41,12 @@
   #include <windows.h>
   #endif
   +#define OVS_POLLIN POLLIN
+#define OVS_POLLOUT POLLOUT
+#define OVS_POLLERR POLLERR
+#define OVS_POLLNVAL POLLNVAL
+#define OVS_POLLHUP POLLHUP
+
   #ifdef  __cplusplus
   extern "C" {
   #endif
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
index 09f7c6ecf..97d8d1dab 100644
--- a/lib/fatal-signal.c
+++ b/lib/fatal-signal.c
@@ -271,7 +271,7 @@ fatal_signal_wait(void)
   #ifdef _WIN32
       poll_wevent_wait(wevent);
   #else
-    poll_fd_wait(signal_fds[0], POLLIN);
+    poll_fd_wait(signal_fds[0], OVS_POLLIN);
   #endif
   }
   diff --git a/lib/latch-unix.c b/lib/latch-unix.c
index 2995076d6..fea61ab28 100644
--- a/lib/latch-unix.c
+++ b/lib/latch-unix.c
@@ -67,7 +67,7 @@ latch_is_set(const struct latch *latch)
       int retval;
         pfd.fd = latch->fds[0];
-    pfd.events = POLLIN;
+    pfd.events = POLLIN; /* This is POLL specific, it should use
POLL only macro */
       do {
           retval = poll(&pfd, 1, 0);
       } while (retval < 0 && errno == EINTR);
@@ -83,5 +83,5 @@ latch_is_set(const struct latch *latch)
   void
   latch_wait_at(const struct latch *latch, const char *where)
   {
-    poll_fd_wait_at(latch->fds[0], POLLIN, where);
+    poll_fd_wait_at(latch->fds[0], OVS_POLLIN, where);
   }
diff --git a/lib/netdev-afxdp.c b/lib/netdev-afxdp.c
index 482400d8d..ef367e5ea 100644
--- a/lib/netdev-afxdp.c
+++ b/lib/netdev-afxdp.c
@@ -184,7 +184,7 @@ xsk_rx_wakeup_if_needed(struct xsk_umem_info *umem,
         if (xsk_ring_prod__needs_wakeup(&umem->fq)) {
           pfd.fd = fd;
-        pfd.events = POLLIN;
+        pfd.events = OVS_POLLIN;
             ret = poll(&pfd, 1, 0);
           if (OVS_UNLIKELY(ret < 0)) {
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index 7875636cc..45385b187 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -659,7 +659,7 @@ netdev_bsd_rxq_wait(struct netdev_rxq *rxq_)
   {
       struct netdev_rxq_bsd *rxq = netdev_rxq_bsd_cast(rxq_);
   -    poll_fd_wait(rxq->fd, POLLIN);
+    poll_fd_wait(rxq->fd, OVS_POLLIN);
   }
     /* Discards all packets waiting to be received from 'rxq'. */
@@ -752,7 +752,7 @@ netdev_bsd_send_wait(struct netdev *netdev_, int
qid OVS_UNUSED)
           /* TAP device always accepts packets. */
           poll_immediate_wake();
       } else if (dev->pcap) {
-        poll_fd_wait(dev->fd, POLLOUT);
+        poll_fd_wait(dev->fd, OVS_POLLOUT);
       } else {
           /* We haven't even tried to send a packet yet. */
           poll_immediate_wake();
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 6add3e2fc..d0dc00d97 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -808,7 +808,7 @@ netdev_linux_wait(const struct netdev_class
*netdev_class OVS_UNUSED)
       }
       sock = netdev_linux_notify_sock();
       if (sock) {
-        nl_sock_wait(sock, POLLIN);
+        nl_sock_wait(sock, OVS_POLLIN);
       }
   }
   @@ -1465,7 +1465,7 @@ static void
   netdev_linux_rxq_wait(struct netdev_rxq *rxq_)
   {
       struct netdev_rxq_linux *rx = netdev_rxq_linux_cast(rxq_);
-    poll_fd_wait(rx->fd, POLLIN);
+    poll_fd_wait(rx->fd, OVS_POLLIN);
   }
     static int
diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
index dfecb9778..1747274de 100644
--- a/lib/netlink-notifier.c
+++ b/lib/netlink-notifier.c
@@ -1,4 +1,5 @@
   /*
+
    * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
    *
    * Licensed under the Apache License, Version 2.0 (the "License");
@@ -27,6 +28,7 @@
   #include "netlink-socket.h"
   #include "openvswitch/ofpbuf.h"
   #include "openvswitch/vlog.h"
+#include "openvswitch/poll-loop.h"
     VLOG_DEFINE_THIS_MODULE(netlink_notifier);
   @@ -219,7 +221,7 @@ nln_wait(struct nln *nln)
   {
       nln->has_run = false;
       if (nln->notify_sock) {
-        nl_sock_wait(nln->notify_sock, POLLIN);
+        nl_sock_wait(nln->notify_sock, OVS_POLLIN);
       }
   }
   diff --git a/lib/poll-loop.c b/lib/poll-loop.c
index 4e751ff2c..3902d6c1f 100644
--- a/lib/poll-loop.c
+++ b/lib/poll-loop.c
@@ -80,7 +80,7 @@ find_poll_node(struct poll_loop *loop, int fd,
HANDLE wevent)
   /* On Unix based systems:
    *
    *     Registers 'fd' as waiting for the specified 'events' (which
should be
- *     POLLIN or POLLOUT or POLLIN | POLLOUT).  The following call to
+ *     OVS_POLLIN or OVS_POLLOUT or OVS_POLLIN | OVS_POLLOUT).  The
following call to
    *     poll_block() will wake up when 'fd' becomes ready for one or
more of the
    *     requested events. The 'fd's are given to poll() function later.
    *
@@ -130,8 +130,8 @@ poll_create_node(int fd, HANDLE wevent, short int
events, const char *where)
       }
   }
   -/* Registers 'fd' as waiting for the specified 'events' (which
should be POLLIN
- * or POLLOUT or POLLIN | POLLOUT).  The following call to
poll_block() will
+/* Registers 'fd' as waiting for the specified 'events' (which
should be OVS_POLLIN
+ * or OVS_POLLOUT or OVS_POLLIN | OVS_POLLOUT).  The following call
to poll_block() will
    * wake up when 'fd' becomes ready for one or more of the requested
events.
    *
    * On Windows, 'fd' must be a socket.
@@ -265,20 +265,20 @@ log_wakeup(const char *where, const struct
pollfd *pollfd, int timeout)
       ds_put_cstr(&s, "wakeup due to ");
       if (pollfd) {
           char *description = describe_fd(pollfd->fd);
-        if (pollfd->revents & POLLIN) {
-            ds_put_cstr(&s, "[POLLIN]");
+        if (pollfd->revents & OVS_POLLIN) {
+            ds_put_cstr(&s, "[OVS_POLLIN]");
           }
-        if (pollfd->revents & POLLOUT) {
-            ds_put_cstr(&s, "[POLLOUT]");
+        if (pollfd->revents & OVS_POLLOUT) {
+            ds_put_cstr(&s, "[OVS_POLLOUT]");
           }
-        if (pollfd->revents & POLLERR) {
-            ds_put_cstr(&s, "[POLLERR]");
+        if (pollfd->revents & OVS_POLLERR) {
+            ds_put_cstr(&s, "[OVS_POLLERR]");
           }
-        if (pollfd->revents & POLLHUP) {
-            ds_put_cstr(&s, "[POLLHUP]");
+        if (pollfd->revents & OVS_POLLHUP) {
+            ds_put_cstr(&s, "[OVS_POLLHUP]");
           }
-        if (pollfd->revents & POLLNVAL) {
-            ds_put_cstr(&s, "[POLLNVAL]");
+        if (pollfd->revents & OVS_POLLNVAL) {
+            ds_put_cstr(&s, "[OVS_POLLNVAL]");
           }
           ds_put_format(&s, " on fd %d (%s)", pollfd->fd, description);
           free(description);
@@ -349,10 +349,10 @@ poll_block(void)
           wevents[i] = node->wevent;
           if (node->pollfd.fd && node->wevent) {
               short int wsa_events = 0;
-            if (node->pollfd.events & POLLIN) {
+            if (node->pollfd.events & OVS_POLLIN) {
                   wsa_events |= FD_READ | FD_ACCEPT | FD_CLOSE;
               }
-            if (node->pollfd.events & POLLOUT) {
+            if (node->pollfd.events & OVS_POLLOUT) {
                   wsa_events |= FD_WRITE | FD_CONNECT | FD_CLOSE;
               }
               WSAEventSelect(node->pollfd.fd, node->wevent, wsa_events);
diff --git a/lib/process.c b/lib/process.c
index 78de4b8df..7a7f182e1 100644
--- a/lib/process.c
+++ b/lib/process.c
@@ -592,7 +592,7 @@ process_wait(struct process *p)
       if (p->exited) {
           poll_immediate_wake();
       } else {
-        poll_fd_wait(fds[0], POLLIN);
+        poll_fd_wait(fds[0], OVS_POLLIN);
       }
   #else
       OVS_NOT_REACHED();
diff --git a/lib/route-table-bsd.c b/lib/route-table-bsd.c
index 34d42cfab..3dfa80c7f 100644
--- a/lib/route-table-bsd.c
+++ b/lib/route-table-bsd.c
@@ -113,7 +113,7 @@ retry:
             memset(&pfd, 0, sizeof(pfd));
           pfd.fd = rtsock;
-        pfd.events = POLLIN;
+        pfd.events = OVS_POLLIN;
           /*
            * The timeout value below is somehow arbitrary.
            * It's to detect the lost of routing messages due to
diff --git a/lib/rtbsd.c b/lib/rtbsd.c
index 564595c3a..aa0fa0860 100644
--- a/lib/rtbsd.c
+++ b/lib/rtbsd.c
@@ -159,7 +159,7 @@ rtbsd_notifier_wait(void)
   {
       ovs_mutex_lock(&rtbsd_mutex);
       if (notify_sock >= 0) {
-        poll_fd_wait(notify_sock, POLLIN);
+        poll_fd_wait(notify_sock, OVS_POLLIN);
       }
       ovs_mutex_unlock(&rtbsd_mutex);
   }
diff --git a/lib/socket-util.c b/lib/socket-util.c
index 4f1ffecf5..d37867365 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -259,7 +259,7 @@ check_connection_completion(int fd)
       int retval;
         pfd.fd = fd;
-    pfd.events = POLLOUT;
+    pfd.events = OVS_POLLOUT;
     #ifndef _WIN32
       do {
@@ -280,17 +280,17 @@ check_connection_completion(int fd)
               pfd.revents |= pfd.events;
           }
           if (FD_ISSET(fd, &exset)) {
-            pfd.revents |= POLLERR;
+            pfd.revents |= OVS_POLLERR;
           }
       }
   #endif
       if (retval == 1) {
-        if (pfd.revents & (POLLERR | POLLHUP)) {
+        if (pfd.revents & (OVS_POLLERR | OVS_POLLHUP)) {
               ssize_t n = send(fd, "", 1, 0);
               if (n < 0) {
                   return sock_errno();
               } else {
-                VLOG_ERR_RL(&rl, "poll return POLLERR but send
succeeded");
+                VLOG_ERR_RL(&rl, "poll return OVS_POLLERR but send
succeeded");
                   return EPROTO;
               }
           }
diff --git a/lib/stream-fd.c b/lib/stream-fd.c
index 46ee7ae27..62f768d45 100644
--- a/lib/stream-fd.c
+++ b/lib/stream-fd.c
@@ -150,11 +150,11 @@ fd_wait(struct stream *stream, enum
stream_wait_type wait)
       switch (wait) {
       case STREAM_CONNECT:
       case STREAM_SEND:
-        poll_fd_wait(s->fd, POLLOUT);
+        poll_fd_wait(s->fd, OVS_POLLOUT);
           break;
         case STREAM_RECV:
-        poll_fd_wait(s->fd, POLLIN);
+        poll_fd_wait(s->fd, OVS_POLLIN);
           break;
         default:
@@ -271,7 +271,7 @@ static void
   pfd_wait(struct pstream *pstream)
   {
       struct fd_pstream *ps = fd_pstream_cast(pstream);
-    poll_fd_wait(ps->fd, POLLIN);
+    poll_fd_wait(ps->fd, OVS_POLLIN);
   }
     static const struct pstream_class fd_pstream_class = {
diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
index 078fcbc3a..3b7f9865e 100644
--- a/lib/stream-ssl.c
+++ b/lib/stream-ssl.c
@@ -210,10 +210,10 @@ want_to_poll_events(int want)
           OVS_NOT_REACHED();
         case SSL_READING:
-        return POLLIN;
+        return OVS_POLLIN;
         case SSL_WRITING:
-        return POLLOUT;
+        return OVS_POLLOUT;
         default:
           OVS_NOT_REACHED();
@@ -811,7 +811,7 @@ ssl_wait(struct stream *stream, enum
stream_wait_type wait)
           } else {
               switch (sslv->state) {
               case STATE_TCP_CONNECTING:
-                poll_fd_wait(sslv->fd, POLLOUT);
+                poll_fd_wait(sslv->fd, OVS_POLLOUT);
                   break;
                 case STATE_SSL_CONNECTING:
@@ -965,7 +965,7 @@ static void
   pssl_wait(struct pstream *pstream)
   {
       struct pssl_pstream *pssl = pssl_pstream_cast(pstream);
-    poll_fd_wait(pssl->fd, POLLIN);
+    poll_fd_wait(pssl->fd, OVS_POLLIN);
   }
     const struct pstream_class pssl_pstream_class = {
diff --git a/manpages.mk b/manpages.mk
index dc201484c..54a3a82ad 100644
--- a/manpages.mk
+++ b/manpages.mk
@@ -104,7 +104,6 @@ utilities/bugtool/ovs-bugtool.8: \
   utilities/bugtool/ovs-bugtool.8.in:
   lib/ovs.tmac:
   -
   utilities/ovs-dpctl-top.8: \
       utilities/ovs-dpctl-top.8.in \
       lib/ovs.tmac
@@ -155,8 +154,6 @@ lib/common-syn.man:
   lib/common.man:
   lib/ovs.tmac:
   -lib/ovs.tmac:
-
   utilities/ovs-testcontroller.8: \
       utilities/ovs-testcontroller.8.in \
       lib/common.man \
diff --git a/tests/test-netflow.c b/tests/test-netflow.c
index d2322d450..90516d274 100644
--- a/tests/test-netflow.c
+++ b/tests/test-netflow.c
@@ -229,7 +229,7 @@ test_netflow_main(int argc, char *argv[])
               break;
           }
   -        poll_fd_wait(sock, POLLIN);
+        poll_fd_wait(sock, OVS_POLLIN);
           unixctl_server_wait(server);
           poll_block();
       }
diff --git a/tests/test-sflow.c b/tests/test-sflow.c
index 460d4d6c5..ae169e726 100644
--- a/tests/test-sflow.c
+++ b/tests/test-sflow.c
@@ -739,7 +739,7 @@ test_sflow_main(int argc, char *argv[])
               break;
           }
   -        poll_fd_wait(sock, POLLIN);
+        poll_fd_wait(sock, OVS_POLLIN);
           unixctl_server_wait(server);
           poll_block();
       }
diff --git a/utilities/nlmon.c b/utilities/nlmon.c
index 32aa948c6..e8c6bfa24 100644
--- a/utilities/nlmon.c
+++ b/utilities/nlmon.c
@@ -141,7 +141,7 @@ main(int argc OVS_UNUSED, char *argv[])
               }
           }
   -        nl_sock_wait(sock, POLLIN);
+        nl_sock_wait(sock, OVS_POLLIN);
           poll_block();
       }
   }







--
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to