This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


[Xen-devel] Re: [PATCH R4 6/7] mm: Extend memory hotplug API to allow me

On Tue, Mar 08, 2011 at 03:51:12PM -0800, Dave Hansen wrote:
> On Tue, 2011-03-08 at 22:50 +0100, Daniel Kiper wrote:
> > +int add_virtual_memory(u64 *size)
> > +{
> > +   int nid;
> > +   u64 start;
> > +
> > +   start = PFN_PHYS(SECTION_ALIGN(max_pfn));
> > +   *size = (((*size >> PAGE_SHIFT) & PAGE_SECTION_MASK) + 
> Why use PFN_PHYS() in one case but not the other?

I know that this is the same, however, I think PFN_PHYS() usage suggest
that I do a PFN/address manipulation. It is not true in that case (I do
an operation on region size) and I would like to avoid that ambiguity.

> I'd also highly suggest using the ALIGN() macro in cases like this.  It
> makes it much more readable:


>       *size = PFN_PHYS(ALIGN(*size, SECTION_SIZE)));
> > +   nid = memory_add_physaddr_to_nid(start);
> > +
> > +   return add_memory(nid, start, *size);
> > +}
> Could you talk a little bit more about how 'size' gets used?  Also, are
> we sure we want an interface where we're so liberal with 'size'?  It
> seems like requiring that it be section-aligned is a fair burden to
> place on the caller.  That way, we're not in a position of _guessing_
> what the caller wants (aligning up or down).

I do not have like this function since I created it. However,
I decided to sent it for review. It does not simplify anything
(add_memory() as a generic function is sufficient) and it is
too inflexible. Now, I am sure that everything in its body
should be moved to platform specific module (in that case Xen).
I am going to that on next patch release.


Xen-devel mailing list

<Prev in Thread] Current Thread [Next in Thread>