Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increase the size of uvm memory allocation #11

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

0x5459
Copy link

@0x5459 0x5459 commented Sep 17, 2024

1<<16 is a very small number. I request to increase this limit. Perhaps we can discuss whether this limit can be removed.

@@ -257,7 +257,7 @@ static void *alloc_internal(size_t size, bool zero_memory)
// Make sure that (sizeof(hdr) + size) is what it should be
BUILD_BUG_ON(sizeof(uvm_vmalloc_hdr_t) != offsetof(uvm_vmalloc_hdr_t *, ptr));

assert(size <= (1 << 16));
assert(size <= (1 << 32));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (1 << 16) limit is there because the kernel heap used for these allocations supports a malloc-style interface (where you don't need to specify a size value when freeing memory) only for allocations sizes up to (1 << 16) (see the MAX_MCACHE_ORDER constant in the Nanos source at src/config.h). So it cannot be changed to a larger value.
If we want to remove this limit, we need to be able to use the vmalloc API when allocations larger that (1 << MAX_MCACHE_ORDER) are requested. This would entail:

  • changing the UVM_KMALLOC_THRESHOLD definition in kernel_open/nvidia_uvm/uvm_kvmalloc.h from infinity to (1 << MAX_MCACHE_ORDER)
  • changing the is_vmalloc_addr() definition in kernel_open/common/inc/nv_nanos.h from false to (objcache_from_object(u64_from_pointer(p), PAGESIZE_2M) == INVALID_ADDRESS)
  • changing the vfree() definition in kernel_open/common/inc/nv_nanos.h from kfree to NV_KFREE; this means the macro will take an additional size parameter, for which the macro invocations can use the alloc_size value of the corresponding uvm_vmalloc_hdr_t struct (all vmalloc allocations use this header)

@0x5459 0x5459 force-pushed the increase-uvm-alloc-size branch from e2ab22b to 7632b7c Compare November 4, 2024 03:25
Copy link
Member

@francescolavra francescolavra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before submitting changes for review, the resulting code should be tested. These changes haven't even been build-tested.
Also, since we are removing the size limit for alloc_internal(), the assert() there should be removed altogether.

kernel-open/nvidia-uvm/uvm_kvmalloc.h Outdated Show resolved Hide resolved
@0x5459 0x5459 force-pushed the increase-uvm-alloc-size branch from 8660efa to 32b874e Compare November 13, 2024 06:37
@@ -711,7 +711,7 @@ _nvswitch_os_free

if (is_vmalloc_addr(ptr))
{
vfree(ptr);
vfree(ptr, -1ull);
Copy link
Author

@0x5459 0x5459 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francescolavra I don't know how to call vfree here. Please guide me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I export the uvm_vmalloc_hdr_t structure for reference in kernel-open/nvidia/linux_nvswitch.c?

@0x5459 0x5459 force-pushed the increase-uvm-alloc-size branch from 1c3ccab to 4bdb132 Compare November 13, 2024 09:32
hdr = container_of(p, uvm_vmalloc_hdr_t, ptr); \
NV_KFREE(p, hdr->alloc_size); \
} while (0)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work if memory is allocated from outside the nvidia-uvm module, because it won't have the uvm_vmalloc_hdr_t header.
To deal with the vfree() call in linux_nvswitch.c, I would not use vmalloc/vfree at all in the nvswitch code; instead, I would use use another Nanos kernel heap, i.e. get_kernel_heaps()->malloc, which can take -1 as size argument when freeing memory. So, _nvswitch_os_malloc() will be as below:

    void *ptr = allocate(get_kernel_heaps()->malloc, size);

    if (ptr == INVALID_ADDRESS)
        return NULL;

    return ptr;

and _nvswitch_os_free() will be as below:

    if (!ptr)
        return;

    deallocate((get_kernel_heaps()->malloc), ptr, -1ull);

The vfree() macro will only be called from the nvidia-uvm module, where we can easily extract the size parameter from the uvm_vmalloc_hdr_t header.

#include <config.h>
#define _CONFIG_H_
#endif

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary, the config.h header is included internally by other Nanos header files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants