Re: [PATCH] mm: make reserve_bootmem can crossed the nodes

[Johannes Weiner <hannes@saeurebad.de>]

Hi,

"Yinghai Lu" <yhlu.kernel@gmail.com> writes:

> On Thu, Mar 13, 2008 at 5:13 PM, Johannes Weiner <hannes@saeurebad.de> wrote:
>> Hi,
>>
>>  "Yinghai Lu" <yhlu.kernel@gmail.com> writes:
>>
>
>>  >  static void __init free_bootmem_core(bootmem_data_t *bdata, unsigned long addr,
>>  > @@ -407,6 +432,11 @@ unsigned long __init init_bootmem_node(p
>>  >  void __init reserve_bootmem_node(pg_data_t *pgdat, unsigned long physaddr,
>>  >                                unsigned long size, int flags)
>>  >  {
>>  > +     int ret;
>>  > +
>>  > +     ret = can_reserve_bootmem_core(pgdat->bdata, physaddr, size, flags);
>>  > +     if (ret < 0)
>>  > +             return;
>>  >       reserve_bootmem_core(pgdat->bdata, physaddr, size, flags);
>>
>>  I don't get it.  Sorry.  What is the purpose of
>>  can_reserve_bootmem_core()?  It does exactly what reserve_bootmem_core
>>  does besides actually setting the bits.  All the pre-checking you wanted
>>  to have out of the way is repeated again in reserve_bootmem_core()
>>  (well, almost all).
>
> can_reserve_bootmem_core is check if there is some pages is reserved
> already with
>>  > +                     if (flags & BOOTMEM_EXCLUSIVE)
>>  > +                             return -EBUSY;
>
> so it will avoid the restoring later.

Yes, I understood that.  But you skipped the lower part of my email:

Your current state now is _not_ that you have one function that
prechecks the range and another function that reserves it!  You have
_two_ functions checking the range and the second reserving it.

Why double-check most of the things?  If you want to have a pre-check
function, _move_ all the pre-checks into another function, not
copy-paste them.

And is the condition of trying to reserve a range twice, the second time
exclusively, so common that it is worth iterating twice over the nodes
(once for checking, once for reserving) instead of just unwinding the
reservation if it fails in between?

On something else: is there a bug when a memory range is reserved with
BOOTMEM_EXCLUSIVE and then again without this flag?  The second call
does not return an error then.  Is my understanding of
BOOTMEM_EXCLUSIVE's semantics wrong?

	Hannes