Message ID | 20220803133541.18b82ec9344ed0e8b975fe5b@linux-foundation.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [GIT,PULL] MM updates for 5.20-rc1 | expand |
On Wed, Aug 3, 2022 at 1:35 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > There are 6-7 conflicts with your tree here. All very minor, mainly > related to the rename of Documentation/vm to Documentation/mm. What? No. Yeah, the doc conflicts were annoying enough, with the Chinese translations having incorrect pointers to the original sources both before and after the move in the MM tree. But that xfs conflict was positively nasty and definitely not minor. Had to look up Darrick Wong's "here's how to do it" message from the linux-next days. So I think I got it all right, but "very minor" it wasn't. Linus
On Wed, Aug 3, 2022 at 1:35 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > - memcg debug/visibility work from Roman Gushchin Well, not just memcg. There's that new CONFIG_SHRINKER_DEBUG too. Which looks like yet another "people add Kconfig options incorrectly". We don't make new features "default y" unless there's some truly pressing reason for it (ie "99.9% of all people will want this" or "we used to always compile this in, now it's optional"). And shouldn't that thing be under the DEBUG_VM heading anyway? I removed the completely bogus and inappropriate "default y". People, please stop doing that 'default y' thing. I realize that everybody always thinks their own code is *sooo* important that it should be enabled everywhere, but if we've gone 30 years without it in the past, maybe it's not so critical after all, and maybe people shouldn't randomly enable it unless they WANT it. Ok? The main reasons for 'default y' tend to be literally - this used to be unconditional, now we have a config variable for it, so let's make it 'default y' so that people don't suddenly lose functionality - this feature truly does cure cancer - this isn't actually a feature, but is a gating question to other features that you may want to just shut up That last case is mainly used by the network driver subsystem, where it asks 'Do you want to see drivers by vendor Xyzzy?', and it defaults to indeed show those options. But admittedly that network driver case is also _partly_ guided by that first case, ie it has often been something where a group of drivers were moved to be under a "do you care about this vendor" situation. Linus
The pull request you sent on Wed, 3 Aug 2022 13:35:41 -0700:
> git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2022-08-03
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/6614a3c3164a5df2b54abb0b3559f51041cf705b
Thank you!
On Fri, Aug 05, 2022 at 04:32:34PM -0700, Linus Torvalds wrote: > On Wed, Aug 3, 2022 at 1:35 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > - memcg debug/visibility work from Roman Gushchin > > Well, not just memcg. > > There's that new CONFIG_SHRINKER_DEBUG too. > > Which looks like yet another "people add Kconfig options incorrectly". > > We don't make new features "default y" unless there's some truly > pressing reason for it (ie "99.9% of all people will want this" or "we > used to always compile this in, now it's optional"). Hi Linus! It actually was 'default n' in one of the earlier version of the patchset and has been switched to 'default y' based on the following feedback from Dave Chinner (https://lore.kernel.org/lkml/YmiWK56bOHyrr64u@rh/): No. The argument that "if we turn it off there's no overhead" means one of two things: 1. nobody turns it on and it never gets tested and so bitrots and is useless, or 2. distro's all turn it on because some tool they ship or customer they ship to wants it. Either way, hiding it behind a config option is not an acceptible solution for mering poorly thought out infrastructure. Personally I think that the feature is not that useful for the majority of users (this is why default was n), but it's not adding much of the overhead, so I had no strong reasons to oppose Dave. Cc'ing him just in case. Thanks!
On Fri, 5 Aug 2022 16:20:44 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Aug 3, 2022 at 1:35 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > There are 6-7 conflicts with your tree here. All very minor, mainly > > related to the rename of Documentation/vm to Documentation/mm. > > What? No. Yeah, the doc conflicts were annoying enough, with the > Chinese translations having incorrect pointers to the original sources > both before and after the move in the MM tree. > > But that xfs conflict was positively nasty and definitely not minor. > Had to look up Darrick Wong's "here's how to do it" message from the > linux-next days. > > So I think I got it all right, but "very minor" it wasn't. Huh. sorry, there was no XFS conflict with your tree when I did my test merge and I don't recall seeing emails from Darrick or Stephen on this one :( I guess that taking a peek at linux-next would be a good way to get at the tested resolution for these things? Emails which might prove useful later in this merge window are: EIF tree: https://lkml.kernel.org/r/20220726194643.47f6c020@canb.auug.org.au bitmap tree: https://lkml.kernel.org/r/20220726194151.71e511e7@canb.auug.org.au crypto: https://lkml.kernel.org/r/20220718195342.6817be63@canb.auug.org.au bitmap again: https://lkml.kernel.org/r/20220715211409.4fba48e8@canb.auug.org.au
On Fri, 5 Aug 2022 17:04:28 -0700 Roman Gushchin <roman.gushchin@linux.dev> wrote: > On Fri, Aug 05, 2022 at 04:32:34PM -0700, Linus Torvalds wrote: > > On Wed, Aug 3, 2022 at 1:35 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > - memcg debug/visibility work from Roman Gushchin > > > > Well, not just memcg. > > > > There's that new CONFIG_SHRINKER_DEBUG too. > > > > Which looks like yet another "people add Kconfig options incorrectly". > > > > We don't make new features "default y" unless there's some truly > > pressing reason for it (ie "99.9% of all people will want this" or "we > > used to always compile this in, now it's optional"). > > Hi Linus! > > It actually was 'default n' in one of the earlier version of the patchset > and has been switched to 'default y' based on the following feedback from > Dave Chinner (https://lore.kernel.org/lkml/YmiWK56bOHyrr64u@rh/): > > No. The argument that "if we turn it off there's no overhead" means > one of two things: > > 1. nobody turns it on and it never gets tested and so bitrots and is > useless, or > 2. distro's all turn it on because some tool they ship or customer > they ship to wants it. > > Either way, hiding it behind a config option is not an acceptible > solution for mering poorly thought out infrastructure. > > Personally I think that the feature is not that useful for the majority > of users (this is why default was n), but it's not adding much of the > overhead, so I had no strong reasons to oppose Dave. > Cc'ing him just in case. > We should have changelogged these considerations. I've asked Joe if checkpatch can get a "default y" detector, to draw attention to this in the future.
On Fri, 2022-08-05 at 17:07 -0700, Andrew Morton wrote: > On Fri, 5 Aug 2022 17:04:28 -0700 Roman Gushchin <roman.gushchin@linux.dev> wrote: > > > On Fri, Aug 05, 2022 at 04:32:34PM -0700, Linus Torvalds wrote: > > > On Wed, Aug 3, 2022 at 1:35 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > > > - memcg debug/visibility work from Roman Gushchin > > > > > > Well, not just memcg. > > > > > > There's that new CONFIG_SHRINKER_DEBUG too. > > > > > > Which looks like yet another "people add Kconfig options incorrectly". > > > > > > We don't make new features "default y" unless there's some truly > > > pressing reason for it (ie "99.9% of all people will want this" or "we > > > used to always compile this in, now it's optional"). > > > > Hi Linus! > > > > It actually was 'default n' in one of the earlier version of the patchset > > and has been switched to 'default y' based on the following feedback from > > Dave Chinner (https://lore.kernel.org/lkml/YmiWK56bOHyrr64u@rh/): > > > > No. The argument that "if we turn it off there's no overhead" means > > one of two things: > > > > 1. nobody turns it on and it never gets tested and so bitrots and is > > useless, or > > 2. distro's all turn it on because some tool they ship or customer > > they ship to wants it. > > > > Either way, hiding it behind a config option is not an acceptible > > solution for mering poorly thought out infrastructure. > > > > Personally I think that the feature is not that useful for the majority > > of users (this is why default was n), but it's not adding much of the > > overhead, so I had no strong reasons to oppose Dave. > > Cc'ing him just in case. > > > > We should have changelogged these considerations. > > I've asked Joe if checkpatch can get a "default y" detector, to draw > attention to this in the future. Perhaps: --- scripts/checkpatch.pl | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index bf7c5abeefaad..1abec0cd217e6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -3523,6 +3523,19 @@ sub process { } } +# check new Kconfig entries for default=y +# Only applies when adding the entry originally and if the entry is not +# conditional on anything like default y if <foo> + if ($realfile =~ /Kconfig/ && + !$file && + $line =~ /\+\s*default\s+y\s*$/i) { + if (WARN("KCONFIG_DEFAULT_Y", + "Kconfig entries should generally not be default y\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/default(\s+)y/default${1}n/; + } + } + # check MAINTAINERS entries if ($realfile =~ /^MAINTAINERS$/) { # check MAINTAINERS entries for the right form