[libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC

[Ján Tomko]

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)

>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. ;)

Jano

>
>I don't think that code movement should cause that big of a trouble.
>You'll get some context conflicts but that's always the case with 30+
>unmerged patches.
>
>Michal
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180913/90733cdc/attachment-0001.sig>