Date: Fri, 3 Oct 2014 10:35:17 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Larry Baird <lab@gta.com> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org>, Ryan Stone <rysto32@gmail.com>, Dimitry Andric <dim@FreeBSD.org>, Bryan Drewery <bdrewery@FreeBSD.org> Subject: Re: Kernel/Compiler bug Message-ID: <20141003073517.GC26076@kib.kiev.ua> In-Reply-To: <20141003015456.GA27080@gta.com> References: <20141001031553.GA14360@gta.com> <CAFMmRNxAYcr8eEY0SJsX3zkRadjT29-mfsGcSTmG_Yx-Hidi6w@mail.gmail.com> <20141001134044.GA57022@gta.com> <FBB9E4C3-55B9-4917-9953-F8BC9AE43619@FreeBSD.org> <542C8C75.30007@FreeBSD.org> <20141002075537.GU26076@kib.kiev.ua> <20141002140232.GA52387@gta.com> <20141002143345.GY26076@kib.kiev.ua> <20141003015456.GA27080@gta.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Oct 02, 2014 at 09:54:56PM -0400, Larry Baird wrote:
> > The easiest thing to do is to record the stack depth for kernel mode
> > on entry into interrupt. Interrupt handlers are usually well written
> > and do not consume a lot of stack.
> >
> > Look at the intr_event_handle(), which is the entry point. The mode can
> > be deduced from trapframe passed. The kernel stack for the thread is
> > described by td->td_kstack (base, i.e. bottom) and td->td_kstack_pages
> > (size), so the top of the stack is at td_kstack + td_kstack_size [*].
> > The current stack consumption could be taken from reading %rsp register,
> > or you may take the address of any local variable as well.
> >
> > * - there are pcb and usermode fpu save area at the top of the stack, and
> > actual kernel stack top is right below fpu save area. This should not
> > be important for your measurements, since you are looking at how close
> > the %rsp gets to the bottom.
>
> This idea worked very well. Booting a GENERIC 10.1-BETA3 kernel I get a
> maximum stack used of 5247 bytes. This was with a minimal virtual box
> configuration. It would be interesting to hear about users with more exotic
> hardware and or configurations. Not sure if I have the KASSERT correct.
>
I have several notes. Mostly, it comes from my desire to make the patch
committable.
A global one is that the profiling of the stack use should be hidden
under some kernel config option.
>
>
> Index: kern_intr.c
> ===================================================================
> --- kern_intr.c (revision 44897)
> +++ kern_intr.c (working copy)
> @@ -1386,6 +1386,12 @@
> }
> }
>
> +static int max_kern_thread_stack;
Add 'usage' somewhere in the name of the var and sysctl ?
> +
> +SYSCTL_INT(_kern, OID_AUTO, max_kern_thread_stack, CTLFLAG_RD,
> + &max_kern_thread_stack, 0,
> + "Maxiumum stack used by a kernel thread");
> +
> /*
> * Main interrupt handling body.
> *
> @@ -1407,6 +1413,22 @@
>
> td = curthread;
>
> + /*
> + * Check for maximum stack used bya kernel thread.
> + */
> + if (!TRAPF_USERMODE(frame)) {
Just a note, the test for interruption of the usermode is not strictly
needed, it only optimizes the execution, since interrupt from usermode
would have only the trap frame on the stack. Might be, this should
be commented.
> + char *top = (char *)(td->td_kstack + td->td_kstack_pages *
> + PAGE_SIZE - 1);
> + char *current = (char *)&ih;
Use the address of top ? It should be deeper in the stack, and account
for the normal current function stack use, assuming compiler did not
flatten out the frame. Anyway, it assumes that the stack grows down.
Also, there are some situations, where the hardware might switch to
dedicated stack for interrupt handling. It is impossible right now
on amd64 and hw interrupts, but might become used in future, or on
other arches. It makes sense to check that current value falls into
the td stack region, before using it.
> + int used = top - current;
> +
> + if (used > max_kern_thread_stack) {
> + max_kern_thread_stack = used;
This should be a loop with atomic cas, to not loose update from other
thread.
> + KASSERT(max_kern_thread_stack < KSTACK_PAGES * PAGE_SIZE,
> + "Maximum kernel thread stack exxceeded");
Assert is not needed, we put a guard page below the thread stack to
catch the overflow. You have seen it already with double fault on x86.
> + }
> + }
> +
> /* An interrupt with no event or handlers is a stray interrupt. */
> if (ie == NULL || TAILQ_EMPTY(&ie->ie_handlers))
> return (EINVAL);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141003073517.GC26076>