Message ID | 20240517192239.9285edd85f8ef893bb508a61@linux-foundation.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [GIT,PULL] MM updates for 6.10-rc1 | expand |
On Fri, 17 May 2024 at 19:22, Andrew Morton <akpm@linux-foundation.org> wrote: > > include/linux/slab.h > https://lkml.kernel.org/r/20240429114302.7af809e8@canb.auug.org.au This is not only a merge conflict, your tree is actively buggy. You have introduced changes like this: -static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t flags) -{ - return kvmalloc_array(n, size, flags | __GFP_ZERO); -} +#define kvcalloc(_n, _size, _flags) kvmalloc_array(_n, _size, _flags|__GFP_ZERO) and that's just completely wrong. Note the "_flags|__GFP_ZERO": yes, the bitwise or is fairly low down in the operator precedence rules, and it probably work sin practice because most cases will just pass in a simple expression for the flags, but it's still *horribly* wrong. I'm going to take this pull and fix up the cases I find, but I'm not happy with this kind of trivial C preprocessor misuse. I also note that you have *SEVEN* pointless merges that have no explanation for them. I'm happy that you use git, but that means that you also need to either (a) not do merges at all and treat it as a patch queue (b) do merges _properly_ and not throw them around like some madman And doing them properly means not only writing good commit messages, but actually having good reasons for them. As it is, we have 5d1bc760583f ("merge mm-hotfixes-stable into mm-nonmm-stable to pick up needed changes") 640958fde130 ("Merge branch 'master' into mm-stable") 4e2e36129225 ("Merge branch 'master' into mm-stable") 1dd4505cf4c8 ("Merge branch 'master' into mm-stable") 71919308943d ("Merge branch 'master' into mm-stable") b228ab57e51b ("Merge branch 'master' into mm-stable") 5e2806112864 ("Merge branch 'master' into mm-stable") and those one-liners are all the explanation there are for the merges, and NONE OF THOSE MERGES ARE VALID IN THE FIRST PLACE! They shouldn't have been merges! You literally have no new code on the other side. You should have just fast-forwarded to the new state, since your side didn't contain anything relevant any more. So the merges are doubly wrong. They have no explanation, but part of the reason that they have no explanation is probably exactly the fact that there *IS* no explanation for them. There is one fundamental rule of merges: if you can't explain why you'd need to merge, DON'T DO IT. There are lots of smaller rules too. See Documentation/maintainer/rebasing-and-merging.rst for at least some of them. Linus Linus
On Sun, 19 May 2024 at 08:32, Linus Torvalds <torvalds@linuxfoundation.org> wrote: > > I'm going to take this pull and fix up the cases I find, but I'm not > happy with this kind of trivial C preprocessor misuse. I did some other maco handling cleanup too and tried to regularize some of this all, and it seems to work for me. But somebody should double-check, and it's possible these patterns should all be regularized further with a few helper macros for the whole "add __GFP_ZERO to argument list" or similar. Linus
The pull request you sent on Fri, 17 May 2024 19:22:39 -0700:
> https://lkml.kernel.org/r/20240412120421.27d86c34@canb.auug.org.au io_uring/io_uring.c
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/61307b7be41a1f1039d1d1368810a1d92cb97b44
Thank you!
On Sun, May 19, 2024 at 09:48:49AM -0700, Linus Torvalds wrote: > On Sun, 19 May 2024 at 08:32, Linus Torvalds > <torvalds@linuxfoundation.org> wrote: > > > > I'm going to take this pull and fix up the cases I find, but I'm not > > happy with this kind of trivial C preprocessor misuse. > > I did some other maco handling cleanup too and tried to regularize > some of this all, and it seems to work for me. But somebody should > double-check, and it's possible these patterns should all be > regularized further with a few helper macros for the whole "add > __GFP_ZERO to argument list" or similar. I just double checked slab.h, gfp.h and percpu.h, and scanned through the diff vs. 6.9 for include/linux/ - looks like you got everything. I think we can slim down the API surface of slab.h some more too, we're now exposing three different ways of saying "trace/track this allocation here": _trace, _track_caller and _noprof vs. normal; I think after a cycle we can see if the old variants are still needed or can be consolidated somehow.
On Sun, 19 May 2024 08:32:44 -0700 Linus Torvalds <torvalds@linuxfoundation.org> wrote: > On Fri, 17 May 2024 at 19:22, Andrew Morton <akpm@linux-foundation.org> wrote: > > > > include/linux/slab.h > > https://lkml.kernel.org/r/20240429114302.7af809e8@canb.auug.org.au > > This is not only a merge conflict, your tree is actively buggy. > > You have introduced changes like this: > > -static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t > size, gfp_t flags) > -{ > - return kvmalloc_array(n, size, flags | __GFP_ZERO); > -} > +#define kvcalloc(_n, _size, _flags) kvmalloc_array(_n, _size, > _flags|__GFP_ZERO) > > and that's just completely wrong. Note the "_flags|__GFP_ZERO": yes, > the bitwise or is fairly low down in the operator precedence rules, > and it probably work sin practice because most cases will just pass in > a simple expression for the flags, but it's still *horribly* wrong. > > I'm going to take this pull and fix up the cases I find, but I'm not > happy with this kind of trivial C preprocessor misuse. Thanks, I've asked Suren and Kent to check it all over. > I also note that you have *SEVEN* pointless merges that have no > explanation for them. I'm happy that you use git, but that means that > you also need to either > > (a) not do merges at all and treat it as a patch queue > > (b) do merges _properly_ and not throw them around like some madman > > And doing them properly means not only writing good commit messages, > but actually having good reasons for them. As it is, we have > > 5d1bc760583f ("merge mm-hotfixes-stable into mm-nonmm-stable to pick > up needed changes") > 640958fde130 ("Merge branch 'master' into mm-stable") > 4e2e36129225 ("Merge branch 'master' into mm-stable") > 1dd4505cf4c8 ("Merge branch 'master' into mm-stable") > 71919308943d ("Merge branch 'master' into mm-stable") > b228ab57e51b ("Merge branch 'master' into mm-stable") > 5e2806112864 ("Merge branch 'master' into mm-stable") This is me advancing the master branch once per week until we hit -rc4. I don't understand why these merges were visible to this pull. I sent: : The following changes since commit 5d1bc760583f225032f91bd88853f4c26acaf4e0: : : merge mm-hotfixes-stable into mm-nonmm-stable to pick up needed changes (2024-04-25 20:54:12 -0700) : : are available in the Git repository at: : : git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm tags/mm-stable-2024-05-17-19-19 : : for you to fetch changes up to 76edc534cc289308130272a2ac28694fc9b72a03: : : memcg, oom: cleanup unused memcg_oom_gfp_mask and memcg_oom_order (2024-05-11 15:41:37 -0700) : This has worked OK before,
On Sun, 19 May 2024 at 11:36, Andrew Morton <akpm@linux-foundation.org> wrote: > > This is me advancing the master branch once per week until we hit -rc4. Yeah. You really shouldn't do that with a merge commit. If what you want to do is just advance the master branch, you should just rebase on top (if rebasing is what you want to do). Instead you seem to merge _and_ then rebase. So you do the worst of both worlds. You don't get the history cleanup of rebasing, and you don't get the history preservation of merging. You do both, and it's wrong. > I don't understand why these merges were visible to this pull. I sent: > > : The following changes since commit 5d1bc760583f225032f91bd88853f4c26acaf4e0: No. That is NOT how git works. You claiming to send only the changes after a commit doesn't magically make the history that precedes that commit go away. With git, when you send a branch, you send the WHOLE HISTORY. That is fundamentally how git works. You trying to change the "from this commit forwards" doesn't actually change anything at all, it only changes the wording in the pull request itself. You can't just do a pull request and try to say "only send changes since this commit". Git is *not* the broken sh*t that you have used before that is just a queue of patches. Every single commit carries the whole history that leads up to it. And that is fundamental (and is fundamentally why patch-queue based stuff is broken garbage), because proper history is what makes merging work and scale. It's pretty much what makes git not CVS, or the horror that is darcs and "patch algebra". So when you do a pull request, the whole history is always visible. You can't make it not visible. > This has worked OK before, Your previous histories haven't had those broken merges in them, so your previous pulls haven't had them either. I don't know what you have changed in your setup, but your "advance the master branch once per week" process is completely broken. The problem isn't the pull request. The source of the problem is something in your "advance" scripting. Linus
On Sun, 19 May 2024 at 11:56, Linus Torvalds <torvalds@linuxfoundation.org> wrote: > > Instead you seem to merge _and_ then rebase. So you do the worst of both worlds. Hmm. Your non-MM pull request doesn't show this pattern, so the problem seems to be purely about something you have done differently in the MM branch. Linus
On Sun, May 19, 2024 at 11:02 AM Kent Overstreet <kent.overstreet@linux.dev> wrote: > > On Sun, May 19, 2024 at 09:48:49AM -0700, Linus Torvalds wrote: > > On Sun, 19 May 2024 at 08:32, Linus Torvalds > > <torvalds@linuxfoundation.org> wrote: > > > > > > I'm going to take this pull and fix up the cases I find, but I'm not > > > happy with this kind of trivial C preprocessor misuse. > > > > I did some other maco handling cleanup too and tried to regularize > > some of this all, and it seems to work for me. But somebody should > > double-check, and it's possible these patterns should all be > > regularized further with a few helper macros for the whole "add > > __GFP_ZERO to argument list" or similar. > > I just double checked slab.h, gfp.h and percpu.h, and scanned through > the diff vs. 6.9 for include/linux/ - looks like you got everything. Sorry about that. Yeah, I could not find any other place that was not fixed. Thanks for noticing and fixing them! > > I think we can slim down the API surface of slab.h some more too, we're > now exposing three different ways of saying "trace/track this allocation > here": _trace, _track_caller and _noprof vs. normal; I think after a > cycle we can see if the old variants are still needed or can be > consolidated somehow. >