Re: [PATCH 2/7] qemu: Abstract code for memory controller setting into a helper

[Osier Yang <jyang@xxxxxxxxxx>]

On 20/05/13 19:08, Daniel P. Berrange wrote:
> On Fri, May 17, 2013 at 07:59:32PM +0800, Osier Yang wrote:
> > ---
> >   src/qemu/qemu_cgroup.c | 120 ++++++++++++++++++++++++++++---------------------
> >   1 file changed, 68 insertions(+), 52 deletions(-)
> >
> > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> > index 0c4792e..2751be0 100644
> > --- a/src/qemu/qemu_cgroup.c
> > +++ b/src/qemu/qemu_cgroup.c
> > @@ -455,6 +455,72 @@ qemuSetupBlkioCgroup(virDomainObjPtr vm)
> >       return 0;
> >   }
> >   
> My email client is showing trailing whitespace here. Not sure
> if that is genuine or not. Also 2 blank lines between functions

I found no trailing spaces.

> > +static int
> > +qemuSetupMemoryCgroup(virDomainObjPtr vm) {
> Put the '{' on a new line as you did with previous patches

Okay.

> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +    unsigned long long hard_limit;
> > +    int rc;
> > +    int i;
> > +
> > +    if (!virCgroupHasController(priv->cgroup,VIR_CGROUP_CONTROLLER_MEMORY)) {
> > +        if (vm->def->mem.hard_limit != 0 ||
> > +            vm->def->mem.soft_limit != 0 ||
> > +            vm->def->mem.swap_hard_limit != 0) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("Memory cgroup is not available on this host"));
> > +            return -1;
> > +        } else {
> > +            VIR_WARN("Could not autoset a RSS limit for domain %s", vm->def->name);
> > +            return 0;
> > +        }
> Not sure why we need this VIR_WARN at all. If no limits are set in the XML,
> then we should not warn about a missing feature that we don't actually
> need.

Agreed. Having a warning for no XML config is confused. I removed it.
> > @@ -668,58 +734,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
> >       if (qemuSetupBlkioCgroup(vm) < 0)
> >           goto cleanup;
> >   
> > -    if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) {
> > -        unsigned long long hard_limit = vm->def->mem.hard_limit;
> > -
> > -        if (!hard_limit) {
> > -            /* If there is no hard_limit set, set a reasonable one to avoid
> > -             * system thrashing caused by exploited qemu.  A 'reasonable
> > -             * limit' has been chosen:
> > -             *     (1 + k) * (domain memory + total video memory) + (32MB for
> > -             *     cache per each disk) + F
> > -             * where k = 0.5 and F = 200MB.  The cache for disks is important as
> > -             * kernel cache on the host side counts into the RSS limit. */
> > -            hard_limit = vm->def->mem.max_balloon;
> > -            for (i = 0; i < vm->def->nvideos; i++)
> > -                hard_limit += vm->def->videos[i]->vram;
> > -            hard_limit = hard_limit * 1.5 + 204800;
> > -            hard_limit += vm->def->ndisks * 32768;
> > -        }
> > -
> > -        rc = virCgroupSetMemoryHardLimit(priv->cgroup, hard_limit);
> > -        if (rc != 0) {
> > -            virReportSystemError(-rc,
> > -                                 _("Unable to set memory hard limit for domain %s"),
> > -                                 vm->def->name);
> > -            goto cleanup;
> > -        }
> > -        if (vm->def->mem.soft_limit != 0) {
> > -            rc = virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit);
> > -            if (rc != 0) {
> > -                virReportSystemError(-rc,
> > -                                     _("Unable to set memory soft limit for domain %s"),
> > -                                     vm->def->name);
> > -                goto cleanup;
> > -            }
> > -        }
> > -
> > -        if (vm->def->mem.swap_hard_limit != 0) {
> > -            rc = virCgroupSetMemSwapHardLimit(priv->cgroup, vm->def->mem.swap_hard_limit);
> > -            if (rc != 0) {
> > -                virReportSystemError(-rc,
> > -                                     _("Unable to set swap hard limit for domain %s"),
> > -                                     vm->def->name);
> > -                goto cleanup;
> > -            }
> > -        }
> > -    } else if (vm->def->mem.hard_limit != 0 ||
> > -               vm->def->mem.soft_limit != 0 ||
> > -               vm->def->mem.swap_hard_limit != 0) {
> > -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                       _("Memory cgroup is not available on this host"));
> > -    } else {
> > -        VIR_WARN("Could not autoset a RSS limit for domain %s", vm->def->name);
> > -    }
> > +    if (qemuSetupMemoryCgroup(vm) < 0)
> > +        goto cleanup;
> ACK with warning removed
Pushed. Thanks.

Osier

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