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

[Pavel Hrdina]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180913/1a9f3e1b/attachment-0001.sig>