[lkml]   [2015]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCHSET v2] ->follow_link() without dropping from RCU mode
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() 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.

 \ /
  Last update: 2015-12-13 05:01    [W:0.086 / U:4.644 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site