Installer using 100% of a CPU core

  • Done
  • quality assurance status badge
Details
5 participants
  • Bengt Richter
  • Ludovic Courtès
  • Maxim Cournoyer
  • Mathieu Othacehe
  • pelzflorian (Florian Pelz)
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
important
M
M
Maxim Cournoyer wrote on 29 Jan 2020 06:19
(name . bug-guix)(address . bug-guix@gnu.org)
87a766u9et.fsf@gmail.com
While using the Guix 1.0.1 installer, I noticed that it was constantly
maxing one of the cores on the machine.

The guilty process is:

Toggle snippet (3 lines)
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
root 377 99.8 0.3 37268 10328 ? Rs 08:20 377:09 /gnu/store/8asv37pjsq3n4b4hgz9ys840f9j531hk-kmscon-0.0.0-1.01dd0a2/libexec/kmscon/kmscon --login --vt tty1 --no-switchvt --login -- /gnu/store/cb92798ps1xpla3ai14ik81vkl7jbw97-installer -p
L
L
Ludovic Courtès wrote on 29 Jan 2020 17:27
control message for bug #39341
(address . control@debbugs.gnu.org)
87d0b26xdi.fsf@gnu.org
severity 39341 important
quit
L
L
Ludovic Courtès wrote on 25 Apr 2021 23:41
Re: bug#39341: Installer using 100% of a CPU core
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87h7juf3dk.fsf@gnu.org
Hi!

(Cc: Florian + Mathieu)

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (8 lines)
> While using the Guix 1.0.1 installer, I noticed that it was constantly
> maxing one of the cores on the machine.
>
> The guilty process is:
>
> USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
> root 377 99.8 0.3 37268 10328 ? Rs 08:20 377:09 /gnu/store/8asv37pjsq3n4b4hgz9ys840f9j531hk-kmscon-0.0.0-1.01dd0a2/libexec/kmscon/kmscon --login --vt tty1 --no-switchvt --login -- /gnu/store/cb92798ps1xpla3ai14ik81vkl7jbw97-installer -p

Good news! This is finally fixed:


(Mathieu, Florian: let me know if anything looks fishy.)

The commit log says it all. With this patch, a new FIFO is opened after
each keymap-change request so kmscon no longer polls on a stale file
descriptor.

I’ve verified that keymap choices in the installer are honored, that you
can change anytime via the F1 menu, and that there’s no file descriptor
leak in the kmscon process.

Now people will be less likely to hear the fan of their laptop right
from the start of the installation process. :-)

Ludo’.
Closed
P
P
pelzflorian (Florian Pelz) wrote on 26 Apr 2021 16:32
(name . Ludovic Courtès)(address . ludo@gnu.org)
20210426143225.fljn47jmgj5hhmra@pelzflorian.localdomain
On Sun, Apr 25, 2021 at 11:41:59PM +0200, Ludovic Courtès wrote:
65;6003;1c> I’ve verified that keymap choices in the installer are honored, that you
Toggle quote (3 lines)
> can change anytime via the F1 menu, and that there’s no file descriptor
> leak in the kmscon process.

Keyboard layout selection completely broke for me, I will check again
later if the mistake is on my part.

Regards,
Florian
Closed
L
L
Ludovic Courtès wrote on 26 Apr 2021 18:07
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
87fszdav22.fsf@gnu.org
Hi,

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

Toggle quote (8 lines)
> On Sun, Apr 25, 2021 at 11:41:59PM +0200, Ludovic Courtès wrote:
> 65;6003;1c> I’ve verified that keymap choices in the installer are honored, that you
>> can change anytime via the F1 menu, and that there’s no file descriptor
>> leak in the kmscon process.
>
> Keyboard layout selection completely broke for me, I will check again
> later if the mistake is on my part.

How did you test?

I made the initial choice in the early menu and later modified it by
pressing F1 and “Change keyboard layout”.

Ludo’.
Closed
P
P
pelzflorian (Florian Pelz) wrote on 26 Apr 2021 18:14
(name . Ludovic Courtès)(address . ludo@gnu.org)
20210426161459.rfgxir2n5ya73vmv@pelzflorian.localdomain
On Mon, Apr 26, 2021 at 06:07:33PM +0200, Ludovic Courtès wrote:
Toggle quote (19 lines)
> Hi,
>
> "pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:
>
> > On Sun, Apr 25, 2021 at 11:41:59PM +0200, Ludovic Courtès wrote:
> > 65;6003;1c> I’ve verified that keymap choices in the installer are honored, that you
> >> can change anytime via the F1 menu, and that there’s no file descriptor
> >> leak in the kmscon process.
> >
> > Keyboard layout selection completely broke for me, I will check again
> > later if the mistake is on my part.
>
> How did you test?
>
> I made the initial choice in the early menu and later modified it by
> pressing F1 and “Change keyboard layout”.
>
> Ludo’.

I selected the layout to be Amharic (or anything else, even QWERTZ
Deutsch/German) both directly and via F1. Keyboard layout of the same
image (guix git master, with updated guix package) works on QEMU and
fails on real hardware where an old image worked.

Regards,
Florian
Closed
L
L
Ludovic Courtès wrote on 27 Apr 2021 12:38
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
878s546mhs.fsf@gnu.org
Hi,

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

Toggle quote (25 lines)
> On Mon, Apr 26, 2021 at 06:07:33PM +0200, Ludovic Courtès wrote:
>> Hi,
>>
>> "pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:
>>
>> > On Sun, Apr 25, 2021 at 11:41:59PM +0200, Ludovic Courtès wrote:
>> > 65;6003;1c> I’ve verified that keymap choices in the installer are honored, that you
>> >> can change anytime via the F1 menu, and that there’s no file descriptor
>> >> leak in the kmscon process.
>> >
>> > Keyboard layout selection completely broke for me, I will check again
>> > later if the mistake is on my part.
>>
>> How did you test?
>>
>> I made the initial choice in the early menu and later modified it by
>> pressing F1 and “Change keyboard layout”.
>>
>> Ludo’.
>
> I selected the layout to be Amharic (or anything else, even QWERTZ
> Deutsch/German) both directly and via F1. Keyboard layout of the same
> image (guix git master, with updated guix package) works on QEMU and
> fails on real hardware where an old image worked.

What do you mean by “fails on real hardware”? That the keyboard layout
is unchanged? Is there anything in /var/log/messages or wherever kmscon
writes its logs?

Ludo’.
Closed
P
P
pelzflorian (Florian Pelz) wrote on 27 Apr 2021 14:32
(name . Ludovic Courtès)(address . ludo@gnu.org)
20210427123209.jyqw77edsbt5qhdj@pelzflorian.localdomain
Attachment: file
Closed
B
B
Bengt Richter wrote on 27 Apr 2021 19:43
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
20210427174356.GA5653@LionPure
Attachment: file
Closed
P
P
pelzflorian (Florian Pelz) wrote on 27 Apr 2021 21:58
(name . Bengt Richter)(address . bokr@bokr.com)
20210427195825.vjp26nofm3yemf5p@pelzflorian.localdomain
Hello Bengt,

On Tue, Apr 27, 2021 at 07:43:56PM +0200, Bengt Richter wrote:
Toggle quote (5 lines)
> Perhaps this old bug in some form?
> https://bugzilla.gnome.org/show_bug.cgi?id=776570
> and solution linked from there might help?
> http://unix.stackexchange.com/questions/333368

No, I believe it is unrelated to the old bug
https://bugs.gnu.org/40493 when I changed kmscon, since

- this affects the layout switch at the start of the installer or
when pressing F1 (Alt+Shift is unrelated, I should not have
mentioned it),

- reverting Ludo’s fix d904abe0768 to the original bug makes layout
selection work again (but causes the original bug 100% CPU usage)

- German keyboard layout is affected too, even though it does not use
Alt+Shift toggleing, and

- apparently even with Ludo’s d904abe0768 the layout switch from the
start of the installer can still be triggered somehow, since
keyboard layout eventually switched to Amharic, but I don’t know how
I triggered it last time.

Regards,
Florian
Closed
L
L
Ludovic Courtès wrote on 27 Apr 2021 23:26
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
8735vbwh9q.fsf@gnu.org
Hi,

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

Toggle quote (30 lines)
> On Tue, Apr 27, 2021 at 12:38:23PM +0200, Ludovic Courtès wrote:
>> What do you mean by “fails on real hardware”? That the keyboard layout
>> is unchanged?
>
> Yes.
>
>
>> Is there anything in /var/log/messages
>
> No, nothing out of the ordinary, only the installer pages I went
> through.
>
>> or wherever kmscon
>> writes its logs?
>
> I try with
>
> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
> index 24b3ea785b..0eff24828c 100644
> --- a/gnu/services/base.scm
> +++ b/gnu/services/base.scm
> @@ -2330,6 +2330,7 @@ This service is not part of @var{%base-services}."
>
> (define kmscon-command
> #~(list
> + #$(file-append strace "/bin/strace") "-o" "/var/log/kmscon-out" "-f"
> #$(file-append kmscon "/bin/kmscon") "--login"
> "--vt" #$virtual-terminal
> "--no-switchvt" ;Prevent a switch to the virtual terminal.

I tried this in ‘guix system vm gnu/system/install.scm’. I proceed like
so:

1. after the installer’s welcome screen I choose French layout;
2. the next dialog is the host name, which is where I confirm I really
got French layout;
3. from there I press F1, choose German layout, confirm by typing into
the host name dialog that I got German layout;
4. I repeat step #3 with a variety of layouts.

Changing layouts there just works and is instantaneous. (Note that this
only changes the layout of kmscon, so tty3 & co. are unaffected.)

The strace log shows this when changing layouts (FD 17 corresponds to
/tmp/kmscon-165-keymap-update, the FIFO node):

Toggle snippet (36 lines)
165 epoll_wait(3, [{EPOLLIN, {u32=17731792, u64=17731792}}, {EPOLLIN|EPOLLHUP, {u32=18285248, u64=18285248}}, {EPOLLIN, {u32=17639536, u64=17639536}}], 32, -1) = 3
165 epoll_wait(12, [{EPOLLIN, {u32=17678528, u64=17678528}}], 32, 0) = 1
165 read(20, "\33[5;22H\33[44m\33[K\33[6;22H\33[K\33[7;22H"..., 16384) = 1657
165 read(20, 0x10f6c28, 16384) = -1 EAGAIN (Resource temporarily unavailable)
165 read(17, "p", 1) = 1
165 read(17, "c", 1) = 1
165 read(17, "1", 1) = 1
165 read(17, "0", 1) = 1
165 read(17, "5", 1) = 1
165 read(17, "\0", 1) = 1
165 read(17, "f", 1) = 1
165 read(17, "r", 1) = 1
165 read(17, "\0", 1) = 1
165 read(17, "\0", 1) = 1
165 read(17, "\0", 1) = 1
165 read(17, "", 1) = 0
165 stat("/gnu/store/m734r6j7g74x9k74sgjb8835pg7dnqbk-libxkbcommon-1.0.3/etc/xkb", 0x7fff179314b0) = -1 ENOENT (No such file or directory)
165 stat("/gnu/store/qz3zdrz12rxawlkvah9qjhjyf6fh1v98-xkeyboard-config-2.31/share/X11/xkb", {st_mode=S_IFDIR|0555, st_size=4096, ...}) = 0

[...]

165 openat(AT_FDCWD, "/gnu/store/qz3zdrz12rxawlkvah9qjhjyf6fh1v98-xkeyboard-config-2.31/share/X11/xkb/symbols/inet", O_RDONLY) = 22
165 fstat(22, {st_mode=S_IFREG|0444, st_size=64040, ...}) = 0
165 mmap(NULL, 64040, PROT_READ, MAP_SHARED, 22, 0) = 0x7f50d23b5000
165 brk(0x12bb000) = 0x12bb000
165 munmap(0x7f50d23b5000, 64040) = 0
165 close(22) = 0
165 epoll_ctl(3, EPOLL_CTL_DEL, 17, NULL) = 0
165 close(17) = 0
165 getpid() = 165
165 unlink("/tmp/kmscon-165-keymap-update") = 0
165 mknod("/tmp/kmscon-165-keymap-update", S_IFIFO|0700) = 0
165 openat(AT_FDCWD, "/tmp/kmscon-165-keymap-update", O_RDONLY|O_NONBLOCK) = 17
165 epoll_ctl(3, EPOLL_CTL_ADD, 17, {EPOLLIN, {u32=19410896, u64=19410896}}) = 0

The last lines show that we delete the FIFO from the poll set, close the
FIFO, recreate it, and re-add it to the poll set.

So for me it works exactly as intended.

Do you have a scenario I could follow to try to reproduce the problem?

Thanks,
Ludo’.
Closed
P
P
pelzflorian (Florian Pelz) wrote on 28 Apr 2021 09:02
(name . Ludovic Courtès)(address . ludo@gnu.org)
20210428070210.g3lntjoek5y6hyi7@pelzflorian.localdomain
Attachment: file
Closed
P
P
pelzflorian (Florian Pelz) wrote on 28 Apr 2021 09:06
(name . Ludovic Courtès)(address . ludo@gnu.org)
20210428070619.myzkbfywyjj7i2ld@pelzflorian.localdomain
On Wed, Apr 28, 2021 at 09:02:10AM +0200, pelzflorian (Florian Pelz) wrote:
Toggle quote (39 lines)
> On Tue, Apr 27, 2021 at 11:26:41PM +0200, Ludovic Court�s wrote:
> > 165 openat(AT_FDCWD, "/gnu/store/qz3zdrz12rxawlkvah9qjhjyf6fh1v98-xkeyboard-config-2.31/share/X11/xkb/symbols/inet", O_RDONLY) = 22
> > 165 fstat(22, {st_mode=S_IFREG|0444, st_size=64040, ...}) = 0
> > 165 mmap(NULL, 64040, PROT_READ, MAP_SHARED, 22, 0) = 0x7f50d23b5000
> > 165 brk(0x12bb000) = 0x12bb000
> > 165 munmap(0x7f50d23b5000, 64040) = 0
> > 165 close(22) = 0
> > 165 epoll_ctl(3, EPOLL_CTL_DEL, 17, NULL) = 0
> > 165 close(17) = 0
> > 165 getpid() = 165
> > 165 unlink("/tmp/kmscon-165-keymap-update") = 0
> > 165 mknod("/tmp/kmscon-165-keymap-update", S_IFIFO|0700) = 0
> > 165 openat(AT_FDCWD, "/tmp/kmscon-165-keymap-update", O_RDONLY|O_NONBLOCK) = 17
> > 165 epoll_ctl(3, EPOLL_CTL_ADD, 17, {EPOLLIN, {u32=19410896, u64=19410896}}) = 0
> > --8<---------------cut here---------------end--------------->8---
> >
> > The last lines show that we delete the FIFO from the poll set, close the
> > FIFO, recreate it, and re-add it to the poll set.
>
> I got:
>
> 231 openat(AT_FDCWD, "/gnu/store/qz3zdrz12rxawlkvah9qjhjyf6fh1v98-xkeyboard-config-2.31/share/X11/xkb/symbols/inet", O_RDONLY) = 31
> 231 fstat(31, {st_mode=S_IFREG|0444, st_size=64040, ...}) = 0
> 231 mmap(NULL, 64040, PROT_READ, MAP_SHARED, 31, 0) = 0x7f1b1f5c7000
> 231 munmap(0x7f1b1f5c7000, 64040) = 0
> 231 close(31) = 0
> 231 epoll_ctl(3, EPOLL_CTL_DEL, 26, NULL) = 0
> 231 close(26) = 0
> 231 getpid() = 231
> 231 unlink("/tmp/kmscon-231-keymap-update") = 0
> 231 mknod("/tmp/kmscon-231-keymap-update", S_IFIFO|0700) = 0
> 231 openat(AT_FDCWD, "/tmp/kmscon-231-keymap-update", O_RDONLY|O_NONBLOCK) = 26
> 231 epoll_ctl(3, EPOLL_CTL_ADD, 26, {EPOLLIN, {u32=9609568, u64=9609568}}) = 0
> 231 epoll_wait(3, [{EPOLLIN, {u32=8123856, u64=8123856}}], 32, -1) = 1
> 231 read(14, "\2\0\0\0 \0\0\0000\373{\0\0\0\0\0\326\0\0\0^\335\t\0\347,\0\0]\0\0\0", 1024) = 32
> 231 ioctl(14, DRM_IOCTL_MODE_PAGE_FLIP, 0x7fff282d6b50) = 0
>
> Slightly different. This is the next occurrence after the pci05\0fr.

The brk does happen, just later, if it is important.

Regards,
Florian
Closed
L
L
Ludovic Courtès wrote on 28 Apr 2021 15:43
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
87sg3asexh.fsf@gnu.org
Hi,

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

Toggle quote (22 lines)
> On Tue, Apr 27, 2021 at 11:26:41PM +0200, Ludovic Courtès wrote:
>> I tried this in ‘guix system vm gnu/system/install.scm’. I proceed like
>> so:
>>
>> 1. after the installer’s welcome screen I choose French layout;
>> 2. the next dialog is the host name, which is where I confirm I really
>> got French layout;
>
> At the host name dialog, I still have QWERTY layout, on both my Beebox
> and my Macbook. I can however switch in QEMU on the same Macbook. Oh
> and I just noticed now; the same USB drive does switch layouts on my
> fastest machine. All are UEFI.
>
>> 3. from there I press F1, choose German layout, confirm by typing into
>> the host name dialog that I got German layout;
>
> No effect, I still got QWERTY layout.
>
>> 4. I repeat step #3 with a variety of layouts.
>
> Same.

[...]

Toggle quote (3 lines)
> My layout is still QWERTY after waiting for some time; I do not know
> how the layout switch was eventually triggered after a while last time.

Hmm I don’t know what to think. Could you confirm that keyboard
switching works if you revert d904abe0768293b2322dbf355b6e41d94e769d78?

To anyone reading this: could you please give it a try and report back?
You can run the image in a VM from current master:

./pre-inst-env guix system vm gnu/system/install.scm

or better yet, build the ISO, copy it to USB, and boot it on bare metal:

./pre-inst-env guix system image -t iso9660 gnu/system/install.scm

Thanks,
Ludo’.
Closed
P
P
pelzflorian (Florian Pelz) wrote on 28 Apr 2021 17:20
(name . Ludovic Courtès)(address . ludo@gnu.org)
20210428152034.xuod5s5e43z7gfia@pelzflorian.localdomain
On Wed, Apr 28, 2021 at 03:43:06PM +0200, Ludovic Courtès wrote:
Toggle quote (3 lines)
> Hmm I don’t know what to think. Could you confirm that keyboard
> switching works if you revert d904abe0768293b2322dbf355b6e41d94e769d78?

Yes, revert fixes it but goes back to 100% CPU usage.

Note that layout switching works on QEMU for me and bare-metal only
fails on some of my machines.

Regards,
Florian
Closed
L
L
Ludovic Courtès wrote on 28 Apr 2021 17:22
(name . pelzflorian (Florian Pelz))(address . pelzflorian@pelzflorian.de)
87im46sabp.fsf@gnu.org
"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

Toggle quote (3 lines)
> My layout is still QWERTY after waiting for some time; I do not know
> how the layout switch was eventually triggered after a while last time.

I reproduced the bug on the bare metal. I noticed in /proc/PID/fd/ a
couple of leaked /tmp/kmscon-PID-keymap-update file descriptors with
“(deleted)” (17 and 20), but repeating F1 → “Change keyboard layout”
doesn’t leak additional FDs. I don’t see which code path leads to this.

To be continued…

Ludo’.
Closed
M
M
Mathieu Othacehe wrote on 29 Apr 2021 11:33
(name . Ludovic Courtès)(address . ludo@gnu.org)
8735v91llu.fsf@gnu.org
Hey Ludo,

Your recent patch is just revealing an issue we always had with this
Kmscon patch. Basically, without the "unlink" called you introduced, the
FIFO fd was added only to the first discovered input and the keyboard
layout was only applied to that very input.

Conveniently, that input was always the main user keyboard I guess. The
attached patch fixes that issue by registering the FIFO on the first
input, but applying the keyboard layout to all the inputs.

Thanks,

Mathieu
From 1a0fddd844ced62c802db0d6d133af45880435f0 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <othacehe@gnu.org>
Date: Thu, 29 Apr 2021 11:11:32 +0200
Subject: [PATCH] gnu: kmscon: Fix layout setup.

Kmscon may discover multiple inputs, corresponding to multiple devices. This
means that the uxkb_dev_keymap_update function may be called multiple times,
and the FIFO is registered on each input poll loop.

When a new layout is written on the FIFO by the installer, the first input
picking up the message, will apply the new layout. However, that input may not
be the input that the user is currently using.

To fix it, register the FIFO on the first input poll loop, but apply the new
layout on all the inputs in the uxkb_keymap_update_handler function.

* gnu/packages/patches/kmscon-runtime-keymap-switch.patch
(uxkb_keymap_update_handler): Apply the new layout to all the inputs.
(uxkb_dev_keymap_update): Register the FIFO fd only on the first input poll loop.
---
.../kmscon-runtime-keymap-switch.patch | 29 ++++++++++++++-----
1 file changed, 22 insertions(+), 7 deletions(-)

Toggle diff (63 lines)
diff --git a/gnu/packages/patches/kmscon-runtime-keymap-switch.patch b/gnu/packages/patches/kmscon-runtime-keymap-switch.patch
index 480aaecad2..abff9c460d 100644
--- a/gnu/packages/patches/kmscon-runtime-keymap-switch.patch
+++ b/gnu/packages/patches/kmscon-runtime-keymap-switch.patch
@@ -132,7 +132,7 @@ index 925c755..8fe08f8 100644
#include <xkbcommon/xkbcommon.h>
#include "shl_hook.h"
#include "shl_llog.h"
-@@ -178,6 +181,95 @@ static void timer_event(struct ev_timer *timer, uint64_t num, void *data)
+@@ -178,6 +181,110 @@ static void timer_event(struct ev_timer *timer, uint64_t num, void *data)
shl_hook_call(dev->input->hook, dev->input, &dev->repeat_event);
}
@@ -171,7 +171,23 @@ index 925c755..8fe08f8 100644
+
+ llog_info(dev->input, "HANDLER CALLED %s|%s|%s\n",
+ model, layout, variant);
-+ uxkb_desc_init(dev->input, model, layout, variant, options, NULL);
++
++ struct uterm_input *input = dev->input;
++ struct shl_dlist *iter;
++
++ /* Apply the new layout to all the inputs. */
++ shl_dlist_for_each(iter, &input->devices) {
++ struct uterm_input_dev *dev;
++ dev = shl_dlist_entry(iter,
++ struct uterm_input_dev,
++ list);
++ uxkb_desc_init(dev->input, model, layout, variant, options, NULL);
++ dev->state = xkb_state_new(dev->input->keymap);
++ if (!dev->state) {
++ llog_error(dev->input, "cannot create XKB state");
++ return;
++ }
++ }
+
+ /* The client will now close the FIFO. Close it too, and re-create a
+ * FIFO so other clients can eventually connect. */
@@ -180,11 +196,6 @@ index 925c755..8fe08f8 100644
+ dev->rupdate_fd = -1;
+ uxkb_dev_keymap_update(dev);
+
-+ dev->state = xkb_state_new(dev->input->keymap);
-+ if (!dev->state) {
-+ llog_error(dev->input, "cannot create XKB state");
-+ return;
-+ }
+}
+
+int uxkb_dev_keymap_update(struct uterm_input_dev *dev)
@@ -193,6 +204,10 @@ index 925c755..8fe08f8 100644
+ char *file;
+ int pid = getpid();
+
++ /* Add the FIFO fd only to the first input poll loop. */
++ if (dev->rupdate_fd > 0)
++ return 0;
++
+ ret = asprintf(&file, INPUT_KEYMAP_UPDATE_FILE, pid);
+ if (ret < 0)
+ return ret;
--
2.31.1
L
L
Ludovic Courtès wrote on 29 Apr 2021 11:59
(name . Mathieu Othacehe)(address . othacehe@gnu.org)
875z05mmxi.fsf@gnu.org
Hello!

Mathieu Othacehe <othacehe@gnu.org> skribis:

Toggle quote (9 lines)
> Your recent patch is just revealing an issue we always had with this
> Kmscon patch. Basically, without the "unlink" called you introduced, the
> FIFO fd was added only to the first discovered input and the keyboard
> layout was only applied to that very input.
>
> Conveniently, that input was always the main user keyboard I guess. The
> attached patch fixes that issue by registering the FIFO on the first
> input, but applying the keyboard layout to all the inputs.

Oh, fun (indeed I tested on a laptop with an external USB keyboard).

Toggle quote (20 lines)
>>From 1a0fddd844ced62c802db0d6d133af45880435f0 Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <othacehe@gnu.org>
> Date: Thu, 29 Apr 2021 11:11:32 +0200
> Subject: [PATCH] gnu: kmscon: Fix layout setup.
>
> Kmscon may discover multiple inputs, corresponding to multiple devices. This
> means that the uxkb_dev_keymap_update function may be called multiple times,
> and the FIFO is registered on each input poll loop.
>
> When a new layout is written on the FIFO by the installer, the first input
> picking up the message, will apply the new layout. However, that input may not
> be the input that the user is currently using.
>
> To fix it, register the FIFO on the first input poll loop, but apply the new
> layout on all the inputs in the uxkb_keymap_update_handler function.
>
> * gnu/packages/patches/kmscon-runtime-keymap-switch.patch
> (uxkb_keymap_update_handler): Apply the new layout to all the inputs.
> (uxkb_dev_keymap_update): Register the FIFO fd only on the first input poll loop.

Tested in a VM: it switches layouts like crazy, doesn’t leak a single
FD, and generally behaves as expected.

Thumbs up!

Thanks,
Ludo’.
M
M
Mathieu Othacehe wrote on 29 Apr 2021 12:13
(name . Ludovic Courtès)(address . ludo@gnu.org)
87wnslz9d7.fsf@gnu.org
Toggle quote (3 lines)
> Tested in a VM: it switches layouts like crazy, doesn’t leak a single
> FD, and generally behaves as expected.

Thanks for testing :) I pushed a variant of this patch both on master
and version-1.3.0 branches.

Mathieu
P
P
pelzflorian (Florian Pelz) wrote on 29 Apr 2021 12:47
(name . Mathieu Othacehe)(address . othacehe@gnu.org)
20210429104735.7756uyhv3uagxs23@pelzflorian.localdomain
On Thu, Apr 29, 2021 at 11:33:17AM +0200, Mathieu Othacehe wrote:
Toggle quote (3 lines)
> Conveniently, that input was always the main user keyboard I guess. The
> attached patch fixes that issue […]

Aha! Thank you.

This patch fixed it on my Beebox PC. Note that my Beebox only ever
had a single input device: a USB keyboard.

Now I can also confirm how before your fix, Mathieu, I triggered the
keyboard layout switch the one time it did actually switch: By
unplugging and replugging my USB keyboard. Then it used the new
layout. (When I use another computer, I take the keyboard to the new
computer, that is why I did that.)

Regards,
Florian
?