On Sun, Aug 27, 2023 at 09:01:04AM +0100, Lorenzo Stoakes wrote:
> On Mon, Aug 21, 2023 at 09:20:16PM +0100, Matthew Wilcox (Oracle) wrote:
> > - if (page && order) {
> > - /*
> > - * Communicate the allocation size to the driver:
> > - * if we managed to secure a high-order allocation,
> > - * set its first page's private to this order;
> > - * !PagePrivate(page) means it's just a normal page.
> > - */
> > - split_page(page, order);
> > - SetPagePrivate(page);
> > - set_page_private(page, order);
>
> I'm guessing this was used in conjunction with the page_private() logic
> that existed below and can simply be discarded now?
As far as I can tell, yes.
> > - }
> > + folio = __folio_alloc_node(PERF_AUX_GFP, order, node);
> > + } while (!folio && order--);
> >
> > - return page;
> > + if (order)
> > + folio_ref_add(folio, (1 << order) - 1);
>
> Can't order go to -1 if we continue to fail to allocate a folio?
Hm, yes.
> > @@ -707,17 +696,21 @@ int rb_alloc_aux(struct perf_buffer *rb, struct perf_event *event,
> >
> > rb->free_aux = event->pmu->free_aux;
> > for (rb->aux_nr_pages = 0; rb->aux_nr_pages < nr_pages;) {
> > - struct page *page;
> > - int last, order;
> > + struct folio *folio;
> > + unsigned int i, nr, order;
> > + void *addr;
> >
> > order = min(max_order, ilog2(nr_pages - rb->aux_nr_pages));
> > - page = rb_alloc_aux_page(node, order);
> > - if (!page)
> > + folio = rb_alloc_aux_folio(node, order);
> > + if (!folio)
> > goto out;
> > + addr = folio_address(folio);
> > + nr = folio_nr_pages(folio);
>
> I was going to raise the unspeakably annoying nit about this function returning
> a long, but then that made me wonder why, given folio->_folio_nr_pages is an
> unsigned int folio_nr_pages() returns a long in the first instance?
Futureproofing the API. While I don't expect us to be allocating
order-32 folios any time soon, and x86 has excluded p4d-sizes from the
architecture, I don't want to have to go through and audit everywhere
when it turns out we do want to support such a thing on some architecture.
(on x86 PMD - order 9, PUD - order 18, P4D - order 27, so we'd need a
hypothetical P5D level, or a machine with, say 16k pages, which would
go PMD-11, PUD-22, P4D-33, and be managing a folio of size 2^57 bytes)
But supporting nr_pages stored directly in the otherwise wasted space
in the folio seemed like a cheap win, and there was no reason to take
up the extra 32 bits.
Anyway, here, we know we're not getting back an arbitrary folio that
somebody else allocated, we're allocating for ourselves, and we know we're
allocating something rather smaller than that, so it's fine to calculate
in terms of unsigned int. If I were writing in rust, I'd use usize,
but since nobody's done the work to make rust types available in C yet,
I haven't done that either.
We actually use usize as a variable name in a couple of hundred places,
so turning it into a generally available type is harder than one might
like.
