[libvirt] [libvirt PATCH v3 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/20180913074814.9697-1-fidencio@redhat.com
Test syntax-check passed
src/conf/domain_conf.c   | 22 ++++--------
src/conf/domain_conf.h   | 70 +++----------------------------------
src/libvirt_private.syms |  7 +++-
src/lxc/lxc_cgroup.c     | 69 ++-----------------------------------
src/qemu/qemu_cgroup.c   | 61 ++-------------------------------
src/qemu/qemu_command.c  |  5 +--
src/qemu/qemu_domain.c   |  3 +-
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     |  5 +++
src/util/virmem.h        | 66 +++++++++++++++++++++++++++++++++++
13 files changed, 263 insertions(+), 210 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 v3 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.

I know that Pavel Hrdina suggested taking a different path[0] for this
series but I do believe Michal's and Jano's points are valid, thus I've
decided to just do the changes suggested by Michal (at least for now).

[0]: https://www.redhat.com/archives/libvir-list/2018-September/msg00534.html

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.

changes since v2:
- Fixed `make syntax-check` errors that I've missed before (didn't have
  cppi installed);
- Created a new section in src/libvirt_private.syms to correctly export
  the virBlkioDeviceArrayClear symbol;
- Removed a virlkio.c from libvirt_setuid_rpc_client_la__SOURCES (why
  did I add it there in the first place? ;-))
- Explicitly included virmem.h in qemu_{domain,command}.c.
  Although there's nothing enforcing it, I do believe it's a good
  practive that should be followed and I've checked with Andrea and
  Peter about doing that and both agreed with the approach.


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/conf/domain_conf.c   | 22 ++++--------
 src/conf/domain_conf.h   | 70 +++----------------------------------
 src/libvirt_private.syms |  7 +++-
 src/lxc/lxc_cgroup.c     | 69 ++-----------------------------------
 src/qemu/qemu_cgroup.c   | 61 ++-------------------------------
 src/qemu/qemu_command.c  |  5 +--
 src/qemu/qemu_domain.c   |  3 +-
 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     |  5 +++
 src/util/virmem.h        | 66 +++++++++++++++++++++++++++++++++++
 13 files changed, 263 insertions(+), 210 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

Subject: [libvirt PATCH v2 0/4] MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [libvirt PATCH v3 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 09:48:10AM +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.
> 
> I know that Pavel Hrdina suggested taking a different path[0] for this
> series but I do believe Michal's and Jano's points are valid, thus I've
> decided to just do the changes suggested by Michal (at least for now).

That's OK, but I'm still not convinced that vircgroup.c is the correct
place.  These new functions convert some data structure into multiple
cgroup API calls.

By that logic we could do the same for virTypedParams which would lead
into adding several helpers that would covert data structures into
virTypedParams and that feels wrong.

Another thing that I don't like is moving the blkio and mem tune code
into src/util because in util we have helpers to work with OS tools
or OS related data and some really generic tools that could be easily
used outside of libvirt and virtualization related code (yes, there are
some exception that should be most likely fixed).

If we want to deduplicate qemu_cgroup.c and lxc_cgroup.c we should
introduce src/conf/domain_cgroup.c where we would place all common
domain related code that configure cgroups.

Pavel

> 
> [0]: https://www.redhat.com/archives/libvir-list/2018-September/msg00534.html
> 
> 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.
> 
> changes since v2:
> - Fixed `make syntax-check` errors that I've missed before (didn't have
>   cppi installed);
> - Created a new section in src/libvirt_private.syms to correctly export
>   the virBlkioDeviceArrayClear symbol;
> - Removed a virlkio.c from libvirt_setuid_rpc_client_la__SOURCES (why
>   did I add it there in the first place? ;-))
> - Explicitly included virmem.h in qemu_{domain,command}.c.
>   Although there's nothing enforcing it, I do believe it's a good
>   practive that should be followed and I've checked with Andrea and
>   Peter about doing that and both agreed with the approach.
> 
> 
> 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/conf/domain_conf.c   | 22 ++++--------
>  src/conf/domain_conf.h   | 70 +++----------------------------------
>  src/libvirt_private.syms |  7 +++-
>  src/lxc/lxc_cgroup.c     | 69 ++-----------------------------------
>  src/qemu/qemu_cgroup.c   | 61 ++-------------------------------
>  src/qemu/qemu_command.c  |  5 +--
>  src/qemu/qemu_domain.c   |  3 +-
>  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     |  5 +++
>  src/util/virmem.h        | 66 +++++++++++++++++++++++++++++++++++
>  13 files changed, 263 insertions(+), 210 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
> 
> Subject: [libvirt PATCH v2 0/4] MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> 
> --
> 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 v3 0/4] Share cgroup code that is duplicated between QEMU and LXC
Posted by Michal Privoznik 5 years, 6 months ago
On 09/13/2018 02:33 PM, Pavel Hrdina wrote:
> On Thu, Sep 13, 2018 at 09:48:10AM +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.
>>
>> I know that Pavel Hrdina suggested taking a different path[0] for this
>> series but I do believe Michal's and Jano's points are valid, thus I've
>> decided to just do the changes suggested by Michal (at least for now).
> 
> That's OK, but I'm still not convinced that vircgroup.c is the correct
> place.  These new functions convert some data structure into multiple
> cgroup API calls.
> 
> By that logic we could do the same for virTypedParams which would lead
> into adding several helpers that would covert data structures into
> virTypedParams and that feels wrong.
> 
> Another thing that I don't like is moving the blkio and mem tune code
> into src/util because in util we have helpers to work with OS tools
> or OS related data and some really generic tools that could be easily
> used outside of libvirt and virtualization related code (yes, there are
> some exception that should be most likely fixed).

Well, blkio and memtune code is process oriented code (just like
bandwidth code for instance). It is true we currently use it only in
domain code, but that shouldn't be reason to keep it under src/conf.
Just like we are not keeping bandwidth code there.

> 
> If we want to deduplicate qemu_cgroup.c and lxc_cgroup.c we should
> introduce src/conf/domain_cgroup.c where we would place all common
> domain related code that configure cgroups.

In my view, src/conf should contain only XML configuration related code
(i.e. XML parser/formatter). It's only our laziness that we dumped a lot
of unrelated code there.

Anyway, this conversation now looks like bikesheding to me. The code
deduplication is what matters and where the code is going to live
doesn't matter that much. Therefore, I will no longer object.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list