Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1483556 > unrolled thread

[PATCH] random: Fix kernel panic due to system_wq use before init

Back to article view | Back to linux.kernel

Contents

  [PATCH] random: Fix kernel panic due to system_wq use before init Waiman Long <Waiman.Long@hpe.com> - 2016-09-14 21:10 +0200
    Re: [PATCH] random: Fix kernel panic due to system_wq use before init Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-14 21:20 +0200
      Re: [PATCH] random: Fix kernel panic due to system_wq use before init Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-14 23:10 +0200
      Re: [PATCH] random: Fix kernel panic due to system_wq use before init Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-19 11:30 +0200
      Re: [PATCH] random: Fix kernel panic due to system_wq use before init Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-19 14:50 +0200
        Re: [PATCH] random: Fix kernel panic due to system_wq use before init Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-19 17:00 +0200
          Re: [PATCH] random: Fix kernel panic due to system_wq use before init Matt Fleming <matt@codeblueprint.co.uk> - 2016-09-20 16:10 +0200
    Re: [PATCH] random: Fix kernel panic due to system_wq use before init Linus Torvalds <torvalds@linux-foundation.org> - 2016-09-14 21:20 +0200
      Re: [PATCH] random: Fix kernel panic due to system_wq use before init Tejun Heo <tj@kernel.org> - 2016-09-14 22:00 +0200
        Re: [PATCH] random: Fix kernel panic due to system_wq use before init Tejun Heo <tj@kernel.org> - 2016-09-15 00:30 +0200

#1483556 — [PATCH] random: Fix kernel panic due to system_wq use before init

While booting a 4.8-rc6 kernel on a 16-socket 768-thread Broadwell-EX
system, the kernel panic'ed with the following log:

[   51.837010] BUG: unable to handle kernel NULL pointer dereference at 0000000000000102
[   51.845635] IP: [<ffffffff810a49d2>] __queue_work+0x32/0x420
[   52.004366] Call Trace:
[   52.007053]  <IRQ>
[   52.009171]  [<ffffffff810a4de7>] queue_work_on+0x27/0x40
[   52.015306]  [<ffffffff8146ebd7>] credit_entropy_bits+0x1d7/0x2a0
[   52.022002]  [<ffffffff8146f339>] ?  add_interrupt_randomness+0x1b9/0x210
[   52.029366]  [<ffffffff8146f339>] add_interrupt_randomness+0x1b9/0x210
[   52.036544]  [<ffffffff810ea670>] handle_irq_event_percpu+0x40/0x80
[   52.043430]  [<ffffffff810ea6eb>] handle_irq_event+0x3b/0x60
[   52.049655]  [<ffffffff810ede48>] handle_level_irq+0x88/0x100
[   52.055968]  [<ffffffff8103004b>] handle_irq+0xab/0x130
[   52.061708]  [<ffffffff81092861>] ? _local_bh_enable+0x21/0x50
[   52.068125]  [<ffffffff81037bf5>] ? __exit_idle+0x5/0x30
[   52.073965]  [<ffffffff816fc9ed>] do_IRQ+0x4d/0xd0
[   52.079229]  [<ffffffff816fa88c>] common_interrupt+0x8c/0x8c
[   52.085444]  <EOI>
[   52.087568]  [<ffffffff8106bea9>] ? try_to_free_pmd_page+0x9/0x40
[   52.094462]  [<ffffffff8106bea5>] ? try_to_free_pmd_page+0x5/0x40
[   52.101157]  [<ffffffff8106bf2a>] ?  __unmap_pmd_range.part.5+0x4a/0x70
[   52.108330]  [<ffffffff8106c330>] unmap_pmd_range+0x130/0x250
[   52.114644]  [<ffffffff8106cb7b>] __cpa_process_fault+0x47b/0x5a0
[   52.121339]  [<ffffffff8106d7cb>] __change_page_attr+0x78b/0x9e0
[   52.127946]  [<ffffffff81065e45>] ?  __raw_callee_save___native_queued_spin_unlock+0x15/0x30
[   52.137124]  [<ffffffff8106da98>] __change_page_attr_set_clr+0x78/0x300
[   52.144404]  [<ffffffff81229180>] ? __slab_alloc+0x4d/0x5c
[   52.150436]  [<ffffffff8106f15f>] kernel_map_pages_in_pgd+0x8f/0xd0
[   52.157333]  [<ffffffff81dc0fb0>] efi_setup_page_tables+0xcc/0x1d9
[   52.164124]  [<ffffffff81dc0a8d>] efi_enter_virtual_mode+0x35e/0x4af
[   52.171117]  [<ffffffff81d9e109>] start_kernel+0x41f/0x4c8
[   52.177142]  [<ffffffff81d9dad8>] ? set_init_arg+0x55/0x55
[   52.183168]  [<ffffffff81d9d120>] ?  early_idt_handler_array+0x120/0x120
[   52.190440]  [<ffffffff81d9d5d6>] x86_64_start_reservations+0x2a/0x2c
[   52.197516]  [<ffffffff81d9d724>] x86_64_start_kernel+0x14c/0x16f
[   52.204214] Code: 89 e5 41 57 41 56 49 89 f6 41 55 41 89 fd 41 54 49 89 d4 53 48 83 ec 10 89 7d d4 ff 14 25 a0 a7 c2 81 f6 c4 02 0f 85 0d 03 00 00 <41> f6 86 02 01 00 00 01 0f 85 ae 02 00 00 49 c7 c7 18 41 01 00
[   52.225516] RIP  [<ffffffff810a49d2>] __queue_work+0x32/0x420
[   52.231838]  RSP <ffff88bd7f403de8>
[   52.235667] CR2: 0000000000000102
[   52.239667] ---[ end trace 2ee7ea9d2908eb72 ]---
[   52.244743] Kernel panic - not syncing: Fatal exception in interrupt

Looking at the panic'ed instruction indicated that system_wq was
used by credit_entropy_bits() before it it was initialized in an
early_initcall.

This patch prevents the schedule_work() call from being made before
system_wq is initialized.

Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
---
 drivers/char/random.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 3efb3bf..3afc519 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -730,8 +730,12 @@ retry:
 		    r->entropy_total >= 2*random_read_wakeup_bits) {
 			struct entropy_store *other = &blocking_pool;
 
-			if (other->entropy_count <=
-			    3 * other->poolinfo->poolfracbits / 4) {
+			/*
+			 * We cannot call schedule_work() before system_wq
+			 * is initialized.
+			 */
+			if (system_wq && (other->entropy_count <=
+			    3 * other->poolinfo->poolfracbits / 4)) {
 				schedule_work(&other->push_work);
 				r->entropy_total = 0;
 			}
-- 
1.7.1

[toc] | [next] | [standalone]

#1483570

On Wed, Sep 14, 2016 at 12:14 PM, Waiman Long <waiman.long@hpe.com> wrote:
>
> In the stack backtrace above, the kernel hadn't even reached SMP boot after
> about 50s. That was extremely slow. I tried the 4.7.3 kernel and it booted
> up fine. So I suspect that there may be too many interrupts going on and it
> consumes most of the CPU cycles. The prime suspect is the random driver, I
> think.

Any chance of bisecting it at least partially? The random driver
doesn't do interrupts itself, it just gets called by other drivers
doing intterrupts. So if there are too many of them, that would be
something else..

               Linus

[toc] | [prev] | [next] | [standalone]

#1483665

On Wed, Sep 14, 2016 at 12:34 PM, Waiman Long <waiman.long@hpe.com> wrote:
>
> I can try, but the 16-socket system that I have at the moment takes a long
> time (more than an hour) for one shutdown-reboot cycle. It may not be really
> more interrupts in 4.8, it may be that the random driver just somehow run
> very slow on my test machine as it seems to have a major rewrite in the 4.8
> cycle.

Looking at the random driver updates since 4.7, the only thing I see
is that .crng_fast_load() for the chacha20 randomness. And that should
trigger only until it's been initialized, so the cost looks like it
should be limited.

Is there some fundamental reason you think it's the random driver?
Other than the oops? Because I'd be more inclined to suspect just some
apic issue or something, where an actual interrupt line ends up
screaming or whatever. Is this UV? There's also the CPU hotplug state
machine changes etc.

But a few rounds of bisecting should hopefully cut down on the
suspects a lot. A *full* bisect might be 16-17 rounds, but if you can
do just four or five rounds of bisection, that should still cut it
down from 14k commits to "only" several hundred..

               Linus

[toc] | [prev] | [next] | [standalone]

#1486285

On Sun, 18 Sep, at 11:09:08PM, Waiman Long wrote:
> 
> I have finally finished bisecting the problem. I was wrong in saying that
> the 4.7.3 kernel had no problem. It did have. There were some slight
> differences between the 4.8 and 4.7 kernel config files that I used. After
> some further testing, it was found that the bootup problem only happened
> when the following kernel config option was defined:
> 
> CONFIG_EFI_MIXED=y
> 
> Bisecting reviewed that the following 4.6 patch was the first patch that had
> this problem:
> 
> c9f2a9a65e4855b74d92cdad688f6ee4a1a323ff
> [PATCH] x86/efi: Hoist page table switching code into efi_call_virt()
> 
> I did testing on my test system with three different partition sizes:
> 1) 16-socket Broadwell-EX with 12TB memory
> 2) 8-socket Broadwell-EX with 6TB memory
> 3) 4-socket Broadwell-EX with 3TB memory
> 
> Only the 16-socket and 8-socket configurations had this problem. I am not
> sure if over 4TB of main memory is a factor or not.

Yes, I think it's a safe bet that the amount of main memory is a major
factor here. Thanks for the report.

The only real difference when CONFIG_EFI_MIXED is enabled for 64-bit
kernel and 64-bit firmware is that main memory will be mapped into the
EFI page tables at its physical addresses.

I'll go stare at the code and get back to you.

[toc] | [prev] | [next] | [standalone]

#1486431

On Sun, 18 Sep, at 11:09:08PM, Waiman Long wrote:
> On 09/14/2016 03:19 PM, Linus Torvalds wrote:
> >On Wed, Sep 14, 2016 at 12:14 PM, Waiman Long<waiman.long@hpe.com>  wrote:
> >>In the stack backtrace above, the kernel hadn't even reached SMP boot after
> >>about 50s. That was extremely slow. I tried the 4.7.3 kernel and it booted
> >>up fine. So I suspect that there may be too many interrupts going on and it
> >>consumes most of the CPU cycles. The prime suspect is the random driver, I
> >>think.
> >Any chance of bisecting it at least partially? The random driver
> >doesn't do interrupts itself, it just gets called by other drivers
> >doing intterrupts. So if there are too many of them, that would be
> >something else..
> >
> >                Linus
> 
> I have finally finished bisecting the problem. I was wrong in saying that
> the 4.7.3 kernel had no problem. It did have. There were some slight
> differences between the 4.8 and 4.7 kernel config files that I used. After
> some further testing, it was found that the bootup problem only happened
> when the following kernel config option was defined:
> 
> CONFIG_EFI_MIXED=y
 
Could you try this patch? It won't be the final version, because it
doesn't address the root cause of the crash, which looks like page
table corruption of some kind, but it should at least confirm that
this is the buggy code,

---

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 677e29e29473..8dd3784eb075 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -245,7 +245,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	 * text and allocate a new stack because we can't rely on the
 	 * stack pointer being < 4GB.
 	 */
-	if (!IS_ENABLED(CONFIG_EFI_MIXED))
+	if (!IS_ENABLED(CONFIG_EFI_MIXED) || efi_is_native())
 		return 0;
 
 	/*

[toc] | [prev] | [next] | [standalone]

#1486569

On Mon, 19 Sep, at 10:48:12AM, Waiman Long wrote:
> 
> With this patch applied, I am able to successfully boot both the 16-socket
> 12-TB and 8-socket 6TB configurations without problem.
> 
> Tested-by: Waiman Long <Waiman.Long@hpe.com>
 
Could you please show your dmesg after booting with efi=debug? The
part I'm interested in is the dump of the EFI memory map.

[toc] | [prev] | [next] | [standalone]

#1487380

On Mon, 19 Sep, at 01:09:22PM, Waiman Long wrote:
> On 09/19/2016 10:51 AM, Matt Fleming wrote:
> >On Mon, 19 Sep, at 10:48:12AM, Waiman Long wrote:
> >>With this patch applied, I am able to successfully boot both the 16-socket
> >>12-TB and 8-socket 6TB configurations without problem.
> >>
> >>Tested-by: Waiman Long<Waiman.Long@hpe.com>
> >
> >Could you please show your dmesg after booting with efi=debug? The
> >part I'm interested in is the dump of the EFI memory map.
> 
> Attached is the output of dmesg with efi=debug.

Bingo. There's some more type conversion bugs in the pat code, that's
the real bug.

Here's what I've got queued up, along with the first patch I sent.

----8<----
From e535ec0899d1fe52ec3a84c9bc03457ac67ad6f7 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt@codeblueprint.co.uk>
Date: Tue, 20 Sep 2016 14:26:21 +0100
Subject: [PATCH 1/2] x86/mm/pat: Prevent hang during boot when mapping pages

There's a mixture of signed 32-bit and unsigned 32-bit and 64-bit data
types used for keeping track of how many pages have been mapped.

This leads to hangs during boot when mapping large numbers of pages
(multiple terabytes, as reported by Waiman) because those values are
interpreted as being negative.

commit 742563777e8d ("x86/mm/pat: Avoid truncation when converting
cpa->numpages to address") fixed one of those bugs, but there is
another lurking in __change_page_attr_set_clr().

Additionally, the return value type for the populate_*() functions can
return negative values when a large number of pages have been mapped,
triggering the error paths even though no error occurred.

Consistently use 64-bit types on 64-bit platforms when counting pages.
Even in the signed case this gives us room for regions 8PiB
(pebibytes) in size whilst still allowing the usual negative value
error checking idiom.

Reported-by: Waiman Long <waiman.long@hpe.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
CC: Theodore Ts'o <tytso@mit.edu>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Scott J Norton <scott.norton@hpe.com>
Cc: Douglas Hatch <doug.hatch@hpe.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/mm/pageattr.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 849dc09fa4f0..e3353c97d086 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -917,11 +917,11 @@ static void populate_pte(struct cpa_data *cpa,
 	}
 }
 
-static int populate_pmd(struct cpa_data *cpa,
-			unsigned long start, unsigned long end,
-			unsigned num_pages, pud_t *pud, pgprot_t pgprot)
+static long populate_pmd(struct cpa_data *cpa,
+			 unsigned long start, unsigned long end,
+			 unsigned num_pages, pud_t *pud, pgprot_t pgprot)
 {
-	unsigned int cur_pages = 0;
+	long cur_pages = 0;
 	pmd_t *pmd;
 	pgprot_t pmd_pgprot;
 
@@ -991,12 +991,12 @@ static int populate_pmd(struct cpa_data *cpa,
 	return num_pages;
 }
 
-static int populate_pud(struct cpa_data *cpa, unsigned long start, pgd_t *pgd,
-			pgprot_t pgprot)
+static long populate_pud(struct cpa_data *cpa, unsigned long start, pgd_t *pgd,
+			 pgprot_t pgprot)
 {
 	pud_t *pud;
 	unsigned long end;
-	int cur_pages = 0;
+	long cur_pages = 0;
 	pgprot_t pud_pgprot;
 
 	end = start + (cpa->numpages << PAGE_SHIFT);
@@ -1052,7 +1052,7 @@ static int populate_pud(struct cpa_data *cpa, unsigned long start, pgd_t *pgd,
 
 	/* Map trailing leftover */
 	if (start < end) {
-		int tmp;
+		long tmp;
 
 		pud = pud_offset(pgd, start);
 		if (pud_none(*pud))
@@ -1078,7 +1078,7 @@ static int populate_pgd(struct cpa_data *cpa, unsigned long addr)
 	pgprot_t pgprot = __pgprot(_KERNPG_TABLE);
 	pud_t *pud = NULL;	/* shut up gcc */
 	pgd_t *pgd_entry;
-	int ret;
+	long ret;
 
 	pgd_entry = cpa->pgd + pgd_index(addr);
 
@@ -1327,7 +1327,8 @@ static int cpa_process_alias(struct cpa_data *cpa)
 
 static int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias)
 {
-	int ret, numpages = cpa->numpages;
+	unsigned long numpages = cpa->numpages;
+	int ret;
 
 	while (numpages) {
 		/*
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1483571

Ugh, I detest this patch.

My gut feeling is that a driver (even a fairly core one like the
random code) should not have to know these kinds of details like
"schedule_work() needs system_wq to have been initialized".

I'm wondering if we couldn't just initialize "system_wq" earlier.
Right now init_workqueues() is an "early_initcall()", so it's at the
same priority as a number of other random early initcalls. My gut
feeling is that it should be initialized even earlier, with the
scheduler.

Because dammit, we use "schedule_work()" as if it was a pretty core
scheduler thing, and having to have some odd knowledge of system_wq
initialization details in the rest of the kernel sounds really really
wrong.

I don't think the random code is at all special in maybe wanting to
schedule some work despite being an "early" initcall.

Adding Tejun to the cc, and quoting the whole email.

Tejun, comments?

             Linus

On Wed, Sep 14, 2016 at 12:03 PM, Waiman Long <Waiman.Long@hpe.com> wrote:
> While booting a 4.8-rc6 kernel on a 16-socket 768-thread Broadwell-EX
> system, the kernel panic'ed with the following log:
>
> [   51.837010] BUG: unable to handle kernel NULL pointer dereference at 0000000000000102
> [   51.845635] IP: [<ffffffff810a49d2>] __queue_work+0x32/0x420
> [   52.004366] Call Trace:
> [   52.007053]  <IRQ>
> [   52.009171]  [<ffffffff810a4de7>] queue_work_on+0x27/0x40
> [   52.015306]  [<ffffffff8146ebd7>] credit_entropy_bits+0x1d7/0x2a0
> [   52.022002]  [<ffffffff8146f339>] ?  add_interrupt_randomness+0x1b9/0x210
> [   52.029366]  [<ffffffff8146f339>] add_interrupt_randomness+0x1b9/0x210
> [   52.036544]  [<ffffffff810ea670>] handle_irq_event_percpu+0x40/0x80
> [   52.043430]  [<ffffffff810ea6eb>] handle_irq_event+0x3b/0x60
> [   52.049655]  [<ffffffff810ede48>] handle_level_irq+0x88/0x100
> [   52.055968]  [<ffffffff8103004b>] handle_irq+0xab/0x130
> [   52.061708]  [<ffffffff81092861>] ? _local_bh_enable+0x21/0x50
> [   52.068125]  [<ffffffff81037bf5>] ? __exit_idle+0x5/0x30
> [   52.073965]  [<ffffffff816fc9ed>] do_IRQ+0x4d/0xd0
> [   52.079229]  [<ffffffff816fa88c>] common_interrupt+0x8c/0x8c
> [   52.085444]  <EOI>
> [   52.087568]  [<ffffffff8106bea9>] ? try_to_free_pmd_page+0x9/0x40
> [   52.094462]  [<ffffffff8106bea5>] ? try_to_free_pmd_page+0x5/0x40
> [   52.101157]  [<ffffffff8106bf2a>] ?  __unmap_pmd_range.part.5+0x4a/0x70
> [   52.108330]  [<ffffffff8106c330>] unmap_pmd_range+0x130/0x250
> [   52.114644]  [<ffffffff8106cb7b>] __cpa_process_fault+0x47b/0x5a0
> [   52.121339]  [<ffffffff8106d7cb>] __change_page_attr+0x78b/0x9e0
> [   52.127946]  [<ffffffff81065e45>] ?  __raw_callee_save___native_queued_spin_unlock+0x15/0x30
> [   52.137124]  [<ffffffff8106da98>] __change_page_attr_set_clr+0x78/0x300
> [   52.144404]  [<ffffffff81229180>] ? __slab_alloc+0x4d/0x5c
> [   52.150436]  [<ffffffff8106f15f>] kernel_map_pages_in_pgd+0x8f/0xd0
> [   52.157333]  [<ffffffff81dc0fb0>] efi_setup_page_tables+0xcc/0x1d9
> [   52.164124]  [<ffffffff81dc0a8d>] efi_enter_virtual_mode+0x35e/0x4af
> [   52.171117]  [<ffffffff81d9e109>] start_kernel+0x41f/0x4c8
> [   52.177142]  [<ffffffff81d9dad8>] ? set_init_arg+0x55/0x55
> [   52.183168]  [<ffffffff81d9d120>] ?  early_idt_handler_array+0x120/0x120
> [   52.190440]  [<ffffffff81d9d5d6>] x86_64_start_reservations+0x2a/0x2c
> [   52.197516]  [<ffffffff81d9d724>] x86_64_start_kernel+0x14c/0x16f
> [   52.204214] Code: 89 e5 41 57 41 56 49 89 f6 41 55 41 89 fd 41 54 49 89 d4 53 48 83 ec 10 89 7d d4 ff 14 25 a0 a7 c2 81 f6 c4 02 0f 85 0d 03 00 00 <41> f6 86 02 01 00 00 01 0f 85 ae 02 00 00 49 c7 c7 18 41 01 00
> [   52.225516] RIP  [<ffffffff810a49d2>] __queue_work+0x32/0x420
> [   52.231838]  RSP <ffff88bd7f403de8>
> [   52.235667] CR2: 0000000000000102
> [   52.239667] ---[ end trace 2ee7ea9d2908eb72 ]---
> [   52.244743] Kernel panic - not syncing: Fatal exception in interrupt
>
> Looking at the panic'ed instruction indicated that system_wq was
> used by credit_entropy_bits() before it it was initialized in an
> early_initcall.
>
> This patch prevents the schedule_work() call from being made before
> system_wq is initialized.
>
> Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
> ---
>  drivers/char/random.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 3efb3bf..3afc519 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -730,8 +730,12 @@ retry:
>                     r->entropy_total >= 2*random_read_wakeup_bits) {
>                         struct entropy_store *other = &blocking_pool;
>
> -                       if (other->entropy_count <=
> -                           3 * other->poolinfo->poolfracbits / 4) {
> +                       /*
> +                        * We cannot call schedule_work() before system_wq
> +                        * is initialized.
> +                        */
> +                       if (system_wq && (other->entropy_count <=
> +                           3 * other->poolinfo->poolfracbits / 4)) {
>                                 schedule_work(&other->push_work);
>                                 r->entropy_total = 0;
>                         }
> --
> 1.7.1
>

[toc] | [prev] | [next] | [standalone]

#1483597

Hello, Linus.

On Wed, Sep 14, 2016 at 12:14:30PM -0700, Linus Torvalds wrote:
> I'm wondering if we couldn't just initialize "system_wq" earlier.
> Right now init_workqueues() is an "early_initcall()", so it's at the
> same priority as a number of other random early initcalls. My gut
> feeling is that it should be initialized even earlier, with the
> scheduler.
> 
> Because dammit, we use "schedule_work()" as if it was a pretty core
> scheduler thing, and having to have some odd knowledge of system_wq
> initialization details in the rest of the kernel sounds really really
> wrong.
> 
> I don't think the random code is at all special in maybe wanting to
> schedule some work despite being an "early" initcall.
> 
> Adding Tejun to the cc, and quoting the whole email.
> 
> Tejun, comments?

We've used keventd_up() for this purpose and it hasn't been big enough
an issue as workqueue usages during earlyboot are very rare (only five
users right now).  But, yeah, it's getting used a more and more and
there's no harm in moving it to earlier.  I'll see how early I can
push it.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1483730

Hello,

On Wed, Sep 14, 2016 at 03:55:51PM -0400, Tejun Heo wrote:
> We've used keventd_up() for this purpose and it hasn't been big enough
> an issue as workqueue usages during earlyboot are very rare (only five
> users right now).  But, yeah, it's getting used a more and more and
> there's no harm in moving it to earlier.  I'll see how early I can
> push it.

So, we can't really start executing work items much before the init
task is started because other kthreads can't be created or scheduled
before then; however, we can build up all of the workqueue data
structures very early once memory allocation and idr are operational
sans actual worker creation and onlining.  This allows creating
workqueues and queueing work items from very early during the boot and
the queued work items will start executing as soon as init task starts
executing.  Obviously, if someone tries to flush anything, it's not
gonna work but that really shouldn't happen during early boot.

The patch I'm playing with now looks like the following and it seems
to work fine.  I'll verify it further and add some niceties like
WARN_ONs on premature flushes and removal of unnecessary keventd_up()
tests.

Thanks.


Index: work/include/linux/workqueue.h
===================================================================
--- work.orig/include/linux/workqueue.h
+++ work/include/linux/workqueue.h
@@ -631,4 +631,7 @@ int workqueue_online_cpu(unsigned int cp
 int workqueue_offline_cpu(unsigned int cpu);
 #endif
 
+int __init workqueue_init_early(void);
+int __init workqueue_init(void);
+
 #endif
Index: work/init/main.c
===================================================================
--- work.orig/init/main.c
+++ work/init/main.c
@@ -476,6 +476,11 @@ static void __init mm_init(void)
 	ioremap_huge_init();
 }
 
+static void my_test_work_fn(struct work_struct *work)
+{
+	printk("XXX my_test_work\n");
+}
+
 asmlinkage __visible void __init start_kernel(void)
 {
 	char *command_line;
@@ -551,6 +556,13 @@ asmlinkage __visible void __init start_k
 		 "Interrupts were enabled *very* early, fixing it\n"))
 		local_irq_disable();
 	idr_init_cache();
+	workqueue_init_early();
+
+	{
+		static DECLARE_WORK(my_test_work, my_test_work_fn);
+		schedule_work(&my_test_work);
+	}
+
 	rcu_init();
 
 	/* trace_printk() and trace points may be used after this */
@@ -1005,6 +1017,7 @@ static noinline void __init kernel_init_
 
 	smp_prepare_cpus(setup_max_cpus);
 
+	workqueue_init();
 	do_pre_smp_initcalls();
 	lockup_detector_init();
 
Index: work/kernel/workqueue.c
===================================================================
--- work.orig/kernel/workqueue.c
+++ work/kernel/workqueue.c
@@ -290,6 +290,8 @@ module_param_named(disable_numa, wq_disa
 static bool wq_power_efficient = IS_ENABLED(CONFIG_WQ_POWER_EFFICIENT_DEFAULT);
 module_param_named(power_efficient, wq_power_efficient, bool, 0444);
 
+static bool wq_online;
+
 static bool wq_numa_enabled;		/* unbound NUMA affinity enabled */
 
 /* buf for wq_update_unbound_numa_attrs(), protected by CPU hotplug exclusion */
@@ -3352,7 +3354,7 @@ static struct worker_pool *get_unbound_p
 		goto fail;
 
 	/* create and start the initial worker */
-	if (!create_worker(pool))
+	if (wq_online && !create_worker(pool))
 		goto fail;
 
 	/* install */
@@ -3417,6 +3419,7 @@ static void pwq_adjust_max_active(struct
 {
 	struct workqueue_struct *wq = pwq->wq;
 	bool freezable = wq->flags & WQ_FREEZABLE;
+	unsigned long flags;
 
 	/* for @wq->saved_max_active */
 	lockdep_assert_held(&wq->mutex);
@@ -3425,7 +3428,7 @@ static void pwq_adjust_max_active(struct
 	if (!freezable && pwq->max_active == wq->saved_max_active)
 		return;
 
-	spin_lock_irq(&pwq->pool->lock);
+	spin_lock_irqsave(&pwq->pool->lock, flags);
 
 	/*
 	 * During [un]freezing, the caller is responsible for ensuring that
@@ -3448,7 +3451,7 @@ static void pwq_adjust_max_active(struct
 		pwq->max_active = 0;
 	}
 
-	spin_unlock_irq(&pwq->pool->lock);
+	spin_unlock_irqrestore(&pwq->pool->lock, flags);
 }
 
 /* initialize newly alloced @pwq which is associated with @wq and @pool */
@@ -5455,7 +5458,7 @@ static void __init wq_numa_init(void)
 	wq_numa_enabled = true;
 }
 
-static int __init init_workqueues(void)
+int __init workqueue_init_early(void)
 {
 	int std_nice[NR_STD_WORKER_POOLS] = { 0, HIGHPRI_NICE_LEVEL };
 	int i, cpu;
@@ -5488,16 +5491,6 @@ static int __init init_workqueues(void)
 		}
 	}
 
-	/* create the initial worker */
-	for_each_online_cpu(cpu) {
-		struct worker_pool *pool;
-
-		for_each_cpu_worker_pool(pool, cpu) {
-			pool->flags &= ~POOL_DISASSOCIATED;
-			BUG_ON(!create_worker(pool));
-		}
-	}
-
 	/* create default unbound and ordered wq attrs */
 	for (i = 0; i < NR_STD_WORKER_POOLS; i++) {
 		struct workqueue_attrs *attrs;
@@ -5538,4 +5531,23 @@ static int __init init_workqueues(void)
 
 	return 0;
 }
-early_initcall(init_workqueues);
+
+int __init workqueue_init(void)
+{
+	struct worker_pool *pool;
+	int cpu, bkt;
+
+	/* create the initial workers */
+	for_each_online_cpu(cpu) {
+		for_each_cpu_worker_pool(pool, cpu) {
+			pool->flags &= ~POOL_DISASSOCIATED;
+			BUG_ON(!create_worker(pool));
+		}
+	}
+
+	hash_for_each(unbound_pool_hash, bkt, pool, hash_node)
+		BUG_ON(!create_worker(pool));
+
+	return 0;
+}
+

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web