linux-kernel - Re: [PATCH -v9 00/31] use lmb with x86

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1269901944.2286.18.camel@concordia>
Date:	Tue, 30 Mar 2010 09:32:24 +1100
From:	Michael Ellerman <michael@...erman.id.au>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Ingo Molnar <mingo@...e.hu>, Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Miller <davem@...emloft.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH -v9 00/31] use lmb with x86

On Mon, 2010-03-29 at 15:17 -0700, Yinghai Lu wrote:
> On 03/29/2010 03:10 PM, Michael Ellerman wrote:
> > On Mon, 2010-03-29 at 09:52 -0700, Yinghai Lu wrote:
> >> On 03/29/2010 05:22 AM, Michael Ellerman wrote:
> >>> On Sun, 2010-03-28 at 19:42 -0700, Yinghai Lu wrote:
> >>>> the new lmb could be used to early_res in x86.
> >>>>
> >>>> Suggested by: David, Ben, and Thomas
> >>>>
> >>>> First three patches should go into 2.6.34
> >>>>
> >>>> -v6: change sequence as requested by Thomas
> >>>> -v7: seperate them to more patches
> >>>> -v8: add boundary checking to make sure not free partial page.
> >>>> -v9: use lmb_debug to control print out of reserve_lmb.
> >>>>      add e820 clean up, and e820 become __initdata
> >>>
> >>> Bike shedding perhaps, but can you maintain the naming convention, ie.
> >>> lmb_xxx() rather than xxx_lmb(). Neither is necessarily better, but all
> >>> the existing functions use the lmb_xxx() style.
> >>>
> >>
> >> so you want
> >>
> >> find_lmb_area ==> lmb_find_area
> >> reserve_lmb ==> lmb_reserve
> >> free_lmb ==> lmb_free
> >>
> >> first one is ok, 
> >>
> >> but next two we already have lmb_reserved and lmb_free without checking and increasing the size of region array.
> > 
> > That was the point of my other mail. We now have two lmb APIs, one which
> > checks if the array will overflow and one which doesn't. That seems like
> > a bad idea. Having one called lmb_free() and one called free_lmb() is
> > definitely a bad idea, because it's completely non obvious which one
> > caters for overflow.
> 
> I want to keep the affects to other lmb users to minium at first.

That's a good plan, but I don't think this is the nicest way to do it.

> and we can merge those functions later.
> 
> or you insist on merging them in this patchset?

No I don't insist.

I _suggest_ that if we want to avoid affecting existing lmb users, then
the checking logic should go into the existing API, but be #ifdef'ed for
now - eg. CONFIG_DYNAMIC_LMB or something. That way you avoid affecting
existing users (more or less), but you don't add a new API that you then
have to remove later.

Having said that I don't think it really does affect existing users that
much. We still have the statically defined region arrays, and they're
still the same size, so sparc and powerpc should never need to resize,
except on machines where we currently run out of space in the array
anyway.

cheers

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)