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

Groups > linux.kernel > #1296363 > unrolled thread

Re: [RFC] free_pages stuff

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: [RFC] free_pages stuff Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-22 01:10 +0100
    Re: [RFC] free_pages stuff Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-22 02:10 +0100
      Re: [RFC] free_pages stuff Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-22 02:30 +0100
        Re: [RFC] free_pages stuff Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-22 04:20 +0100
      Re: [RFC] free_pages stuff Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-22 03:30 +0100
        Re: [RFC] free_pages stuff Geert Uytterhoeven <geert@linux-m68k.org> - 2015-12-22 09:30 +0100
          Re: [RFC] free_pages stuff Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-22 22:10 +0100

#1296363 — Re: [RFC] free_pages stuff

On Mon, Dec 21, 2015 at 3:46 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>         FWIW, I'd done a proof-of-concept patch series converting the things
> to
>         * free_page() and free_pages() taking the address to free as a pointer
>         * get_zeroed_page() returning a pointer
>         * get_free_page()/get_free_pages() added, both returning a pointer
>         * __get_dma_pages() replaced with get_dma_page() (again, returns
> a pointer)

Absolutely not.

I will not take this, and it's stupid in the extreme.

No way in hell do we suddenly change the semantics of an interface
that has been around from basically day #1. That's just crazy talk.

Just looking at the diffstat should have made you realize that this is
stupid. The confusion it causes, the pain it causes for backports, and
just the fundamental idiocy of changing an long-standing interface
without changing the name is just not acceptable.

If you want to have versions of the function that return pointers, you
had damn well better give them new names. Not use the same name for a
different function, causing confusion and forcing this kind of crazy
"change everything at once" flag-day patches, and pain for
backporting.

And quite frankly, even the "new name" is likely a bad idea. If you
want to allocate a page, and get a pointer, just use "kmalloc()".
Boom, done!

So I don't know how many ways I can say "NO", but I'll not take
anythign like this. It's *completely* wrong.

                 Linus
--
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] | [next] | [standalone]

#1296408

On Mon, Dec 21, 2015 at 04:03:11PM -0800, Linus Torvalds wrote:

> If you want to have versions of the function that return pointers, you
> had damn well better give them new names. Not use the same name for a
> different function, causing confusion and forcing this kind of crazy
> "change everything at once" flag-day patches, and pain for
> backporting.

*shrug*

Up to you.  I'll cherry-pick the fixes for bugs found in process and leave
the rest alone.
 
> And quite frankly, even the "new name" is likely a bad idea. If you
> want to allocate a page, and get a pointer, just use "kmalloc()".
> Boom, done!

Erm...  You've just described the absolute majority of callers.  Really.
Counting typecasts demonstrates _that_ very clearly.  Any place that
wanted to allocate a page and get a pointer needs a typecast in mainline;
any place that wanted to allocate a page and get a number would need
one after this series.  Similar for freeing something that is a pointer
and is a number respectively.

The total after that series was 70 typecasts added and 1408 removed.  In other
words, "want to allocate a page and get a pointer" outnumbers the "want to
allocate a page and get a number" a _lot_.

Do you really mean that we are overusing __get_free_page() and friends that
much and that we should simply use kmalloc() instead?  I'm not saying that
it's wrong - a lot of places clearly would be fine with kmalloc/kfree.
For something like page table allocations kmalloc() is obviously wrong (and
they also are of "get a pointer" sort), but that's a very small fraction.

> So I don't know how many ways I can say "NO", but I'll not take
> anythign like this. It's *completely* wrong.

OK.  Don't get me wrong - I'm not fond of all-over-the-tree changes either;
I wanted to figure out how the damn thing is actually used and I have found
that.  What (if anything) to do with that is a separate question.

For me the bottom line so far is that we have a lot of places where page
allocator is used and the majority of those uses the result as a pointer.
That, with the calling conventions we have (and had all along), means tons
of boilerplate.  It also means a lot of opportunities to mix physical,
virtual and DMA addresses, since typechecking is completely bypassed by
those typecasts.

If I understood you correctly, in a lot of those cases the answer should've
been "just use kmalloc() and be done with that".  And something like
e.g. debugfs read and write methods of some wireless NIC driver
certainly could use kmalloc(); any concerns about extra overhead are
ridiculous there.  The same goes for e.g. sysfs symlink body generated
when we run into one, etc.  I'm not suggesting to start converting existing
code to kmalloc; that only goes for new code being written, obviously.

PS: in case you've said that "NO" earlier and I'd missed your replies, my
apologies for keeping that up; this is really the first response from you I've
seen on this topic.
--
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] | [prev] | [next] | [standalone]

#1296417

[ Grr. Resending because the stupid android gmail app still can't do
text emails ]

On Dec 21, 2015 17:04, "Al Viro" <viro@zeniv.linux.org.uk> wrote:
>
> > And quite frankly, even the "new name" is likely a bad idea. If you
> > want to allocate a page, and get a pointer, just use "kmalloc()".
> > Boom, done!
>
> Erm...  You've just described the absolute majority of callers.  Really.

That wasn't my point.

I totally believe that most of the legacy users actually wanted a pointer.

But that doesn't mean that we should just convert a legacy interface.
We should either just create a new interface and leave old users
alone, or if we care about that code and really want to remove the
cast, maybe it should just use kmalloc() instead.

Long ago, allocating a page using kmalloc() was a bad idea, because
there was overhead for it in the allocation and the code.

These days, kmalloc() not only doesn't have the allocation overhead,
but may actually scale better too, thanks to percpu caches etc.

So my point here is that not only is it wrong to change the calling
convention for a legacy function (and it really probably doesn't get
much more legacy than get_free_page - I think it's been around
forever), but even the "let's make up a new name" conversion may be
wrong, because it's entirely possible that the code in question should
just be using kmalloc().

So I don't think an automatic conversion is a good idea. I suspect
that old code that somebody isn't actively working on should just be
left alone, and code that *is* actively worked on should maybe
consider kmalloc().

And if the code really explicitly wants a page (or set of aligned
pages) for some vm reason, I suspect having the cast there isn't a bad
thing. It's clearly not just a random pointer allocation if the bit
pattern of the pointer matters.

And yes, most of the people who used to want "unsigned long" have long
since been converted to take "struct page *" instead, since things
like the VM wants highmem pages etc.  There's a reason why the
historical interface returns "unsigned long": it _used_ to be the
right thing for a lot of code. The fact that there now are more casts
than not are about changing use patterns, but I don't think that means
that we should change the calling convention that has a historical
reason for it.

     Linus
--
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] | [prev] | [next] | [standalone]

#1296510

On Mon, Dec 21, 2015 at 05:23:11PM -0800, Linus Torvalds wrote:

> And if the code really explicitly wants a page (or set of aligned
> pages) for some vm reason, I suspect having the cast there isn't a bad
> thing. It's clearly not just a random pointer allocation if the bit
> pattern of the pointer matters.
> 
> And yes, most of the people who used to want "unsigned long" have long
> since been converted to take "struct page *" instead, since things
> like the VM wants highmem pages etc.  There's a reason why the
> historical interface returns "unsigned long": it _used_ to be the
> right thing for a lot of code. The fact that there now are more casts
> than not are about changing use patterns, but I don't think that means
> that we should change the calling convention that has a historical
> reason for it.

Umm...  Depends on how early you look.  In 0.01:
fs/exec.c:179:                  if (!(page[i]=get_free_page()))
fs/inode.c:208: if (!(inode->i_size=get_free_page())) {
kernel/fork.c:70:       p = (struct task_struct *) get_free_page();
mm/memory.c:143:                if (!(to_page_table = (unsigned long *) get_free_page()))
mm/memory.c:185:                if (!(tmp=get_free_page()))
mm/memory.c:203:        if (!(new_page=get_free_page()))
mm/memory.c:241:        if (tmp=get_free_page())

the last 3 are "we want struct page *" variety, the rest is "want a pointer"
stuff (exec.c one is copy_strings() and the only use of pages[...] is a cast
to char *, inode.c one is get_pipe_inode() - I guess you didn't want to bother
with adding pipe-specific fields to struct inode at that point and since
->i_size is unused for pipes anyway reused it to keep a pointer to pipe
buffer).  Even then it wasn't the majority.

By 2.0 (still before highmem) the fraction was already _way_ below 50%...
--
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] | [prev] | [next] | [standalone]

#1296427

On Mon, Dec 21, 2015 at 05:16:44PM -0800, Linus Torvalds wrote:
> On Dec 21, 2015 17:04, "Al Viro" <viro@zeniv.linux.org.uk> wrote:
> >
> > > And quite frankly, even the "new name" is likely a bad idea. If you
> > > want to allocate a page, and get a pointer, just use "kmalloc()".
> > > Boom, done!
> >
> > Erm...  You've just described the absolute majority of callers.  Really.
> 
> That wasn't my point.
> 
> I totally believe that most of the legacy users actually wanted a pointer.
> 
> But that doesn't mean that we should just convert a legacy interface. We
> should either just create a new interface and leave old users alone, or if
> we care about that code and really want to remove the cast, maybe it should
> just use kmalloc() instead.
> 
> Long ago, allocating a page using kmalloc() was a bad idea, because there
> was overhead for it in the allocation and the code.
> 
> These days, kmalloc() not only doesn't have the allocation overhead, but
> may actually scale better too, thanks to percpu caches etc.
> 
> So my point here is that not only is it wrong to change the calling
> convention for a legacy function (and it really probably doesn't get much
> more legacy than get_free_page - I think it's been around forever), but

Yes - present in v0.01, with similar situation re callers even back then ;-)

> even the "let's make up a new name" conversion may be wrong, because it's
> entirely possible that the code in question should just be using kmalloc().
> 
> So I don't think an automatic conversion is a good idea. I suspect that old
> code that somebody isn't actively working on should just be left alone, and
> code that *is* actively worked on should maybe consider kmalloc().

Agreed.  Again, what I really wanted was to get the clear picture of what
uses _are_ there.  In more details than just "grepping seems to indicate
that...".  It's really pretty much all of them.

> And if the code really explicitly wants a page (or set of aligned pages)
> for some vm reason, I suspect having the cast there isn't a bad thing. It's
> clearly not just a random pointer allocation if the bit pattern of the
> pointer matters.

BTW, I'm not sure we don't have code that would assume that
kmalloc(PAGE_SIZE,...) always returns something PAGE_SIZE-aligned.

FWIW, pointer-returning get_free_page() would not be a flagday change at
all - we only have __get_free_page()/__get_free_pages() right now.  And I'm
not sure that it wouldn't make sense to add void *-returning variants without
underscores - not for bulk conversion of existing callers, but for new
places that want a page.  Because most of the new ones (and new ones keep
appearing; it's not just ancient code) still want a pointer.  And yes, quite
a few of those should be using something else.

Example (went into the tree just three months ago):
static inline void *scif_zalloc(size_t size)
{
        void *ret = NULL;
        size_t align = ALIGN(size, PAGE_SIZE);

        if (align && get_order(align) < MAX_ORDER)
                ret = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
                                               get_order(align));
        return ret ? ret : vzalloc(align);
}

This clearly should be using kzalloc() instead of __get_free_pages() (and
I'm not sure whether the cutoff is right - similar "kmalloc if not too
large, vmalloc otherwise" tends to have cutoff lower than MAX_ORDER;
PAGE_ALLOC_COSTLY_ORDER is more common).  The callers do not look like
they would care about page alignment - at least quite a few of them do
not.

Incidentally, those caller include the following example of lousy naming:
        (*pages)->phys_addr = scif_zalloc(nr_pages * sizeof(dma_addr_t));
First of all, the address is clearly virtual - it's an array!  What's more,
I really wonder whether it's DMA or physical addresses that are stored there.
It's declared as an array of dma_addr_t, but...
                (*pages)->phys_addr[i] =
                        __scif_off_to_dma_addr(window, offset +
                                               (i * PAGE_SIZE));
                (*pages)->phys_addr[i] = scif_get_phys((*pages)->phys_addr[i],
                                                        ep);
and
static phys_addr_t scif_get_phys(phys_addr_t phys, struct scif_endpt *ep)
seems to indicate something fishy going on.

Typechecking for different kinds of addresses is really too weak, and the
amount of casts we have around them doesn't help either ;-/
--
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] | [prev] | [next] | [standalone]

#1296612

On Tue, Dec 22, 2015 at 3:22 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> And if the code really explicitly wants a page (or set of aligned pages)
>> for some vm reason, I suspect having the cast there isn't a bad thing. It's
>> clearly not just a random pointer allocation if the bit pattern of the
>> pointer matters.
>
> BTW, I'm not sure we don't have code that would assume that
> kmalloc(PAGE_SIZE,...) always returns something PAGE_SIZE-aligned.

Yeah, needs-to-be-PAGE_SIZE-aligned is probably one of the main
reasons of not calling kmalloc().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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] | [prev] | [next] | [standalone]

#1297064

On Tue, Dec 22, 2015 at 09:21:14AM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 22, 2015 at 3:22 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> And if the code really explicitly wants a page (or set of aligned pages)
> >> for some vm reason, I suspect having the cast there isn't a bad thing. It's
> >> clearly not just a random pointer allocation if the bit pattern of the
> >> pointer matters.
> >
> > BTW, I'm not sure we don't have code that would assume that
> > kmalloc(PAGE_SIZE,...) always returns something PAGE_SIZE-aligned.
> 
> Yeah, needs-to-be-PAGE_SIZE-aligned is probably one of the main
> reasons of not calling kmalloc().

FWIW, looking through the fs/* uses of __get_free_pages() and its wrappers...
	* affs_grow_extcache() - might as well have been kmalloc (and
I'm not sure that fixed PAGE_SIZE is the best size there, actually).
	* afs_mntpt_do_automount() - kmalloc().
	* bfs_dump_imap() - kmalloc().
	* bm_entry_read() - kmalloc(), and might be better off with
single_open-style seqfile.
	* ceph_alloc_readdir_reply_buffer() - kmalloc().
	* configfs fill_write_buffer() - kmalloc(), and it might be worth
a helper similar to memdup_user() that would allocate size + 1 bytes,
copy size bytes from user and put '\0' after them.  There's a lot of
->write() instances open-coding that.
	* configfs fill_read_buffer() - kmalloc().
	* configfs_follow_link() - kmalloc().
	* ext4_calculate_overhead() - kmalloc().
	* fuse_follow_link() - kmalloc().
	* fuse_do_ioctl() - kmalloc(), and it might as well do more accurate
size calculation
	* hfs_mdb_get() - 8Kb kmalloc().
	* isofs_readdir() - kmalloc().
	* jbd2_alloc() - really wants alignment; might make sense to have
3 more private slabs there (they are using kmem_cache_alloc() for sub-page
allocation, with explicit alignments set when creating those; anything
from 8 pages and up goes to vmalloc()).
	* jfs_readdir() - kmalloc().
	* jfs lbmLogInit() - alloc_page().  _This_ is one that really wants
struct page (and uses virt_to_page() to get it after get_zeroed_page()).
	* kernfs_iop_follow_link() - kmalloc().
	* simple_transaction_get() - kmalloc().
	* copy_mount_options() - kmalloc().
	* nfs_do_submount() - kmalloc().
	* nfs_follow_referral() - kmalloc().
	* nfs4_replace_transport() - kmalloc().
	* nfs_show_devname() - kmalloc().
	* nfsd_buffered_readdir() - kmalloc().
	* nilfs_ioctl_wrap_copy() - kmalloc() (and I'm not sure that PAGE_SIZE
is the best possible variant there).
	* fs/ocfs2/dlm/dlmdebug.c ones - kmalloc(), all of them.
	* dlm_alloc_pagevec() - used to allocate hash tables.  kmalloc() will
definitely do, but I would consider using a single kmalloc or vmalloc instead
of an array of page-sized blocks.  Guaranteed extra dereference on every
hash lookup is potentially painful.
	* dlm_migrate_lockres() - kmalloc().
	* dlm_request_all_locks_handler() - kmalloc().
	* ovl_read_symlink() - kmalloc().
	* do_proc_readlink() - kmalloc().
	* proc_pid_cmdline_read() - kmalloc().
	* proc_pid_attr_write() - memdup_user().
	* mem_rw() - kmalloc().
	* environ_read() - kmalloc().
	* dquot_init() - hash allocation; alloc_large_system_hash(), unless
there's something I'm missing...
	* poll_get_entry() - kmalloc(), probably.
	* fs/proc/vmcore.c - interesting one; it allocates a buffer to
store elf headers from crashdump, and everything would be simple, expect
for mmap() support in there.  Which wants it to be page-aligned and uses
remap_pfn_range() from that sucker.

IOW, there is one place that can't live with kmalloc() due to alignment
requirements (jbd2_alloc()), one place that wants struct page * (in jfs)
and one place that wants page-aligned buffer it will feed to remap_pfn_range().
And 35 places that have no good reason for using __get_free_pages() or
its wrappers.

So at least for fs/* the answer is definitely "almost all places
where we are using page allocator would be better off with something
else".

Documentation/which-allocator-should-I-use might be a good idea...  Notes
below are just a skeleton - a lot of details need to be added; in particular,
there should be a part on "I have this kind of address and I want that;
when and how should that be done?", completely missing here.  And there
should be a big scary warning along the lines of "this is NOT an invitation
for a flood of checkpatch-inspired patches"...

Comments, corrections and additions would be very welcome.

1) Most of the time kmalloc() is the right thing to use.
Limitations: alignment is no better than word, not available very early in
bootstrap, allocated memory is physically contiguous, so large allocations
are best avoided.

2) kmem_cache_alloc() allows to specify the alignment at cache creation
time.  Otherwise it's similar to kmalloc().  Normally it's used for
situations where we have a lot of instances of some type and want dynamic
allocation of those.

3) vmalloc() is for large allocations.  They will be page-aligned,
but *not* physically contiguous.  OTOH, large physically contiguous
allocations are generally a bad idea.  Unlike other allocators, there's
no variant that could be used in interrupt; freeing is possible there,
but allocation is not.  Note that non-blocking variant *does* exist -
__vmalloc(size, GFP_ATOMIC, PAGE_KERNEL) can be used in atomic
contexts; it's the interrupt ones that are no-go.

4) if it's very early in bootstrap, alloc_bootmem() and friends
may be the only option.  Rule of the thumb: if it's already printed
Memory: ...../..... available.....
you shouldn't be using that one.  Allocations are physically contiguous
and at that point large physically contiguous allocations are still OK.

5) if you need to allocate memory for DMA, use dma_alloc_coherent()
and friends.  They'll give you both the virtual address for your use
and DMA address refering to the same memory for use by device; do *NOT*
try to derive the latter from the former; use of virt_to_bus() et.al.
is a Bloody Bad Idea(tm).

6) if you need a reference to struct page, use alloc_page/alloc_pages.

7) in some cases (page tables, for the most obvious example), __get_free_page()
and friends might be the right answer.  In principle, it's case (6), but
it returns page_address(page) instead of the page itself.  Historically that
was the first API introduced, so a _lot_ of places that should've been using
something else ended up using that.  Do not assume that being lower level
makes it faster than e.g. kmalloc() - this is simply not true.
--
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] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web