Re: [PATCH v3 2/3] hvc_init(): Enforce one-time initialization.

[Miche Baker-Harvey <miche@google.com>]

Amit,

You said that the work would be serialized "due to port additions
being on work items on the same workqueue".  I'm not seeing that.
I've double checked this by using a mutex_trylock in
hvc_console::hvc_alloc(), and here's the relevant output from dmesg:

root@myubuntu:~# dmesg | grep MBH
[3307216.210274] MBH: got hvc_ports_mutex
[3307216.210690] MBH: trylock of hvc_ports_mutex failed
[3307216.211143] MBH: got hvc_ports_mutex

This is in a system with two virtio console ports, each of which is a
console.  I think if the VIRTIO_CONSOLE_CONSOLE_PORT message handling
were actually being serialized, the trylock should never fail.

What's the source of the serialization for the workqueue items?  At
first reading it looks like the control_work_handler gets called for
each virtio interrupt?

Miche


On Wed, Nov 23, 2011 at 2:38 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Thu) 17 Nov 2011 [10:57:37], Miche Baker-Harvey wrote:
>> Rusty, Michael, Stephen, et al,
>>
>> Thanks for your comments on these patches.
>>
>> For what I'm trying to do, all three patches are necessary, but maybe
>> I'm going about it the wrong way. Your input would be appreciated.
>> I'm in no way claiming that these patches are "right", just that it's
>> working for me, and that what's in the current pool is not.
>>
>> What I'm trying to do is:
>> On X86,
>> under KVM,
>> start a virtio console device,
>> with multiple ports on the device,
>> at least one of which is also a console (as well as ttyS0).
>>
>> (Eventually, we want to be able to add virtio console ports on the
>> fly, and to have multiple virtio console ports be consoles.)
>
> Are you using kvm-tool to do this? =A0QEMU can already hot-plug ports
> and virtio-console (virtio-serial) devices.
>
>> When all three of the patches are in place, this works great. (By
>> great, I mean that getty's start up on all of ttyS0, hvc0 and hvc1,
>> and console output goes to ttyS0 and to hvc0.
>> "who" shows three users: =A0ttyS0, hvc0, and hvc1.
>> "cat /proc/consoles" shows both ttyS0 and hvc0.
>> I can use all three getty's, and console output really does appear on
>> both the consoles.
>>
>> Based on Rusty's comments, I tried removing each of the patches
>> individually. Here's what happens today. I've seen other failure modes
>> depending on what precisely I'm passing the guest.
>> There's three patches:
>> 1/3 "fix locking of vtermno"
>> 2/3 "enforce one-time initialization with hvc_init
>> "3/3 "use separate struct console * for each console"
>>
>> If I remove the "fix locking of vtermno", I only get one virtio
>> console terminal. =A0"who" shows the ttyS0 and the hvc0, and I can log
>> into the gettys on both. I don't get the second virtio console getty.
>> Interestingly, hvc0 shows up in /proc/consoles twice, and in fact the
>> console output is dumped twice to hvc0 (as you'd expect from looking
>> at printk.c, each line appears twice, followed by the next line.)
>
> I don't really understand why. =A0"fix locking of vtermno" adds locks in
> init_port_console(), which is called from add_port(), which should be
> serialised due to port additions being on work items on the same
> workqueue. =A0I don't see a race here.
>
>> If I remove the "enforce one-time initialization with hvc_init" patch,
>> which makes sure only a single thread is doing the hvc_init, and gates
>> anyone from continuing until it has completed, I get different
>> failures, including hangs, and dereferences of NULL pointers.
>>
>> If I remove the "use separate struct console * for each console"patch,
>> what I'm seeing now is that while all of ttyS0, hvc0, and hvc1 are
>> present with gettys running on them, of the three, only ttyS0 is a
>> console.
>
> I don't see any difference in my testing with and without these
> patches.
>
> This is how I tested with qemu:
>
> ./x86_64-softmmu/qemu-system-x86_64 -m 512 -smp 2 -chardev
> socket,path=3D/tmp/foo,server,nowait,id=3Dfoo -chardev
> socket,path=3D/tmp/bar,server,nowait,id=3Dbar -device virtio-serial
> -device virtconsole,chardev=3Dfoo,nr=3D4 -device
> virtconsole,chardev=3Dbar,nr=3D3 -net none =A0-kernel
> /home/amit/src/linux/arch/x86/boot/bzImage -append 'root=3D/dev/sda1
> console=3Dtty0 console=3DttyS0' -initrd /tmp/initramfs.img
> /guests/f14-nolvm.qcow2 -enable-kvm -snapshot
>
> With this setup, with and without patches, I can spawn two consoles
> via:
>
> /sbin/agetty /dev/hvc0 9600 vt100
> /sbin/agetty /dev/hvc1 9600 vt100
>
> (Strange thing is, the second one gives a 'password incorrect' error
> on login attempts, while the first one logs in fine. =A0I do remember
> testing multiple consoles just fine a year and a half back, so no idea
> why this isn't behaving as expected -- but it mostly looks like a
> userspace issue rather than kernel one.)
>
> As mentioned earlier, I've not looked at the hvc code, but given my
> testing results, I'd like to first understand what you're seeing and
> what your environment is.
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Amit
>