Bug 523130

This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux major release.  Product Management has requested further
review of this request by Red Hat Engineering, for potential inclusion in a Red
Hat Enterprise Linux Major release.  This request is not yet committed for
inclusion.

Updates from bug 499644:

Comment 6 Miroslav Rezanina 2009-09-16 04:04:39 EDT
    
    Created an attachment (id=361207) [details]
    Returning memory  not contained in e820 map
    
    General solution - returns all not mapped memory.    
    
Comment 7 Justin M. Forbes 2009-10-07 12:44:41 EDT
    
    Grabbing this one to get the patch tested and into Fedora.    
    
Comment 8 Miroslav Rezanina 2009-10-08 01:02:52 EDT
   
    There should be optimized version of patch in Jeremy's repo. However, this
    version has wrong location of return_unused_memory - it is in
    xen_memory_setup not in xen_post_allocator_init.
    
    branch rebase/core-freemem branch at
    git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git    
    
Comment 9 Justin M. Forbes 2009-10-14 14:18:14 EDT
    
    Indeed, the version of the patch in jeremy's repo does not actually seem to
    return the unused memory.    
    
Comment 10 Jeremy Fitzhardinge 2009-10-14 14:31:24 EDT
    
    No, there's a problem with it.  Aside from releasing a bit too early, the 
    test to make sure the page being released actually belongs to the domain 
    seems to always return false, so it never wants to release anything.  I'm 
    not sure why; it looks OK to me, but I haven't investigated it in detail 
    yet.

Comment 11 Paolo Bonzini 2010-02-23 12:03:31 EST

    Created an attachment (id=395766) [details]
    patch as committed to jeremy's repo
    
    Attaching patch from Jeremy's repo for posterity.

An update from Jeremy was put on the Fedora counterpart for this bug.

"I just had another look at this and noticed a rather obvious bug in the patch:
"mfn = pfn_to_mfn(mfn);" rather than "mfn = pfn_to_mfn(*p*fn);".  Fixing this
makes it free memory properly as expected."

This patch still isn't in Linus' upstream, but it is (and the corrected version) in Jeremy's tree. It's not that big of a deal for rhel 6.0 though, since ballooning doesn't work. Moving to 6.1 and will backport the patch when getting ballooning to work.

(In reply to comment #4)
> Moving to 6.1 and will backport the patch when
> getting ballooning to work.    

What's wrong with ballooning?

(In reply to comment #5)
> (In reply to comment #4)
> > Moving to 6.1 and will backport the patch when
> > getting ballooning to work.    
> 
> What's wrong with ballooning?    

It hasn't ever worked for the kernels I've worked with since Fedora 11, and I just never got a chance to debug it. I haven't tried your tree in quite a while though, so I can certainly give it a go and see if it works. Or, I would happily take pointers to specific patch(es) that you know enabled it :-)

Thanks,
Andrew

Just for reference the ballooning bug is bug 523122. Jeremy has inspired me to take a look upstream now though, so I'm currently testing. I will update the ballooning bug with whatever I find out.

All upstream patches needed are

7243521 xen: release unused free memory
8b539bf xen: make sure pages are really part of domain before freeing
552e061 xen: fix scanning for freeable pages
fe4461d xen: little formatting fix
485c44e xen: use phys_addr_t for last_end to fix 32-bit crash
28c3e64 xen/core: don't bother trying to free pages beyond the ones Xen gave us

I'll post a patch soon that includes all those.

Created attachment 425155 [details]
return unused memory to dom0

I've attached a single patch that gets all the patches listed in comment 8. I've also tested it a bit and confirmed it works as well as upstream. The problem is neither seem to do anything for the test case of this bug.  With both upstream and rhel6 with this patch I get the following message on boot

released 0 pages of unused memory

and sure enough, we still can't balloon into what we would expect as unused memory. So the conditions to free memory don't seem to be getting met by this test case, or something doesn't work. I'll have to test more before posting it.

The poor bug already bounced 6.0 -> 6.1 -> 6.0, but seeing that even upstream refuses to release unused pages for our test case indicates to me that we need to look at this more closely before posting. The looking should be done during 6.1. Bouncing it again, 6.0 -> 6.1.

This request was evaluated by Red Hat Product Management for inclusion
in a Red Hat Enterprise Linux maintenance release. Product Management has 
requested further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed 
products. This request is not yet committed for inclusion in an Update release.

See bug 613606 comment #23 item 2.

Comment 8 patches in upstream kernel committed as:

093d7b4639951 xen: release unused free memory
f89e048e76da7 xen: make sure pages are really part of domain before freeing

After additional debugging these patches (comment 19) doesn't affect xen pv guest
in any way. Here is call order:
============
setup_arch ->
 -> setup_memory_map
   -> x86_init.resources.memory_setup == xen_memory_setup
           before xen_return_unused_memory call we have e820 map like this
           that contains all memory provide by xen:
             BIOS-provided physical RAM map:
               Xen: 0000000000000000 - 00000000000a0000 (usable)
               Xen: 00000000000a0000 - 0000000000100000 (reserved)
               Xen: 0000000000100000 - 0000000200000000 (usable
*      -> xen_return_unused_memory 

** -> parse_early_param
=============

*
code fragment:
@xen_return_unused_memory
        for (i = 0; i < e820->nr_map && last_end < max_addr; i++) {
                phys_addr_t end = e820->map[i].addr;
                end = min(max_addr, end);

                released += xen_release_chunk(last_end, end);
                last_end = e820->map[i].addr + e820->map[i].size;
        }
in best case is nop since xen_release_chunk will not do anything if end <= last_end and with our example map pairs (last_end, end) will look like:
  last_end   end
  0x0        0x0
  0xa0000    0xa0000
  0x100000   0x100000
i.e. last_end == end => xen_release_chunk is nop
in worst case xen_release_chunk may try to release not existed gap if e820 is sparse.
 
last fragment of xen_return_unused_memory
        if (last_end < max_addr) 
                released += xen_release_chunk(last_end, max_addr);

has sense only if
   last_end(e820 val) < max_addr (max hv provided addr)
and that could be only if "mem=" parameter was applied to e820 map.
However ** parse_early_param is called after x86_init.resources.memory_setup in setup_memory_map. So patches:
093d7b4639951 xen: release unused free memory
f89e048e76da7 xen: make sure pages are really part of domain before freeing
are effectively nop code now.

Problem exist not only in rhel6 code but in upstream as well.

Asked upstream, no answer so far.
However found in bug 499644 comment 4 a link
http://lists.xensource.com/archives/html/xen-devel/2009-08/threads.html#00573

Looks like upstream idea is unmap holes in e820 and they don't care about what
mem=xxx parameter in guest kernel says.

So propose to close bug as WONTFIX?