Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290546 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHSET v2] ->follow_link() without dropping from RCU mode Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-13 04:50 +0100

#1290546 — Re: [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() 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [standalone]

Back to top | Article view | linux.kernel

csiph-web