Re: [Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Denis V. Lunev]

On 09/21/2015 05:44 PM, Marc Marí wrote:
> On Mon, 21 Sep 2015 16:43:27 +0300
> "Denis V. Lunev" <address@hidden> wrote:
>
> > On 09/08/2015 04:53 PM, Marc Marí wrote:
> > > The current module infrastructure has been improved to enable
> > > dynamic module loading.
> > >
> > > This reduces the load time for very simple guests. For the following
> > > configuration (very loaded)
> > >
> > > ./configure --enable-sdl --enable-gtk --enable-vte --enable-curses \
> > >       --enable-vnc --enable-vnc-{jpeg,tls,sasl,png} --enable-virtfs \
> > >       --enable-brlapi --enable-curl --enable-fdt --enable-bluez \
> > >       --enable-kvm --enable-rdma --enable-uuid --enable-vde \
> > >       --enable-linux-aio --enable-cap-ng --enable-attr
> > > --enable-vhost-net \ --enable-vhost-scsi --enable-spice
> > > --enable-rbd --enable-libiscsi \ --enable-smartcard-nss
> > > --enable-guest-agent --enable-libusb \ --enable-usb-redir
> > > --enable-lzo --enable-snappy --enable-bzip2 \ --enable-seccomp
> > > --enable-coroutine-pool --enable-glusterfs \ --enable-tpm
> > > --enable-libssh2 --enable-vhdx --enable-numa \ --enable-tcmalloc
> > > --target-list=x86_64-softmmu
> > >
> > > With modules disabled, there are 142 libraries loaded at startup.
> > > Time is the following:
> > >    LD time: 0.065 seconds
> > >    QEMU time: 0.02 seconds
> > >    Total time: 0.085 seconds
> > >
> > > With this patch series and modules enabled, there are 128 libraries
> > > loaded at startup. Time is the following:
> > >    LD time: 0.02 seconds
> > >    QEMU time: 0.02 seconds
> > >    Total time: 0.04 seconds
> > >
> > > Where LD time is the time between the program startup and the jump
> > > to main, and QEMU time is the time between the start of main and
> > > the first kvm_entry.
> > >
> > > These results are just with a few block drivers, that were already
> > > a module. Adding more modules (block or not block) should be easy,
> > > and will reduce the load time even more.
> > >
> > > Marc Marí (2):
> > >     Add dynamic module loading for block drivers
> > >     Add dynamic generation of module_block.h
> > >
> > >    .gitignore                      |   1 +
> > >    Makefile                        |  10 ++-
> > >    block.c                         |  70 +++++++++++++++++++++
> > >    configure                       |   2 +-
> > >    include/qemu/module.h           |   3 +
> > >    scripts/modules/module_block.py | 134
> > > ++++++++++++++++++++++++++++++++++++++++
> > > util/module.c                   |  38 ++++-------- 7 files changed,
> > > 227 insertions(+), 31 deletions(-) create mode 100755
> > > scripts/modules/module_block.py
> >   From my point of view the design looks a bit complex.
> > The approach should be quite similar to one used
> > in Linux kernel.
> >
> > If the block driver is configured as a module, block_init
> > should register proper hooks and create generic lists.
> > C code parsing does not look like a good approach
> > to me.
> >
> > If block_init is a bad name, we could use something like
> > module_init.
> I think applying the kind of modules of the Linux kernel here would
> mean reworking an important part of the drivers that want to be
> converted into modules.
>
> If that's the case, I think it's better to have a less good solution
> that is still good enough, and doesn't mean reworking half of the QEMU
> codebase. Correct me if I'm wrong.
>
> But of course, this is not the only valid approach.
>
> Thanks
> Marc
Frankly speaking this does not look to me like a very big deal
and a huge rework. Build system should create 2 object files
for a modularized driver:
- one with driver stub which has required properties aka has_probe, name etc
  and a bit meaning that the driver is not present
- one with driver code
and from my point of view changes will not be big with this
approach.

Anyway, this is my opinion about this stuff. That is all, I not
against your change, I would like to note that there is a better
way which will allow further improvements.

Den