Date: Mon, 1 Nov 2010 23:02:19 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: alc@freebsd.org, Attilio Rao <attilio@freebsd.org>, freebsd-current@freebsd.org, kib@freebsd.org, Andriy Gapon <avg@freebsd.org> Subject: Re: panic in uma_startup for many-core amd64 system Message-ID: <AANLkTi=aJ9KgyK87RP42ZrM8h012-uDso%2BmmTfz8Lfin@mail.gmail.com> In-Reply-To: <201011011514.50322.jhb@freebsd.org> References: <4C9B9B9C.6000807@freebsd.org> <4CBD40E2.7030507@freebsd.org> <AANLkTi=6aMwph7-mh4QUUUiiox2S%2BN86agZRgAVw6=QT@mail.gmail.com> <201011011514.50322.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Nov 1, 2010 at 8:14 PM, John Baldwin <jhb@freebsd.org> wrote:
> On Monday, November 01, 2010 1:09:22 pm Giovanni Trematerra wrote:
>> On Tue, Oct 19, 2010 at 8:55 AM, Andriy Gapon <avg@freebsd.org> wrote:
>> > on 19/10/2010 00:01 Giovanni Trematerra said the following:
>> >>
>> >> Your patch seems just a work around about initial slab size where the
>> >> keg is backed.
>> >
>> > Well, setting aside my confusion with the terminology - yes, the patch=
is
> just
>> > that, and precisely because I only tried to solve that particular prob=
lem.
>> >
>> >> Having dynamic slab sizes would allow to have the keg backed on a lar=
ger
> slab
>> >> without going OFFPAGE.
>> >
>> > I agree in principle.
>> > But without seeing code that implements that I can't guess if it would
> really be
>> > more efficient or more maintainable, i.e. more useful in general.
>> > Still a very good idea.
>> >
>>
>> Here the patch that was in my mind.
>> The patch doesn't implement dynamic slab size just allow
>> to have a multipage slab to back uma_zone objects.
>> I'm going to work more on the topic "dynamic slab size" soon.
>> I tested the patch on qemu with -smp 32.
>> I'm looking for real hw to test the patch on.
>>
>> here some interesting output:
>> qemulab# vmstat -z | more
>> ITEM =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 SIZE =A0LIMIT =A0 =A0 USED =A0 =
=A0 FREE =A0 =A0 =A0REQ FAIL SLEEP
>>
>> UMA Kegs: =A0 =A0 =A0 =A0 =A0 =A0 =A0 208, =A0 =A0 =A00, =A0 =A0 149, =
=A0 =A0 =A0 4, =A0 =A0 149, =A0 0, =A0 0
>> UMA Zones: =A0 =A0 =A0 =A0 =A0 =A0 4480, =A0 =A0 =A00, =A0 =A0 149, =A0 =
=A0 =A0 0, =A0 =A0 149, =A0 0, =A0 0
>> UMA Slabs: =A0 =A0 =A0 =A0 =A0 =A0 =A0568, =A0 =A0 =A00, =A0 =A0 836, =
=A0 =A0 =A0 4, =A0 =A01187, =A0 0, =A0 0
>> UMA RCntSlabs: =A0 =A0 =A0 =A0 =A0568, =A0 =A0 =A00, =A0 =A0 202, =A0 =
=A0 =A0 1, =A0 =A0 202, =A0 0, =A0 0
>> UMA Hash: =A0 =A0 =A0 =A0 =A0 =A0 =A0 256, =A0 =A0 =A00, =A0 =A0 =A0 2, =
=A0 =A0 =A013, =A0 =A0 =A0 3, =A0 0, =A0 0
>>
>> qemulab# sysctl kern | grep cpu
>> kern.ccpu: 0
>> =A0 <cpu count=3D"32" mask=3D"0xffffffff">0, 1, 2, 3, 4, 5, 6, 7, 8, 9, =
10,
>> 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27,
>> 28, 29, 30, 31</cpu>
>> kern.smp.cpus: 32
>> kern.smp.maxcpus: 32
>>
>> Any feedback will be appreciate.
>>
>> --
>> Giovanni Trematerra
>>
>>
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> diff -r 36572cbc6817 sys/vm/uma_core.c
>> --- a/sys/vm/uma_core.c =A0 =A0 =A0 Tue Oct 05 04:49:04 2010 -0400
>> +++ b/sys/vm/uma_core.c =A0 =A0 =A0 Mon Nov 01 11:54:38 2010 -0400
>> @@ -930,27 +930,36 @@ startup_alloc(uma_zone_t zone, int bytes
>> =A0{
>> =A0 =A0 =A0 uma_keg_t keg;
>> =A0 =A0 =A0 uma_slab_t tmps;
>> -
>> - =A0 =A0 keg =3D zone_first_keg(zone);
>> + =A0 =A0 u_int16_t pages;
>> +
>> + =A0 =A0 keg =A0 =3D zone_first_keg(zone);
>> + =A0 =A0 pages =3D bytes / PAGE_SIZE;
>> +
>> + =A0 =A0 /* Account for remainder */
>> + =A0 =A0 if ((pages * PAGE_SIZE) < bytes)
>> + =A0 =A0 =A0 =A0 =A0 =A0 pages++;
>> + =A0 =A0 KASSERT(pages > 0, ("startup_alloc can't reserve 0 pages\n"));
>
> You can use 'pages =3D howmany(bytes, PAGE_SIZE)' here.
Thanks for the hint.
> Also, why did you make
> pages a uint16_t instead of an int? =A0An int is generally more convenien=
t
> unless you really need a uint16_t (and C99 spells it without an _ after t=
he
> leading 'u'.. FYI).
Uhm just to be coherent with field uk_ppera of struct keg, but I think
I can just use an int.
BTW is new code supposed to use C99 form even if the rest of the file
use u_int* form?
>
>> =A0 =A0 =A0 /*
>> =A0 =A0 =A0 =A0* Check our small startup cache to see if it has pages re=
maining.
>> =A0 =A0 =A0 =A0*/
>> =A0 =A0 =A0 mtx_lock(&uma_boot_pages_mtx);
>> - =A0 =A0 if ((tmps =3D LIST_FIRST(&uma_boot_pages)) !=3D NULL) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 LIST_REMOVE(tmps, us_link);
>> + =A0 =A0 do {
>> + =A0 =A0 =A0 =A0 =A0 =A0 if ((tmps =3D LIST_FIRST(&uma_boot_pages)) !=
=3D NULL)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 LIST_REMOVE(tmps, us_link);
>> + =A0 =A0 } while (--pages && tmps !=3D NULL);
>> + =A0 =A0 if (tmps !=3D NULL) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtx_unlock(&uma_boot_pages_mtx);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 *pflag =3D tmps->us_flags;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return (tmps->us_data);
>> - =A0 =A0 }
>> + =A0 =A0 } else if (booted =3D=3D 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 panic("UMA: Increase vm.boot_pages");
>> =A0 =A0 =A0 mtx_unlock(&uma_boot_pages_mtx);
>> - =A0 =A0 if (booted =3D=3D 0)
>> - =A0 =A0 =A0 =A0 =A0 =A0 panic("UMA: Increase vm.boot_pages");
>
> Probably best to make the pages test here explicit. =A0Also, is there any=
reason
> you can't do this as:
>
> =A0 =A0 =A0 =A0while (--pages > 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tmps =3D LIST_FIRST(&uma_boot_pages);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (tmps !=3D NULL)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0LIST_REMOVE(tmps, us_link)=
;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else if (booted =3D=3D 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0panic(...);
> =A0 =A0 =A0 =A0}
>
Well, no, even if I'll need to initialize tmps to NULL otherwise the
compiler will
raise a warning.
do {} while(); might be still better than a while(){}. bytes parameter
will never be
zero so pages will always be at least one and KASSERT will catch some
wired behavior.
Anyway that looks to me more readable, thanks. I could add an "else
break;" just in
the few cases that "pages" is still > 0 and tmps =3D=3D NULL, that could
be useless though.
> One question btw, how does this work since if you need to allocate more t=
han 1
> page it seems that the 'tmps' values for all but the last are simply igno=
red
> and leaked?
When you extract one item from the list you have tmps->us_data
pointing to start address of the memory page. The pages are contiguous
in decrescent
order of address (see below) so when you extract 2 items the last one
will point at
the start address of 2 contiguous pages of memory, just what I need to retu=
rn.
>
> Is there some unwritten assumption that the free pages are all virtually
> contiguous (and in order), so you can just pull off a run of X and use
> the address from X?
>
sys/vm/vm_page.c:351 @@ vm_page_startup(vm_offset_t vaddr)
/*
* Allocate memory for use when boot strapping the kernel memory
* allocator.
*/
new_end =3D end - (boot_pages * UMA_SLAB_SIZE);
new_end =3D trunc_page(new_end);
mapped =3D pmap_map(&vaddr, new_end, end,
VM_PROT_READ | VM_PROT_WRITE);
bzero((void *)mapped, end - new_end);
uma_startup((void *)mapped, boot_pages);
--------
sys/vm/uma_core.c:1683 @@ uma_startup(void *bootmem, int boot_pages)
#ifdef UMA_DEBUG
printf("Filling boot free list.\n");
#endif
for (i =3D 0; i < boot_pages; i++) {
slab =3D (uma_slab_t)((u_int8_t *)bootmem + (i * UMA_SLAB_SIZE));
slab->us_data =3D (u_int8_t *)slab;
slab->us_flags =3D UMA_SLAB_BOOT;
LIST_INSERT_HEAD(&uma_boot_pages, slab, us_link);
}
So if I'm not wrong I'd say that the pages are virtually contiguous.
Just for the record, attilio@ made a light review of this patch in private.
--
Giovanni Trematerra
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=aJ9KgyK87RP42ZrM8h012-uDso%2BmmTfz8Lfin>