[PATCH 1/3] pseudo: Handle too many files deadlock

[PATCH 1/3] pseudo: Handle too many files deadlock

[Richard Purdie]

If we have large amounts of parallelism, pseudo can end up with too
many open connections and will no longer accept further connections,
hanging. This patch works around that by closing some clients, allowing
turnover of connections and unblocking the system. The downside is a small
but theoretical window of data loss. This is likely better than locking
up entirely though. Discussions with Peter are onging about how we could
better fix this.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 .../pseudo/files/toomanyfiles.patch                | 62 ++++++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_1.8.2.bb       |  1 +
 2 files changed, 63 insertions(+)
 create mode 100644 meta/recipes-devtools/pseudo/files/toomanyfiles.patch

diff --git a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
new file mode 100644
index 0000000..14a8975
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
@@ -0,0 +1,62 @@
+Currently if we max out the maximum number of files, pseudo can deadlock, unable to
+accept new connections yet unable to move forward and unblock the other processes
+waiting either.
+
+Rather than hang, when this happens, close out inactive connections, allowing us
+to accept the new ones. The disconnected clients will simply reconnect. There is
+a small risk of data loss here sadly but its better than hanging.
+
+RP
+2017/4/25
+
+Upstream-Status: Pending [Peter is aware of the issue]
+
+Index: pseudo-1.8.2/pseudo_server.c
+===================================================================
+--- pseudo-1.8.2.orig/pseudo_server.c
++++ pseudo-1.8.2/pseudo_server.c
+@@ -581,6 +581,7 @@ pseudo_server_loop(void) {
+ 	int rc;
+ 	int fd;
+ 	int loop_timeout = pseudo_server_timeout;
++	int hitmaxfiles;
+ 
+ 	clients = malloc(16 * sizeof(*clients));
+ 
+@@ -597,6 +598,7 @@ pseudo_server_loop(void) {
+ 	active_clients = 1;
+ 	max_clients = 16;
+ 	highest_client = 0;
++	hitmaxfiles = 0;
+ 
+ 	pseudo_debug(PDBGF_SERVER, "server loop started.\n");
+ 	if (listen_fd < 0) {
+@@ -663,10 +665,18 @@ pseudo_server_loop(void) {
+ 						message_time.tv_usec -= 1000000;
+ 						++message_time.tv_sec;
+ 					}
++				} else if (hitmaxfiles) {
++					/* In theory there is a potential race here where if we close a client, 
++					   it may have sent us a fastop message which we don't act upon.
++					   If we don't close a filehandle we'll loop indefinitely thought. 
++					   Only close one per loop iteration in the interests of caution */
++					close_client(i);
++					histmaxfiles = 0;
+ 				}
+ 				if (die_forcefully)
+ 					break;
+ 			}
++			hitmaxfiles = 0;
+ 			if (!die_forcefully && 
+ 			    (FD_ISSET(clients[0].fd, &events) ||
+ 			     FD_ISSET(clients[0].fd, &reads))) {
+@@ -688,6 +698,9 @@ pseudo_server_loop(void) {
+                                          */
+                                         pseudo_server_timeout = DEFAULT_PSEUDO_SERVER_TIMEOUT;
+                                         die_peacefully = 0;
++				} else if (errno == EMFILE) {
++					hitmaxfiles = 1;
++					pseudo_debug(PDBGF_SERVER, "Hit max open files, dropping a client.\n");
+ 				}
+ 			}
+ 			pseudo_debug(PDBGF_SERVER, "server loop complete [%d clients left]\n", active_clients);
diff --git a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
index b427b9a..9bcd031 100644
--- a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
@@ -7,6 +7,7 @@ SRC_URI = "http://downloads.yoctoproject.org/releases/pseudo/${BPN}-${PV}.tar.bz
            file://moreretries.patch \
            file://efe0be279901006f939cd357ccee47b651c786da.patch \
            file://b6b68db896f9963558334aff7fca61adde4ec10f.patch \
+           file://toomanyfiles.patch \
            "
 
 SRC_URI[md5sum] = "7d41e72188fbea1f696c399c1a435675"
-- 
2.7.4

[PATCH 2/3] package_deb: Enable multithreaded package creation

[Richard Purdie]

Allow the creation of debs to happen in parallel, making best use of resources
on multiprocessor systems.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/classes/package_deb.bbclass | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/meta/classes/package_deb.bbclass b/meta/classes/package_deb.bbclass
index 04b9197..9cab80b 100644
--- a/meta/classes/package_deb.bbclass
+++ b/meta/classes/package_deb.bbclass
@@ -51,6 +51,8 @@ def debian_arch_map(arch, tune):
 # INSTALL_TASK_DEB - task name
 
 python do_package_deb () {
+    from multiprocessing import Process
+
     oldcwd = os.getcwd()
 
     packages = d.getVar('PACKAGES')
@@ -62,11 +64,24 @@ python do_package_deb () {
     if os.access(os.path.join(tmpdir, "stamps", "DEB_PACKAGE_INDEX_CLEAN"),os.R_OK):
         os.unlink(os.path.join(tmpdir, "stamps", "DEB_PACKAGE_INDEX_CLEAN"))
 
-    for pkg in packages.split():
-        deb_write_pkg(pkg, d)
+    max_process = int(d.getVar("BB_NUMBER_THREADS") or os.cpu_count() or 1)
+    launched = []
+    pkgs = packages.split()
+    while pkgs:
+        if len(launched) < max_process:
+            p = Process(target=deb_write_pkg, args=(pkgs.pop(), d))
+            p.start()
+            launched.append(p)
+        for q in launched:
+            # The finished processes are joined when calling is_alive()
+            if not q.is_alive():
+                launched.remove(q)
+    for p in launched:
+        p.join()
 
     os.chdir(oldcwd)
 }
+do_package_deb[vardepsexclude] = "BB_NUMBER_THREADS"
 
 def deb_write_pkg(pkg, d):
     import re, copy
-- 
2.7.4

[PATCH 3/3] package_ipk: Parallelise ipk creation

[Richard Purdie]

Allow the creation of ipks to happen in parallel, making best use of resources
on multiprocessor systems.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/classes/package_ipk.bbclass | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/meta/classes/package_ipk.bbclass b/meta/classes/package_ipk.bbclass
index d2ce3b3..282d212 100644
--- a/meta/classes/package_ipk.bbclass
+++ b/meta/classes/package_ipk.bbclass
@@ -17,6 +17,8 @@ OPKG_ARGS += "${@['', '--add-exclude ' + ' --add-exclude '.join((d.getVar('PACKA
 OPKGLIBDIR = "${localstatedir}/lib"
 
 python do_package_ipk () {
+    from multiprocessing import Process
+
     oldcwd = os.getcwd()
 
     workdir = d.getVar('WORKDIR')
@@ -37,11 +39,24 @@ python do_package_ipk () {
     if os.access(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"), os.R_OK):
         os.unlink(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"))
 
-    for pkg in packages.split():
-        ipk_write_pkg(pkg, d)
+    max_process = int(d.getVar("BB_NUMBER_THREADS") or os.cpu_count() or 1)
+    launched = []
+    pkgs = packages.split()
+    while pkgs:
+        if len(launched) < max_process:
+            p = Process(target=ipk_write_pkg, args=(pkgs.pop(), d))
+            p.start()
+            launched.append(p)
+        for q in launched:
+            # The finished processes are joined when calling is_alive()
+            if not q.is_alive():
+                launched.remove(q)
+    for p in launched:
+        p.join()
 
     os.chdir(oldcwd)
 }
+do_package_ipk[vardepsexclude] = "BB_NUMBER_THREADS"
 
 def ipk_write_pkg(pkg, d):
     import re, copy
-- 
2.7.4

Re: [PATCH 3/3] package_ipk: Parallelise ipk creation

[Denys Dmytriyenko]

On Wed, Jun 14, 2017 at 02:42:37PM +0100, Richard Purdie wrote:
> Allow the creation of ipks to happen in parallel, making best use of resources
> on multiprocessor systems.

This is nice! Thanks for looking into the issue of not optimal utilization of 
all the available processors and cores.


> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes/package_ipk.bbclass | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/classes/package_ipk.bbclass b/meta/classes/package_ipk.bbclass
> index d2ce3b3..282d212 100644
> --- a/meta/classes/package_ipk.bbclass
> +++ b/meta/classes/package_ipk.bbclass
> @@ -17,6 +17,8 @@ OPKG_ARGS += "${@['', '--add-exclude ' + ' --add-exclude '.join((d.getVar('PACKA
>  OPKGLIBDIR = "${localstatedir}/lib"
>  
>  python do_package_ipk () {
> +    from multiprocessing import Process
> +
>      oldcwd = os.getcwd()
>  
>      workdir = d.getVar('WORKDIR')
> @@ -37,11 +39,24 @@ python do_package_ipk () {
>      if os.access(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"), os.R_OK):
>          os.unlink(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"))
>  
> -    for pkg in packages.split():
> -        ipk_write_pkg(pkg, d)
> +    max_process = int(d.getVar("BB_NUMBER_THREADS") or os.cpu_count() or 1)
> +    launched = []
> +    pkgs = packages.split()
> +    while pkgs:
> +        if len(launched) < max_process:
> +            p = Process(target=ipk_write_pkg, args=(pkgs.pop(), d))
> +            p.start()
> +            launched.append(p)
> +        for q in launched:
> +            # The finished processes are joined when calling is_alive()
> +            if not q.is_alive():
> +                launched.remove(q)
> +    for p in launched:
> +        p.join()
>  
>      os.chdir(oldcwd)
>  }
> +do_package_ipk[vardepsexclude] = "BB_NUMBER_THREADS"
>  
>  def ipk_write_pkg(pkg, d):
>      import re, copy
> -- 
> 2.7.4
> 
> -- 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [PATCH 3/3] package_ipk: Parallelise ipk creation

[Leonardo Sandoval]

On Wed, 2017-06-14 at 14:42 +0100, Richard Purdie wrote:
> Allow the creation of ipks to happen in parallel, making best use of resources
> on multiprocessor systems.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes/package_ipk.bbclass | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/classes/package_ipk.bbclass b/meta/classes/package_ipk.bbclass
> index d2ce3b3..282d212 100644
> --- a/meta/classes/package_ipk.bbclass
> +++ b/meta/classes/package_ipk.bbclass
> @@ -17,6 +17,8 @@ OPKG_ARGS += "${@['', '--add-exclude ' + ' --add-exclude '.join((d.getVar('PACKA
>  OPKGLIBDIR = "${localstatedir}/lib"
>  
>  python do_package_ipk () {
> +    from multiprocessing import Process
> +
>      oldcwd = os.getcwd()
>  
>      workdir = d.getVar('WORKDIR')
> @@ -37,11 +39,24 @@ python do_package_ipk () {
>      if os.access(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"), os.R_OK):
>          os.unlink(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"))
>  
> -    for pkg in packages.split():
> -        ipk_write_pkg(pkg, d)


this part looks quite similar as the 2/3, perhaps a single function
inside package.bbclass?

> +    max_process = int(d.getVar("BB_NUMBER_THREADS") or os.cpu_count() or 1)
> +    launched = []
> +    pkgs = packages.split()
> +    while pkgs:
> +        if len(launched) < max_process:
> +            p = Process(target=ipk_write_pkg, args=(pkgs.pop(), d))
> +            p.start()
> +            launched.append(p)
> +        for q in launched:
> +            # The finished processes are joined when calling is_alive()
> +            if not q.is_alive():
> +                launched.remove(q)
> +    for p in launched:
> +        p.join()
>  
>      os.chdir(oldcwd)
>  }
> +do_package_ipk[vardepsexclude] = "BB_NUMBER_THREADS"
>  
>  def ipk_write_pkg(pkg, d):
>      import re, copy
> -- 
> 2.7.4
> 

Re: [PATCH 3/3] package_ipk: Parallelise ipk creation

[Khem Raj]

On Wed, Jun 14, 2017 at 6:42 AM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> Allow the creation of ipks to happen in parallel, making best use of resources
> on multiprocessor systems.
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes/package_ipk.bbclass | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/meta/classes/package_ipk.bbclass b/meta/classes/package_ipk.bbclass
> index d2ce3b3..282d212 100644
> --- a/meta/classes/package_ipk.bbclass
> +++ b/meta/classes/package_ipk.bbclass
> @@ -17,6 +17,8 @@ OPKG_ARGS += "${@['', '--add-exclude ' + ' --add-exclude '.join((d.getVar('PACKA
>  OPKGLIBDIR = "${localstatedir}/lib"
>
>  python do_package_ipk () {
> +    from multiprocessing import Process
> +
>      oldcwd = os.getcwd()
>
>      workdir = d.getVar('WORKDIR')
> @@ -37,11 +39,24 @@ python do_package_ipk () {
>      if os.access(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"), os.R_OK):
>          os.unlink(os.path.join(tmpdir, "stamps", "IPK_PACKAGE_INDEX_CLEAN"))
>
> -    for pkg in packages.split():
> -        ipk_write_pkg(pkg, d)
> +    max_process = int(d.getVar("BB_NUMBER_THREADS") or os.cpu_count() or 1)
> +    launched = []
> +    pkgs = packages.split()
> +    while pkgs:
> +        if len(launched) < max_process:
> +            p = Process(target=ipk_write_pkg, args=(pkgs.pop(), d))
> +            p.start()
> +            launched.append(p)
> +        for q in launched:
> +            # The finished processes are joined when calling is_alive()
> +            if not q.is_alive():
> +                launched.remove(q)
> +    for p in launched:
> +        p.join()
>
>      os.chdir(oldcwd)
>  }
> +do_package_ipk[vardepsexclude] = "BB_NUMBER_THREADS"
>
>  def ipk_write_pkg(pkg, d):
>      import re, copy

on a 44 core system, this patch bails out like below. Once I revert
this it works all fine.

ERROR: westeros-wpe-image-1.0-r0 do_rootfs: Unable to install
packages. Command
'/mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/westeros-wpe-image/1.0-r0/recipe-sysroot-native/usr/bin/opkg
--volatile-cache -f /mnt/a/oe/build/tmp
/work/raspberrypi3-bec-linux-musleabi/westeros-wpe-image/1.0-r0/opkg.conf
-t /mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/westeros-wpe-image/1.0-r0/temp/ipktemp/
-o /mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/west
eros-wpe-image/1.0-r0/rootfs  --force_postinstall
--prefer-arch-to-version   install 96boards-tools kernel-modules opkg
packagegroup-core-boot packagegroup-core-ssh-openssh
packagegroup-ml-wpe packagegroup-westeros psplash-raspberrypi run
-postinsts runit runit-serialgetty' returned 255:
Collected errors:
 * opkg_prepare_url_for_install: Couldn't find anything to satisfy
'96boards-tools'.
 * rm_r: Failed to open dir
/mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/westeros-wpe-image/1.0-r0/temp/ipktemp//opkg-J9PI7V:
No such file or directory.

ERROR: westeros-wpe-image-1.0-r0 do_rootfs: Function failed: do_rootfs

Re: [PATCH 3/3] package_ipk: Parallelise ipk creation

[Richard Purdie]

On Thu, 2017-06-15 at 19:28 -0700, Khem Raj wrote:
> On Wed, Jun 14, 2017 at 6:42 AM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > 
> on a 44 core system, this patch bails out like below. Once I revert
> this it works all fine.
> 
> ERROR: westeros-wpe-image-1.0-r0 do_rootfs: Unable to install
> packages. Command
> '/mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/westeros-
> wpe-image/1.0-r0/recipe-sysroot-native/usr/bin/opkg
> --volatile-cache -f /mnt/a/oe/build/tmp
> /work/raspberrypi3-bec-linux-musleabi/westeros-wpe-image/1.0-
> r0/opkg.conf
> -t /mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/westeros-
> wpe-image/1.0-r0/temp/ipktemp/
> -o /mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/west
> eros-wpe-image/1.0-r0/rootfs  --force_postinstall
> --prefer-arch-to-version   install 96boards-tools kernel-modules opkg
> packagegroup-core-boot packagegroup-core-ssh-openssh
> packagegroup-ml-wpe packagegroup-westeros psplash-raspberrypi run
> -postinsts runit runit-serialgetty' returned 255:
> Collected errors:
>  * opkg_prepare_url_for_install: Couldn't find anything to satisfy
> '96boards-tools'.
>  * rm_r: Failed to open dir
> /mnt/a/oe/build/tmp/work/raspberrypi3-bec-linux-musleabi/westeros-
> wpe-image/1.0-r0/temp/ipktemp//opkg-J9PI7V:
> No such file or directory.
> 
> ERROR: westeros-wpe-image-1.0-r0 do_rootfs: Function failed:
> do_rootfs

FWIW this was actually from the package_ipk Source: change. I've
reproduced it, and revised that patch.

Cheers,

Richard

Re: [PATCH 1/3] pseudo: Handle too many files deadlock

[Gary Thomas]

On 2017-06-14 15:42, Richard Purdie wrote:
> If we have large amounts of parallelism, pseudo can end up with too
> many open connections and will no longer accept further connections,
> hanging. This patch works around that by closing some clients, allowing
> turnover of connections and unblocking the system. The downside is a small
> but theoretical window of data loss. This is likely better than locking
> up entirely though. Discussions with Peter are onging about how we could
> better fix this.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>   .../pseudo/files/toomanyfiles.patch                | 62 ++++++++++++++++++++++
>   meta/recipes-devtools/pseudo/pseudo_1.8.2.bb       |  1 +
>   2 files changed, 63 insertions(+)
>   create mode 100644 meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> 
> diff --git a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> new file mode 100644
> index 0000000..14a8975
> --- /dev/null
> +++ b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> @@ -0,0 +1,62 @@
> +Currently if we max out the maximum number of files, pseudo can deadlock, unable to
> +accept new connections yet unable to move forward and unblock the other processes
> +waiting either.
> +
> +Rather than hang, when this happens, close out inactive connections, allowing us
> +to accept the new ones. The disconnected clients will simply reconnect. There is
> +a small risk of data loss here sadly but its better than hanging.
> +
> +RP
> +2017/4/25
> +
> +Upstream-Status: Pending [Peter is aware of the issue]
> +
> +Index: pseudo-1.8.2/pseudo_server.c
> +===================================================================
> +--- pseudo-1.8.2.orig/pseudo_server.c
> ++++ pseudo-1.8.2/pseudo_server.c
> +@@ -581,6 +581,7 @@ pseudo_server_loop(void) {
> + 	int rc;
> + 	int fd;
> + 	int loop_timeout = pseudo_server_timeout;
> ++	int hitmaxfiles;
> +
> + 	clients = malloc(16 * sizeof(*clients));
> +
> +@@ -597,6 +598,7 @@ pseudo_server_loop(void) {
> + 	active_clients = 1;
> + 	max_clients = 16;
> + 	highest_client = 0;
> ++	hitmaxfiles = 0;
> +
> + 	pseudo_debug(PDBGF_SERVER, "server loop started.\n");
> + 	if (listen_fd < 0) {
> +@@ -663,10 +665,18 @@ pseudo_server_loop(void) {
> + 						message_time.tv_usec -= 1000000;
> + 						++message_time.tv_sec;
> + 					}
> ++				} else if (hitmaxfiles) {
> ++					/* In theory there is a potential race here where if we close a client,
> ++					   it may have sent us a fastop message which we don't act upon.
> ++					   If we don't close a filehandle we'll loop indefinitely thought.
> ++					   Only close one per loop iteration in the interests of caution */
> ++					close_client(i);
> ++					histmaxfiles = 0;

Typo?  s/histmaxfiles/hitmaxfiles/

> + 				}
> + 				if (die_forcefully)
> + 					break;
> + 			}
> ++			hitmaxfiles = 0;
> + 			if (!die_forcefully &&
> + 			    (FD_ISSET(clients[0].fd, &events) ||
> + 			     FD_ISSET(clients[0].fd, &reads))) {
> +@@ -688,6 +698,9 @@ pseudo_server_loop(void) {
> +                                          */
> +                                         pseudo_server_timeout = DEFAULT_PSEUDO_SERVER_TIMEOUT;
> +                                         die_peacefully = 0;
> ++				} else if (errno == EMFILE) {
> ++					hitmaxfiles = 1;
> ++					pseudo_debug(PDBGF_SERVER, "Hit max open files, dropping a client.\n");
> + 				}
> + 			}
> + 			pseudo_debug(PDBGF_SERVER, "server loop complete [%d clients left]\n", active_clients);
> diff --git a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
> index b427b9a..9bcd031 100644
> --- a/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
> +++ b/meta/recipes-devtools/pseudo/pseudo_1.8.2.bb
> @@ -7,6 +7,7 @@ SRC_URI = "http://downloads.yoctoproject.org/releases/pseudo/${BPN}-${PV}.tar.bz
>              file://moreretries.patch \
>              file://efe0be279901006f939cd357ccee47b651c786da.patch \
>              file://b6b68db896f9963558334aff7fca61adde4ec10f.patch \
> +           file://toomanyfiles.patch \
>              "
>   
>   SRC_URI[md5sum] = "7d41e72188fbea1f696c399c1a435675"
> 


-- 
------------------------------------------------------------
Gary Thomas                 |  Consulting for the
MLB Associates              |    Embedded world
------------------------------------------------------------

Re: [PATCH 1/3] pseudo: Handle too many files deadlock

[Richard Purdie]

On Wed, 2017-06-14 at 15:51 +0200, Gary Thomas wrote:
> On 2017-06-14 15:42, Richard Purdie wrote:
> > 
> > If we have large amounts of parallelism, pseudo can end up with too
> > many open connections and will no longer accept further
> > connections,
> > hanging. This patch works around that by closing some clients,
> > allowing
> > turnover of connections and unblocking the system. The downside is
> > a small
> > but theoretical window of data loss. This is likely better than
> > locking
> > up entirely though. Discussions with Peter are onging about how we
> > could
> > better fix this.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >   .../pseudo/files/toomanyfiles.patch                | 62
> > ++++++++++++++++++++++
> >   meta/recipes-devtools/pseudo/pseudo_1.8.2.bb       |  1 +
> >   2 files changed, 63 insertions(+)
> >   create mode 100644 meta/recipes-
> > devtools/pseudo/files/toomanyfiles.patch
> > 
> > diff --git a/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> > b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> > new file mode 100644
> > index 0000000..14a8975
> > --- /dev/null
> > +++ b/meta/recipes-devtools/pseudo/files/toomanyfiles.patch
> > @@ -0,0 +1,62 @@
> > +Currently if we max out the maximum number of files, pseudo can
> > deadlock, unable to
> > +accept new connections yet unable to move forward and unblock the
> > other processes
> > +waiting either.
> > +
> > +Rather than hang, when this happens, close out inactive
> > connections, allowing us
> > +to accept the new ones. The disconnected clients will simply
> > reconnect. There is
> > +a small risk of data loss here sadly but its better than hanging.
> > +
> > +RP
> > +2017/4/25
> > +
> > +Upstream-Status: Pending [Peter is aware of the issue]
> > +
> > +Index: pseudo-1.8.2/pseudo_server.c
> > +==================================================================
> > =
> > +--- pseudo-1.8.2.orig/pseudo_server.c
> > ++++ pseudo-1.8.2/pseudo_server.c
> > +@@ -581,6 +581,7 @@ pseudo_server_loop(void) {
> > + 	int rc;
> > + 	int fd;
> > + 	int loop_timeout = pseudo_server_timeout;
> > ++	int hitmaxfiles;
> > +
> > + 	clients = malloc(16 * sizeof(*clients));
> > +
> > +@@ -597,6 +598,7 @@ pseudo_server_loop(void) {
> > + 	active_clients = 1;
> > + 	max_clients = 16;
> > + 	highest_client = 0;
> > ++	hitmaxfiles = 0;
> > +
> > + 	pseudo_debug(PDBGF_SERVER, "server loop started.\n");
> > + 	if (listen_fd < 0) {
> > +@@ -663,10 +665,18 @@ pseudo_server_loop(void) {
> > + 						message_time.tv_u
> > sec -= 1000000;
> > + 						++message_time.tv
> > _sec;
> > + 					}
> > ++				} else if (hitmaxfiles) {
> > ++					/* In theory there is a
> > potential race here where if we close a client,
> > ++					   it may have sent us a
> > fastop message which we don't act upon.
> > ++					   If we don't close a
> > filehandle we'll loop indefinitely thought.
> > ++					   Only close one per
> > loop iteration in the interests of caution */
> > ++					close_client(i);
> > ++					histmaxfiles = 0;
> Typo?  s/histmaxfiles/hitmaxfiles/

I wondered if anyone would spot that! :)

Its fixed in the version on master-next.

Cheers,

Richard

✗ patchtest: failure for "pseudo: Handle too many files ..." and 2 more

[Patchwork]

== Series Details ==

Series: "pseudo: Handle too many files ..." and 2 more
Revision: 1
URL   : https://patchwork.openembedded.org/series/7239/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             A patch file has been added, but does not have a Signed-off-by tag [test_signed_off_by_presence] 
  Suggested fix    Sign off the added patch file (meta/recipes-devtools/pseudo/files/toomanyfiles.patch)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe