Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode

[Al Viro]

On Fri, Dec 11, 2015 at 01:54:25AM +0000, Al Viro wrote:

> 	BTW, why are we passing unsigned long to free_page()?  We have
> a bit under 700 callers; excluding the ones that have an explicit cast
> to unsigned long right in the argument of call leaves ~150, and the rest
> tend to contain a lot of pointer casts of unsigned long thing they are feeding
> to free_page() (IOW, they would be just as happy if they kept it as a pointer
> all along).  Sure, that would mean __get_free_page() et.al. returning void *,
> but I don't see any problems with that either...  Is that just for historical
> reasons, or is there anything more subtle I'm missing here?

The situation with free_pages() is even funnier - we have 313 call sites,
and after converting it to void(void *, unsigned) 31 of them need casts
to void *.  Right now the mainline has 249 of those call sites with
cast to unsigned long (or ulong, in several places).  Then there's a bunch
of places where we do __get_free_pages(), then use it a lot (all with
casts to pointers), then pass to free_pages() - those would be just fine
with storing it as void *, but even leaving those aside...

The current signature is contrary to actual use - nearly 80% of call
sites are forced to cast a pointer to unsigned long, only to have it
cast back to void * in free_pages() itself, we would obviously be better
off if we'd switched to just passing the damn thing as a pointer.  Especially
since that 80% turn into 90% once you add the callers that could easily
switch to storing the value eventually passed to free_pages() as a pointer.

And free_page() is basically the same story, only with twice as many
call sites...

While we are at it: __get_free_pages() has 238 call sites.  193 of them
immediately cast to pointer.  And there's a bunch of places like this:
static inline void __tlb_alloc_page(struct mmu_gather *tlb)
{
        unsigned long addr = __get_free_pages(GFP_NOWAIT | __GFP_NOWARN, 0);

        if (addr) {
                tlb->pages = (void *)addr;
                tlb->max = PAGE_SIZE / sizeof(struct page *);
        }
}
where the cast is not immediate, but might as well had been.  And conversion
of free_pages() whittles it down even more.  For __get_free_page() the
situation is about the same, ditto for get_zeroed_page().

I realize that get_free_page() has been returning unsigned long since 0.01,
but looking at 0.01... it might as well had been returning void * - wouldn't
be more inconvenient.  The kludge you had in get_pipe_inode() would've been
a bit more obviously wrong, but that's about it ;-)

Seriously, though - what do you think about a flagday commit right after
4.5-rc1 switching all those guys to void *?  For __get_user_pages(),
__get_user_page(), get_zeroed_page() - return values, for free_pages(),
free_page() - argument.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/