On Sat, Nov 18, 2023 at 11:14:25AM -0800, Linus Torvalds wrote: > On Sat, 18 Nov 2023 at 10:33, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > What are the alleged "number of bugs all over the kernel" that the old > > MAX_ORDER definition had? In light of the other "MAX" definitions I > > found from a second of grepping, I really think the argument for that > > was just wrong. > > Bah. I looked at the commits leading up to it, and I really think that > "if that's the fallout from 24 years of history, then it wasn't a huge > deal". That's part that hit upstream and never got caught. I personally spend some time debugging due MAX_ORDER-1 thing. > Compared to the inevitable problems that the semantic change will > cause for backports (and already caused for this merge window), I > still think this was all likely a mistake. I think the mistake I made is leaving the name the same. Just renaming it to MAX_PAGE_ORDER or something would give a heads up to not-yet-upstream code. > In fact, the biggest takeaway from those patches is that we probably > should have introduced some kind of "this is the max _size_ you can > allocate", because a few of them were related to that particular > calculation. > > The floppy driver example is actually a good example, in that that is > *exactly* what it wanted, but it had been just done wrong as > > #define MAX_LEN (1UL << MAX_ORDER << PAGE_SHIFT) > > and variations of that is what half the fixes were.. > > IOW, I really think we should just have added a > > #define MAX_GFP_SIZE (1UL << (PAGE_SHIFT+MAX_ORDER-1)) > > and the rest were basically all just the trivial off-by-one things. > > The __iommu_dma_alloc_pages() code is just so odd that any MAX_ORDER > issues are more about "WTH is this code doing" And the "fix" is just > more of the same. > > The *whole* point of having the max for a power-of-two thing is that > you can do things like > > mask = (1<<MAX)-1; > > and that's exactly what the iommu code was trying to do. Except for > some *unfathomable* reason the iommu code did > > order_mask &= (2U << MAX_ORDER) - 1; > > and honestly, you can't blame MAX_ORDER for the confusion. What a > strange off-by-one. MAX_ORDER was *literally* designed to be easy for > this case, and people screwed it up. Agreed. > And no, it's *not* because "inclusive is more intuitive". As > mentioned, this whole "it's past the end* is the common case for MAX > values in the mm, for exactly these kinds of reasons. I already > listted several other cases. > > The reason there were 9 bugs is because MAX_ORDER is an old thing, and > testing doesn't catch it because nobody triggers the max case. The > crazy iommu code goes back to 2016, for example. > > That > > 84 files changed, 223 insertions(+), 253 deletions(-) > > really seems bogus for the 9 fixes that had accumulated in this area > in the last 24 years. > > Oh well. Now we have places with 'MAX_ORDER + 1' instead of 'MAX_ORDER > - 1'. I'm not seeing that it's a win, and I do think "semantic change > after 24 years" is a loss and is going to cause problems. I think it worth introducing NR_ORDERS for MAX_ORDER + 1 to define how many page orders page allocator supports. Sounds good? -- Kiryl Shutsemau / Kirill A. Shutemov
