On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
> 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
About a month now with no further nitpicks. What tree should these changes
go through if there are no issues? Andrew's, Jessica's ?
Luis
On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
> On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
>> 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
>
> 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
--
Kees Cook
Pixel Security
+++ Kees Cook [20/06/17 17:23 -0700]:
>On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
>> On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
>>> 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
>>
>> 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? :-) ]
Thanks,
Jessica
On Mon, Jun 26, 2017 at 11:37:36PM +0200, Jessica Yu wrote:
> +++ Kees Cook [20/06/17 17:23 -0700]:
> > On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
> > > On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
> > > > 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
> > >
> > > 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).
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 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.
> [ 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".
Either way, let me know!
Luis
On Tue, Jun 27, 2017 at 12:44:42AM +0200, Luis R. Rodriguez wrote:
> On Mon, Jun 26, 2017 at 11:37:36PM +0200, Jessica Yu wrote:
> > +++ Kees Cook [20/06/17 17:23 -0700]:
> > > On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
> > > > On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
> > > > > 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?
> >
> > 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]
Luis
On Tue 2017-06-27 02:27:44, Luis R. Rodriguez wrote:
> On Tue, Jun 27, 2017 at 12:44:42AM +0200, Luis R. Rodriguez wrote:
> > On Mon, Jun 26, 2017 at 11:37:36PM +0200, Jessica Yu wrote:
> > > +++ Kees Cook [20/06/17 17:23 -0700]:
> > > > On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
> > > > > On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
> > > > > > 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?
> > >
> > > 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]
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.
Best Regards,
Petr
+++ Petr Mladek [27/06/17 10:13 +0200]:
>On Tue 2017-06-27 02:27:44, Luis R. Rodriguez wrote:
>> On Tue, Jun 27, 2017 at 12:44:42AM +0200, Luis R. Rodriguez wrote:
>> > On Mon, Jun 26, 2017 at 11:37:36PM +0200, Jessica Yu wrote:
>> > > +++ Kees Cook [20/06/17 17:23 -0700]:
>> > > > On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
>> > > > > On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
>> > > > > > 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?
>> > >
>> > > 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]
>
>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.
Huh, I wasn't aware either :-/ But I agree, judging from Linus'
response [0], it's probably best to use the well established wait_*
variants. I'm not sure I understood why the patch switched to swait,
but in any case I don't think we'd be hitting the "thundering herd"
problem very often here (and if we do, we could just use exclusive
wait. But in that scenario I'd be more interested in why a normal
system would be battered with more than 50 in-kernel modprobe requests
at a time).
[0] https://marc.info/?l=linux-kernel&m=149851347228696&w=2
+++ Luis R. Rodriguez [27/06/17 00:44 +0200]:
>On Mon, Jun 26, 2017 at 11:37:36PM +0200, Jessica Yu wrote:
>> +++ Kees Cook [20/06/17 17:23 -0700]:
>> > On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <[email protected]> wrote:
>> > > On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote:
>> > > > 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
>> > >
>> > > 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).
>
>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 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.
Of course! I should have rephrased and said *by this upcoming merge window.
>> [ 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".
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.
Thanks!
Jessica
On Tue, Jun 27, 2017 at 05:26:05PM +0200, Jessica Yu wrote:
> +++ Luis R. Rodriguez [27/06/17 00:44 +0200]:
> > 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.
OK, I'll rework 03-04 with the regular wait and submit aiming towards Andrew's tree.
> I'm not the maintainer for kmod.c, if that's what you mean by
> decoupling.
I was suggesting adding kmod.c to MODULE SUPPORT list as its all module
related, adding my self to the list and I'd just take on helping kmod.c stuff.
My point was that it feels odd to decouple kmod.c from module stuff. But lets
give it a shot with them separated first and see how that goes.
> 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.
Sure.
Luis