Message ID | 20230427153345.451d86681f9c6775ea579e5a@linux-foundation.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [GIT,PULL] MM updates for 6.4-rc1 | expand |
On Thu, Apr 27, 2023 at 3:33 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > - Suren Baghdasaryan has improved mmap_lock scalability by switching to > per-VMA locking. .. related to this, we have that new PER_VMA_LOCK_STATS config entry, which is 'default y' and has basically no help test. Please don't do that. I don't think any of the VM debugging stuff should likely be 'default y' in the first place, but if they are, they should most definitely have a good *explanation* for why they should be on for a normal user. Linus
The pull request you sent on Thu, 27 Apr 2023 15:33:45 -0700:
> https://lkml.kernel.org/r/CAKXUXMyVeg2kQK_edKHtMD3eADrDK_PKhCSVkMrLDdYgTQQ5rg@mail.gmail.com Thanks.
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7fa8a8ee9400fe8ec188426e40e481717bc5e924
Thank you!
On Thu, Apr 27, 2023 at 3:33 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2023-04-27-15-30 Ok, so all the merge conflicts looked straightforward enough (except the one I asked Vlastimil to look at - and that conflict was tiny and straightforward too, it was just in a really grotty place). However, let's not get cocky - so I'd ask people double-check my conflict resolution, even if it looked fairly straightforward. Mistakes happen. Most of the bulk of the conflicts were around __filemap_get_folio() now returning an ERR_PTR, particularly then with Willy doing the ext4 folio conversion. So Christoph, Willy, mind just double-checking me? Linus
On Thu, Apr 27, 2023 at 8:15 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Most of the bulk of the conflicts were around __filemap_get_folio() > now returning an ERR_PTR, particularly then with Willy doing the ext4 > folio conversion. > > So Christoph, Willy, mind just double-checking me? Just to clarify: I do take a look at linux-next, but I do it as an after-the-fact "let's double-check my resolution", and some of my resolutions don't end up being identical because I end up doing things differently. For example, I found the linux-next resolution in ext4_read_merkle_tree_page() to be nonsensical. I think it probably generates the same code as my resolution, but doing "&folio->page" on an ERR_PTR folio is some funky funky sh*t. Only after looking at the linux-next resolution did I grep around and notice that that kind of funky struff had already made it into mainline in non-conflicting places. Willy - it seems to be you spreading that crazy pattern. Please stop. Anyway. I did some things differently, and while I think my resolution is the right one, the fact that it is different could also just mean that I'm confused. Linus
On Thu, Apr 27, 2023 at 08:15:56PM -0700, Linus Torvalds wrote: > Most of the bulk of the conflicts were around __filemap_get_folio() > now returning an ERR_PTR, particularly then with Willy doing the ext4 > folio conversion. > > So Christoph, Willy, mind just double-checking me? The merge looks fine to me.
On Thu, Apr 27, 2023 at 8:03 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Thu, Apr 27, 2023 at 3:33 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > - Suren Baghdasaryan has improved mmap_lock scalability by switching to > > per-VMA locking. > > .. related to this, we have that new PER_VMA_LOCK_STATS config entry, > which is 'default y' and has basically no help test. > > Please don't do that. > > I don't think any of the VM debugging stuff should likely be 'default > y' in the first place, but if they are, they should most definitely > have a good *explanation* for why they should be on for a normal user. Understood. I wanted these stats enabled by default to be able to identify possible pathological cases and to also let users disable them if they can't tolerate even a small overhead in the pagefault path. Should I document this reasoning for the config option? > > Linus
On Fri, Apr 28, 2023 at 9:03 AM Suren Baghdasaryan <surenb@google.com> wrote: > > I wanted these stats enabled by default to be able to identify > possible pathological cases and to also let users disable them if they > can't tolerate even a small overhead in the pagefault path. Should I > document this reasoning for the config option? You should document what the stats actually count (at a high enough level for a user to understand), and why anybody would want to keep them on. Honestly, 99% of the time, these are things that *developers* think they might want, but that nobody else will ever ever use. Really, ask yourself if a normal user would ever look at them? Now, ask yourself whether this might be something that a cloud provider would want to look at to gather statistics. And if it's the latter case, then it should be "default n", because the default should be for the people who DO NOT KNOW, AND DO NOT CARE. The cloud provider will be using a custom config anyway. The default is irrelevant for that use. The use that *matters* is literally the clueless end user who I bet will never look at these numbers, and will never be asked for them. Linus
On Fri, Apr 28, 2023 at 9:08 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, Apr 28, 2023 at 9:03 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > I wanted these stats enabled by default to be able to identify > > possible pathological cases and to also let users disable them if they > > can't tolerate even a small overhead in the pagefault path. Should I > > document this reasoning for the config option? > > You should document what the stats actually count (at a high enough > level for a user to understand), and why anybody would want to keep > them on. > > Honestly, 99% of the time, these are things that *developers* think > they might want, but that nobody else will ever ever use. > > Really, ask yourself if a normal user would ever look at them? > > Now, ask yourself whether this might be something that a cloud > provider would want to look at to gather statistics. > > And if it's the latter case, then it should be "default n", because > the default should be for the people who DO NOT KNOW, AND DO NOT CARE. > > The cloud provider will be using a custom config anyway. The default > is irrelevant for that use. The use that *matters* is literally the > clueless end user who I bet will never look at these numbers, and will > never be asked for them. Ok, sounds like this should be 'default n'. I'll prepare a patch. Thanks! > > Linus
On Fri, Apr 28, 2023 at 9:14 AM Suren Baghdasaryan <surenb@google.com> wrote: > > On Fri, Apr 28, 2023 at 9:08 AM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > On Fri, Apr 28, 2023 at 9:03 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > > > I wanted these stats enabled by default to be able to identify > > > possible pathological cases and to also let users disable them if they > > > can't tolerate even a small overhead in the pagefault path. Should I > > > document this reasoning for the config option? > > > > You should document what the stats actually count (at a high enough > > level for a user to understand), and why anybody would want to keep > > them on. > > > > Honestly, 99% of the time, these are things that *developers* think > > they might want, but that nobody else will ever ever use. > > > > Really, ask yourself if a normal user would ever look at them? > > > > Now, ask yourself whether this might be something that a cloud > > provider would want to look at to gather statistics. > > > > And if it's the latter case, then it should be "default n", because > > the default should be for the people who DO NOT KNOW, AND DO NOT CARE. > > > > The cloud provider will be using a custom config anyway. The default > > is irrelevant for that use. The use that *matters* is literally the > > clueless end user who I bet will never look at these numbers, and will > > never be asked for them. > > Ok, sounds like this should be 'default n'. I'll prepare a patch. Thanks! Should I send a replacement patch for "mm: introduce per-VMA lock statistics" or a followup patch fixing it? > > > > > Linus
On Fri, Apr 28, 2023 at 9:17 AM Suren Baghdasaryan <surenb@google.com> wrote: > > Should I send a replacement patch for "mm: introduce per-VMA lock > statistics" or a followup patch fixing it? I've merged the MM pull request, so it's all in the kernel now, but I'd like to see a follow-up patch with more of a help text and that 'default n'. Linus
On Fri, Apr 28, 2023 at 9:20 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, Apr 28, 2023 at 9:17 AM Suren Baghdasaryan <surenb@google.com> wrote: > > > > Should I send a replacement patch for "mm: introduce per-VMA lock > > statistics" or a followup patch fixing it? > > I've merged the MM pull request, so it's all in the kernel now, but > I'd like to see a follow-up patch with more of a help text and that > 'default n'. Posted https://lore.kernel.org/all/20230428173533.18158-1-surenb@google.com/ to address this. > > Linus
On Thu, Apr 27, 2023 at 03:33:45PM -0700, Andrew Morton wrote: > > Linus, please merge this cycles's batch of MM changes. This is almost > everything - I'll have another 5-10 patches next week. (cut) > The following changes since commit ef832747a82dfbc22a3702219cc716f449b24e4a: > > nilfs2: initialize unused bytes in segment summary blocks (2023-04-18 14:22:14 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2023-04-27-15-30 > > for you to fetch changes up to 4d4b6d66db63ceed399f1fb1a4b24081d2590eb1: > > mm,unmap: avoid flushing TLB in batch if PTE is inaccessible (2023-04-27 13:42:16 -0700) > > ---------------------------------------------------------------- Hello Andrew, It seems like the changes in mm/dmapool.c somehow got omitted in this PR. $ git log --oneline 4d4b6d66db63ceed399f1fb1a4b24081d2590eb1 --not v6.3 mm/dmapool.c 2d55c16c0c54 dmapool: create/destroy cleanup a4de12a032fa dmapool: link blocks across pages 9d062a8a4c6d dmapool: don't memset on free twice 887aef615818 dmapool: simplify freeing 2591b516533b dmapool: consolidate page initialization 36d1a28921a4 dmapool: rearrange page alloc failure handling 52e7d5653979 dmapool: move debug code to own functions 19f504584038 dmapool: speedup DMAPOOL_DEBUG with init_on_alloc 347e4e44c0a9 dmapool: cleanup integer types 65216545436b dmapool: use sysfs_emit() instead of scnprintf() 7f796d141c07 dmapool: remove checks for dev == NULL $ git diff 4d4b6d66db63ceed399f1fb1a4b24081d2590eb1 v6.3 mm/dmapool.c <empty> It seems like the final commit, 2d55c16c0c54 ("dmapool: create/destroy cleanup") somehow reverted all the previous changes to this file. Looking at how that patch looked like on the list: https://patchwork.kernel.org/project/linux-mm/patch/20230126215125.4069751-13-kbusch@meta.com/ the diff is way smaller than what can be seen in 2d55c16c0c54. Additionally, you might want to pick up: https://patchwork.kernel.org/project/linux-mm/patch/20230221165400.1595247-1-kbusch@meta.com/ as it has a Fixes tag that references one of the commits above. Kind regards, Niklas
On Thu, 4 May 2023 10:39:51 +0000 Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > On Thu, Apr 27, 2023 at 03:33:45PM -0700, Andrew Morton wrote: > > > > Linus, please merge this cycles's batch of MM changes. This is almost > > everything - I'll have another 5-10 patches next week. > > (cut) > > > The following changes since commit ef832747a82dfbc22a3702219cc716f449b24e4a: > > > > nilfs2: initialize unused bytes in segment summary blocks (2023-04-18 14:22:14 -0700) > > > > are available in the Git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2023-04-27-15-30 > > > > for you to fetch changes up to 4d4b6d66db63ceed399f1fb1a4b24081d2590eb1: > > > > mm,unmap: avoid flushing TLB in batch if PTE is inaccessible (2023-04-27 13:42:16 -0700) > > > > ---------------------------------------------------------------- > > Hello Andrew, > > It seems like the changes in mm/dmapool.c somehow got omitted in this PR. > > $ git log --oneline 4d4b6d66db63ceed399f1fb1a4b24081d2590eb1 --not v6.3 mm/dmapool.c > 2d55c16c0c54 dmapool: create/destroy cleanup > a4de12a032fa dmapool: link blocks across pages > 9d062a8a4c6d dmapool: don't memset on free twice > 887aef615818 dmapool: simplify freeing > 2591b516533b dmapool: consolidate page initialization > 36d1a28921a4 dmapool: rearrange page alloc failure handling > 52e7d5653979 dmapool: move debug code to own functions > 19f504584038 dmapool: speedup DMAPOOL_DEBUG with init_on_alloc > 347e4e44c0a9 dmapool: cleanup integer types > 65216545436b dmapool: use sysfs_emit() instead of scnprintf() > 7f796d141c07 dmapool: remove checks for dev == NULL > > $ git diff 4d4b6d66db63ceed399f1fb1a4b24081d2590eb1 v6.3 mm/dmapool.c > <empty> > > It seems like the final commit, 2d55c16c0c54 ("dmapool: create/destroy > cleanup") somehow reverted all the previous changes to this file. > > > Looking at how that patch looked like on the list: > https://patchwork.kernel.org/project/linux-mm/patch/20230126215125.4069751-13-kbusch@meta.com/ > the diff is way smaller than what can be seen in 2d55c16c0c54. Well I don't know how I did this, sorry. The patch "dmapool: create/destroy cleanup" was OK in mm-unstable (and linux-next) from Jan 26 to Feb 26, so the series has had decent linux-next testing. It became messed up on Feb 26. I've reconstituted dmapool: remove checks for dev == NULL dmapool: use sysfs_emit() instead of scnprintf() dmapool: cleanup integer types dmapool: speedup DMAPOOL_DEBUG with init_on_alloc dmapool: move debug code to own functions dmapool: rearrange page alloc failure handling dmapool: consolidate page initialization dmapool: simplify freeing dmapool: don't memset on free twice dmapool: link blocks across pages dmapool: create/destroy cleanup and pushed the result out to the mm-unstable branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm. 3a64f2e22553 dmapool: create/destroy cleanup 1d1e67d45a37 dmapool: link blocks across pages 7e54c3f0e123 dmapool: don't memset on free twice cb569a63de3b dmapool: simplify freeing 874938254ae7 dmapool: consolidate page initialization b97304656ad5 dmapool: rearrange page alloc failure handling c31e8ec45299 dmapool: move debug code to own functions 1e05e5bdce74 dmapool: speedup DMAPOOL_DEBUG with init_on_alloc 7ea3ff961459 dmapool: cleanup integer types adf388b29d25 dmapool: use sysfs_emit() instead of scnprintf() 8491f7f301ad dmapool: remove checks for dev == NULL Please check that all is as expected. > Additionally, you might want to pick up: > https://patchwork.kernel.org/project/linux-mm/patch/20230221165400.1595247-1-kbusch@meta.com/ > as it has a Fixes tag that references one of the commits above. That fix has been folded into "dmapool: link blocks across pages".
On Thu, May 04, 2023 at 07:10:22PM -0700, Andrew Morton wrote:
> Please check that all is as expected.
Thanks for the quick turn-around. This looks correct and passes my
sanity tests.