| Message ID | 20230821202016.2910321-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Convert perf ringbuffer to folios | expand |
On 8/22/23 04:20, Matthew Wilcox (Oracle) wrote: > Use the folio API to alloc & free memory. Also pass in the node ID > instead of the CPU ID since we already did that calculation in the > caller. And call numa_mem_id() instead of leaving the node ID as > -1 and having the MM call numa_mem_id() each time. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Yin Fengwei <fengwei.yin@intel.com> Regards Yin, Fengwei > --- > kernel/events/ring_buffer.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > index fb1e180b5f0a..cc90d5299005 100644 > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -770,7 +770,7 @@ void rb_free_aux(struct perf_buffer *rb) > #ifndef CONFIG_PERF_USE_VMALLOC > > /* > - * Back perf_mmap() with regular GFP_KERNEL-0 pages. > + * Back perf_mmap() with regular GFP_KERNEL pages. > */ > > static struct page * > @@ -785,25 +785,23 @@ __perf_mmap_to_page(struct perf_buffer *rb, unsigned long pgoff) > return virt_to_page(rb->data_pages[pgoff - 1]); > } > > -static void *perf_mmap_alloc_page(int cpu) > +static void *perf_mmap_alloc_page(int node) > { > - struct page *page; > - int node; > + struct folio *folio; > > - node = (cpu == -1) ? cpu : cpu_to_node(cpu); > - page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0); > - if (!page) > + folio = __folio_alloc_node(GFP_KERNEL | __GFP_ZERO, 0, node); > + if (!folio) > return NULL; > > - return page_address(page); > + return folio_address(folio); > } > > static void perf_mmap_free_page(void *addr) > { > - struct page *page = virt_to_page(addr); > + struct folio *folio = virt_to_folio(addr); > > - page->mapping = NULL; > - __free_page(page); > + folio->mapping = NULL; > + folio_put(folio); > } > > struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) > @@ -818,17 +816,17 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) > if (order_base_2(size) > PAGE_SHIFT+MAX_ORDER) > goto fail; > > - node = (cpu == -1) ? cpu : cpu_to_node(cpu); > + node = (cpu == -1) ? numa_mem_id() : cpu_to_node(cpu); > rb = kzalloc_node(size, GFP_KERNEL, node); > if (!rb) > goto fail; > > - rb->user_page = perf_mmap_alloc_page(cpu); > + rb->user_page = perf_mmap_alloc_page(node); > if (!rb->user_page) > goto fail_user_page; > > for (i = 0; i < nr_pages; i++) { > - rb->data_pages[i] = perf_mmap_alloc_page(cpu); > + rb->data_pages[i] = perf_mmap_alloc_page(node); > if (!rb->data_pages[i]) > goto fail_data_pages; > }
On Mon, Aug 21, 2023 at 09:20:13PM +0100, Matthew Wilcox (Oracle) wrote: > Use the folio API to alloc & free memory. Also pass in the node ID > instead of the CPU ID since we already did that calculation in the > caller. And call numa_mem_id() instead of leaving the node ID as > -1 and having the MM call numa_mem_id() each time. It's a bit of a nit or perhaps an aside, but with this change if you passed -1 to the newly invoked __folio_alloc_node() you will also trigger a VM_BUG_ON() :) > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > kernel/events/ring_buffer.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c > index fb1e180b5f0a..cc90d5299005 100644 > --- a/kernel/events/ring_buffer.c > +++ b/kernel/events/ring_buffer.c > @@ -770,7 +770,7 @@ void rb_free_aux(struct perf_buffer *rb) > #ifndef CONFIG_PERF_USE_VMALLOC > > /* > - * Back perf_mmap() with regular GFP_KERNEL-0 pages. > + * Back perf_mmap() with regular GFP_KERNEL pages. > */ > > static struct page * > @@ -785,25 +785,23 @@ __perf_mmap_to_page(struct perf_buffer *rb, unsigned long pgoff) > return virt_to_page(rb->data_pages[pgoff - 1]); > } > > -static void *perf_mmap_alloc_page(int cpu) > +static void *perf_mmap_alloc_page(int node) Nitty point but since we're dealing with folios here maybe rename to perf_mmap_alloc_folio()? > { > - struct page *page; > - int node; > + struct folio *folio; > > - node = (cpu == -1) ? cpu : cpu_to_node(cpu); > - page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0); > - if (!page) > + folio = __folio_alloc_node(GFP_KERNEL | __GFP_ZERO, 0, node); > + if (!folio) > return NULL; > > - return page_address(page); > + return folio_address(folio); > } > > static void perf_mmap_free_page(void *addr) Same comment re: rename -> perf_mmap_free_folio() > { > - struct page *page = virt_to_page(addr); > + struct folio *folio = virt_to_folio(addr); > > - page->mapping = NULL; > - __free_page(page); > + folio->mapping = NULL; > + folio_put(folio); > } > > struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) > @@ -818,17 +816,17 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) > if (order_base_2(size) > PAGE_SHIFT+MAX_ORDER) > goto fail; > > - node = (cpu == -1) ? cpu : cpu_to_node(cpu); > + node = (cpu == -1) ? numa_mem_id() : cpu_to_node(cpu); This is a good change in general, it's pretty yucky that this code just assumes -1 == NUMA_NO_NODE and that all invoked functions would handle this correctly. > rb = kzalloc_node(size, GFP_KERNEL, node); > if (!rb) > goto fail; > > - rb->user_page = perf_mmap_alloc_page(cpu); > + rb->user_page = perf_mmap_alloc_page(node); > if (!rb->user_page) > goto fail_user_page; > > for (i = 0; i < nr_pages; i++) { > - rb->data_pages[i] = perf_mmap_alloc_page(cpu); > + rb->data_pages[i] = perf_mmap_alloc_page(node); > if (!rb->data_pages[i]) > goto fail_data_pages; > } > -- > 2.40.1 > Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>
On Sun, Aug 27, 2023 at 08:33:39AM +0100, Lorenzo Stoakes wrote: > > @@ -785,25 +785,23 @@ __perf_mmap_to_page(struct perf_buffer *rb, unsigned long pgoff) > > return virt_to_page(rb->data_pages[pgoff - 1]); > > } > > > > -static void *perf_mmap_alloc_page(int cpu) > > +static void *perf_mmap_alloc_page(int node) > > Nitty point but since we're dealing with folios here maybe rename to > perf_mmap_alloc_folio()? Since it's an explicit order-0 allocation, does that really make sense? > > { > > - struct page *page; > > - int node; > > + struct folio *folio; > > > > - node = (cpu == -1) ? cpu : cpu_to_node(cpu); > > - page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0); > > - if (!page) > > + folio = __folio_alloc_node(GFP_KERNEL | __GFP_ZERO, 0, node); > > + if (!folio) > > return NULL; > > > > - return page_address(page); > > + return folio_address(folio); > > }
On Wed, Sep 13, 2023 at 03:31:05PM +0200, Peter Zijlstra wrote: > On Sun, Aug 27, 2023 at 08:33:39AM +0100, Lorenzo Stoakes wrote: > > > > @@ -785,25 +785,23 @@ __perf_mmap_to_page(struct perf_buffer *rb, unsigned long pgoff) > > > return virt_to_page(rb->data_pages[pgoff - 1]); > > > } > > > > > > -static void *perf_mmap_alloc_page(int cpu) > > > +static void *perf_mmap_alloc_page(int node) > > > > Nitty point but since we're dealing with folios here maybe rename to > > perf_mmap_alloc_folio()? > > Since it's an explicit order-0 allocation, does that really make sense? > True, it does ultimately yield a single page and doesn't reference its metadata so it's not the end of the world to keep it as-is (it was very nitty after all!) > > > { > > > - struct page *page; > > > - int node; > > > + struct folio *folio; > > > > > > - node = (cpu == -1) ? cpu : cpu_to_node(cpu); > > > - page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0); > > > - if (!page) > > > + folio = __folio_alloc_node(GFP_KERNEL | __GFP_ZERO, 0, node); > > > + if (!folio) > > > return NULL; > > > > > > - return page_address(page); > > > + return folio_address(folio); > > > } >
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c index fb1e180b5f0a..cc90d5299005 100644 --- a/kernel/events/ring_buffer.c +++ b/kernel/events/ring_buffer.c @@ -770,7 +770,7 @@ void rb_free_aux(struct perf_buffer *rb) #ifndef CONFIG_PERF_USE_VMALLOC /* - * Back perf_mmap() with regular GFP_KERNEL-0 pages. + * Back perf_mmap() with regular GFP_KERNEL pages. */ static struct page * @@ -785,25 +785,23 @@ __perf_mmap_to_page(struct perf_buffer *rb, unsigned long pgoff) return virt_to_page(rb->data_pages[pgoff - 1]); } -static void *perf_mmap_alloc_page(int cpu) +static void *perf_mmap_alloc_page(int node) { - struct page *page; - int node; + struct folio *folio; - node = (cpu == -1) ? cpu : cpu_to_node(cpu); - page = alloc_pages_node(node, GFP_KERNEL | __GFP_ZERO, 0); - if (!page) + folio = __folio_alloc_node(GFP_KERNEL | __GFP_ZERO, 0, node); + if (!folio) return NULL; - return page_address(page); + return folio_address(folio); } static void perf_mmap_free_page(void *addr) { - struct page *page = virt_to_page(addr); + struct folio *folio = virt_to_folio(addr); - page->mapping = NULL; - __free_page(page); + folio->mapping = NULL; + folio_put(folio); } struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) @@ -818,17 +816,17 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags) if (order_base_2(size) > PAGE_SHIFT+MAX_ORDER) goto fail; - node = (cpu == -1) ? cpu : cpu_to_node(cpu); + node = (cpu == -1) ? numa_mem_id() : cpu_to_node(cpu); rb = kzalloc_node(size, GFP_KERNEL, node); if (!rb) goto fail; - rb->user_page = perf_mmap_alloc_page(cpu); + rb->user_page = perf_mmap_alloc_page(node); if (!rb->user_page) goto fail_user_page; for (i = 0; i < nr_pages; i++) { - rb->data_pages[i] = perf_mmap_alloc_page(cpu); + rb->data_pages[i] = perf_mmap_alloc_page(node); if (!rb->data_pages[i]) goto fail_data_pages; }
Use the folio API to alloc & free memory. Also pass in the node ID instead of the CPU ID since we already did that calculation in the caller. And call numa_mem_id() instead of leaving the node ID as -1 and having the MM call numa_mem_id() each time. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- kernel/events/ring_buffer.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)