Message-ID: <ZEGopJ8VAYnE7LQ2@bombadil.infradead.org>
Date:   Thu, 20 Apr 2023 14:03:32 -0700
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>, lucas.demarchi@...el.com
Cc:     david@...hat.com, patches@...ts.linux.dev,
        linux-modules@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, pmladek@...e.com,
        petr.pavlu@...e.com, prarit@...hat.com,
        torvalds@...ux-foundation.org, rafael@...nel.org,
        christophe.leroy@...roup.eu, tglx@...utronix.de,
        peterz@...radead.org, song@...nel.org, rppt@...nel.org,
        dave@...olabs.net, willy@...radead.org, vbabka@...e.cz,
        mhocko@...e.com, dave.hansen@...ux.intel.com,
        colin.i.king@...il.com, jim.cromie@...il.com,
        catalin.marinas@....com, jbaron@...mai.com,
        rick.p.edgecombe@...el.com, j.granados@...sung.com
Subject: Re: [PATCH] module: add debugging auto-load duplicate module support

On Thu, Apr 20, 2023 at 07:32:10AM +0200, Greg KH wrote:
> On Wed, Apr 19, 2023 at 04:32:30PM -0700, Luis Chamberlain wrote:
> > > It's not "wasted", as it is returned when the module is determined to be
> > > a duplicate.  Otherwise everyone will want this enabled as they think it
> > > will actually save memory.
> > 
> > I'll change the language to be clear the issue is memory pressure early
> > on boot. I'll also add a bit of language to help at least guide people
> > to realize that the real value-add for this, ie, I'll have to mention we
> > suspect issue is udev and not module auto-loading and that this however
> > may still help find a few cases we can optimize for.
> 
> This isn't udev's "problem", all it is doing is what the kernel asked it
> to do.  The kernel said "Here's a new device I found, load a module for
> it please!"

If you believe that then the issue is still a kernel issue, and the
second part to that sentence "load a module for it" was done without
consideration of the implications, or without optimizations in mind.
Given the implications were perhaps not well understood it is unfair
for us to be hard on ourselves on that. But now we know, ideally if we
could we *should* only issue a request for a module *once* during boot.

Where does the kernel actually say "load a module"?

Isn't that just an implied gesture?

> And it's the kmod code here, not udev itself doing all of this.

Yes, IMHO kmod could and *should* be enhanced to share a loading context
during boot so to avoid duplicates too and then udev would have to
embrace such functionality. That's going to take time to propagate, as
you can imagine.

> Why not
> just rate-limit it in userspace if your system can't handle 10's of
> thousands of kmod calls all at once? I think many s390 systems did this
> decades ago when they were controlling 10's of thousands of scsi devices
> and were hit with "device detection storms" at boot like this.

Boot is a special context and in this particular case I agree userspace
kmod could/should be extended to avoid duplicate module requests in that
context. But likewise the kernel should only have to try to issue a
request for a single module once, if it could easily do that.

This does beg the question, why force userspace to rate limit if we
can do better in the kernel? Specially if *we're the ones*, as you say,
that are hinting to userspace to shoot back loading modules for us and we
know we're just going to drop duplicates?

We discussed the possibility to rate limit finit_module() in-kernel [0], but
that was early in this process as I investigated the root cause of the
duplicates. I didn't feel comfortable with such an approach then but now
I have a clearer rationale as to why I felt so uncomfortable with it.
Rate limiting can be a a bandaid, a ball park guess and if one can
understand the issue a bit better, you may not need to to rate limit.

My kludge-of-concept RFC patch which takes the concepts in this patch but
applies them to kread [1] is an attempt for us to see what that could look
like -- and it goes so far as to survive easily the otherwise pathological
stress test of say (assuming you have all deps loaded and don't have XFS
loaded for an existing mount already):

stress-ng --module 1 --module-name xfs

Given Dave's original reported issue should be fixed now by the patch
"module: avoid allocation if module is already present and ready", there
really is no rush in keeping a kludges around but this does be the
question to analyze this case a bit more carefully with the long term
in mind.

[0] https://lkml.kernel.org/r/CAHk-=wjsBkyD+y_zfif1MXWYEkvPt+TPLdVjP41573oKEOu4qA@mail.gmail.com
[1] https://lore.kernel.org/all/ZDmAvwi+KNvie+OI@bombadil.infradead.org/T/#md172510af8fdf7e0f76f6caafee9c99f7a8b6de7

> If you really think it's the kernel's fault, just slow down the kernel's
> device detection code for the specific bus that is having problems.
> We've worked hard to make the kernel boot really fast and device
> detection happen in parallel, maybe that wasn't a good idea for some
> systems and so they need to boot slower.  If so, then just turn off the
> parallel probing for the offending bus type.

I think it is counter productive to do that, it's not like we have not
identified the scaling issue or can't solve this.

> What specific devices and bus types are the problem here for these systems?

My best assessment of the situation is that each CPU in udev ends up triggering
a load of duplicate set of modules, not just one, but *a lot*. Not sure
what heuristics udev uses to load a set of modules per CPU.

Given the slopes of the graphs I have shown on how this scales per CPU [2]
we can project at what point in time this will again likely be an issue if
we don't avoid the duplicates in userspace through kmod or address dealing
with them as in my kludge-of-concept on kreads. Given the secondary vmalloc
is dealt with the patch "module: avoid allocation if module is already present
and ready" that leaves us only to deal with virtual memory pressure from the
kread. In-kernel we may be able to also see if we can use seq reads for the
initial kread and decompression. That remains to be seen, but it would
also be a way for this not cause possible boot failures.

[2] https://lore.kernel.org/all/ZDmAvwi+KNvie+OI@bombadil.infradead.org/T/#mf7f8412c3486f59a3e4cf02b65bf0c4f9ba60ef0

  Luis

Powered by blists - more mailing lists