Re: [libvirt] [PATCH v4] memtune: Let virsh know the unlimited value for memory tunables
2011/1/13 Nikunj A. Dadhania <nikunj(a)linux.vnet.ibm.com>:
On Wed, 12 Jan 2011 10:21:04 -0700, Eric Blake
<eblake(a)redhat.com> wrote:
> On 01/12/2011 12:56 AM, Nikunj A. Dadhania wrote:
> > Here is the patch, now the set calls are also ull.
> >
> > Still virCgroupGetMemoryUsage is not changed, this will require changes
> > in virDomainInfoPtr (info->memory). I am not sure if I should have them
> > in this patch.
>
> It can be a separate patch, if desired, but it is probably still needed.
>
virCgroupGetMemoryUsage is called only from lxcDomainGetInfo, that is
the reason I thought that this change may not be needed.
> > +++ b/tools/virsh.c
> > @@ -2987,9 +2987,14 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
> > params[i].value.l);
> > break;
> > case VIR_DOMAIN_MEMORY_PARAM_ULLONG:
> > - vshPrint(ctl, "%-15s: %llu\n", params[i].field,
> > - params[i].value.ul);
> > + {
> > + if (params[i].value.ul ==
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED)
> > + vshPrint(ctl, "%-15s: unlimited\n",
params[i].field);
> > + else
> > + vshPrint(ctl, "%-15s: %llu\n",
params[i].field,
> > + params[i].value.ul);
>
> Do we want any back-compat considerations? That is, if a newer virsh is
> talking to an older server, which still answered INT64_MAX>>10 instead
> of the new VIR_DOMAIN_MEMORY_PARAM_UNLIMITED, should we recognize that
> situation as another reason to print "unlimited"?
>
As Mattias suggested in the other mail, this adds more complications. My
take is to have VIR_DOMAIN_MEMORY_PARAM_UNLIMITED as the max value.
Here is the patch which adds setting as well as displaying the
"unlimited" values. Now in virsh to specify "unlimited" the user
would
need to pass "-1"
for example:
virsh # memtune lxcbb1 --hard-limit -1
From: Nikunj A. Dadhania <nikunj(a)linux.vnet.ibm.com>
Display and set unlimited when the memory cgroup settings. Unlimited is
represented by INT64_MAX in memory cgroup.
v4: Fix handling of setting unlimited values
v3: Make virCgroupSet memory call ull
Signed-off-by: Nikunj A. Dadhania <nikunj(a)linux.vnet.ibm.com>
Reported-by: Justin Clift <jclift(a)redhat.com>
---
include/libvirt/libvirt.h.in | 1
src/lxc/lxc_driver.c | 2 -
src/qemu/qemu_driver.c | 2 -
src/util/cgroup.c | 93 +++++++++++++++++++++++++++++++-----------
src/util/cgroup.h | 14 +++---
tools/virsh.c | 13 +++++-
6 files changed, 90 insertions(+), 35 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 3c6a54a..3ee47b9 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -696,6 +696,7 @@ typedef enum {
*/
#define VIR_DOMAIN_MEMORY_FIELD_LENGTH 80
+#define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED INT64_MAX
If I understand it correctly we currently use INT64_MAX>>10 to
indicate "unlimited". Therefore, we should define
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED to INT64_MAX>>10 as this would
preserve the existing behavior.
I think this patch started to fix the problem that virsh memtune
reports 9007199254740991 (== INT64_MAX>>10) when the value actually
means unlimited. We want virsh to report "unlimited" in this case.
As we cannot fix the problem in a better way (like using -1 for
unlimited) anymore, we try to workaround it:
First add a define VIR_DOMAIN_MEMORY_PARAM_UNLIMITED for the magic
value INT64_MAX>>10, but stick to this value, because changing it
could break existing applications.
Second make virsh memtune detect VIR_DOMAIN_MEMORY_PARAM_UNLIMITED and
print "unlimited" in that case instead of the actual numeric value.
Third make virsh memtune accept -1 as unlimited and translate it to
VIR_DOMAIN_MEMORY_PARAM_UNLIMITED.
You already addressed the second and third point in your patch so
we're close to a proper workaround.
Matthias