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 >>