linux-kernel - Re: [PATCH 1/2] Generic Virtual Memmap suport for SPARSEMEM

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:	Mon, 02 Apr 2007 14:22:04 -0700
From:	Dave Hansen <hansendc@...ibm.com>
To:	Christoph Lameter <clameter@....com>
Cc:	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	Martin Bligh <mbligh@...gle.com>, linux-mm@...ck.org,
	Andi Kleen <ak@...e.de>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH 1/2] Generic Virtual Memmap suport for SPARSEMEM

On Mon, 2007-04-02 at 14:00 -0700, Christoph Lameter wrote:
> On Mon, 2 Apr 2007, Dave Hansen wrote:
> > > +	} else
> > > +		return __alloc_bootmem_node(NODE_DATA(node), size, size,
> > > +					__pa(MAX_DMA_ADDRESS));
> > > +}
> > 
> > Hmmmmmmm.  Can we combine this with sparse_index_alloc()?  Also, why not
> > just use the slab for this?
> 
> Use a slab for page sized allocations? No.

Why not?  We use it above for sparse_index_alloc() and if it is doing
something wrong, I'd love to fix it.  Can you elaborate?

> > Let's get rid of the _block() part, too.  I'm not sure it does any good.
> > At least make it _bytes() so that we know what the units are.  Also, if
> > you're just going to round up internally and _not_ use the slab, can you
> > just make the argument in pages, or even order?
> 
> Its used for page sized allocations.

Ok, then let's make it take pages of some kind or its argument.  

> > Can you think of any times when we'd want that BUG_ON() to be a
> > WARN_ON(), instead?  I can see preferring having my mem_map[] on the
> > wrong node than hitting a BUG().
> 
> We should probably have some error handling there instead of the BUG.
> 
> > > +#ifndef ARCH_POPULATES_VIRTUAL_MEMMAP
> > > +/*
> > > + * Virtual memmap populate functionality for architectures that support
> > > + * PMDs for huge pages like i386, x86_64 etc.
> > > + */
> > 
> > How about:
> > 
> > /*
> >  * Virtual memmap support for architectures that use Linux pagetables
> >  * natively in hardware, and support mapping huge pages with PMD
> >  * entries.
> >  */
> > 
> > It wouldn't make sense to map the vmemmap area with Linux pagetables on
> > an arch that didn't use them in hardware, right?  So, perhaps this
> > doesn't quite belong in mm/sparse.c.  Perhaps we need
> > arch/x86/sparse.c. ;)
> 
> I just extended this in V2 to also work on IA64. Its pretty generic.

Can you extend it to work on ppc? ;)

You haven't posted V2, right?

> > 
> >          map = alloc_remap(nid, sizeof(struct page) * PAGES_PER_SECTION);
> >          if (map)
> >                  return map;
> > 
> > +        map = alloc_vmemmap(map, PAGES_PER_SECTION, nid);
> > +        if (map)
> > +                return map;
> > +
> >          map = alloc_bootmem_node(NODE_DATA(nid),
> >                          sizeof(struct page) * PAGES_PER_SECTION);
> >          if (map)
> >                  return map;
> > 
> > Then, do whatever magic you want in alloc_vmemmap().
> 
> That would break if alloc_vmemmap returns NULL because it cannot allocate 
> memory.

OK, that makes sense.  However, it would still be nice to hide that
#ifdef somewhere that people are a bit less likely to run into it.  It's
just one #ifdef, so if you can kill it, great.  Otherwise, they pile up
over time and _do_ cause real readability problems.

-- 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/