Re: [Qemu-devel] [PATCH 2/2] Add monitor command mem-nodes

[Eduardo Habkost]

On Thu, Jun 06, 2013 at 05:30:16PM +0800, Wanlong Gao wrote:
> On 06/05/2013 11:54 PM, Eduardo Habkost wrote:
> > On Wed, Jun 05, 2013 at 07:57:42AM -0500, Anthony Liguori wrote:
> >> Wanlong Gao <address@hidden> writes:
> >>
> >>> Add monitor command mem-nodes to show the huge mapped
> >>> memory nodes locations.
> >>>
> >>> (qemu) info mem-nodes
> >>> /proc/14132/fd/13: 00002aaaaac00000-00002aaaeac00000: node0
> >>> /proc/14132/fd/13: 00002aaaeac00000-00002aab2ac00000: node1
> >>> /proc/14132/fd/14: 00002aab2ac00000-00002aab2b000000: node0
> >>> /proc/14132/fd/14: 00002aab2b000000-00002aab2b400000: node1
> >>
> >> This creates an ABI that we don't currently support.  Memory hotplug or
> >> a variety of things can break this mapping and then we'd have to provide
> >> an interface to describe that the mapping was broken.
> > 
> > What do you mean by "breaking this mapping", exactly? Would the backing
> > file of existing guest RAM ever change? (It would require a memory copy
> > from one file to another, why would QEMU ever do that?)
> > 
> >>
> >> Also, it only works with hugetlbfs which is probbably not widely used
> >> given the existance of THP.
> > 
> > Quoting yourself at
> > http://article.gmane.org/gmane.comp.emulators.kvm.devel/58227:
> > 
> >>> It's extremely likely that if you're doing NUMA pinning, you're also 
> >>> doing large pages via hugetlbfs.  numactl can already set policies for 
> >>> files in hugetlbfs so all you need to do is have a separate hugetlbfs 
> >>> file for each numa node.
> >>>
> >>> Then you have all the flexibility of numactl and you can implement node 
> >>> migration external to QEMU if you so desire.
> > 
> > And if we simply report where are the backing files and offsets being
> > used for guest RAM, one could simply use
> > 'numactl --file --offset --length', so we don't even need separate
> > files/mem-paths for each node.
> 
> Does "numactl" work after QEMU process mapped the hugetlbfs file?
> I'm afraid the mempolicy set after it will take no effect.

I was always expecting it to work. I will make some tests to find out.

> 
> And, if PCI-passthrough is used, direct-attached-device uses DMA transfer
> between device and qemu process. All pages of the guest will be pinned by 
> get_user_pages().
> 
> KVM_ASSIGN_PCI_DEVICE ioctl
>   kvm_vm_ioctl_assign_device()
>     =>kvm_assign_device()
>       => kvm_iommu_map_memslots()
>         => kvm_iommu_map_pages()
>            => kvm_pin_pages()
> 
> So, with direct-attached-device, all guest page's page count will be +1 and
> any page migration will not work. AutoNUMA won't too. Then any numa directions
> are also ignored.

We could have a new -mem-path-like option where we could tell QEMU
exactly which hugetlbfs/tmpfs files should be used for each memory
region. This way an external tool could create the files, set the policy
on them, and then tell QEMU to use them.


> 
> Mel? Andrea?
> 
> > 
> > Does THP work with tmpfs, already? If it does, people who doesn't want
> > hugetlbfs and want numa tuning to work with THP could just use tmpfs for
> > -mem-path.
> > 
> >>
> >> I had hoped that we would get proper userspace interfaces for describing
> >> memory groups but that appears to have stalled out.
> > 
> > I would love to have it. But while we don't have it, sharing the
> > tmpfs/hugetlbfs backing files seem to work just fine as a mechanism to
> > let other tools manipulate guest memory policy. We just need to let
> > external tools know where the backing files are.
> > 
> >>
> >> Does anyone know if this is still on the table?
> >>
> >> If we can't get a proper kernel interface, then perhaps we need to add
> >> full libnuma support but that would really be unfortunate...
> > 
> > Why isn't the "info mem-nodes" solution (I mean: not this version, but a
> > proper QMP version that exposes all the information we need) an option?
> > 
> 
> And the shortage of hugetlbfs is that we can't know how many virtual
> machines, so that we can't determine how many huge pages to be reserved
> for virtual machines.

Then we just need to have THP working on tmpfs, and people who don't
want hugetlbfs could use tmpfs for -mem-path.


> 
> In order to set numa mempolices on THP or normal memories, as Anthony said,
> I think we should add full libnuma support into QEMU, and allow manually
> set mempolices in the QEMU command line and through QEMU monitor after
> QEMU started. The external tools can't do exactly the right thing.
> 
> So IMO the right direction is the objected proposal
>  - Message-ID: <address@hidden>
>    http://article.gmane.org/gmane.comp.emulators.kvm.devel/57684
>  - Message-ID: <address@hidden>
>    http://article.gmane.org/gmane.comp.emulators.kvm.devel/58835
> 
> If you agree, I'll refact this patch set for review.

I believe there are many different ways to avoid having QEMU
reimplementing what numactl and other tools can already do, and we
should consider those solutions before going that way.


But my main question about this specific patch is: if you are not
exposing this information so other tools could set policy themselves and
QEMU can't set the policy itself yet, what's exactly the purpose of this
patch?

> 
> Thanks,
> Wanlong Gao
> 
> > 
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> >>
> >>>
> >>> Refer to the proposal of Eduardo and Daniel.
> >>> http://article.gmane.org/gmane.comp.emulators.kvm.devel/93476
> >>>
> >>> Signed-off-by: Wanlong Gao <address@hidden>
> >>> ---
> >>>  monitor.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 45 insertions(+)
> >>>
> >>> diff --git a/monitor.c b/monitor.c
> >>> index eefc7f0..85c865f 100644
> >>> --- a/monitor.c
> >>> +++ b/monitor.c
> >>> @@ -74,6 +74,10 @@
> >>>  #endif
> >>>  #include "hw/lm32/lm32_pic.h"
> >>>  
> >>> +#if defined(CONFIG_NUMA)
> >>> +#include <numaif.h>
> >>> +#endif
> >>> +
> >>>  //#define DEBUG
> >>>  //#define DEBUG_COMPLETION
> >>>  
> >>> @@ -1759,6 +1763,38 @@ static void mem_info(Monitor *mon, const QDict 
> >>> *qdict)
> >>>  }
> >>>  #endif
> >>>  
> >>> +#if defined(CONFIG_NUMA)
> >>> +static void mem_nodes(Monitor *mon, const QDict *qdict)
> >>> +{
> >>> +    RAMBlock *block;
> >>> +    int prevnode, node;
> >>> +    unsigned long long c, start, area;
> >>> +    int fd;
> >>> +    int pid = getpid();
> >>> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> >>> +        if (!(fd = block->fd))
> >>> +            continue;
> >>> +        prevnode = -1;
> >>> +        start = 0;
> >>> +        area = (unsigned long long)block->host;
> >>> +        for (c = 0; c < block->length; c += TARGET_PAGE_SIZE) {
> >>> +            if (get_mempolicy(&node, NULL, 0, c + block->host,
> >>> +                              MPOL_F_ADDR | MPOL_F_NODE) < 0)
> >>> +                continue;
> >>> +            if (node == prevnode)
> >>> +                continue;
> >>> +            if (prevnode != -1)
> >>> +                monitor_printf(mon, "/proc/%d/fd/%d: %016Lx-%016Lx: 
> >>> node%d\n",
> >>> +                               pid, fd, start + area, c + area, 
> >>> prevnode);
> >>> +            prevnode = node;
> >>> +            start = c;
> >>> +         }
> >>> +         monitor_printf(mon, "/proc/%d/fd/%d: %016Lx-%016Lx: node%d\n",
> >>> +                        pid, fd, start + area, c + area, prevnode);
> >>> +    }
> >>> +}
> >>> +#endif
> >>> +
> >>>  #if defined(TARGET_SH4)
> >>>  
> >>>  static void print_tlb(Monitor *mon, int idx, tlb_t *tlb)
> >>> @@ -2567,6 +2603,15 @@ static mon_cmd_t info_cmds[] = {
> >>>          .mhandler.cmd = mem_info,
> >>>      },
> >>>  #endif
> >>> +#if defined(CONFIG_NUMA)
> >>> +    {
> >>> +        .name       = "mem-nodes",
> >>> +        .args_type  = "",
> >>> +        .params     = "",
> >>> +        .help       = "show the huge mapped memory nodes location",
> >>> +        .mhandler.cmd = mem_nodes,
> >>> +    },
> >>> +#endif
> >>>      {
> >>>          .name       = "mtree",
> >>>          .args_type  = "",
> >>> -- 
> >>> 1.8.3.rc2.10.g0c2b1cf
> >>
> > 
> 

-- 
Eduardo