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

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]

Date:	Fri, 14 Mar 2008 03:50:57 +0100
From:	Johannes Weiner <hannes@...urebad.de>
To:	"Yinghai Lu" <yhlu.kernel@...il.com>
Cc:	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Ingo Molnar" <mingo@...e.hu>, "Andi Kleen" <ak@...e.de>,
	"Christoph Lameter" <clameter@....com>,
	linux-kernel@...r.kernel.org,
	"Yasunori Goto" <y-goto@...fujitsu.com>,
	"KAMEZAWA Hiroyuki" <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH] mm: make reserve_bootmem can crossed the nodes

Hi,

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

> On Thu, Mar 13, 2008 at 5:13 PM, Johannes Weiner <hannes@...urebad.de> wrote:
>> Hi,
>>
>>  "Yinghai Lu" <yhlu.kernel@...il.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
--
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/