From: Arnd Bergmann <arnd@arndb.de>
To: Dave Martin <Dave.Martin@arm.com>
Cc: "Catalin Marinas" <Catalin.Marinas@arm.com>,
"Will Deacon" <Will.Deacon@arm.com>,
"Luis R. Rodriguez" <mcgrof@kernel.org>,
"Kees Cook" <keescook@chromium.org>,
"Alex Bennée" <alex.bennee@linaro.org>,
"Eric W. Biederman" <ebiederm@xmission.com>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] sysctl: add register_sysctl() dummy helper
Date: Mon, 6 Nov 2017 21:36:36 +0100 [thread overview]
Message-ID: <CAK8P3a188UYR0uQf0b0hOcTYsccbBq1CaqyZsnsJ8gsn8EnSVA@mail.gmail.com> (raw)
In-Reply-To: <20171106174128.GW19485@e103592.cambridge.arm.com>
On Mon, Nov 6, 2017 at 6:41 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> On Mon, Nov 06, 2017 at 05:12:03PM +0000, Arnd Bergmann wrote:
>> On Mon, Nov 6, 2017 at 5:50 PM, Dave Martin <Dave.Martin@arm.com> wrote:
>> > On Mon, Nov 06, 2017 at 01:36:40PM +0000, Arnd Bergmann wrote:
>> >> The register_sysctl() function has been around for five years with commit
>> >> fea478d4101a ("sysctl: Add register_sysctl for normal sysctl users")
>> >> but now that arm64 started using it, I ran into a compile error:
>> >>
>> >> arch/arm64/kernel/armv8_deprecated.c: In function 'register_insn_emulation_sysctl':
>> >> arch/arm64/kernel/armv8_deprecated.c:257:2: error: implicit declaration of function 'register_sysctl'
>> >
>> > Hmmm, looks like I missed this combination in testing.
>> >
>> > I wonder whether ARMV8_DEPRECATED without SYSCTL is really a good idea
>> > though: in that config, we build a lot of dead code and leak some
>> > memory today. The default emulation is still potentially useful, but
>> > all the support for runtime twiddling of the emulation modes becomes
>> > useless.
>> >
>> > For parallel reasons, the SVE sysctl stuff is protected by #ifdef
>> > CONFIG_SYSCTL, which is why I didn't get a similar splat there.
>> >
>> >
>> > So, although this patch is superficially sensible, it may tend to hide
>> > bugs: code that calls register_sysctl() when CONFIG_SYSCTL=n is
>> > suspicious and probably needs review... no?
>>
>> I think your analysis for this code is correct, we waste a lot of memory
>> if we do it like this. However in the general case where we register a
>> statically allocated 'struct ctl_table', it wouldn't be an issue, because
>> gcc could then eliminate all the dead code.
>>
>> Adding a CONFIG_SYSCTL #ifdef or Kconfig dependency would
>> probably be reasonable for armv8_deprecated.c, for the rest of
>> the kernel, having that wrapper is probably better.
>>
>> I don't really care how this gets fixed, as long as some solution gets
>> merged.
>
> Well, since your patch makes things more consistent, I'm happy to
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>
>
> The armv8_deprecated code looks suspicious in any case, so I'll propose
> a fix for that separately -- adding a Kconfig dependency seems simplest.
Thanks, your other patch looks good too (replied with an Ack there).
Andrew, can you pick up my patch into -mm then? It seems we still
want it anyway, but there is no longer a dependency with the arm64
tree, so no reason to pick merge it there.
Arnd
next prev parent reply other threads:[~2017-11-06 20:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-06 13:36 [PATCH] sysctl: add register_sysctl() dummy helper Arnd Bergmann
2017-11-06 16:50 ` Dave Martin
2017-11-06 17:12 ` Arnd Bergmann
2017-11-06 17:41 ` Dave Martin
2017-11-06 20:36 ` Arnd Bergmann [this message]
2017-11-06 16:51 ` Kees Cook
2017-11-07 1:01 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAK8P3a188UYR0uQf0b0hOcTYsccbBq1CaqyZsnsJ8gsn8EnSVA@mail.gmail.com \
--to=arnd@arndb.de \
--cc=Catalin.Marinas@arm.com \
--cc=Dave.Martin@arm.com \
--cc=Will.Deacon@arm.com \
--cc=akpm@linux-foundation.org \
--cc=alex.bennee@linaro.org \
--cc=ebiederm@xmission.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.