| Message ID | 20240403211818.10023-1-ihrachys@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] Use listen backlog = 64 for all connections. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 3 Apr 2024, at 23:18, Ihar Hrachyshka wrote: > Before the patch, the size of the backlog depended on the type of socket > (UNIX vs INET) as well as on the language (C vs Python), specifically: > > - python used backlog size = 10 for all sockets; > - C used 64 for UNIX sockets but 10 for INET sockets. > > This consolidates the values across the board. It effectively bumps the > number of simultaneous connections to python unixctl servers to 64. Also > for INET C servers too. > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> Hi Ihar, Thanks for submitting the patch. The patch looks fine to me, however, can you elaborate on why you want to increase the size to 64? Is 10 giving problems in specific scenarios? Cheers, Eelco Acked-by: Eelco Chaudron <echaudro@redhat.com>
On Thu, Apr 4, 2024 at 2:36 AM Eelco Chaudron <echaudro@redhat.com> wrote: > > > On 3 Apr 2024, at 23:18, Ihar Hrachyshka wrote: > > > Before the patch, the size of the backlog depended on the type of socket > > (UNIX vs INET) as well as on the language (C vs Python), specifically: > > > > - python used backlog size = 10 for all sockets; > > - C used 64 for UNIX sockets but 10 for INET sockets. > > > > This consolidates the values across the board. It effectively bumps the > > number of simultaneous connections to python unixctl servers to 64. Also > > for INET C servers too. > > > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > > Hi Ihar, > > Thanks for submitting the patch. The patch looks fine to me, however, can > you elaborate on why you want to increase the size to 64? Is 10 giving > problems in specific scenarios? > > I guess I should've included it in the commit message, (and I am happy to send v2 with it updated), but... 1. Originally* the patch was implemented to allow more parallel fmt_pkt calls in OVN test suite (that rely on a python AF_UNIX unixctl server to transform scapy string format strings into byte strings). The problem with parallel handling of more than 10 unixctl AF_UNIX requests to python servers was noticed here: https://github.com/ovn-org/ovn/commit/0baca3e519756cbe98a32526ccc637bb73468743 2. Now, Brian also reports listen backlog issues in OpenStack environments for INET sockets, see: https://mail.openvswitch.org/pipermail/ovs-discuss/2024-April/053049.html * the patch was part of a series of patches - https://patchwork.ozlabs.org/project/openvswitch/list/?series=382739&state=%2A&archive=both - that generally improve AF_UNIX unixctl socket handling, which I plan to revive later, but nothing stops us from merging this before I get to it. > Cheers, > > Eelco > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > >
On 4 Apr 2024, at 21:17, Ihar Hrachyshka wrote: > On Thu, Apr 4, 2024 at 2:36 AM Eelco Chaudron <echaudro@redhat.com> wrote: > >> >> >> On 3 Apr 2024, at 23:18, Ihar Hrachyshka wrote: >> >>> Before the patch, the size of the backlog depended on the type of socket >>> (UNIX vs INET) as well as on the language (C vs Python), specifically: >>> >>> - python used backlog size = 10 for all sockets; >>> - C used 64 for UNIX sockets but 10 for INET sockets. >>> >>> This consolidates the values across the board. It effectively bumps the >>> number of simultaneous connections to python unixctl servers to 64. Also >>> for INET C servers too. >>> >>> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> >> >> Hi Ihar, >> >> Thanks for submitting the patch. The patch looks fine to me, however, can >> you elaborate on why you want to increase the size to 64? Is 10 giving >> problems in specific scenarios? >> >> > I guess I should've included it in the commit message, (and I am happy to > send v2 with it updated), but... > > 1. Originally* the patch was implemented to allow more parallel fmt_pkt > calls in OVN test suite (that rely on a python AF_UNIX unixctl server to > transform scapy string format strings into byte strings). The problem with > parallel handling of more than 10 unixctl AF_UNIX requests to python > servers was noticed here: > https://github.com/ovn-org/ovn/commit/0baca3e519756cbe98a32526ccc637bb73468743 > 2. Now, Brian also reports listen backlog issues in OpenStack environments > for INET sockets, see: > https://mail.openvswitch.org/pipermail/ovs-discuss/2024-April/053049.html > > * the patch was part of a series of patches - > https://patchwork.ozlabs.org/project/openvswitch/list/?series=382739&state=%2A&archive=both > - that generally improve AF_UNIX unixctl socket handling, which I plan to > revive later, but nothing stops us from merging this before I get to it. > Yes, I noticed that conversation right after I replied ;) Let’s wait for Ilya’s feedback before sending a v2. //Eelco
On 4/5/24 11:15, Eelco Chaudron wrote: > > > On 4 Apr 2024, at 21:17, Ihar Hrachyshka wrote: > >> On Thu, Apr 4, 2024 at 2:36 AM Eelco Chaudron <echaudro@redhat.com> wrote: >> >>> >>> >>> On 3 Apr 2024, at 23:18, Ihar Hrachyshka wrote: >>> >>>> Before the patch, the size of the backlog depended on the type of socket >>>> (UNIX vs INET) as well as on the language (C vs Python), specifically: >>>> >>>> - python used backlog size = 10 for all sockets; >>>> - C used 64 for UNIX sockets but 10 for INET sockets. >>>> >>>> This consolidates the values across the board. It effectively bumps the >>>> number of simultaneous connections to python unixctl servers to 64. Also >>>> for INET C servers too. >>>> >>>> Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> >>> >>> Hi Ihar, >>> >>> Thanks for submitting the patch. The patch looks fine to me, however, can >>> you elaborate on why you want to increase the size to 64? Is 10 giving >>> problems in specific scenarios? >>> >>> >> I guess I should've included it in the commit message, (and I am happy to >> send v2 with it updated), but... >> >> 1. Originally* the patch was implemented to allow more parallel fmt_pkt >> calls in OVN test suite (that rely on a python AF_UNIX unixctl server to >> transform scapy string format strings into byte strings). The problem with >> parallel handling of more than 10 unixctl AF_UNIX requests to python >> servers was noticed here: >> https://github.com/ovn-org/ovn/commit/0baca3e519756cbe98a32526ccc637bb73468743 >> 2. Now, Brian also reports listen backlog issues in OpenStack environments >> for INET sockets, see: >> https://mail.openvswitch.org/pipermail/ovs-discuss/2024-April/053049.html >> >> * the patch was part of a series of patches - >> https://patchwork.ozlabs.org/project/openvswitch/list/?series=382739&state=%2A&archive=both >> - that generally improve AF_UNIX unixctl socket handling, which I plan to >> revive later, but nothing stops us from merging this before I get to it. >> > > Yes, I noticed that conversation right after I replied ;) Let’s wait for Ilya’s feedback before sending a v2. I think, this patch is fine. Expanding the commit message with more context will definitely be useful. Also, this patch spans multiple files, but having 'stream: ' or 'socket: ' subject prefix should be appropriate, I think. Best regards, Ilya Maximets.
diff --git a/lib/socket-util.c b/lib/socket-util.c index 3eb3a3816..2d89fce85 100644 --- a/lib/socket-util.c +++ b/lib/socket-util.c @@ -760,7 +760,7 @@ inet_open_passive(int style, const char *target, int default_port, } /* Listen. */ - if (style == SOCK_STREAM && listen(fd, 10) < 0) { + if (style == SOCK_STREAM && listen(fd, 64) < 0) { error = sock_errno(); VLOG_ERR("%s: listen: %s", target, sock_strerror(error)); goto error; diff --git a/python/ovs/stream.py b/python/ovs/stream.py index 82fbb0d68..dbb6b2e1f 100644 --- a/python/ovs/stream.py +++ b/python/ovs/stream.py @@ -620,7 +620,7 @@ class PassiveStream(object): raise Exception('Unknown connection string') try: - sock.listen(10) + sock.listen(64) except socket.error as e: vlog.err("%s: listen: %s" % (name, os.strerror(e.error))) sock.close()
Before the patch, the size of the backlog depended on the type of socket (UNIX vs INET) as well as on the language (C vs Python), specifically: - python used backlog size = 10 for all sockets; - C used 64 for UNIX sockets but 10 for INET sockets. This consolidates the values across the board. It effectively bumps the number of simultaneous connections to python unixctl servers to 64. Also for INET C servers too. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- lib/socket-util.c | 2 +- python/ovs/stream.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)