On Thu, Sep 13, 2018 at 12:59:48AM +0200, Ján Tomko wrote: > On Wed, Sep 12, 2018 at 03:18:26PM +0200, Michal Privoznik wrote: > > On 09/12/2018 12:46 PM, Pavel Hrdina wrote: > > > On Wed, Sep 12, 2018 at 10:57:32AM +0200, Fabiano Fidêncio wrote: > > > > virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() and > > > > virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are the most similar > > > > functions between QEMU and LXC code. > > > > > > > > Let's move their common code to virCgroup. > > > > > > > > Mind that the first two patches are basically preparing the ground for > > > > the changes introduced in the last two patches. > > > > > > Hi, definitely good idea to remove code duplication! > > > > > > We have similar issue with the virDomainSetBlkioParameters for QEMU and > > > LXC drivers. The code to set cgroup values is the same. > > > > > > Since the common object is domain how about introducing > > > virDomainSetupBlkioTune and virDomainSetupMemTune and move it into > > > domain_conf.c, that way we don't have to extract domain specific data > > > into src/util. > > > > I'd rather remove stuff from doman_conf.c than add something there. It's > > already the biggest file by far margin: > > > > libvirt.git $ find . -type f -iname \*.c -exec du -h {} \; | sort -h > > 416K ./src/qemu/qemu_domain.c > > 696K ./src/qemu/qemu_driver.c > > 956K ./src/conf/domain_conf.c > > > > And moving code from domain_conf is something we do from time to time. > > Just look at all those virnet* includes at the beginning of domain_conf.h. > > > > Another reason for moving the code out is that blkio tune is not domain > > specific. In the future, we might want to limit say iohelper and thus > > move it into its specific cgroup. > > > > I'm not that convinced about 2/4 to be honest. Memory specification is > > pretty much domain centric. > > Both seem to follow cgroups to a point, so they're process-centric, not > domain-centric. So if they're needed on lower (src/util) layers, > separating them makes sense. > > (Not that domain_conf does not deserve to get both the parser and the > formatter to be split out at this line count) So I definitely agree that we should move a lot of code out of domain_conf.c, the main reason why I actually suggested to move it there in the fist place is because there is virDomainObj. The ideal thing would be to separate all the virDomainObj code out of domain_conf.c like we have for a lot of other objects. > > > But it follows my first point - moving code > > out from domain_conf.c. And it de-duplicates the code. > > > > > > > > Another benefit is that it will not cause me merge conflicts because I'm > > > rewriting cgroup code and adding support for cgroup v2. > > Such downstream branches should not interfere with upstream libvirt > development. IOW: it's your opportunity as a maintainer to offer to > merge this patch on top of your changes if merging them now would cause > you too much trouble. ;) I was just mentioning it to inform upstream that there is such work going on, it was not an argument against this change :). Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list