kmod: few simple enhancements

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44
[1] https://github.com/mcgrof/test_request_module

On May 19, 2017 1:45 PM, "Dmitry Torokhov" <[email protected]> wrote:

Quoted 23 lines · Luis R. Rodriguez

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Quoted 8 lines

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/

history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44

Quoted 1 line

[1] https://github.com/mcgrof/test_request_module

If we actually run into this issue, instead of slamming the system with bazillion concurrent requests, can we wait for the other modprobes to finish and then continue?

Yes ! That I have a patch that does precisely that ! That is actually still *not enough* to not fail fatally but this would be subject of another series with more debatable approaches.

This at least pushes us to closer safer limits for now while also making it configurable.

Quoted 43 lines · Luis R. Rodriguez

On May 19, 2017 1:45 PM, "Dmitry Torokhov" <[email protected]> wrote:

Quoted 23 lines · Luis R. Rodriguez

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Quoted 8 lines

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/

history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44

Quoted 1 line

[1] https://github.com/mcgrof/test_request_module

If we actually run into this issue, instead of slamming the system with bazillion concurrent requests, can we wait for the other modprobes to finish and then continue?

Quoted 4 lines

Yes ! That I have a patch that does precisely that ! That is actually still *not enough* to not fail fatally but this would be subject of another series with more debatable approaches.

Then please post it.

Quoted 2 lines

This at least pushes us to closer safer limits for now while also making it configurable.

Making it configurable depending on how big/little box is makes no sense,

especially if the above is implemented, as depth of modprobe invocations depends on configuration and not computing power of the hardware the system is running on.

Quoted 44 lines · Dmitry Torokhov

Quoted 43 lines · Luis R. Rodriguez

On May 19, 2017 1:45 PM, "Dmitry Torokhov" <[email protected]> wrote:

Quoted 23 lines · Luis R. Rodriguez

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Quoted 8 lines

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/

history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44

Quoted 1 line

[1] https://github.com/mcgrof/test_request_module

If we actually run into this issue, instead of slamming the system with bazillion concurrent requests, can we wait for the other modprobes to finish and then continue?

Quoted 6 lines

Quoted 4 lines

Yes ! That I have a patch that does precisely that ! That is actually still *not enough* to not fail fatally but this would be subject of another series with more debatable approaches.

Then please post it.

Will do.

Quoted 5 lines

Quoted 2 lines

This at least pushes us to closer safer limits for now while also making it configurable.

Making it configurable depending on how big/little box is makes no sense,

If we set a hard limit then we need to patch a system if we need to increment it. This is rather stupid given we have no current heuristics to make kmod loading deterministic from userspace, and in the worst case this can be fatal. General system size is a good first guess, but making it configurable is really key given current limitations. I'll post further patches which reveals some of these issues more clearly.

Quoted 3 lines

especially if the above is implemented, as depth of modprobe invocations depends on configuration and not computing power of the hardware the system is running on.

You seem to agree making it configurable is sensible , but not depending on the system size ?

Quoted 71 lines · Luis R. Rodriguez

Quoted 44 lines · Dmitry Torokhov

Quoted 43 lines · Luis R. Rodriguez

On May 19, 2017 1:45 PM, "Dmitry Torokhov" <[email protected]> wrote:

Quoted 23 lines · Luis R. Rodriguez

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Quoted 8 lines

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/

history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44

Quoted 1 line

[1] https://github.com/mcgrof/test_request_module

If we actually run into this issue, instead of slamming the system with bazillion concurrent requests, can we wait for the other modprobes to finish and then continue?

Quoted 6 lines

Quoted 4 lines

Yes ! That I have a patch that does precisely that ! That is actually still *not enough* to not fail fatally but this would be subject of another series with more debatable approaches.

Then please post it.

Will do.

Quoted 5 lines

Quoted 2 lines

This at least pushes us to closer safer limits for now while also making it configurable.

Making it configurable depending on how big/little box is makes no sense,

If we set a hard limit then we need to patch a system if we need to increment it. This is rather stupid given we have no current heuristics to make kmod loading deterministic from userspace, and in the worst case this can be fatal. General system size is a good first guess, but making it configurable is really key given current limitations. I'll post further patches which reveals some of these issues more clearly.

Quoted 3 lines

especially if the above is implemented, as depth of modprobe invocations depends on configuration and not computing power of the hardware the system is running on.

Quoted 2 lines

You seem to agree making it configurable is sensible , but not depending on the system size ?

No, I am saying that making it configurable based on system size makes no sense at all, and making it configurable given you already have patches removing hard failures gives no benefit.

Quoted 75 lines · Dmitry Torokhov

Quoted 73 lines · Luis R. Rodriguez

Quoted 44 lines · Dmitry Torokhov

Quoted 43 lines · Luis R. Rodriguez

On May 19, 2017 1:45 PM, "Dmitry Torokhov" <[email protected]> wrote:

Quoted 23 lines · Luis R. Rodriguez

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Quoted 8 lines

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/

history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44

Quoted 1 line

[1] https://github.com/mcgrof/test_request_module

If we actually run into this issue, instead of slamming the system with bazillion concurrent requests, can we wait for the other modprobes to finish and then continue?

Quoted 6 lines

Quoted 4 lines

Yes ! That I have a patch that does precisely that ! That is actually still *not enough* to not fail fatally but this would be subject of another series with more debatable approaches.

Then please post it.

Will do.

Quoted 5 lines

Quoted 2 lines

This at least pushes us to closer safer limits for now while also making it configurable.

Making it configurable depending on how big/little box is makes no sense,

If we set a hard limit then we need to patch a system if we need to increment it. This is rather stupid given we have no current heuristics to make kmod loading deterministic from userspace, and in the worst case this can be fatal. General system size is a good first guess, but making it configurable is really key given current limitations. I'll post further patches which reveals some of these issues more clearly.

Quoted 3 lines

especially if the above is implemented, as depth of modprobe invocations depends on configuration and not computing power of the hardware the system is running on.

You seem to agree making it configurable is sensible , but not depending on the system size ?

Quoted 3 lines

No, I am saying that making it configurable based on system size makes no sense at all, and making it configurable given you already have patches removing hard failures gives no benefit.

Ah no, the problem is that hard failures are not yet removed in this patch set at all! This series only contains the things I thought were non-radical really.

In fact even with the subsequent patches from my 2nd series I'll eventually post post -- these fatal issues are not cured at all unless we dance with userspace a bit, or unless as you suggest we have *all* pending theads wait without killing any.

*This* small patch should enable folks to move the needle to a more *fair* limit, its also useful to backport a simple fix even if the other stuff is not merged, *but* it *also* provides a way for systems to move away from the slippery slope if they know what they are doing.

Quoted 77 lines · Luis R. Rodriguez

Quoted 75 lines · Dmitry Torokhov

Quoted 73 lines · Luis R. Rodriguez

Quoted 44 lines · Dmitry Torokhov

Quoted 43 lines · Luis R. Rodriguez

On May 19, 2017 1:45 PM, "Dmitry Torokhov" <[email protected]> wrote:

Quoted 23 lines · Luis R. Rodriguez

We currently statically limit the number of modprobe threads which we allow to run concurrently to 50. As per Keith Owens, this was a completely arbitrary value, and it was set in the 2.3.38 days [0] over 16 years ago in year 2000.

Although we haven't yet hit our lower limits, experimentation [1] shows that when and if we hit this limit in the worst case, will be fatal -- consider get_fs_type() failures upon mount on a system which has many partitions, some of which might even be with the same filesystem. Its best to be prudent and increase and set this value to something more sensible which ensures we're far from hitting the limit and also allows default build/user run time override.

The worst case is fatal given that once a module fails to load there is a period of time during which subsequent request for the same module will fail, so in the case of partitions its not just one request that could fail, but whole series of partitions. This later issue of a module request failure domino effect can be addressed later, but increasing the limit to something more meaninful should at least give us enough cushion to avoid this for a while.

Set this value up with a bit more meaninful modern limits:

Quoted 8 lines

Bump this up to 64 max for small systems (CONFIG_BASE_SMALL) Bump this up to 128 max for larger systems (!CONFIG_BASE_SMALL)

Also allow the default max limit to be further fine tuned at compile time and at initialization at run time at boot up using the kernel
parameter: max_modprobes.

[0] https://git.kernel.org/cgit/linux/kernel/git/history/

history.git/commit/?id=ab1c4ec7410f6ec64e1511d1a7d850fc99c09b44

Quoted 1 line

[1] https://github.com/mcgrof/test_request_module

If we actually run into this issue, instead of slamming the system with bazillion concurrent requests, can we wait for the other modprobes to finish and then continue?

Quoted 6 lines

Quoted 4 lines

Yes ! That I have a patch that does precisely that ! That is actually still *not enough* to not fail fatally but this would be subject of another series with more debatable approaches.

Then please post it.

Will do.

Quoted 5 lines

Quoted 2 lines

This at least pushes us to closer safer limits for now while also making it configurable.

Making it configurable depending on how big/little box is makes no sense,

If we set a hard limit then we need to patch a system if we need to increment it. This is rather stupid given we have no current heuristics to make kmod loading deterministic from userspace, and in the worst case this can be fatal. General system size is a good first guess, but making it configurable is really key given current limitations. I'll post further patches which reveals some of these issues more clearly.

Quoted 3 lines

especially if the above is implemented, as depth of modprobe invocations depends on configuration and not computing power of the hardware the system is running on.

You seem to agree making it configurable is sensible , but not depending on the system size ?

Quoted 7 lines

Quoted 3 lines

No, I am saying that making it configurable based on system size makes no sense at all, and making it configurable given you already have patches removing hard failures gives no benefit.

Ah no, the problem is that hard failures are not yet removed in this patch set at all! This series only contains the things I thought were non-radical really.

I know they are not removed in this patch set.

Quoted 1 line

Quoted 4 lines

In fact even with the subsequent patches from my 2nd series I'll eventually post post -- these fatal issues are not cured at all unless we dance with userspace a bit, or unless as you suggest we have *all* pending theads wait without killing any.

Well, that is too bad, I understood you already implemented what I suggested.

Quoted 4 lines

*This* small patch should enable folks to move the needle to a more *fair* limit, its also useful to backport a simple fix even if the other stuff is not merged, *but* it *also* provides a way for systems to move away from the slippery slope if they know what they are doing.

Look, you are trying to push a band-aid solution for a problem that is purely theoretical (as you say in your patch description we are not hitting this problem in practice, only your test does).

There is
no slippery slope for systems to move away, no need to backport anything. We seem to agree that a better solution is possible (throttle number of concurrently running modprobes without killing requesters), and with that solution the band-aid will no longer be needed.

So please implement and post the proper fix for the issue.

On Thu, May 25, 2017 at 10:30 AM, Dmitry Torokhov

Quoted 7 lines

There is
no slippery slope for systems to move away, no need to backport anything. We seem to agree that a better solution is possible (throttle number of concurrently running modprobes without killing requesters), and with that solution the band-aid will no longer be needed.

So please implement and post the proper fix for the issue.

Alright, will do away with this patch and just go for the jugular of the issue.

Quoted 7 lines · Luis R. Rodriguez

On Thu, May 25, 2017 at 10:30 AM, Dmitry Torokhov

Quoted 6 lines

There is
no slippery slope for systems to move away, no need to backport anything. We seem to agree that a better solution is possible (throttle number of concurrently running modprobes without killing requesters), and with that solution the band-aid will no longer be needed.

Quoted 3 lines

Quoted 1 line

So please implement and post the proper fix for the issue.

Alright, will do away with this patch and just go for the jugular of the issue.

I gave this some more thought, even if we go with the throttling right away in practice you'll end up with a dmesg notice of a throttle kicking in once you *do*

reach this. We are forcing only 50 concurrent threads and making this a static limit with no good reason than 2.3.38 days evaluation from 16 years ago (2000). If we throttle we are going to throttle with a 2.3.38 days limit. And you advocate that ?

Quoted 10 lines · Luis R. Rodriguez

Quoted 9 lines · Luis R. Rodriguez

On Thu, May 25, 2017 at 10:30 AM, Dmitry Torokhov

Quoted 7 lines

There is
no slippery slope for systems to move away, no need to backport anything. We seem to agree that a better solution is possible (throttle number of concurrently running modprobes without killing requesters), and with that solution the band-aid will no longer be needed.

So please implement and post the proper fix for the issue.

Quoted 4 lines

Quoted 1 line

Alright, will do away with this patch and just go for the jugular of the issue.

I gave this some more thought, even if we go with the throttling right away in practice you'll end up with a dmesg notice of a throttle kicking in once you *do*

So remove it. The warning was meaningful when we rejected requests, now it is not.

Quoted 4 lines

reach this. We are forcing only 50 concurrent threads and making this a static limit with no good reason than 2.3.38 days evaluation from 16 years ago (2000). If we throttle we are going to throttle with a 2.3.38 days limit. And you advocate that ?

Yes. Can you give me reason why slamming the system with more than 50 modprobes is a good idea in 4.12 days? Does the increased limit decreases boot time? By how much?

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Luis R. Rodriguez

Quoted 9 lines · Luis R. Rodriguez

On Thu, May 25, 2017 at 10:30 AM, Dmitry Torokhov

Quoted 7 lines

There is
no slippery slope for systems to move away, no need to backport anything. We seem to agree that a better solution is possible (throttle number of concurrently running modprobes without killing requesters), and with that solution the band-aid will no longer be needed.

So please implement and post the proper fix for the issue.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Alright, will do away with this patch and just go for the jugular of the issue.

I gave this some more thought, even if we go with the throttling right away in practice you'll end up with a dmesg notice of a throttle kicking in once you *do*

So remove it. The warning was meaningful when we rejected requests, now it is not.

Great.

Quoted 5 lines

Quoted 4 lines

reach this. We are forcing only 50 concurrent threads and making this a static limit with no good reason than 2.3.38 days evaluation from 16 years ago (2000). If we throttle we are going to throttle with a 2.3.38 days limit. And you advocate that ?

Quoted 3 lines

Yes. Can you give me reason why slamming the system with more than 50 modprobes is a good idea in 4.12 days? Does the increased limit decreases boot time? By how much?

If in practice we are not hitting the limit the point is moot, and when we do I agree we can re-evaluate. With my stress test driver on a test case we can push as hard as bringing out the OOM killer even if we throttle, fun.

The module list has been using RCU in a lot of other calls for a while now, we just overlooked changing this one over to use RCU.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/module.c +0 -0

index 4a3665f8f837..70f494638974 100644

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c +10 -2

-		return 0;
+		goto out;
+
 	atomic_dec(&kmod_concurrent);
-	return -EBUSY;
+	ret = -EBUSY;
+out:
+	preempt_enable();
+	return ret;
 }
 
 static void kmod_umh_threads_put(void)
 {
+	/*
+	 * Preemption is not needed given once work is done we can
+	 * pace ourselves on our way out.
+	 */
 	atomic_dec(&kmod_concurrent);
 }
 

Signature · 1 line

2.11.0

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Quoted 39 lines · Dmitry Torokhov

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

Quoted 40 lines · Luis R. Rodriguez

Quoted 39 lines · Dmitry Torokhov

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 6 lines

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

I am saying that atomic_inc_return() will avoid situation where you have more than one threads incrementing the counter and believing that they are [not] allowed to start modprobe.

I have no idea why you think preempt_disable() would help here. It only ensures that current thread will not be preempted between the point where you update the counter and where you check the result. It does not stop interrupts nor does it affect other threads that might be updating the same counter.

Quoted 52 lines · Dmitry Torokhov

Quoted 40 lines · Luis R. Rodriguez

Quoted 39 lines · Dmitry Torokhov

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 6 lines

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

I am saying that atomic_inc_return() will avoid situation where you have more than one threads incrementing the counter and believing that they are [not] allowed to start modprobe.

Quoted 5 lines

I have no idea why you think preempt_disable() would help here. It only ensures that current thread will not be preempted between the point where you update the counter and where you check the result. It does not stop interrupts nor does it affect other threads that might be updating the same counter.

The preemption was inspired by __module_get() and try_module_get(), was that rather silly ?

Quoted 53 lines · Luis R. Rodriguez

Quoted 52 lines · Dmitry Torokhov

Quoted 40 lines · Luis R. Rodriguez

Quoted 39 lines · Dmitry Torokhov

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 6 lines

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

I am saying that atomic_inc_return() will avoid situation where you have more than one threads incrementing the counter and believing that they are [not] allowed to start modprobe.

Quoted 8 lines

Quoted 5 lines

I have no idea why you think preempt_disable() would help here. It only ensures that current thread will not be preempted between the point where you update the counter and where you check the result. It does not stop interrupts nor does it affect other threads that might be updating the same counter.

The preemption was inspired by __module_get() and try_module_get(), was that rather silly ?

As far as I can see prrempt_disable() was needed in __module_get() when modules user per-cpu refcounts: you did not want to move away from CPU while manipulating refcount.

Now that modules use simple atomics for refcounting I think these preempt_disable() and preempt_enable() can be removed.

Quoted 63 lines · Dmitry Torokhov

Quoted 53 lines · Luis R. Rodriguez

Quoted 52 lines · Dmitry Torokhov

Quoted 40 lines · Luis R. Rodriguez

Quoted 39 lines · Dmitry Torokhov

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 6 lines

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

I am saying that atomic_inc_return() will avoid situation where you have more than one threads incrementing the counter and believing that they are [not] allowed to start modprobe.

Quoted 8 lines

Quoted 5 lines

I have no idea why you think preempt_disable() would help here. It only ensures that current thread will not be preempted between the point where you update the counter and where you check the result. It does not stop interrupts nor does it affect other threads that might be updating the same counter.

The preemption was inspired by __module_get() and try_module_get(), was that rather silly ?

Quoted 6 lines

As far as I can see prrempt_disable() was needed in __module_get() when modules user per-cpu refcounts: you did not want to move away from CPU while manipulating refcount.

Now that modules use simple atomics for refcounting I think these preempt_disable() and preempt_enable() can be removed.

preempt_disable() still might be useful because you do the atomic_dec() when you reach the limit.

By other words, you have three operations that should be atomic:
inc, read, and dec. atomic_inc_return() covers only two of them.

Hmm, a solution might be to use atomic_dec_if_positive(). I would kmod_concurrent to something like kmod_concurrent_allowed, intialize it with the maximum allowed number. Then you could do:

static int kmod_umh_threads_get(void)

{
	if (atomic_dec_if_positive(kmod_concurrent_available) < 0)
		return -EBUSY;
	return 0;
}

Quoted 67 lines · Dmitry Torokhov

Quoted 53 lines · Luis R. Rodriguez

Quoted 52 lines · Dmitry Torokhov

Quoted 40 lines · Luis R. Rodriguez

Quoted 39 lines · Dmitry Torokhov

Quoted 25 lines · Luis R. Rodriguez

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 6 lines

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

I am saying that atomic_inc_return() will avoid situation where you have more than one threads incrementing the counter and believing that they are [not] allowed to start modprobe.

Quoted 8 lines

Quoted 5 lines

I have no idea why you think preempt_disable() would help here. It only ensures that current thread will not be preempted between the point where you update the counter and where you check the result. It does not stop interrupts nor does it affect other threads that might be updating the same counter.

The preemption was inspired by __module_get() and try_module_get(), was that rather silly ?

As far as I can see prrempt_disable() was needed in __module_get() when modules user per-cpu refcounts: you did not want to move away from CPU while manipulating refcount.

Quoted 2 lines

Now that modules use simple atomics for refcounting I think these preempt_disable() and preempt_enable() can be removed.

preempt_disable() still might be useful because you do the atomic_dec() when you reach the limit.

By other words, you have three operations that should be atomic:
inc, read, and dec. atomic_inc_return() covers only two of them.

Hmm, a solution might be to use atomic_dec_if_positive(). I would kmod_concurrent to something like kmod_concurrent_allowed, intialize it with the maximum allowed number. Then you could do:

static int kmod_umh_threads_get(void)

{
	if (atomic_dec_if_positive(kmod_concurrent_available) < 0)
		return -EBUSY;
	return 0;
}

Quoted 53 lines · Dmitry Torokhov

Quoted 52 lines · Luis R. Rodriguez

Quoted 40 lines · Dmitry Torokhov

Quoted 39 lines · Luis R. Rodriguez

Quoted 25 lines

In theory it is possible multiple concurrent threads will try to kmod_umh_threads_get() and as such atomic_inc(&kmod_concurrent) at the same time, therefore enabling a small time during which we've bumped kmod_concurrent but have not really enabled work. By using preemption we mitigate this a bit.

Preemption is not needed when we kmod_umh_threads_put().

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

kernel/kmod.c +4 -0

index 563600fc9bb1..7ea11dbc7564 100644
@@ -113,15 +113,35 @@ static int call_modprobe(char *module_name, int wait)
 
 static int kmod_umh_threads_get(void)
 {
+	int ret = 0;
+
+	/*
+	 * Disabling preemption makes sure that we are not rescheduled here

kernel/kmod.c static int call_modprobe(char *module_name, int wait) · line 129

Quoted 10 lines

kernel/kmod.c +8 -0

+	 *
+	 * Also preemption helps kmod_concurrent is not increased by mistake
+	 * for too long given in theory two concurrent threads could race on
+	 * atomic_inc() before we atomic_read() -- we know that's possible
+	 * and but we don't care, this is not used for object accounting and
+	 * is just a subjective threshold. The alternative is a lock.
+	 */
+	preempt_disable();
 	atomic_inc(&kmod_concurrent);
 	if (atomic_read(&kmod_concurrent) <= max_modprobes)

That is very "fancy" way to basically say:

Quoted 6 lines

Quoted 1 line

if (atomic_inc_return(&kmod_concurrent) <= max_modprobes)

Do you mean to combine the atomic_inc() and atomic_read() in one as you noted (as that is not a change in this patch), *or* that using a memory barrier here with atomic_inc_return() should suffice to address the same and avoid an explicit preemption enable / disable ?

I am saying that atomic_inc_return() will avoid situation where you have more than one threads incrementing the counter and believing that they are [not] allowed to start modprobe.

Quoted 8 lines

Quoted 5 lines

I have no idea why you think preempt_disable() would help here. It only ensures that current thread will not be preempted between the point where you update the counter and where you check the result. It does not stop interrupts nor does it affect other threads that might be updating the same counter.

The preemption was inspired by __module_get() and try_module_get(), was that rather silly ?

As far as I can see prrempt_disable() was needed in __module_get() when modules user per-cpu refcounts: you did not want to move away from CPU while manipulating refcount.

Now that modules use simple atomics for refcounting I think these preempt_disable() and preempt_enable() can be removed.

Just use the simplified rate limit printk when the max modprobe limit is reached, while at it throw out a bone should the error be triggered.

Reviewed-by Petr Mladek <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

kernel/kmod.c +0 -1

index 7ea11dbc7564..56cd2a16e7ac 100644
@@ -166,7 +166,6 @@ int __request_module(bool wait, const char *fmt, ...)
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
 	int ret;
-	static int kmod_loop_msg;
 
 	/*
 	 * We don't allow synchronous module loading from async.  Module

@@ -191,13 +190,8 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	ret = kmod_umh_threads_get();

Quoted 24 lines · Luis R. Rodriguez

Just use the simplified rate limit printk when the max modprobe limit is reached, while at it throw out a bone should the error be triggered.

Reviewed-by Petr Mladek <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

kernel/kmod.c +0 -1

index 7ea11dbc7564..56cd2a16e7ac 100644
@@ -166,7 +166,6 @@ int __request_module(bool wait, const char *fmt, ...)
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
 	int ret;
-	static int kmod_loop_msg;
 
 	/*
 	 * We don't allow synchronous module loading from async.  Module

@@ -191,13 +190,8 @@ int __request_module(bool wait, const char *fmt, ...)
 

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 194

Quoted 11 lines

kernel/kmod.c +2 -7

 	ret = kmod_umh_threads_get();
 	if (ret) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg < 5) {
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-			kmod_loop_msg++;
-		}
+		pr_err_ratelimited("%s: module \"%s\" reached limit (%u) of concurrent modprobe calls\n",
+				   __func__, module_name, max_modprobes);

This is completely different behavior, isn't it? Instead of reporting first 5 occurrences we now reporting every once in a while. Why is this new behavior better?

When checking if we want to allow a kmod thread to kick off we increment, then read to see if we should enable a thread. If we were over the allowed limit limit we decrement. Splitting the increment far apart from decrement means there could be a time where two increments happen potentially giving a false failure on a thread which should have been allowed.

CPU1 CPU2
atomic_inc()
atomic_inc()
atomic_read()
atomic_read()
atomic_dec()
atomic_dec()

In this case a read on CPU1 gets the atomic_inc()'s and we could negate it from getting a kmod thread. We could try to prevent this with a lock or preemption but that is overkill. We can fix by reducing the number of atomic operations. We do this by inverting the logic of of the enabler, instead of incrementing kmod_concurrent as we get new kmod users, define the variable kmod_concurrent_max as the max number of currently allowed kmod users and as we get new kmod users just decrement it if its still positive. This combines the dec and read in one atomic operation.

In this case we no longer get the same false failure:

CPU1 CPU2
atomic_dec_if_positive()
atomic_dec_if_positive()
atomic_inc()
atomic_inc()

Suggested-by Petr Mladek <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 include/linux/kmod.h |  2 ++
 init/main.c          |  1 +
 kernel/kmod.c        | 44 +++++++++++++++++++++++++-------------------
 3 files changed, 28 insertions(+), 19 deletions(-)

include/linux/kmod.h +2 -0

index c4e441e00db5..8e2f302b214a 100644
@@ -38,10 +38,12 @@ int __request_module(bool wait, const char *name, ...);
 #define request_module_nowait(mod...) __request_module(false, mod)
 #define try_then_request_module(x, mod...) \
 	((x) ?: (__request_module(true, mod), (x)))
+void init_kmod_umh(void);
 #else
 static inline int request_module(const char *name, ...) { return -ENOSYS; }
 static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
+static inline void init_kmod_umh(void) { }
 #endif
 
 

init/main.c +1 -0

index 9ec09ff8a930..9b20be716cf7 100644
@@ -650,6 +650,7 @@ asmlinkage __visible void __init start_kernel(void)
 	thread_stack_cache_init();
 	cred_init();
 	fork_init();
+	init_kmod_umh();
 	proc_caches_init();
 	buffer_init();
 	key_init();

kernel/kmod.c +13 -19

index 563f97e2be36..cafd27b92d19 100644
@@ -46,6 +46,7 @@
 #include <trace/events/module.h>
 
 extern int max_threads;
+unsigned int max_modprobes;
 
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2

@@ -56,6 +57,8 @@ static DEFINE_SPINLOCK(umh_sysctl_lock);
 static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
+static atomic_t kmod_concurrent_max = ATOMIC_INIT(0);
+#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 
 /*
 	modprobe_path is set via /proc/sys.

@@ -127,10 +130,7 @@ int __request_module(bool wait, const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
 	/*

@@ -154,21 +154,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	if (ret)
 		return ret;
 
-	/* If modprobe needs a service that is in a module, we get a recursive
-	 * loop.  Limit the number of running kmod threads to max_threads/2 or
-	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
-	 * would be to run the parents of this process, counting how many times
-	 * kmod was invoked.  That would mean accessing the internals of the
-	 * process tables to get the command line, proc_pid_cmdline is static
-	 * and it is not worth changing the proc code just to handle this case. 
-	 * KAO.
-	 *
-	 * "trace the ppid" is simple, but will fail if someone's
-	 * parent exits.  I think this is as good as it gets. --RR
-	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
+	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
 		/* We may be blaming an innocent here, but unlikely */
 		if (kmod_loop_msg < 5) {
 			printk(KERN_ERR

@@ -184,10 +170,30 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 
-	atomic_dec(&kmod_concurrent);
+	atomic_inc(&kmod_concurrent_max);
+
 	return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
+/*
+ * If modprobe needs a service that is in a module, we get a recursive
+ * loop.  Limit the number of running kmod threads to max_threads/2 or
+ * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
+ * would be to run the parents of this process, counting how many times
+ * kmod was invoked.  That would mean accessing the internals of the

Quoted 140 lines · Luis R. Rodriguez

When checking if we want to allow a kmod thread to kick off we increment, then read to see if we should enable a thread. If we were over the allowed limit limit we decrement. Splitting the increment far apart from decrement means there could be a time where two increments happen potentially giving a false failure on a thread which should have been allowed.

CPU1 CPU2
atomic_inc()
atomic_inc()
atomic_read()
atomic_read()
atomic_dec()
atomic_dec()

In this case a read on CPU1 gets the atomic_inc()'s and we could negate it from getting a kmod thread. We could try to prevent this with a lock or preemption but that is overkill. We can fix by reducing the number of atomic operations. We do this by inverting the logic of of the enabler, instead of incrementing kmod_concurrent as we get new kmod users, define the variable kmod_concurrent_max as the max number of currently allowed kmod users and as we get new kmod users just decrement it if its still positive. This combines the dec and read in one atomic operation.

In this case we no longer get the same false failure:

CPU1 CPU2
atomic_dec_if_positive()
atomic_dec_if_positive()
atomic_inc()
atomic_inc()

Suggested-by Petr Mladek <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 include/linux/kmod.h |  2 ++
 init/main.c          |  1 +
 kernel/kmod.c        | 44 +++++++++++++++++++++++++-------------------
 3 files changed, 28 insertions(+), 19 deletions(-)

include/linux/kmod.h +2 -0

index c4e441e00db5..8e2f302b214a 100644
@@ -38,10 +38,12 @@ int __request_module(bool wait, const char *name, ...);
 #define request_module_nowait(mod...) __request_module(false, mod)
 #define try_then_request_module(x, mod...) \
 	((x) ?: (__request_module(true, mod), (x)))
+void init_kmod_umh(void);
 #else
 static inline int request_module(const char *name, ...) { return -ENOSYS; }
 static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
+static inline void init_kmod_umh(void) { }
 #endif
 
 

init/main.c +1 -0

index 9ec09ff8a930..9b20be716cf7 100644
@@ -650,6 +650,7 @@ asmlinkage __visible void __init start_kernel(void)
 	thread_stack_cache_init();
 	cred_init();
 	fork_init();
+	init_kmod_umh();
 	proc_caches_init();
 	buffer_init();
 	key_init();

kernel/kmod.c +13 -19

index 563f97e2be36..cafd27b92d19 100644
@@ -46,6 +46,7 @@
 #include <trace/events/module.h>
 
 extern int max_threads;
+unsigned int max_modprobes;
 
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2

@@ -56,6 +57,8 @@ static DEFINE_SPINLOCK(umh_sysctl_lock);
 static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
+static atomic_t kmod_concurrent_max = ATOMIC_INIT(0);
+#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 
 /*
 	modprobe_path is set via /proc/sys.

@@ -127,10 +130,7 @@ int __request_module(bool wait, const char *fmt, ...)
 {
 	va_list args;
 	char module_name[MODULE_NAME_LEN];
-	unsigned int max_modprobes;
 	int ret;
-	static atomic_t kmod_concurrent = ATOMIC_INIT(0);
-#define MAX_KMOD_CONCURRENT 50	/* Completely arbitrary value - KAO */
 	static int kmod_loop_msg;
 
 	/*

@@ -154,21 +154,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	if (ret)
 		return ret;
 
-	/* If modprobe needs a service that is in a module, we get a recursive
-	 * loop.  Limit the number of running kmod threads to max_threads/2 or
-	 * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
-	 * would be to run the parents of this process, counting how many times
-	 * kmod was invoked.  That would mean accessing the internals of the
-	 * process tables to get the command line, proc_pid_cmdline is static
-	 * and it is not worth changing the proc code just to handle this case. 
-	 * KAO.
-	 *
-	 * "trace the ppid" is simple, but will fail if someone's
-	 * parent exits.  I think this is as good as it gets. --RR
-	 */
-	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
-	atomic_inc(&kmod_concurrent);
-	if (atomic_read(&kmod_concurrent) > max_modprobes) {
+	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
 		/* We may be blaming an innocent here, but unlikely */
 		if (kmod_loop_msg < 5) {
 			printk(KERN_ERR

@@ -184,10 +170,30 @@ int __request_module(bool wait, const char *fmt, ...)
 
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 
-	atomic_dec(&kmod_concurrent);
+	atomic_inc(&kmod_concurrent_max);
+
 	return ret;
 }
 EXPORT_SYMBOL(__request_module);
+
+/*
+ * If modprobe needs a service that is in a module, we get a recursive
+ * loop.  Limit the number of running kmod threads to max_threads/2 or
+ * MAX_KMOD_CONCURRENT, whichever is the smaller.  A cleaner method
+ * would be to run the parents of this process, counting how many times
+ * kmod was invoked.  That would mean accessing the internals of the

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 194

Quoted 10 lines

kernel/kmod.c +10 -0

+ * process tables to get the command line, proc_pid_cmdline is static
+ * and it is not worth changing the proc code just to handle this case.
+ *
+ * "trace the ppid" is simple, but will fail if someone's
+ * parent exits.  I think this is as good as it gets.
+ */
+void __init init_kmod_umh(void)
+{
+	max_modprobes = min(max_threads/2, MAX_KMOD_CONCURRENT);
+	atomic_set(&kmod_concurrent_max, max_modprobes);

I would love if we could initialize atomic statically. So the trouble we are trying to solve here is we create more threads than kernel supports, with thread count being calculated as:

	threads = div64_u64((u64) totalram_pages * (u64) PAGE_SIZE,
			    (u64) THREAD_SIZE * 8UL);

So to not being serve 50 threads we need to deal with system smaller than 3200 pages, or ~13M memory (assume thread size is 8 pages - 64 bit with kasan, smaller page sizes reduce memory even more). Can you run 4.12 with modules support on machine with such memory?

So maybe we shoudl simply say:

static atomic_t kmod_concurrent_max = ATOMIC_INIT(MAX_KMOD_CONCURRENT_MAX);

and call it a day? So we do not need init_kmod_umh() and don't need to call it from init/main.c.

This adds helpers for getting access to the kmod limit from userspace. This knob should help userspace more gracefully and deterministically handle module loading.

Cc Tom Gundersen <[email protected]>

Cc Petr Mladek <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/sysctl/kernel.txt | 20 ++++++++++++++++++++
 include/linux/kmod.h            |  5 +++++
 kernel/kmod.c                   | 33 +++++++++++++++++++++++++++++++++
 kernel/sysctl.c                 |  7 +++++++
 4 files changed, 65 insertions(+)

Documentation/sysctl/kernel.txt +20 -0

index bac23c198360..080ccdca1f10 100644
@@ -370,6 +370,26 @@ with the "modules_disabled" sysctl.
 
 ==============================================================
 
+kmod-limit:
+
+Get the max amount of concurrent requests (kmod_concurrent) the kernel can
+make out to userspace to call 'modprobe'. This limit is known internally to the
+kernel as max_modprobes. This interface is designed to enable userspace to
+query the kernel for the max_modprobes limit so userspace can more
+deterministically handle module loading by only enabling max_modprobes
+'modprobe' calls at a time.
+
+Dependencies are resolved in userspace through depmod, so one modprobe
+call only bumps the number of concurrent threads (kmod_concurrent) by one.
+Dependencies for a module then are loaded directly in userspace using
+init_module() / finit_module() skipping bumping kmod_concurrent or being
+affected by max_modprobes.
+
+The max_modprobes value is set at build time with CONFIG_MAX_KMOD_CONCURRENT.
+You can override at initialization with the module parameter max_modprobes.
+
+==============================================================
+
 kptr_restrict:
 
 This toggle indicates whether restrictions are placed on

include/linux/kmod.h +5 -0

index 8e2f302b214a..bbc6d59190aa 100644
@@ -39,13 +39,18 @@ int __request_module(bool wait, const char *name, ...);
 #define try_then_request_module(x, mod...) \
 	((x) ?: (__request_module(true, mod), (x)))
 void init_kmod_umh(void);
+unsigned int get_kmod_umh_limit(void);
+int sysctl_kmod_limit(struct ctl_table *table, int write,
+		      void __user *buffer, size_t *lenp, loff_t *ppos);
 #else
 static inline int request_module(const char *name, ...) { return -ENOSYS; }
 static inline int request_module_nowait(const char *name, ...) { return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
 static inline void init_kmod_umh(void) { }
+static inline unsigned int get_kmod_umh_limit(void) { return 0; }
 #endif
 
+#define get_kmod_umh_limit get_kmod_umh_limit
 
 struct cred;
 struct file;

kernel/kmod.c +33 -0

index cafd27b92d19..17de776cf368 100644
@@ -111,6 +111,17 @@ static int call_modprobe(char *module_name, int wait)
 }
 
 /**
+ * get_kmod_umh_limit - get concurrent modprobe thread limit
+ *
+ * Returns the number of allowed concurrent modprobe calls.
+ */
+unsigned int get_kmod_umh_limit(void)
+{
+	return max_modprobes;
+}
+EXPORT_SYMBOL_GPL(get_kmod_umh_limit);
+
+/**
  * __request_module - try to load a kernel module
  * @wait: wait (or not) for the operation to complete
  * @fmt: printf style format string for the name of the module

@@ -194,6 +205,28 @@ void __init init_kmod_umh(void)
 	atomic_set(&kmod_concurrent_max, max_modprobes);
 }
 
+int sysctl_kmod_limit(struct ctl_table *table, int write,
+		      void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	struct ctl_table t;
+	int ret;
+	unsigned int local_max_modprobes = max_modprobes;
+	unsigned int min = 0;
+	unsigned int max = max_threads/2;
+
+	t = *table;
+	t.data = &local_max_modprobes;
+	t.extra1 = &min;
+	t.extra2 = &max;
+
+	if (write)
+		return -EPERM;
+
+	ret = proc_douintvec_minmax(&t, write, buffer, lenp, ppos);
+
+	return ret;
+}
+
 #endif /* CONFIG_MODULES */
 
 static void call_usermodehelper_freeinfo(struct subprocess_info *info)

kernel/sysctl.c +7 -0

index df9f2a367882..d1c1d1999bb1 100644
@@ -682,6 +682,13 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &one,
 		.extra2		= &one,
 	},
+	{
+		.procname	= "kmod-limit",
+		.data		= NULL, /* filled in by handler */
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0444,
+		.proc_handler	= sysctl_kmod_limit,
+	},
 #endif
 #ifdef CONFIG_UEVENT_HELPER
 	{

Signature · 1 line

2.11.0

Quoted 3 lines · Luis R. Rodriguez

This adds helpers for getting access to the kmod limit from userspace. This knob should help userspace more gracefully and deterministically handle module loading.

I think more details is needed before we add a new ABI to the kernel. Why can't userspace submit as much as it wants and the kernel decide how much it will service at once?

atomic operations. We do this by inverting the logic of of the enabler, instead of incrementing kmod_concurrent as we get new kmod users, define the variable kmod_concurrent_max as the max number of currently allowed kmod users and as we get new kmod users just decrement it if its still positive. This combines the dec and read in one atomic operation.

In this case we no longer get the same false failure:

CPU1 CPU2
atomic_dec_if_positive()
atomic_dec_if_positive()
atomic_inc()
atomic_inc()

Suggested-by Petr Mladek <[email protected]>

Suggested-by Dmitry Torokhov <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

If we reach the limit of modprobe_limit threads running the next request_module() call will fail. The original reason for adding a kill was to do away with possible issues with in old circumstances which would create a recursive series of request_module() calls. We can do better than just be super aggressive and reject calls once we've reached the limit by simply making pending callers wait until the threshold has been reduced.

The only difference is the clutch helps with avoiding making request_module() requests fatal more often. With x86_64 qemu, with 4 cores, 4 GiB of RAM it takes the following run time to run both tests:

time ./kmod.sh -t 0008

real    0m12.364s
user    0m0.704s
sys     0m5.373s

time ./kmod.sh -t 0009

real    0m47.638s
user    0m1.033s
sys     0m5.425s

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 kernel/kmod.c                        | 16 +++++++---------
 tools/testing/selftests/kmod/kmod.sh | 24 ++----------------------
 2 files changed, 9 insertions(+), 31 deletions(-)

kernel/kmod.c +0 -1

index 3e346c700e80..46b12fed6fd0 100644
@@ -163,14 +163,11 @@ int __request_module(bool wait, const char *fmt, ...)
 		return ret;
 
 	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
-		/* We may be blaming an innocent here, but unlikely */

Quoted 5 lines · Luis R. Rodriguez

kernel/kmod.c +0 -0

@@ -163,14 +163,11 @@ int __request_module(bool wait, const char *fmt, ...)
 		return ret;
 

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 168

Quoted 12 lines

kernel/kmod.c +3 -8

 	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg < 5) {
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-			kmod_loop_msg++;
-		}
-		return -ENOMEM;
+		pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s\n, throttling...",
+				    atomic_read(&kmod_concurrent_max),
+				    50, module_name);

It is weird to pass the constant '50' via %s.

Also a #define should be
used to keep it in sync with the kmod_concurrent_max initialization.

Quoted 8 lines

kernel/kmod.c +2 -0

+		wait_event_interruptible(kmod_wq,
+					 atomic_dec_if_positive(&kmod_concurrent_max) >= 0);
 	}
 
 	trace_module_request(module_name, wait, _RET_IP_);

@@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 178

Quoted 2 lines

kernel/kmod.c +1 -0

 	atomic_inc(&kmod_concurrent_max);
+	wake_up_all(&kmod_wq);

Does it make sense to wake up all waiters when we released the resource only for one? IMHO, a simple wake_up() should be here.

I am sorry for the late review. The month ran really fast.

Quoted 6 lines · Petr Mladek

Quoted 5 lines · Luis R. Rodriguez

kernel/kmod.c +0 -0

@@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 

Quoted 5 lines

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 178

Quoted 2 lines

kernel/kmod.c +1 -0

 	atomic_inc(&kmod_concurrent_max);
+	wake_up_all(&kmod_wq);

Does it make sense to wake up all waiters when we released the resource only for one? IMHO, a simple wake_up() should be here.

Then we should wake_up() also on failure, otherwise we have the potential to not wake some in a proper time.

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Petr Mladek

Quoted 5 lines · Luis R. Rodriguez

kernel/kmod.c +0 -0

@@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 

Quoted 8 lines

Quoted 5 lines

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 178

Quoted 2 lines

kernel/kmod.c +1 -0

 	atomic_inc(&kmod_concurrent_max);
+	wake_up_all(&kmod_wq);

Does it make sense to wake up all waiters when we released the resource only for one? IMHO, a simple wake_up() should be here.

Then we should wake_up() also on failure, otherwise we have the potential to not wake some in a proper time.

I checked and it turns out we have no error paths after we consume a kmod ticket, if you will. Once we bump with atomic_dec_if_positive() we assume we're moving forward with an attempt, and the only failure path is already bundled with a wake at the end of the __request_module() call.

Then the next question would be *who* exactly gets woken up next if we just use wake_up() ? The common core wake up code varies depending on use and all this reminded me of the complexity we just don't need, so I have now converted to use swait. swait uses list_add() if empty and then iterates with list_first_entry() on wakeup, so that should get the first item added to the wait list.

Works with me. Will run a test a before v4 is sent, but since only 2 patches are modified will only send a respective update for these 2 patches.

Quoted 29 lines · Luis R. Rodriguez

Quoted 7 lines · Luis R. Rodriguez

Quoted 6 lines · Petr Mladek

Quoted 5 lines · Luis R. Rodriguez

kernel/kmod.c +0 -0

@@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 

Quoted 8 lines

Quoted 5 lines

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 178

Quoted 2 lines

kernel/kmod.c +1 -0

 	atomic_inc(&kmod_concurrent_max);
+	wake_up_all(&kmod_wq);

Does it make sense to wake up all waiters when we released the resource only for one? IMHO, a simple wake_up() should be here.

Then we should wake_up() also on failure, otherwise we have the potential to not wake some in a proper time.

I checked and it turns out we have no error paths after we consume a kmod ticket, if you will. Once we bump with atomic_dec_if_positive() we assume we're moving forward with an attempt, and the only failure path is already bundled with a wake at the end of the __request_module() call.

Then the next question would be *who* exactly gets woken up next if we just use wake_up() ? The common core wake up code varies depending on use and all this reminded me of the complexity we just don't need, so I have now converted to use swait. swait uses list_add() if empty and then iterates with list_first_entry() on wakeup, so that should get the first item added to the wait list.

Quoted 2 lines

Works with me. Will run a test a before v4 is sent, but since only 2 patches are modified will only send a respective update for these 2 patches.

Alright, this worked out well! Its just a tiny bit slower on test cases 0008 and 0009 (few seconds) but that's fine, its natural due to the lack of the swake_up_all().

Quoted 6 lines · Petr Mladek

Quoted 5 lines · Luis R. Rodriguez

kernel/kmod.c +0 -0

@@ -163,14 +163,11 @@ int __request_module(bool wait, const char *fmt, ...)
 		return ret;
 

Quoted 14 lines

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 168

Quoted 12 lines

kernel/kmod.c +3 -8

 	if (atomic_dec_if_positive(&kmod_concurrent_max) < 0) {
-		/* We may be blaming an innocent here, but unlikely */
-		if (kmod_loop_msg < 5) {
-			printk(KERN_ERR
-			       "request_module: runaway loop modprobe %s\n",
-			       module_name);
-			kmod_loop_msg++;
-		}
-		return -ENOMEM;
+		pr_warn_ratelimited("request_module: kmod_concurrent_max (%u) close to 0 (max_modprobes: %u), for module %s\n, throttling...",
+				    atomic_read(&kmod_concurrent_max),
+				    50, module_name);

It is weird to pass the constant '50' via %s.

The 50 was passed with %u, so I take it you meant it is odd to use a parameter for it.

Quoted 2 lines

Also a #define should be
used to keep it in sync with the kmod_concurrent_max initialization.

OK.

Quoted 8 lines

Quoted 8 lines

kernel/kmod.c +2 -0

+		wait_event_interruptible(kmod_wq,
+					 atomic_dec_if_positive(&kmod_concurrent_max) >= 0);
 	}
 
 	trace_module_request(module_name, wait, _RET_IP_);

@@ -178,6 +175,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC);
 

Quoted 5 lines

kernel/kmod.c int __request_module(bool wait, const char *fmt, ...) · line 178

Quoted 2 lines

kernel/kmod.c +1 -0

 	atomic_inc(&kmod_concurrent_max);
+	wake_up_all(&kmod_wq);

Does it make sense to wake up all waiters when we released the resource only for one? IMHO, a simple wake_up() should be here.

Then we should wake_up() also on failure, otherwise we have the potential to not wake some in a proper time.

If we reach the limit of modprobe_limit threads running the next request_module() call will fail. The original reason for adding a kill was to do away with possible issues with in old circumstances which would create a recursive series of request_module() calls.

We can do better than just be super aggressive and reject calls once we've reached the limit by simply making pending callers wait until the threshold has been reduced, and then throttling them in, one by one.

This throttling enables requests over the kmod concurrent limit to be processed once a pending request completes. Only the first item queued up to wait is woken up. The assumption here is once a task is woken it will have no other option to also kick the queue to check if there are more pending tasks -- regardless of whether or not it was successful.

By throttling and processing only max kmod concurrent tasks we ensure we avoid unexpected fatal request_module() calls, and we keep memory consumption on module loading to a minimum.

With x86_64 qemu, with 4 cores, 4 GiB of RAM it takes the following run time to run both tests:

time ./kmod.sh -t 0008

real    0m16.523s
user    0m0.879s
sys     0m8.977s

time ./kmod.sh -t 0009

real    0m56.080s
user    0m0.717s
sys     0m10.324s

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 29 lines · Luis R. Rodriguez

If we reach the limit of modprobe_limit threads running the next request_module() call will fail. The original reason for adding a kill was to do away with possible issues with in old circumstances which would create a recursive series of request_module() calls.

We can do better than just be super aggressive and reject calls once we've reached the limit by simply making pending callers wait until the threshold has been reduced, and then throttling them in, one by one.

This throttling enables requests over the kmod concurrent limit to be processed once a pending request completes. Only the first item queued up to wait is woken up. The assumption here is once a task is woken it will have no other option to also kick the queue to check if there are more pending tasks -- regardless of whether or not it was successful.

By throttling and processing only max kmod concurrent tasks we ensure we avoid unexpected fatal request_module() calls, and we keep memory consumption on module loading to a minimum.

With x86_64 qemu, with 4 cores, 4 GiB of RAM it takes the following run time to run both tests:

time ./kmod.sh -t 0008

real    0m16.523s
user    0m0.879s
sys     0m8.977s

Quoted 6 lines

time ./kmod.sh -t 0009

real    0m56.080s
user    0m0.717s
sys     0m10.324s

Signed-off-by Luis R. Rodriguez <[email protected]>

All the changes look fine to me. They make perfect sense.

This v3 nukes the proc sysctl interface in favor for just letting userspace just check kernel revision. Prior to whenever this is merged userspace should try to avoid hammering more than 50 kmod threads as they can fail and it'd get -ENOMEM.

We do away with the old heuristics on assuming you could end up with less than max_threads/2 < 50 threads as Dmitry notes this would mean having a system with 16 MiB of RAM with modules enabled. It simplifies our patch "kmod: reduce atomic operations on kmod_concurrent" considerbly.

Since the sysctl interface is gone, this no longer depends on any other patches, the series is independent. As usual the series is available on my linux-next 20170526-kmod-only branch which is based on next-20170526.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only

Luis

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

Quoted 19 lines · Luis R. Rodriguez

This v3 nukes the proc sysctl interface in favor for just letting userspace just check kernel revision. Prior to whenever this is merged userspace should try to avoid hammering more than 50 kmod threads as they can fail and it'd get -ENOMEM.

We do away with the old heuristics on assuming you could end up with less than max_threads/2 < 50 threads as Dmitry notes this would mean having a system with 16 MiB of RAM with modules enabled. It simplifies our patch "kmod: reduce atomic operations on kmod_concurrent" considerbly.

Since the sysctl interface is gone, this no longer depends on any other patches, the series is independent. As usual the series is available on my linux-next 20170526-kmod-only branch which is based on next-20170526.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only

Luis

Quoted 5 lines

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Quoted 26 lines · Luis R. Rodriguez

Quoted 24 lines

This v3 nukes the proc sysctl interface in favor for just letting userspace just check kernel revision. Prior to whenever this is merged userspace should try to avoid hammering more than 50 kmod threads as they can fail and it'd get -ENOMEM.

We do away with the old heuristics on assuming you could end up with less than max_threads/2 < 50 threads as Dmitry notes this would mean having a system with 16 MiB of RAM with modules enabled. It simplifies our patch "kmod: reduce atomic operations on kmod_concurrent" considerbly.

Since the sysctl interface is gone, this no longer depends on any other patches, the series is independent. As usual the series is available on my linux-next 20170526-kmod-only branch which is based on next-20170526.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only

Luis

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

Quoted 2 lines

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Seems like going through Jessica's would make the most sense?

+++ Kees Cook [20/06/17 17:23 -0700]:

Quoted 27 lines · Luis R. Rodriguez

Quoted 26 lines · Luis R. Rodriguez

Quoted 24 lines

This v3 nukes the proc sysctl interface in favor for just letting userspace just check kernel revision. Prior to whenever this is merged userspace should try to avoid hammering more than 50 kmod threads as they can fail and it'd get -ENOMEM.

We do away with the old heuristics on assuming you could end up with less than max_threads/2 < 50 threads as Dmitry notes this would mean having a system with 16 MiB of RAM with modules enabled. It simplifies our patch "kmod: reduce atomic operations on kmod_concurrent" considerbly.

Since the sysctl interface is gone, this no longer depends on any other patches, the series is independent. As usual the series is available on my linux-next 20170526-kmod-only branch which is based on next-20170526.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only

Luis

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

Quoted 4 lines

Quoted 2 lines

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Seems like going through Jessica's would make the most sense?

Would be happy to take patches 01 (which I need to anyway), 02, possibly 04 if decoupled from the test driver (03).

I can't take patch 03 through my tree just yet, as I haven't had time to give it a look yet :-/

[ Side comment, it seems that kmod.c isn't directly maintained by anyone right now, perhaps Luis would be interested in picking it up? :-) ]

Quoted 14 lines · Jessica Yu

+++ Kees Cook [20/06/17 17:23 -0700]:

Quoted 12 lines · Luis R. Rodriguez

Quoted 9 lines · Luis R. Rodriguez

Quoted 5 lines

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Seems like going through Jessica's would make the most sense?

Quoted 2 lines

Would be happy to take patches 01 (which I need to anyway), 02, possibly 04 if decoupled from the test driver (03).

Feel free to decouple it, but note that then the commit log must then be changed. My own take is this fix is not so critical as it is a corner case, so I have instead preferred to couple in the test case and respective fix together. I'll leave it up to you how to proceed.

Quoted 15 lines · Luis R. Rodriguez

Quoted 14 lines · Jessica Yu

+++ Kees Cook [20/06/17 17:23 -0700]:

Quoted 12 lines · Luis R. Rodriguez

Quoted 9 lines · Luis R. Rodriguez

Quoted 5 lines

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Seems like going through Jessica's would make the most sense?

Quoted 7 lines

Quoted 2 lines

Would be happy to take patches 01 (which I need to anyway), 02, possibly 04 if decoupled from the test driver (03).

Feel free to decouple it, but note that then the commit log must then be changed. My own take is this fix is not so critical as it is a corner case, so I have instead preferred to couple in the test case and respective fix together. I'll leave it up to you how to proceed.

Note: Linus noted swait is actually very special use-case [0] so I'd hate to add a new use case not vetted for. This use case on kmod.c really does *not* require anything but a simple wait though, so really am inclined to let that through unless I hear back...

[0] https://lkml.kernel.org/r/[email protected]

Quoted 29 lines · Luis R. Rodriguez

Quoted 15 lines · Jessica Yu

Quoted 14 lines

+++ Kees Cook [20/06/17 17:23 -0700]:

Quoted 12 lines · Luis R. Rodriguez

Quoted 9 lines · Luis R. Rodriguez

Quoted 5 lines

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Seems like going through Jessica's would make the most sense?

Quoted 7 lines

Quoted 2 lines

Would be happy to take patches 01 (which I need to anyway), 02, possibly 04 if decoupled from the test driver (03).

Feel free to decouple it, but note that then the commit log must then be changed. My own take is this fix is not so critical as it is a corner case, so I have instead preferred to couple in the test case and respective fix together. I'll leave it up to you how to proceed.

Note: Linus noted swait is actually very special use-case [0] so I'd hate to add a new use case not vetted for. This use case on kmod.c really does *not* require anything but a simple wait though, so really am inclined to let that through unless I hear back...

Quoted 1 line

[0] https://lkml.kernel.org/r/[email protected]

Heh, I was not aware of this special case either. The welcoming comment of the swait API confused me as well.

In this light, I suggest to switch the patch to using the normal wait API.

Quoted 33 lines

+++ Kees Cook [20/06/17 17:23 -0700]:

Quoted 31 lines · Luis R. Rodriguez

Quoted 20 lines · Luis R. Rodriguez

Quoted 19 lines

This v3 nukes the proc sysctl interface in favor for just letting userspace just check kernel revision. Prior to whenever this is merged userspace should try to avoid hammering more than 50 kmod threads as they can fail and it'd get -ENOMEM.

We do away with the old heuristics on assuming you could end up with less than max_threads/2 < 50 threads as Dmitry notes this would mean having a system with 16 MiB of RAM with modules enabled. It simplifies our patch "kmod: reduce atomic operations on kmod_concurrent" considerbly.

Since the sysctl interface is gone, this no longer depends on any other patches, the series is independent. As usual the series is available on my linux-next 20170526-kmod-only branch which is based on next-20170526.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only

Luis

Quoted 8 lines

Quoted 5 lines

Luis R. Rodriguez (4):
module: use list_for_each_entry_rcu() on find_module_all()
kmod: reduce atomic operations on kmod_concurrent and simplify
kmod: add test driver to stress test the module loader
kmod: throttle kmod thread limit

About a month now with no further nitpicks. What tree should these changes go through if there are no issues? Andrew's, Jessica's ?

Seems like going through Jessica's would make the most sense?

Quoted 2 lines

Would be happy to take patches 01 (which I need to anyway), 02, possibly 04 if decoupled from the test driver (03).

Feel free to decouple it, but note that then the commit log must then be changed. My own take is this fix is not so critical as it is a corner case, so I have instead preferred to couple in the test case and respective fix together. I'll leave it up to you how to proceed.

Quoted 2 lines

I can't take patch 03 through my tree just yet, as I haven't had time to give it a look yet :-/

Understood. I'd appreciate at least a review though.

Quoted 2 lines

[ Side comment, it seems that kmod.c isn't directly maintained by anyone right now, perhaps Luis would be interested in picking it up? :-) ]

Sure thing, I'm not sure if it makes sense to decouple kernel/kmod.c on MAINTAINERS though, if you do let me know what you'd prefer to call it, "KMOD MODULE USERMODE HELPER" ?

If you prefer to keep them together I can certainly volunteer to review all kmod changes and can send a patch to add kmod and myself under "MODULE SUPPORT".

+++ Luis R. Rodriguez [27/06/17 00:44 +0200]:

Quoted 4 lines

Feel free to decouple it, but note that then the commit log must then be changed. My own take is this fix is not so critical as it is a corner case, so I have instead preferred to couple in the test case and respective fix together. I'll leave it up to you how to proceed.

I'll take 01 and 02 for the next merge window, as they are straightforward. 03/04 can stay together, and as I understand it 04 may need to switch back to using the normal wait_* api.

I'm not the maintainer for kmod.c, if that's what you mean by decoupling.

But I don't think it has one, which is why I'm suggesting
adding it to MAINTAINERS, since you've been actively working on it :) (looking at git log, it looks like Andrew did most of the sign-off's for kmod.c in the past). I think a separate entry in MAINTAINERS is good, with the name you suggested.