mm-move-mremap-from-file_operations-to-vm_operations_struct-fix

fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite() is not defined in this case. Add yet another "must not be called" helper into nommu.c to make the linker happy.

I still think this is pointless, afaics sys_io_setup() simply can't succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend on CONFIG_MMU.

Reported-by Fengguang Wu <[email protected]>

Reported-by Stephen Rothwell <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite() is not defined in this case. Add yet another "must not be called" helper into nommu.c to make the linker happy.

I still think this is pointless, afaics sys_io_setup() simply can't succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend on CONFIG_MMU.

..

mm/nommu.c +0 -0

@@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
 }
 EXPORT_SYMBOL(filemap_map_pages);

Quoted 16 lines · On Fri, 17 Jul 2015 01:14:05 +0200 Oleg Nesterov <[email protected]> wrote:

fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite() is not defined in this case. Add yet another "must not be called" helper into nommu.c to make the linker happy.

I still think this is pointless, afaics sys_io_setup() simply can't succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend on CONFIG_MMU.

..

mm/nommu.c +0 -0

@@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
 }
 EXPORT_SYMBOL(filemap_map_pages);
 

mm/nommu.c void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf) · line 2019

Quoted 9 lines

mm/nommu.c +6 -0

+int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	BUG();
+	return 0;
+}
+
 static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned long addr, void *buf, int len, int write)
 {

So if anyone starts testing aio on NOMMU, this patch will make the whole thing immediately go BUG. This isn't helpful :(

Yes, making AIO depend on MMU sounds better.

Because if it wasn't
busted before, it sure is now!

Quoted 19 lines · Andrew Morton

Quoted 16 lines · On Fri, 17 Jul 2015 01:14:05 +0200 Oleg Nesterov <[email protected]> wrote:

fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite() is not defined in this case. Add yet another "must not be called" helper into nommu.c to make the linker happy.

I still think this is pointless, afaics sys_io_setup() simply can't succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend on CONFIG_MMU.

..

mm/nommu.c +0 -0

@@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
 }
 EXPORT_SYMBOL(filemap_map_pages);
 

Quoted 12 lines

mm/nommu.c void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf) · line 2019

Quoted 9 lines

mm/nommu.c +6 -0

+int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	BUG();
+	return 0;
+}
+
 static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned long addr, void *buf, int len, int write)
 {

So if anyone starts testing aio on NOMMU, this patch will make the whole thing immediately go BUG. This isn't helpful :(

Well, I'm afraid I could miss something, but _afaics_ this can not happen. filemap_page_mkwrite() can't be called if NOMMU.

In particular, simply because sys_io_setup() is the only user (if NOMMU) and it can't succeed. But even if I missed something and it can succeed, ->page_mkwrite() must not be called anyway. But this, again, unless I missed something ;)

Quoted 1 line

Yes, making AIO depend on MMU sounds better.

Perhaps Benjamin can change his mind or correct me.

Quoted 2 lines

Because if it wasn't
busted before, it sure is now!

I hope this change can't make any difference.

Oleg.

Quoted 22 lines · Oleg Nesterov

Quoted 13 lines

mm/nommu.c void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf) · line 2019

Quoted 10 lines

mm/nommu.c +6 -0

+int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	BUG();
+	return 0;
+}
+
 static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned long addr, void *buf, int len, int write)
 {

So if anyone starts testing aio on NOMMU, this patch will make the whole thing immediately go BUG. This isn't helpful :(

Well, I'm afraid I could miss something, but _afaics_ this can not happen. filemap_page_mkwrite() can't be called if NOMMU.

In particular, simply because sys_io_setup() is the only user (if NOMMU) and it can't succeed. But even if I missed something and it can succeed, ->page_mkwrite() must not be called anyway. But this, again, unless I missed something ;)

Quoted 3 lines

Quoted 1 line

Yes, making AIO depend on MMU sounds better.

Perhaps Benjamin can change his mind or correct me.

Either try to fix it correctly,

or disable the config.

Making it just
compile but be knowingly broken is worse than either of those 2 options.

My point was that it is valid for someone to want to use the functionality on a nommu system, and given that it should have worked before the page migration code was added, It Would Be Nice(tm) to return it to that state.

Adding a BUG() like that to the code is just plain broken.

Benjamin,

it seems that we do not understand each other,

Quoted 2 lines

Either try to fix it correctly,

And I think this fix is correct. In a sense that we only add filemap_page_mkwrite() to make the linker happy, it can never be called and thus we can never hit this BUG().

Please look at filemap_fault() in nommu.c,

	int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
	{
		BUG();
		return 0;
	}

this is the same thing. If nothing else, mm/memory.c is not even compiled if NOMMU.

Quoted 1 line

or disable the config.

Yes, I think this makes more sense. but see below...

Quoted 2 lines

Making it just
compile but be knowingly broken is worse than either of those 2 options.

Why? See above. I think this change makes no difference except it fixes the build.

Again, of course I could miss something. Could you explain your point?

Quoted 3 lines

My point was that it is valid for someone to want to use the functionality on a nommu system, and given that it should have worked before the page migration code was added, It Would Be Nice(tm) to return it to that state.

Perhaps it worked on NOMMU before, I have no idea. But currently, afaics, it can not. Even sys_io_setup() can't suceed. So I do not understand why do we allow NOMMU && CONFIG_AIO.

But this is another issue. Of course I won't insist, please forget.

Quoted 2 lines

Adding a BUG() like that to the code is just plain broken.

Why? Could you explain what I have missed?

Oleg.

Quoted 3 lines · Oleg Nesterov

Benjamin,

it seems that we do not understand each other,

...

Quoted 15 lines

Quoted 2 lines

Either try to fix it correctly,

And I think this fix is correct. In a sense that we only add filemap_page_mkwrite() to make the linker happy, it can never be called and thus we can never hit this BUG().

Please look at filemap_fault() in nommu.c,

	int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
	{
		BUG();
		return 0;
	}

Quoted 2 lines

this is the same thing. If nothing else, mm/memory.c is not even compiled if NOMMU.

Using BUG() is the wrong approach. If the code is not needed in NOMMU, then #ifdef it out. Think about it: NOMMU systems are very low memory systems and they should not have dead code compiled in if it is not needed.

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

Quoted 1 line

Why? Could you explain what I have missed?

It's doing half the job. Either the code should be #if'd out or not.

Quoted 3 lines · Benjamin LaHaise

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

There is another interpretation: I think this code must be never called, if it is actually called we have a serious problem which should be loudly reported.

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Benjamin LaHaise

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

Quoted 4 lines

There is another interpretation: I think this code must be never called, if it is actually called we have a serious problem which should be loudly reported.

And not compiling it at all _will_ loudly report it, it'll just report it during linking instead of at run-time, which is a much better time to shout about it.

Quoted 11 lines · Austin S Hemmelgarn

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · Benjamin LaHaise

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

There is another interpretation: I think this code must be never called, if it is actually called we have a serious problem which should be loudly reported.

Quoted 3 lines

And not compiling it at all _will_ loudly report it, it'll just report it during linking instead of at run-time, which is a much better time to shout about it.

And how can we do this?

Quoted 16 lines · Oleg Nesterov

Quoted 14 lines · Austin S Hemmelgarn

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · Benjamin LaHaise

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

There is another interpretation: I think this code must be never called, if it is actually called we have a serious problem which should be loudly reported.

And not compiling it at all _will_ loudly report it, it'll just report it during linking instead of at run-time, which is a much better time to shout about it.

Quoted 2 lines

And how can we do this?

If a function that isn't defined (for example, you use a #if block to comment it out under certain circumstances), then the link will fail rather noisily something references it.

We already know during the
compile that it's a NOMMU kernel, so anything that calls it on a MMU enabled kernel can have a compile time check added

instead of doing the
check at runtime (or even just calling it without checking), thus even further reducing code size.

Quoted 18 lines · Austin S Hemmelgarn

Quoted 16 lines · Oleg Nesterov

Quoted 14 lines · Austin S Hemmelgarn

Quoted 9 lines · Oleg Nesterov

Quoted 3 lines · Benjamin LaHaise

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

There is another interpretation: I think this code must be never called, if it is actually called we have a serious problem which should be loudly reported.

And not compiling it at all _will_ loudly report it, it'll just report it during linking instead of at run-time, which is a much better time to shout about it.

Quoted 5 lines

Quoted 2 lines

And how can we do this?

If a function that isn't defined (for example, you use a #if block to comment it out under certain circumstances), then the link will fail rather noisily something references it.

This is what we are trying to fix.

Quoted 4 lines

We already know during the
compile that it's a NOMMU kernel, so anything that calls it on a MMU enabled kernel can have a compile time check added

It already has. memory.c is not compiled if NOMMU.

The problem is aio_ring_vm_ops which references this function. And btw filemap_fault() too.

And just in case, I won't mind to add ifdef(CONFIG_MMU) there, I am waiting for reply from Benjamin.

Quoted 3 lines

instead of doing the
check at runtime (or even just calling it without checking), thus even further reducing code size.

So what exactly do you suggest to fix the problem?

it should have worked before the page
migration code was added,

Quoted 21 lines · Benjamin LaHaise

Quoted 3 lines · Oleg Nesterov

Benjamin,

it seems that we do not understand each other,

...

Quoted 15 lines

Quoted 2 lines

Either try to fix it correctly,

And I think this fix is correct. In a sense that we only add filemap_page_mkwrite() to make the linker happy, it can never be called and thus we can never hit this BUG().

Please look at filemap_fault() in nommu.c,

	int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
	{
		BUG();
		return 0;
	}

Quoted 6 lines

Quoted 2 lines

this is the same thing. If nothing else, mm/memory.c is not even compiled if NOMMU.

Using BUG() is the wrong approach. If the code is not needed in NOMMU, then #ifdef it out. Think about it: NOMMU systems are very low memory systems and they should not have dead code compiled in if it is not needed.

OK, at least I hope you no longer think that this patch makes this code knowingly broken.

Quoted 2 lines

Don't add BUG(). It's the equivalent approach of saying "I think this code isn't needed, but I'm lazy and not going to remove it properly."

There is another interpretation: I think this code must be never called, if it is actually called we have a serious problem which should be loudly reported.

Quoted 3 lines

Quoted 1 line

Why? Could you explain what I have missed?

It's doing half the job. Either the code should be #if'd out or not.

Again, filemap_page_mkwrite() added to nommu.c matches filemap_fault() and filemap_map_pages() we already have.

But I won't argue, you are maintainer. What exactly do you want me to ifdef? Will you agree with the patch which adds ifdef into aio_ring_vm_ops,

	static const struct vm_operations_struct aio_ring_vm_ops = {
	       .mremap         = aio_ring_mremap,
	#ifdef CONFIG_MMU
	       .fault          = filemap_fault,
	       .map_pages      = filemap_map_pages,
	       .page_mkwrite   = filemap_page_mkwrite,
	#endif
	};

?

Oleg.

Shouldn't we account aio events/pages somehow, say per-user, or in mm->pinned_vm ?

Hi, Oleg,

Quoted 2 lines · Oleg Nesterov

Shouldn't we account aio events/pages somehow, say per-user, or in mm->pinned_vm ?

Ages ago I wrote a patch to account the completion ring to a process' memlock limit:
"[patch] aio: remove aio-max-nr and instead use the memlock rlimit to
limit the number of pages pinned for the aio completion ring"
http://marc.info/?l=linux-aio&m=123661380807041&w=2

The problem with that patch is that it modifies the user/kernel interface. It could be done over time, as Andrew outlined in that thread, but I've been reluctant to take that on.

If you just mean we should account the memory so that the right process can be killed, that sounds like a good idea to me.

Hi Jeff,

Quoted 14 lines · Jeff Moyer

Hi, Oleg,

Quoted 2 lines · Oleg Nesterov

Shouldn't we account aio events/pages somehow, say per-user, or in mm->pinned_vm ?

Ages ago I wrote a patch to account the completion ring to a process' memlock limit:
"[patch] aio: remove aio-max-nr and instead use the memlock rlimit to
limit the number of pages pinned for the aio completion ring"
http://marc.info/?l=linux-aio&m=123661380807041&w=2

Quoted 3 lines

The problem with that patch is that it modifies the user/kernel interface. It could be done over time, as Andrew outlined in that thread, but I've been reluctant to take that on.

See also the usage of mm->pinned_vm and user->locked_vm in perf_mmap(), perhaps aio can do the same...

Quoted 2 lines

If you just mean we should account the memory so that the right process can be killed, that sounds like a good idea to me.

Not sure we actually need this. I only meant that this looks confusing because this memory is actually locked but the kernel doesn't know this.

And btw, I forgot to mention that I triggered OOM on the testing machine with only 512mb ram, and aio-max-nr was huge. So, once again, while this all doesn't look right to me, I do not think this is the real problem.

Except the fact that an unpriviliged user can steal all aio-max-nr events. This probably worth fixing in any case.

And if we accept the fact this memory is locked and if we properly account it, then may be we can just kill aio_migratepage(), aio_private_file(), and change aio_setup_ring() to simply use install_special_mapping(). This will greatly simplify the code. But let me remind that I know nothing about aio, so please don't take my thoughts seriously.

Oleg.

As for accounting locked memory, we don't do that for memory pinned by O_DIRECT either. Given how small the amount of memory aio can pin is compared to O_DIRECT or mlock(), it is unlikely that the accounting of how much aio has pinned will make any real difference in the big picture. A single O_DIRECT i/o can pin megabytes of memory.

Quoted 6 lines · Oleg Nesterov

And if we accept the fact this memory is locked and if we properly account it, then may be we can just kill aio_migratepage(), aio_private_file(), and change aio_setup_ring() to simply use install_special_mapping(). This will greatly simplify the code. But let me remind that I know nothing about aio, so please don't take my thoughts seriously.

No, you can't get rid of that code. The page migration is required when CPUs/memory is offlined and data needs to be moved to another node.

Similarly, support for mremap() is also required for container migration / restoration.

Given how small the amount of memory aio can pin

it is unlikely that the accounting of
how much aio has pinned will make any real difference in the big picture.

But again, again, please ignore. This all is off-topic and my understanding is very limited.

Quoted 3 lines · Oleg Nesterov

But again, again, please ignore. This all is off-topic and my understanding is very limited.

Yes, yes, but sorry for noise and let me repeat...

This memory lives in page-cache/lru, it is visible for shrinker which will unmap these pages for no reason on memory shortage. IOW, aio fools the kernel, this memory looks reclaimable but it is not. And we only do this for migration.

Even if this is not a problem, this does not look right. So perhaps at least mapping_set_unevictable() makes sense. But I simply do not know if migration will work with this change.

And I should have changes the subject a long ago... So what do you think we should do with the build failure?

Oleg.

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov

But again, again, please ignore. This all is off-topic and my understanding is very limited.

Quoted 6 lines

Yes, yes, but sorry for noise and let me repeat...

This memory lives in page-cache/lru, it is visible for shrinker which will unmap these pages for no reason on memory shortage. IOW, aio fools the kernel, this memory looks reclaimable but it is not. And we only do this for migration.

And we have the same problem with O_DIRECT.

Quoted 2 lines

And I should have changes the subject a long ago... So what do you think we should do with the build failure?

I honestly don't care what of the options you do -- please just don't go about adding BUG()s.

Andrew,

this replaces

mm-move-mremap-from-file_operations-to-vm_operations_struct-fix.patch

in -mm tree nacked by Benjamin.

fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite() is not defined in this case. Add ifdef(CONFIG_MMU) into aio_ring_vm_ops.

Note that this only fixes the build error, currently aio doesn't work if !CONFIG_MMU anyway.

Reported-by Fengguang Wu <[email protected]>

Reported-by Stephen Rothwell <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/aio.c +0 -0

index c122624..822c199 100644

fs/aio.c +0 -0

 static int aio_ring_mmap(struct file *file, struct vm_area_struct *vma)

Signature · 1 line

1.7.1

fs/aio.c static int aio_ring_mremap(struct vm_area_struct *vma) · line 346

Quoted 9 lines · Oleg Nesterov

static const struct vm_operations_struct aio_ring_vm_ops = {

fs/aio.c +2 -0

 	.mremap		= aio_ring_mremap,
+#ifdef CONFIG_MMU
 	.fault		= filemap_fault,
 	.map_pages	= filemap_map_pages,
 	.page_mkwrite	= filemap_page_mkwrite,
+#endif
 };

One nit: using #if IS_ENABLED(CONFIG_MMU) is recommended these days not #ifdef CONFIG_MMU.