Re: [PATCH v2 1/1] mm: fix folio_pte_batch() on XEN PV

[David Hildenbrand <david@xxxxxxxxxx>]

On 04.05.25 09:15, Andrew Morton wrote:
> On Sun, 4 May 2025 08:47:45 +0200 David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> > > Methinks max_nr really wants to be unsigned long.
> > We only batch within a single PTE table, so an integer was sufficient.
> >
> > The unsigned value is the result of a discussion with Ryan regarding 
> > similar/related
> > (rmap) functions:
> >
> > "
> > Personally I'd go with signed int (since
> > that's what all the counters in struct folio that we are manipulating are,
> > underneath the atomic_t) then check that nr_pages > 0 in
> > __folio_rmap_sanity_checks().
> > "
> >
> > https://lore.kernel.org/linux-mm/20231204142146.91437-14-david@xxxxxxxxxx/T/#ma0bfff0102f0f2391dfa94aa22a8b7219b92c957
> >
> > As soon as we let "max_nr" be an "unsigned long", also the return value
> > should be an "unsigned long", and everybody calling that function.
> >
> > In this case here, we should likely just use whatever type "max_nr" is.
> >
> > Not sure myself if we should change that here to unsigned long or long. Some
> > callers also operate with the negative values IIRC (e.g., adjust the RSS by 
> > doing -= nr).
> "rss -= nr" doesn't require, expect or anticipate that `nr' can be negative!
The one thing I ran into with "unsigned int" around folio_nr_pages()
was that if you pass

-folio-nr_pages()

into a function that expects an "long" (add vs. remove a value to a counter), 
then
the result might not be what one would expect when briefly glimpsing at the 
code:

#include <stdio.h>

static __attribute__((noinline)) void print(long diff)
{
        printf("%ld\n", diff);
}

static int value_int()
{
        return 12345;
}

static unsigned int value_unsigned_int()
{
        return 12345;
}

static int value_long()
{
        return 12345;
}

static unsigned long value_unsigned_long()
{
        return 12345;
}

int main(void)
{
        print(-value_int());
        print(-value_unsigned_int());
        print(-value_long());
        print(-value_unsigned_long());
        return 0;
}


$ ./tmp
-12345
4294954951
-12345
-12345

So, I am fine with using "unsigned long" (as stated in that commit description 
below).

> > > That will permit the
> > > cleanup of quite a bit of truncation, extension, signedness conversion
> > > and general type chaos in folio_pte_batch()'s various callers.
> > > > And...
> > > Why does folio_nr_pages() return a signed quantity?  It's a count.
> > A partial answer is in 1ea5212aed068 ("mm: factor out large folio handling
> > from folio_nr_pages() into folio_large_nr_pages()"), where I stumbled over the
> > reason for a signed value myself and at least made the other
> > functions be consistent with folio_nr_pages():
> >
> > "
> >       While at it, let's consistently return a "long" value from all these
> >       similar functions.  Note that we cannot use "unsigned int" (even though
> >       _folio_nr_pages is of that type), because it would break some callers that
> >       do stuff like "-folio_nr_pages()".  Both "int" or "unsigned long" would
> >       work as well.
> >
> > "
> >
> > Note that folio_nr_pages() returned a "long" since the very beginning. Probably 
> > using
> > a signed value for consistency because also mapcounts / refcounts are all 
> > signed.
> Geeze.
>
> Can we step back and look at what we're doing?  Anything which counts
> something (eg, has "nr" in the identifier) cannot be negative.
Yes. Unless we want to catch underflows (e.g., mapcount / refcount). For 
"nr_pages" I agree.

> It's that damn "int" thing.  I think it was always a mistake that the C
> language's go-to type is a signed one. 
Yeah. But see above that "unsigned int" in combination with long can also cause 
pain.

> It's a system programming
> language and system software rarely deals with negative scalars.
> Signed scalars are the rare case.
>
> I do expect that the code in and around here would be cleaner and more
> reliable if we were to do a careful expunging of inappropriately signed
> variables.
Maybe, but it would mostly be a "int -> unsigned long" conversion, probably not
much more. I'm not against cleaning that up at all.

> > > And why the heck is folio_pte_batch() inlined?  It's larger then my
> > > first hard disk and it has five callsites!
> > :)
> >
> > In case of fork/zap we really want it inlined because
> >
> > (1) We want to optimize out all of the unnecessary checks we added for other 
> > users
> >
> > (2) Zap/fork code is very sensitive to function call overhead
> >
> > Probably, as that function sees more widespread use, we might want a
> > non-inlined variant that can be used in places where performance doesn't
> > matter all that much (although I am not sure there will be that many).
> a quick test.
>
> before:
>     text           data     bss     dec     hex filename
>    12380            470       0   12850    3232 mm/madvise.o
>    52975           2689      24   55688    d988 mm/memory.o
>    25305           1448    2096   28849    70b1 mm/mempolicy.o
>     8573            924       4    9501    251d mm/mlock.o
>    20950           5864      16   26830    68ce mm/rmap.o
>
>   (120183)
>
> after:
>
>     text           data     bss     dec     hex filename
>    11916            470       0   12386    3062 mm/madvise.o
>    52990           2697      24   55711    d99f mm/memory.o
>    25161           1448    2096   28705    7021 mm/mempolicy.o
>     8381            924       4    9309    245d mm/mlock.o
>    20806           5864      16   26686    683e mm/rmap.o
>
>   (119254)
>
> so uninlining saves a kilobyte of text - less than I expected but
> almost 1%.
As I said, for fork+zap/unmap we really want to inline -- the first two users
of that function when that function was still simpler and resided in 
mm/memory.o. For
the other users, probably okay to have a non-inlined one in mm/util.c .

--
Cheers,

David / dhildenb