lkml.org 
[lkml]   [2010]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -v9 00/31] use lmb with x86
From
Date
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2010-03-30 00:35    [W:0.070 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site