> On Mon, Mar 27, 2023 at 07:01:26PM +0200, Uladzislau Rezki (Sony) wrote:
> > Add vm_map_ram()/vm_unmap_ram() test case to our stress test-suite.
> >
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> > ---
> > lib/test_vmalloc.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 41 insertions(+)
> >
> > diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> > index cd2bdba6d3ed..6633eda4cd4d 100644
> > --- a/lib/test_vmalloc.c
> > +++ b/lib/test_vmalloc.c
> > @@ -53,6 +53,7 @@ __param(int, run_test_mask, INT_MAX,
> > "\t\tid: 128, name: pcpu_alloc_test\n"
> > "\t\tid: 256, name: kvfree_rcu_1_arg_vmalloc_test\n"
> > "\t\tid: 512, name: kvfree_rcu_2_arg_vmalloc_test\n"
> > + "\t\tid: 1024, name: vm_map_ram_test\n"
> > /* Add a new test case description here. */
> > );
> >
> > @@ -358,6 +359,45 @@ kvfree_rcu_2_arg_vmalloc_test(void)
> > return 0;
> > }
> >
> > +static int
> > +vm_map_ram_test(void)
> > +{
> > + unsigned int map_nr_pages;
> > + unsigned char *v_ptr;
> > + unsigned char *p_ptr;
> > + struct page **pages;
> > + struct page *page;
> > + int i;
> > +
> > + map_nr_pages = nr_pages > 0 ? nr_pages:1;
> > + pages = kmalloc(map_nr_pages * sizeof(*page), GFP_KERNEL);
> > + if (!pages)
> > + return -1;
> > +
> > + for (i = 0; i < map_nr_pages; i++) {
> > + page = alloc_pages(GFP_KERNEL, 1);
>
> Pedantry, but given I literally patched this pedantically the other day,
> this could be alloc_page(GFP_KERNEL) :)
>
> > + if (!page)
> > + return -1;
>
> We're leaking memory here right? Should jump to cleanup below.
>
> > +
> > + pages[i] = page;
> > + }
>
>
> You should be able to replace this with something like:-
>
> unsigned long nr_allocated;
>
> ...
>
> nr_allocated = alloc_pages_bulk_array(GFP_KERNEL, map_nr_pages, pages);
> if (nr_allocated != map_nr_pages)
> goto cleanup;
>
> > +
> > + /* Run the test loop. */
> > + for (i = 0; i < test_loop_count; i++) {
> > + v_ptr = vm_map_ram(pages, map_nr_pages, -1);
>
> NIT: The -1 would be clearer as NUMA_NO_NODE
>
> > + *v_ptr = 'a';
> > + vm_unmap_ram(v_ptr, map_nr_pages);
> > + }
> > +
>
> Reference to the above you'd add the cleanup label here:-
>
> cleanup:
>
> > + for (i = 0; i < map_nr_pages; i++) {
> > + p_ptr = page_address(pages[i]);
> > + free_pages((unsigned long)p_ptr, 1);
>
> Nit, can be free_page((unsigned long)p_ptr);
>
Thank you. Will fix all comments, especially switching to the
alloc_page() new API :)
--
Uladzislau Rezki
