On Tue, Sep 22, 2020 at 02:12:19PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 22, 2020 at 12:44:51PM +0100, Matthew Wilcox wrote:
> > I've just kicked off a test run using this:
> >
> > +/*
> > + * Have to be careful when freeing a non-compound allocation in case somebody
> > + * else takes a temporary reference on the first page and then calls put_page()
> > + */
> > void __free_pages(struct page *page, unsigned int order)
> > {
> > - if (put_page_testzero(page))
> > - free_the_page(page, order);
> > + if (likely(page_ref_freeze(page, 1)))
> > + goto free;
> > + if (likely(order == 0 || PageHead(page))) {
> > + if (put_page_testzero(page))
> > + goto free;
> > + return;
> > + }
> > +
> > + prep_compound_page(page, order);
> > + put_page(page);
> > + return;
> > +free:
> > + free_the_page(page, order);
> > }
> > EXPORT_SYMBOL(__free_pages);
> >
> > > > Better ideas?
> > >
> > > IMHO, alloc_pages() with order > 0 and without __GFP_COMP is a weird beast. In
> > > that case it would be probably best to refcount each base page separately. I
> > > don't know how many assumptions this would break :/
> >
> > You sound like a man who's never dealt with device drivers. Multiorder,
> > non-compound allocations are the norm in that world.
>
> This seems easier to reason about:
>
> +/*
> + * If we free a non-compound allocation, another thread may have a
> + * transient reference to the first page. It has no way of knowing
> + * about the rest of the allocation, so we have to free all but the
> + * first page here.
> + */
> void __free_pages(struct page *page, unsigned int order)
> {
> if (put_page_testzero(page))
> free_the_page(page, order);
> + else
> + while (order-- > 0)
> + free_the_page(page + 1 << order, order);
> }
> EXPORT_SYMBOL(__free_pages);
To be honest, I have not dealt with device drivers much myself, but this
looks bogus. We free pages beynd the first one if the first is pinned.
How is it correct?
--
Kirill A. Shutemov
