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

Fabiano Fidêncio posted 4 patches 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180912085736.27456-1-fidencio@redhat.com
Test syntax-check passed
There is a newer version of this series
src/Makefile.am          |  1 +
src/conf/domain_conf.c   | 22 ++++--------
src/conf/domain_conf.h   | 70 +++----------------------------------
src/libvirt_private.syms |  2 ++
src/lxc/lxc_cgroup.c     | 69 ++-----------------------------------
src/qemu/qemu_cgroup.c   | 61 ++-------------------------------
src/qemu/qemu_command.c  |  4 +--
src/util/Makefile.inc.am |  2 ++
src/util/virblkio.c      | 37 ++++++++++++++++++++
src/util/virblkio.h      | 52 ++++++++++++++++++++++++++++
src/util/vircgroup.c     | 74 ++++++++++++++++++++++++++++++++++++++++
src/util/vircgroup.h     |  7 ++++
src/util/virmem.h        | 66 +++++++++++++++++++++++++++++++++++
13 files changed, 259 insertions(+), 208 deletions(-)
create mode 100644 src/util/virblkio.c
create mode 100644 src/util/virblkio.h
create mode 100644 src/util/virmem.h
[libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
Posted by Fabiano Fidêncio 5 years, 6 months ago
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.

changes since v1:
- Michal Privoznik pointed out (as did the `make syntax-check` :-)) that
  we do want to keep src/util independently of the parsing code (thus,
  including "conf/domain_conf.h" in vircgroup.h is not the way to go).
  This has been solved now by partially following Michal's suggestion
  and splitting the structs and functions that would be use in the
  common code to new different files.

Fabiano Fidêncio (4):
  domain_conf: split out virBlkioDevice and virDomainBlkiotune
    definitions
  domain_conf: split out virDomainMemtune and virDomainHugePage
    definitions
  vircgroup: Add virCgroupSetupBlkioTune()
  vircgroup: Add virCgroupSetupMemTune()

 src/Makefile.am          |  1 +
 src/conf/domain_conf.c   | 22 ++++--------
 src/conf/domain_conf.h   | 70 +++----------------------------------
 src/libvirt_private.syms |  2 ++
 src/lxc/lxc_cgroup.c     | 69 ++-----------------------------------
 src/qemu/qemu_cgroup.c   | 61 ++-------------------------------
 src/qemu/qemu_command.c  |  4 +--
 src/util/Makefile.inc.am |  2 ++
 src/util/virblkio.c      | 37 ++++++++++++++++++++
 src/util/virblkio.h      | 52 ++++++++++++++++++++++++++++
 src/util/vircgroup.c     | 74 ++++++++++++++++++++++++++++++++++++++++
 src/util/vircgroup.h     |  7 ++++
 src/util/virmem.h        | 66 +++++++++++++++++++++++++++++++++++
 13 files changed, 259 insertions(+), 208 deletions(-)
 create mode 100644 src/util/virblkio.c
 create mode 100644 src/util/virblkio.h
 create mode 100644 src/util/virmem.h

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
Posted by Pavel Hrdina 5 years, 6 months ago
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.

Another benefit is that it will not cause me merge conflicts because I'm
rewriting cgroup code and adding support for cgroup v2.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
Posted by Michal Privoznik 5 years, 6 months ago
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. 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.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
Posted by Ján Tomko 5 years, 6 months ago
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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v2 0/4] Share cgroup code that is duplicated between QEMU and LXC
Posted by Pavel Hrdina 5 years, 6 months ago
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list