From: james.morse@arm.com (James Morse)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack
Date: Thu, 15 Oct 2015 16:59:21 +0100 [thread overview]
Message-ID: <561FCD59.1090600@arm.com> (raw)
In-Reply-To: <B6AB90B9-3AA9-413A-B357-137710E89C7C@gmail.com>
On 14/10/15 13:12, Jungseok Lee wrote:
> On Oct 14, 2015, at 12:00 AM, Jungseok Lee wrote:
>> On Oct 13, 2015, at 8:00 PM, James Morse wrote:
>>> On 12/10/15 23:13, Jungseok Lee wrote:
>>>> On Oct 13, 2015, at 1:34 AM, James Morse wrote:
>>>>> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
>>>>> (especially for systems with few cpus)?
>>>>
>>>> This would be a single concern. To address this issue, I drop the 'static'
>>>> keyword in thread_info_cache. Please refer to the below hunk.
>>>
>>> Its only a problem on systems with 64K pages, which don't have a multiple
>>> of 4 cpus. I suspect if you turn on 64K pages, you have many cores with
>>> plenty of memory?
>>
>> Yes, the problem 'two kmem_caches' comes from only 64K page system.
>>
>> I don't get the statement 'which don't have a multiple of 4 cpus'.
>> Could you point out what I am missing?
>
> You're talking about sl{a|u}b allocator behavior. If so, I got what you meant.
Yes,
With Nx4 cpus, the (currently) 16K irq stacks take up Nx64K - a nice
multiple of pages, so no wasted memory.
>>> If this has been made a published symbol, it should go in a header file.
>>
>> Sure.
>
> I had the wrong impression that there is a room under include/linux/*.
Yes, I see there isn't anywhere obvious to put it...
> IMO, this is architectural option whether arch relies on thread_info_cache or not.
> In other words, it would be clear to put this extern under arch/*/include/asm/*.
Its up to the arch whether or not to define
CONFIG_ARCH_THREAD_INFO_ALLOCATOR. In the case where it hasn't defined it,
and THREAD_SIZE >= PAGE_SIZE, your change is exposing thread_info_cache on
all architectures, so it ought go in a header file accessible to all
architectures.
Something like this, (only build-tested!):
=========%<=========
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -10,6 +10,8 @@
#include <linux/types.h>
#include <linux/bug.h>
+#include <asm/page.h>
+
struct timespec;
struct compat_timespec;
@@ -145,6 +147,12 @@ static inline bool test_and_clear_restore_sigmask(void)
#error "no set_restore_sigmask() provided and default one won't work"
#endif
+#ifndef CONFIG_ARCH_THREAD_INFO_ALLOCATOR
+#if THREAD_SIZE >= PAGE_SIZE
+extern struct kmem_cache *thread_info_cache;
+#endif /* THREAD_SIZE >= PAGE_SIZE */
+#endif /* CONFIG_ARCH_THREAD_INFO_ALLOCATOR */
+
#endif /* __KERNEL__ */
#endif /* _LINUX_THREAD_INFO_H */
=========%<=========
Quite ugly!
My concern is there could be push-back from the maintainer of
kernel/fork.c, saying "define CONFIG_ARCH_THREAD_INFO_ALLOCATOR if the
generic code isn't what you need", and push-back from the arm64 maintainers
about copy-pasting that chunk into arch/arm64.... both of which are fair,
hence my initial version created a second kmem_cache.
Thanks,
James
next prev parent reply other threads:[~2015-10-15 15:59 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-07 15:28 [PATCH v4 0/2] arm64: Introduce IRQ stack Jungseok Lee
2015-10-07 15:28 ` [PATCH v4 1/2] " Jungseok Lee
2015-10-08 10:25 ` Pratyush Anand
2015-10-08 14:32 ` Jungseok Lee
2015-10-08 16:51 ` Pratyush Anand
2015-10-07 15:28 ` [PATCH v4 2/2] arm64: Expand the stack trace feature to support " Jungseok Lee
2015-10-09 14:24 ` James Morse
2015-10-12 14:53 ` Jungseok Lee
2015-10-12 16:34 ` James Morse
2015-10-12 22:13 ` Jungseok Lee
2015-10-13 11:00 ` James Morse
2015-10-13 15:00 ` Jungseok Lee
2015-10-14 12:12 ` Jungseok Lee
2015-10-15 15:59 ` James Morse [this message]
2015-10-16 13:01 ` Jungseok Lee
2015-10-16 16:06 ` Catalin Marinas
2015-10-17 13:38 ` Jungseok Lee
2015-10-19 16:18 ` Catalin Marinas
2015-10-20 13:08 ` Jungseok Lee
2015-10-21 15:14 ` Jungseok Lee
2015-10-14 7:13 ` AKASHI Takahiro
2015-10-14 12:24 ` Jungseok Lee
2015-10-14 12:55 ` Jungseok Lee
2015-10-15 4:19 ` AKASHI Takahiro
2015-10-15 13:39 ` Jungseok Lee
2015-10-19 6:47 ` AKASHI Takahiro
2015-10-20 13:19 ` Jungseok Lee
2015-10-15 14:24 ` Jungseok Lee
2015-10-15 16:01 ` James Morse
2015-10-16 13:02 ` Jungseok Lee
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=561FCD59.1090600@arm.com \
--to=james.morse@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).