module, fix percpu reserved memory exhaustion

Rusty,

There is likely some subtlety of moving the module mutex that I'm unaware of. What I can say is that this patch seems to resolve the problem for me, or at least through 100+ reboots I have not seen the problem (I'm still testing as I write this).

I'm more than willing to hear an alternative approach, or test an alternative patch.

Thanks,

P.

----8<----

In recent Fedora releases (F17 & F18) some users have reported seeing messages similar to

[   15.478121] Pid: 727, comm: systemd-udevd Tainted: GF 3.8.0-rc2+ #1
[   15.478121] Call Trace:
[   15.478131]  [<ffffffff81153001>] pcpu_alloc+0xa01/0xa60
[   15.478137]  [<ffffffff8163d8b0>] ? printk+0x61/0x63
[   15.478140]  [<ffffffff811532f3>] __alloc_reserved_percpu+0x13/0x20
[   15.478145]  [<ffffffff810c42e2>] load_module+0x1dc2/0x20b0
[   15.478150]  [<ffffffff8164b21e>] ? do_page_fault+0xe/0x10
[   15.478152]  [<ffffffff81647858>] ? page_fault+0x28/0x30
[   15.478155]  [<ffffffff810c46a7>] sys_init_module+0xd7/0x120
[   15.478159]  [<ffffffff8164f859>] system_call_fastpath+0x16/0x1b
[   15.478160] kvm: Could not allocate 304 bytes percpu data
[   15.478174] PERCPU: allocation failed, size=304 align=32, alloc from reserved chunk failed

during system boot. In some cases, users have also reported seeing this message along with a failed load of other modules.

As the message indicates, the reserved chunk of percpu memory (where modules allocate their memory) is exhausted. A debug printk inserted in the code shows

[   15.478533] PRARIT size = 304 > chunk->contig_hint = 208

ie) the reserved chunk of percpu has only 208 bytes of available space.

What is happening is systemd is loading an instance of the kvm module for each cpu found (see commit e9bda3b). When the module load occurs the kernel currently allocates the modules percpu data area prior to checking to see if the module is already loaded or is in the process of being loaded. If the module is already loaded, or finishes load, the module loading code releases the current instance's module's percpu data.

The problem is that these module loads race and it is possible that all of the percpu reserved area is consumed by repeated loads of the same module which results in the failure of other drivers to load.

This patch moves the module percpu allocation after the check for an existing instance of the module.

---
 kernel/module.c |  124 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 85 insertions(+), 39 deletions(-)

index 250092c..e7e9b57 100644
@@ -1929,6 +1929,27 @@ static int verify_export_symbols(struct module *mod)
 	return 0;
 }
 
+static void simplify_percpu_symbols(struct module *mod,
+				    const struct load_info *info)
+{
+	Elf_Shdr *symsec = &info->sechdrs[info->index.sym];
+	Elf_Sym *sym = (void *)symsec->sh_addr;
+	unsigned long secbase;
+	unsigned int i;
+
+	/*
+	 * No need for error checking in this function because
+	 * simplify_symbols has already been called.
+	 */
+	for (i = 1; i < symsec->sh_size / sizeof(Elf_Sym); i++) {
+		/* Divert to percpu allocation if a percpu var. */
+		if (sym[i].st_shndx == info->index.pcpu) {
+			secbase = (unsigned long)mod_percpu(mod);
+			sym[i].st_value += secbase;
+		}
+	}
+}
+
 /* Change all symbols so that st_value encodes the pointer directly. */
 static int simplify_symbols(struct module *mod, const struct load_info *info)
 {

@@ -1976,12 +1997,11 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
 			break;
 
 		default:
-			/* Divert to percpu allocation if a percpu var. */
-			if (sym[i].st_shndx == info->index.pcpu)
-				secbase = (unsigned long)mod_percpu(mod);
-			else
+			/* percpu diverts handled in simplify_percpu_symbols */
+			if (sym[i].st_shndx != info->index.pcpu) {
 				secbase = info->sechdrs[sym[i].st_shndx].sh_addr;
-			sym[i].st_value += secbase;
+				sym[i].st_value += secbase;
+			}
 			break;
 		}
 	}

@@ -2899,11 +2919,29 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
 	return 0;
 }
 
+static int allocate_percpu(struct module *mod, struct load_info *info)
+{
+	Elf_Shdr *pcpusec;
+	int err;
+
+	pcpusec = &info->sechdrs[info->index.pcpu];
+	if (pcpusec->sh_size) {
+		/* We have a special allocation for this section. */
+		pr_debug("module %s attempting to percpu with size %d\n",
+			 mod->name, pcpusec->sh_size);
+		err = percpu_modalloc(mod,
+				      pcpusec->sh_size, pcpusec->sh_addralign);
+		if (err)
+			return -ENOMEM;
+		pcpusec->sh_flags &= ~(unsigned long)SHF_ALLOC;
+	}
+	return 0;
+}
+
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
 	/* Module within temporary copy. */
 	struct module *mod;
-	Elf_Shdr *pcpusec;
 	int err;
 
 	mod = setup_load_info(info, flags);

@@ -2920,16 +2958,6 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	if (err < 0)
 		goto out;
 
-	pcpusec = &info->sechdrs[info->index.pcpu];
-	if (pcpusec->sh_size) {
-		/* We have a special allocation for this section. */
-		err = percpu_modalloc(mod,
-				      pcpusec->sh_size, pcpusec->sh_addralign);
-		if (err)
-			goto out;
-		pcpusec->sh_flags &= ~(unsigned long)SHF_ALLOC;
-	}
-
 	/* Determine total sizes, and put offsets in sh_entsize.  For now
 	   this is done generically; there doesn't appear to be any
 	   special cases for the architectures. */

@@ -2955,7 +2983,6 @@ out:
 /* mod is no longer valid after this! */
 static void module_deallocate(struct module *mod, struct load_info *info)
 {
-	percpu_modfree(mod);
 	module_free(mod, mod->module_init);
 	module_free(mod, mod->module_core);
 }

@@ -3135,18 +3162,54 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Set up MODINFO_ATTR fields */
 	setup_modinfo(mod, info);
 
-	/* Fix up syms, so that st_value is a pointer to location. */
+	/* Fix up non-percpu syms, so that st_value is a pointer to location. */
 	err = simplify_symbols(mod, info);
 	if (err < 0)
 		goto free_modinfo;
 
+	/*
+	 * This mutex protects against concurrent writers of the module
+	 * RCU list.  The code takes it pretty early because there are
+	 * several things that must be completed after checking for an
+	 * existing module, and before adding it to the RCU list.
+	 */
+	mutex_lock(&module_mutex);
+
+	/* Mark state as coming so strong_try_module_get() ignores us. */
+	mod->state = MODULE_STATE_COMING;
+
+	/* is another instance of this module already loading or loaded? */
+again:
+	if ((old = find_module(mod->name)) != NULL) {
+		if (old->state == MODULE_STATE_COMING) {
+			/* Wait in case it fails to load. */
+			mutex_unlock(&module_mutex);
+			err = wait_event_interruptible(module_wq,
+					       finished_loading(mod->name));
+			if (err)
+				goto free_modinfo;
+			mutex_lock(&module_mutex);
+			goto again;
+		}
+		err = -EEXIST;
+		goto unlock;
+	}
+
+	/* allocate this modules percpu reserved area */
+	err = allocate_percpu(mod, info);
+	if (err)
+		goto free_modinfo;
+
+	/* Fix up percpu syms, so that st_value is a pointer to location. */
+	simplify_percpu_symbols(mod, info);
+
 	err = apply_relocations(mod, info);
 	if (err < 0)
-		goto free_modinfo;
+		goto free_percpu;
 
 	err = post_relocation(mod, info);
 	if (err < 0)
-		goto free_modinfo;
+		goto free_percpu;
 
 	flush_module_icache(mod);
 

@@ -3157,31 +3220,12 @@ static int load_module(struct load_info *info, const char __user *uargs,
 		goto free_arch_cleanup;
 	}
 
-	/* Mark state as coming so strong_try_module_get() ignores us. */
-	mod->state = MODULE_STATE_COMING;
-
 	/* Now sew it into the lists so we can get lockdep and oops
 	 * info during argument parsing.  No one should access us, since
 	 * strong_try_module_get() will fail.
 	 * lockdep/oops can run asynchronous, so use the RCU list insertion
 	 * function to insert in a way safe to concurrent readers.
-	 * The mutex protects against concurrent writers.
 	 */
-again:
-	mutex_lock(&module_mutex);
-	if ((old = find_module(mod->name)) != NULL) {
-		if (old->state == MODULE_STATE_COMING) {
-			/* Wait in case it fails to load. */
-			mutex_unlock(&module_mutex);
-			err = wait_event_interruptible(module_wq,
-					       finished_loading(mod->name));
-			if (err)
-				goto free_arch_cleanup;
-			goto again;
-		}
-		err = -EEXIST;
-		goto unlock;
-	}
 
 	/* This has to be done once we're sure module name is unique. */
 	dynamic_debug_setup(info->debug, info->num_debug);

@@ -3228,6 +3272,8 @@ again:
 	kfree(mod->args);
  free_arch_cleanup:
 	module_arch_cleanup(mod);
+ free_percpu:
+	percpu_modfree(mod);
  free_modinfo:
 	free_modinfo(mod);
  free_unload:

1.7.9.3

...

Wow, what a cool bug! Classic unforseen side-effect.

I'd prefer not to do relocations with the module_lock held: it can be relatively slow. Yet we can't do relocations before the per-cpu allocation, obviously. Did you do boot timings before and after?

An alternative would be to put the module into the list even earlier (say, just after layout_and_allocate) so we could block on concurrent loads at that point. But then we have to make sure noone looks in the module too early before it's completely set up, and that's complicated and error-prone too. A separate list is kind of icky.

We currently have PERCPU_MODULE_RESERVE set at 8k: in my 32-bit allmodconfig build, there are only three modules with per-cpu data, totalling 328 bytes. So it's not reasonable to increase that number to paper over this.

This is what a new boot state looks like (pains not to break ksplice). It's two patches, but I'll just post them back to back:

module: add new state MODULE_STATE_UNFORMED.

You should never look at such a module, so it's excised from all paths which traverse the modules list.

We add the state at the end, to avoid gratuitous ABI break (ksplice).

index 7760c6d..4432373 100644
@@ -199,11 +199,11 @@ struct module_use {
 	struct module *source, *target;
 };
 
-enum module_state
-{
-	MODULE_STATE_LIVE,
-	MODULE_STATE_COMING,
-	MODULE_STATE_GOING,
+enum module_state {
+	MODULE_STATE_LIVE,	/* Normal state. */
+	MODULE_STATE_COMING,	/* Full formed, running module_init. */
+	MODULE_STATE_GOING,	/* Going away. */
+	MODULE_STATE_UNFORMED,	/* Still setting it up. */
 };
 
 /**

module: put modules in list much earlier.

Prarit's excellent bug report:

Now we have a new state MODULE_STATE_UNFORMED, we can insert the module into the list (and thus guarantee its uniqueness) before we allocate the per-cpu region.

index a28c141..d0cdf14 100644
@@ -55,6 +55,7 @@ static inline unsigned long bug_addr(const struct bug_entry *bug)
 }
 
 #ifdef CONFIG_MODULES
+/* Updates are protected by module mutex */
 static LIST_HEAD(module_bug_list);
 
 static const struct bug_entry *module_find_bug(unsigned long bugaddr)

Heh ... I did! :) I had a lot of concerns about moving the mutex around so I put in print at the end of boot to see how long the boot time actually was.

[  22.893015] PRARIT: FINAL BOOT MESSAGE

[  22.673214] PRARIT: FINAL BOOT MESSAGE

Both kernel boots showed the problem with kvm loading. A quick grep through my bootlogs of stock kernel + my patch don't show anything greater than 23.539392 and less than 20.980321. Those numbers are similar to the numbers from the stock kernel (23.569450 - 20.898321).

ie) I don't think there's an increase due to calling the relocation under the module mutex, and if there is it is definitely lost within the noise of boot.

The timing were similar. I didn't see any huge delays, etc. Can the relocations really cause a long delay? I thought we were pretty much writing values to memory...

[I should point out that I'm booting a 32 physical/64 logical, with 64GB of memory]

Yeah -- that was my first attempt actually, and it got very complex very quickly. I abandoned that approach in favor of moving the percpu allocations under the lock. I thought that was likely the easiest approach.

I've been thinking about that. The problem is that at the same time the kvm problem occurs I'm attempting to load a debug module that I've written to debug some cpu timer issues that allocates a large amount of percpu data (~.5K/cpu). While extending PERCPU_MODULE_RESERVE to 10k might work now, it might not work tomorrow if I have the need to increase the size of my log buffer.

... that is ;), I prefer your and my approach of fixing this problem.

<snip patch>

Sure, but I'm always nervous about expanding any state machine ;). That's just me though :).

<snip patch>

Rusty, you can change that to an Acked-by if you prefer that. I know some engineers prefer one over the other. I'll also continue doing some reboot testing and will email back in a few days to let you know what the timing looks like.

Thanks!,

P.

For x86 that's true, but look at what ppc64 has to do for example. I'm guessing you don't have a giant Nvidia proprietary driver module loading, either.

It just makes me nervous; this kind of boot slowdown typically won't get diagnosed for several releases, if ever. Now I've done the work, I'm going to apply my patch (with an additional fix: I forgot to change kgdb, which traverses the module list too).

I figured it had to be something big ;)

Well, it looks like PERCPU_MODULE_RESERVE is actually very generous; it could easily be halved. I guess this is because dynamic per-cpu data is now such a nice alternative (thanks to Tejun).

There seem to be two kinds of Acked-by:

1) Acked-by: <maintainer>. ie. "this should go through my tree, but it's going via someone else". I like this: shows the normal maintainer is aware of the change.

2) Acked-by: <random>. ie. "I like the concept of the patch though I haven't actually read it or tested it". Completely useless.

OTOH, Tested-by: means it actually fixed someone's problem.

Thanks!
Rusty.

Ah -- I see. I hadn't thought much about the other arches and I see what ppc64 does ...

:) Imagine what happens at 4096 cpus (SGI territory). I'm wondering about that kvm commit. Maybe the systemd/udev rule needs to be rewritten to avoid a 'kvm loading flood' during boot ... I'll talk with Kay Sievers about it to see if there's a way around that.

Got it. For the record over-the-weekend testing didn't show any bizarre results. The boot times were all around 20-23 seconds.

P.

Hello, Rusty.

Heh, thanks. :)

There are two percpu constants that I basically pulled out of my ass.

* PERCPU_MODULE_RESERVE: It's used on architectures with data model which restricts the distance between code and data. e.g. x86_64 genereates code which can't address full 64bit for static data and as percpu addressing depends on the regular address generation to derive percpu addresses, the percpu offsets need to be restricted somehow. Percpu allocator currently achieves this by reserving certain amount in the first percpu chunk which is guaranteed to have very low (starting from 0) percpu address space offsets.

On those archs, this becomes the hard limit for all module static percpu areas. Maybe 8k is way too high and we can go with 4k. Worth a try I guess.

* PERCPU_DYNAMIC_RESERVE: The amount of space at the end of the first chunk used for dynamic allocation. This isn't as critical as the above and the only reason it exists is because the first chunk is often embedded in the kernel linear address space instead of vmalloc area and the former is often mapped with larger page table size than the latter. The goal of PERCPU_DYNAMIC_RESERVE is to have just enough space to cover the usual percpu allocations without wasting too much space to minimize impact on TLB pressure.

The current number isn't entirely made up. I did some testing with some random config. Don't know how it holds up now. It probably can be increased given that we developed quite a few percpu dynamic allocations since then.

It would be awesome to have a way to somehow dynamically adjust this one; unfortunately, that would require persistent data across boots. :(

Thanks.

tejun