Re: [PATCH] xen/balloon: Memory hotplug support for Xen balloondriver

[Dave Hansen]

On Tue, 2011-03-29 at 20:18 +0200, Daniel Kiper wrote:
> On Mon, Mar 28, 2011 at 08:55:27AM -0700, Dave Hansen wrote:
> > On Mon, 2011-03-28 at 11:47 +0200, Daniel Kiper wrote:
> > >
> > > +static enum bp_state reserve_additional_memory(long credit)
> > > +{
> > > +       int nid, rc;
> > > +       u64 start;
> > > +       unsigned long balloon_hotplug = credit;
> > > +
> > > +       start = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
> > > +       balloon_hotplug = (balloon_hotplug & PAGE_SECTION_MASK) + PAGES_PER_SECTION;
> > > +       nid = memory_add_physaddr_to_nid(start);
> >
> > Is the 'balloon_hotplug' calculation correct?  I _think_ you're trying
> > to round up to the SECTION_SIZE_PAGES.  But, if 'credit' was already
> > section-aligned I think you'll unnecessarily round up to the next
> > SECTION_SIZE_PAGES boundary.  Should it just be:
> >
> > 	balloon_hotplug = ALIGN(balloon_hotplug, PAGES_PER_SECTION);
> 
> Yes, you are right. I am wrong. I will correct that. However, as I said
> ealier I do not like ALIGN() in size context. For me ALIGN() is operation
> on an address which aligns this address to specified boundary. That is
> why I prefer use here open coded version (I agree that it is the same
> to ALIGN()). I think that ROUND() macro would be better in size context.
> However, I am not native english speaker and if I missed something correct
> me, please.

The only problem with open-coding it is that it's more likely to have
bugs.  But, sure, ROUND() sounds OK, as long as it does what you intend.
I'm still not quite sure what your intent here is, or in which direction
you're trying to round and why.

> > You might also want to consider some nicer units for those suckers.
> 
> What do you mind ??? I think that in that context PAGES_PER_SECTION
> is quite good.

Memory management code is tricky.  We keep addresses in many forms:
virtual addresses, physical addresses, pfns, 'struct page', etc...  I've
found it very useful in the past to ensure that I'm explicit about what
I'm dealing with among those.  

In this case, PAGES_PER_SECTION says that "balloon_hotplug" is intended
to be either a physical address or a page count.  But, that only says
what you're filling the variable with, not what you _intend_ it to
contain.

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/