Re: [Qemu-block] [Qemu-devel] [PATCH v4 00/32] Dynamic module loading for block drivers

[Colin Lord]

On 07/19/2016 06:54 AM, Paolo Bonzini wrote:
> 
> 
> On 14/07/2016 21:02, Colin Lord wrote:
>> Here's v4 of the modularization series. Things that have changed since
>> v3 include:
>>
>> - Fix indentation of the generated header file module_block.h
>> - Drivers and probe functions are now all located in the block/
>>   directory, rather than being split between block/ and block/probe/. In
>>   addition the header files for each probe/driver pair are in the block/
>>   directory, not the include/block/driver/ directory (which no longer
>>   exists).
>> - Since the probe files are in block/ now, they follow the naming
>>   pattern of format-probe.c
>> - Renamed crypto probe file to be crypto-probe.c, luks is no longer in
>>   the filename
>> - Fixed formatting of parallels_probe() function header
>> - Enforced consistent naming convention for the probe functions. They
>>   now follow the pattern bdrv_format_probe().
> 
> It's still not clear to me why probes need to be separate for drivers
> that (presumably) will never be modularized.
> 
I was hoping someone more experienced with this project would respond to
you the last time you asked this, so apologies if it felt like I was
ignoring it. I'll tell you what I'm seeing from my perspective though
and hopefully that can get some discussion going.

As far as performance benefits go, you are correct that modularizing the
drivers that these probes are being separated from will have essentially
no effect. At this point it's more of a question of how the project
should be organized. I can see why having modules seems like a nice
organization of things, but on the other hand it does move code around
without accomplishing anything other than a different project structure.

However, the protocol drivers do tend to see noticeable performance
benefits. Most of them get modularized in the first three patches of
this series. This includes the iscsi, glusterfs, ssh, curl, and rbd
drivers. A minimal configuration excluding all of these drivers takes
around 3 ms to reach main(), and each of these drivers respectively add
times of approximately 0.2, 0.9, 1.2, 2.5, and a massive time of 41.6 ms
to the time to main. So some of these aren't even a huge benefit by
themselves but they do add up, and also it makes sense to modularize all
of the protocol drivers if we're already modularizing one.

As for the format drivers, as I said that's a question of project
structure. The dmg driver is in this patch series simply because it was
there in Marc's patches and I didn't remove it, and only recently did I
realize that it wouldn't actually affect performance. I think the whole
deal of probes being separated just fell out from having the dmg driver
modularized. There is maybe an argument to be made that if the protocol
drivers are getting modularized, the format drivers should as well just
for consistency, but at the end of the day it's mostly just personal
preferences I think, and I will add to that that although this series
only modularizes dmg, if this series goes through I'll be working on
modularizing the rest of them as well. Most of them should be trivial to
modularize, although there may be a tricky one or two that I'm not aware
of yet.

Hopefully this has helped make the situation more clear.

Colin

> Paolo
> 
>> Colin Lord (30):
>>   blockdev: prepare iSCSI block driver for dynamic loading
>>   blockdev: Move bochs probe into separate file
>>   blockdev: Move cloop probe to its own file
>>   blockdev: Move luks probe to its own file
>>   blockdev: Move dmg probe to its own file
>>   blockdev: Move parallels probe to its own file
>>   blockdev: Move qcow probe to its own file
>>   blockdev: Move qcow2 probe to its own file
>>   blockdev: Move qed probe to its own file
>>   blockdev: Move raw probe to its own file
>>   blockdev: Move vdi probe to its own file
>>   blockdev: Move vhdx probe to its own file
>>   blockdev: Move vmdk probe to its own file
>>   blockdev: Move vpc probe to its own file
>>   blockdev: Separate bochs probe from its driver
>>   blockdev: Separate cloop probe from its driver
>>   blockdev: Separate luks probe from its driver
>>   blockdev: Separate dmg probe from its driver
>>   blockdev: Separate parallels probe from its driver
>>   blockdev: Separate qcow probe from its driver
>>   blockdev: Separate qcow2 probe from its driver
>>   blockdev: Separate qed probe from its driver
>>   blockdev: Separate raw probe from its driver
>>   blockdev: Separate vdi probe from its driver
>>   blockdev: Separate vhdx probe from its driver
>>   blockdev: Separate vmdk probe from its driver
>>   blockdev: Separate vpc probe from its driver
>>   blockdev: Remove the .bdrv_probe field from BlockDrivers
>>   blockdev: Separate out bdrv_probe_device functions
>>   blockdev: Remove bdrv_probe_device field from BlockDriver
>>
>> Marc Mari (2):
>>   blockdev: Add dynamic generation of module_block.h
>>   blockdev: Add dynamic module loading for block drivers
>>
>>  Makefile                        |   7 ++
>>  block.c                         | 181 
>> ++++++++++++++++++++++++++++++++--------
>>  block/Makefile.objs             |   4 +
>>  block/bochs-probe.c             |  28 +++++++
>>  block/bochs.c                   |  56 +------------
>>  block/bochs.h                   |  40 +++++++++
>>  block/cloop-probe.c             |  22 +++++
>>  block/cloop.c                   |  17 +---
>>  block/crypto-probe.c            |  26 ++++++
>>  block/crypto.c                  |  22 +----
>>  block/dmg-probe.c               |  22 +++++
>>  block/dmg.c                     |  17 +---
>>  block/host_cdrom-probe.c        |  47 +++++++++++
>>  block/host_device-probe.c       |  42 ++++++++++
>>  block/iscsi.c                   |  36 --------
>>  block/parallels-probe.c         |  26 ++++++
>>  block/parallels.c               |  44 +---------
>>  block/parallels.h               |  26 ++++++
>>  block/qcow-probe.c              |  22 +++++
>>  block/qcow.c                    |  32 +------
>>  block/qcow.h                    |  21 +++++
>>  block/qcow2-probe.c             |  22 +++++
>>  block/qcow2.c                   |  14 +---
>>  block/qed-probe.c               |  24 ++++++
>>  block/qed.c                     |  16 +---
>>  block/raw-posix.c               |  55 +-----------
>>  block/raw-probe.c               |  14 ++++
>>  block/raw-win32.c               |  11 +--
>>  block/raw_bsd.c                 |  10 +--
>>  block/vdi-probe.c               |  29 +++++++
>>  block/vdi.c                     |  70 +---------------
>>  block/vdi.h                     |  49 +++++++++++
>>  block/vhdx-probe.c              |  27 ++++++
>>  block/vhdx.c                    |  21 +----
>>  block/vmdk-probe.c              |  67 +++++++++++++++
>>  block/vmdk.c                    |  61 +-------------
>>  block/vmdk.h                    |   7 ++
>>  block/vpc-probe.c               |  16 ++++
>>  block/vpc.c                     |   9 +-
>>  include/block/block_int.h       |   3 -
>>  include/block/probe.h           |  33 ++++++++
>>  include/qemu/module.h           |   3 +
>>  scripts/modules/module_block.py | 108 ++++++++++++++++++++++++
>>  util/module.c                   |  38 +++------
>>  vl.c                            |  38 +++++++++
>>  45 files changed, 948 insertions(+), 535 deletions(-)
>>  create mode 100644 block/bochs-probe.c
>>  create mode 100644 block/bochs.h
>>  create mode 100644 block/cloop-probe.c
>>  create mode 100644 block/crypto-probe.c
>>  create mode 100644 block/dmg-probe.c
>>  create mode 100644 block/host_cdrom-probe.c
>>  create mode 100644 block/host_device-probe.c
>>  create mode 100644 block/parallels-probe.c
>>  create mode 100644 block/parallels.h
>>  create mode 100644 block/qcow-probe.c
>>  create mode 100644 block/qcow.h
>>  create mode 100644 block/qcow2-probe.c
>>  create mode 100644 block/qed-probe.c
>>  create mode 100644 block/raw-probe.c
>>  create mode 100644 block/vdi-probe.c
>>  create mode 100644 block/vdi.h
>>  create mode 100644 block/vhdx-probe.c
>>  create mode 100644 block/vmdk-probe.c
>>  create mode 100644 block/vmdk.h
>>  create mode 100644 block/vpc-probe.c
>>  create mode 100644 include/block/probe.h
>>  create mode 100644 scripts/modules/module_block.py
>>