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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
