bootmem allocator

bootmem allocator

[Cyrill Gorcunov]

Hi Ingo, Peter,

small question. It was a patch recently posted which removes
memset(x, 0, x) after __alloc_bootmem call. There are a few another
code snippets who still call memset(x, 0, x). And who is responsible for
memory clearing? bootmem allocator or caller?

		- Cyrill -

Re: bootmem allocator

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> Hi Ingo, Peter,
> 
> small question. It was a patch recently posted which removes memset(x, 
> 0, x) after __alloc_bootmem call. There are a few another code 
> snippets who still call memset(x, 0, x). And who is responsible for 
> memory clearing? bootmem allocator or caller?

hm, bootmem allocator is supposed to clear memory. We have a couple of 
places that rely on that.

	Ingo

Re: bootmem allocator

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> writes:

> * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
>> Hi Ingo, Peter,
>> 
>> small question. It was a patch recently posted which removes memset(x, 
>> 0, x) after __alloc_bootmem call. There are a few another code 
>> snippets who still call memset(x, 0, x). And who is responsible for 
>> memory clearing? bootmem allocator or caller?
>
> hm, bootmem allocator is supposed to clear memory. We have a couple of 
> places that rely on that.

I was actually considering to change that for the GB pages hugetlbfs
patchkit, because memset for 1G is a little slow and not needed (will be cleared later
anyways) and it might be a problem for very large systems with a lot of such
pages at boot.

-Andi

Re: bootmem allocator

[Yinghai Lu]

On Mon, Apr 7, 2008 at 12:44 PM, Andi Kleen <andi@firstfloor.org> wrote:
>
> Ingo Molnar <mingo@elte.hu> writes:
>
>  > * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>  >
>  >> Hi Ingo, Peter,
>  >>
>  >> small question. It was a patch recently posted which removes memset(x,
>  >> 0, x) after __alloc_bootmem call. There are a few another code
>  >> snippets who still call memset(x, 0, x). And who is responsible for
>  >> memory clearing? bootmem allocator or caller?
>  >
>  > hm, bootmem allocator is supposed to clear memory. We have a couple of
>  > places that rely on that.
>
>  I was actually considering to change that for the GB pages hugetlbfs
>  patchkit, because memset for 1G is a little slow and not needed (will be cleared later
>  anyways) and it might be a problem for very large systems with a lot of such
>  pages at boot.

add another zalloc_bootmem?

YH

Re: bootmem allocator

[Cyrill Gorcunov]

On Tue, Apr 8, 2008 at 1:45 AM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
> On Mon, Apr 7, 2008 at 12:44 PM, Andi Kleen <andi@firstfloor.org> wrote:
>  >
>  > Ingo Molnar <mingo@elte.hu> writes:
>  >
>  >  > * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>  >  >
>  >  >> Hi Ingo, Peter,
>  >  >>
>  >  >> small question. It was a patch recently posted which removes memset(x,
>  >  >> 0, x) after __alloc_bootmem call. There are a few another code
>  >  >> snippets who still call memset(x, 0, x). And who is responsible for
>  >  >> memory clearing? bootmem allocator or caller?
>  >  >
>  >  > hm, bootmem allocator is supposed to clear memory. We have a couple of
>  >  > places that rely on that.
>  >
>  >  I was actually considering to change that for the GB pages hugetlbfs
>  >  patchkit, because memset for 1G is a little slow and not needed (will be cleared later
>  >  anyways) and it might be a problem for very large systems with a lot of such
>  >  pages at boot.
>
>  add another zalloc_bootmem?
>
>  YH
>

I think it would be a good idea ;) Btw maybe would be better to call
memset on the code witch relies on "clear" memory explicitly? So we will
clear memory allocated *only* if we really need this.

Cyrill

Re: bootmem allocator

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> I think it would be a good idea ;) Btw maybe would be better to call
> memset on the code witch relies on "clear" memory explicitly? So we will
> clear memory allocated *only* if we really need this.
> 

Are there any users of bootmem which will allocate a significant amount 
of memory and don't need it zeroed?

	-hpa

Re: bootmem allocator

[Cyrill Gorcunov]

On Tue, Apr 8, 2008 at 8:33 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> Cyrill Gorcunov wrote:
>
> >
> > I think it would be a good idea ;) Btw maybe would be better to call
> > memset on the code witch relies on "clear" memory explicitly? So we will
> > clear memory allocated *only* if we really need this.
>
>  Are there any users of bootmem which will allocate a significant amount of
> memory and don't need it zeroed?
>
>         -hpa
>

Actually, I don't know without reading the code... will take a look
today evening
(have no access to sources in my office).

Re: bootmem allocator

[Cyrill Gorcunov]

[H. Peter Anvin - Mon, Apr 07, 2008 at 09:33:23PM -0700]
> Cyrill Gorcunov wrote:
>> I think it would be a good idea ;) Btw maybe would be better to call
>> memset on the code witch relies on "clear" memory explicitly? So we will
>> clear memory allocated *only* if we really need this.
>
> Are there any users of bootmem which will allocate a significant amount of 
> memory and don't need it zeroed?
>
> 	-hpa
>

Well, the only really significant allocation I found is in
arch/ia64/kernel/iosapi.c:

---[...]---
static struct iosapic_rte_info * __init_refok iosapic_alloc_rte (void)
{
	int i;
	struct iosapic_rte_info *rte;
	int preallocated = 0;

	if (!iosapic_kmalloc_ok && list_empty(&free_rte_list)) {
--->		rte = alloc_bootmem(sizeof(struct iosapic_rte_info) *
				    NR_PREALLOCATE_RTE_ENTRIES);
		if (!rte)
			return NULL;
		for (i = 0; i < NR_PREALLOCATE_RTE_ENTRIES; i++, rte++)
			list_add(&rte->rte_list, &free_rte_list);
	}

	if (!list_empty(&free_rte_list)) {
		rte = list_entry(free_rte_list.next, struct iosapic_rte_info,
				 rte_list);
		list_del(&rte->rte_list);
		preallocated++;
	} else {
		rte = kmalloc(sizeof(struct iosapic_rte_info), GFP_ATOMIC);
		if (!rte)
			return NULL;
	}

	memset(rte, 0, sizeof(struct iosapic_rte_info));
	if (preallocated)
		rte->flags |= RTE_PREALLOCATED;

	return rte;
}
---[...]---

but it requires zeroed memory too. So, no, I didn't found any large
number of bytes allocated by bootmem scheme without needing of its
clearing.

		- Cyrill -

Re: bootmem allocator

[Ingo Molnar]

* Andi Kleen <andi@firstfloor.org> wrote:

> > hm, bootmem allocator is supposed to clear memory. We have a couple 
> > of places that rely on that.
> 
> I was actually considering to change that for the GB pages hugetlbfs 
> patchkit, because memset for 1G is a little slow and not needed (will 
> be cleared later anyways) and it might be a problem for very large 
> systems with a lot of such pages at boot.

changing the default behavior of bootmem alloc to be non-clearing is a 
really bad idea that will only cause unrobustness. The proper approach 
is to add an _opt-in_ API that does not clear memory 
(bootmem_alloc_dontclear() or whatever), available to callers that know 
it for sure that they dont need the clearing.

	Ingo

Re: bootmem allocator

[Andi Kleen]

On Tue, Apr 08, 2008 at 10:04:46AM +0200, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > > hm, bootmem allocator is supposed to clear memory. We have a couple 
> > > of places that rely on that.
> > 
> > I was actually considering to change that for the GB pages hugetlbfs 
> > patchkit, because memset for 1G is a little slow and not needed (will 
> > be cleared later anyways) and it might be a problem for very large 
> > systems with a lot of such pages at boot.
> 
> changing the default behavior of bootmem alloc to be non-clearing is a 
> really bad idea that will only cause unrobustness. The proper approach 
> is to add an _opt-in_ API that does not clear memory 

I was considering that too, but we have so many weird variants of bootmem
with opt in and opt out and even combinations of both now that the whole thing 
is starting to look really pear shaped (I admit I added some of them
in the past myself but I'm not proud). Would be a great project for
someone to consolidate that all a bit.

-Andi

Re: bootmem allocator

[Cyrill Gorcunov]

[Andi Kleen - Tue, Apr 08, 2008 at 10:12:06AM +0200]
| On Tue, Apr 08, 2008 at 10:04:46AM +0200, Ingo Molnar wrote:
| > 
| > * Andi Kleen <andi@firstfloor.org> wrote:
| > 
| > > > hm, bootmem allocator is supposed to clear memory. We have a couple 
| > > > of places that rely on that.
| > > 
| > > I was actually considering to change that for the GB pages hugetlbfs 
| > > patchkit, because memset for 1G is a little slow and not needed (will 
| > > be cleared later anyways) and it might be a problem for very large 
| > > systems with a lot of such pages at boot.
| > 
| > changing the default behavior of bootmem alloc to be non-clearing is a 
| > really bad idea that will only cause unrobustness. The proper approach 
| > is to add an _opt-in_ API that does not clear memory 
| 
| I was considering that too, but we have so many weird variants of bootmem
| with opt in and opt out and even combinations of both now that the whole thing 
| is starting to look really pear shaped (I admit I added some of them
| in the past myself but I'm not proud). Would be a great project for
| someone to consolidate that all a bit.
| 
| -Andi
| 

OK, thanks to all of you

		- Cyrill -

Re: bootmem allocator

[Cyrill Gorcunov]

[Andi Kleen - Tue, Apr 08, 2008 at 10:12:06AM +0200]
| On Tue, Apr 08, 2008 at 10:04:46AM +0200, Ingo Molnar wrote:
| > 
| > * Andi Kleen <andi@firstfloor.org> wrote:
| > 
| > > > hm, bootmem allocator is supposed to clear memory. We have a couple 
| > > > of places that rely on that.
| > > 
| > > I was actually considering to change that for the GB pages hugetlbfs 
| > > patchkit, because memset for 1G is a little slow and not needed (will 
| > > be cleared later anyways) and it might be a problem for very large 
| > > systems with a lot of such pages at boot.
| > 
| > changing the default behavior of bootmem alloc to be non-clearing is a 
| > really bad idea that will only cause unrobustness. The proper approach 
| > is to add an _opt-in_ API that does not clear memory 
| 
| I was considering that too, but we have so many weird variants of bootmem
| with opt in and opt out and even combinations of both now that the whole thing 
| is starting to look really pear shaped (I admit I added some of them
| in the past myself but I'm not proud). Would be a great project for
| someone to consolidate that all a bit.
| 
| -Andi
| 

Andi, could you a bit clarify what exactly do you mean?

		- Cyrill -

Re: bootmem allocator

[H. Peter Anvin]

Andi Kleen wrote:
> On Tue, Apr 08, 2008 at 10:04:46AM +0200, Ingo Molnar wrote:
>> * Andi Kleen <andi@firstfloor.org> wrote:
>>
>>>> hm, bootmem allocator is supposed to clear memory. We have a couple 
>>>> of places that rely on that.
>>> I was actually considering to change that for the GB pages hugetlbfs 
>>> patchkit, because memset for 1G is a little slow and not needed (will 
>>> be cleared later anyways) and it might be a problem for very large 
>>> systems with a lot of such pages at boot.
>> changing the default behavior of bootmem alloc to be non-clearing is a 
>> really bad idea that will only cause unrobustness. The proper approach 
>> is to add an _opt-in_ API that does not clear memory 
> 
> I was considering that too, but we have so many weird variants of bootmem
> with opt in and opt out and even combinations of both now that the whole thing 
> is starting to look really pear shaped (I admit I added some of them
> in the past myself but I'm not proud). Would be a great project for
> someone to consolidate that all a bit.
> 

True, but if the gbpages hugetlbfs case is the *only* case which wants a 
nonclearing interface (which it sounds like it is), then it seems like a 
really bad idea to change the default.

hugetlbfs is a pretty special case.

	-hpa

Re: bootmem allocator

[Mike Travis]

Ingo Molnar wrote:
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
>>> hm, bootmem allocator is supposed to clear memory. We have a couple 
>>> of places that rely on that.
>> I was actually considering to change that for the GB pages hugetlbfs 
>> patchkit, because memset for 1G is a little slow and not needed (will 
>> be cleared later anyways) and it might be a problem for very large 
>> systems with a lot of such pages at boot.
> 
> changing the default behavior of bootmem alloc to be non-clearing is a 
> really bad idea that will only cause unrobustness. The proper approach 
> is to add an _opt-in_ API that does not clear memory 
> (bootmem_alloc_dontclear() or whatever), available to callers that know 
> it for sure that they dont need the clearing.

Yes, changing the default of bootmem_alloc is a bad idea.  I just changed
a bunch of static arrays to bootmem alloc's and it was pointed out early
that not only does bootmem_alloc clear memory, but also panics if it's not
available.

> Also take into account that alloc_bootmem_low() :
> - calls panic() if not enough memory
> - already clears allocated memory
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

A specialized call to bootmem_alloc probably needs it's own API... ;-)

Thanks,
Mike

Re: bootmem allocator

[Andi Kleen]

On Tue, Apr 08, 2008 at 11:00:28PM -0700, Mike Travis wrote:
> Ingo Molnar wrote:
> > * Andi Kleen <andi@firstfloor.org> wrote:
> > 
> >>> hm, bootmem allocator is supposed to clear memory. We have a couple 
> >>> of places that rely on that.
> >> I was actually considering to change that for the GB pages hugetlbfs 
> >> patchkit, because memset for 1G is a little slow and not needed (will 
> >> be cleared later anyways) and it might be a problem for very large 
> >> systems with a lot of such pages at boot.
> > 
> > changing the default behavior of bootmem alloc to be non-clearing is a 
> > really bad idea that will only cause unrobustness. The proper approach 
> > is to add an _opt-in_ API that does not clear memory 
> > (bootmem_alloc_dontclear() or whatever), available to callers that know 
> > it for sure that they dont need the clearing.
> 
> Yes, changing the default of bootmem_alloc is a bad idea.  I just changed
> a bunch of static arrays to bootmem alloc's and it was pointed out early
> that not only does bootmem_alloc clear memory, but also panics if it's not
> available.

There are more and more bootmem calls that don't want the panic actually.
That is why _nopanic was invented (and gets more and more variants)
At some point the default could be even switched.

I think the right way would be to survey the callers (there are not
that many) and then come up with a sane single API that caters to the
majority of them by default and passes flags for the special cases.

-Andi

Re: bootmem allocator

[Mike Travis]

Andi Kleen wrote:
> On Tue, Apr 08, 2008 at 11:00:28PM -0700, Mike Travis wrote:
>> Ingo Molnar wrote:
>>> * Andi Kleen <andi@firstfloor.org> wrote:
>>>
>>>>> hm, bootmem allocator is supposed to clear memory. We have a couple 
>>>>> of places that rely on that.
>>>> I was actually considering to change that for the GB pages hugetlbfs 
>>>> patchkit, because memset for 1G is a little slow and not needed (will 
>>>> be cleared later anyways) and it might be a problem for very large 
>>>> systems with a lot of such pages at boot.
>>> changing the default behavior of bootmem alloc to be non-clearing is a 
>>> really bad idea that will only cause unrobustness. The proper approach 
>>> is to add an _opt-in_ API that does not clear memory 
>>> (bootmem_alloc_dontclear() or whatever), available to callers that know 
>>> it for sure that they dont need the clearing.
>> Yes, changing the default of bootmem_alloc is a bad idea.  I just changed
>> a bunch of static arrays to bootmem alloc's and it was pointed out early
>> that not only does bootmem_alloc clear memory, but also panics if it's not
>> available.
> 
> There are more and more bootmem calls that don't want the panic actually.
> That is why _nopanic was invented (and gets more and more variants)
> At some point the default could be even switched.
> 
> I think the right way would be to survey the callers (there are not
> that many) and then come up with a sane single API that caters to the
> majority of them by default and passes flags for the special cases.
> 
> -Andi

Hi Andi,

I really don't care(*), but there's lot's of code that expects a certain
behavior.  Either all the source calls have to be modified en masse (and you
well know that's difficult given the _zillion_ source trees), or you have to
introduce the new API transparently.  That means leaving a backdoor for old
calls:

     #define bootmem_alloc_low(...) \
	 __new_bootmem_alloc(..., FLAGS_FOR_OLD_BOOTMEM_ALLOC_LOW);

Then I think you're free to optimize away... d;-) [happy face with a baseball cap]

Cheers,
Mike

(*) As long as I don't have to debug problems as a result of the change...! ;-)