linux-kernel - Re: [PATCH] xen/balloon: Memory hotplug support for Xen balloon driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1301495954.21454.3788.camel@nimitz>
Date:	Wed, 30 Mar 2011 07:39:14 -0700
From:	Dave Hansen <dave@...ux.vnet.ibm.com>
To:	Daniel Kiper <dkiper@...-space.pl>
Cc:	ian.campbell@...rix.com, akpm@...ux-foundation.org,
	andi.kleen@...el.com, haicheng.li@...ux.intel.com,
	fengguang.wu@...el.com, jeremy@...p.org, konrad.wilk@...cle.com,
	dan.magenheimer@...cle.com, v.tolstov@...fip.ru, pasik@....fi,
	wdauchy@...il.com, rientjes@...gle.com,
	xen-devel@...ts.xensource.com, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH] xen/balloon: Memory hotplug support for Xen balloon
 driver

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/