[Bug] Alignment calculation in test code is inconsistent and one place incorrect

[raj111samant]

Size and address needs to be aligned to GPU_PAGE_SIZE.
In test code, alignment is performed by direct mathematical operation rather than encapsulating it in ROUND_UP macro.
This has led to bug creep in common.cpp file
if (aligned_mapping) allocated_size = size + GPU_PAGE_SIZE - 1; else allocated_size = size;

Suggested fix:
Use ROUND_UP macro uniformly to adjust for alignment.

[drossetti]

@pakmarkthub could you please have a look at Raj's PR #213 ?

[raj111samant]

@pakmarkthub modified changes as per your request. Please have a look.

[raj111samant]

copying previous conversations from this PR

pakmarkthub commented on Oct 25, 2021, 8:11 AM GMT+5:30

Thank you @raj111samant.

I have one small change request. Can you change the name from ROUND_UP to PAGE_ROUND_UP? Because this function does not support arbitrary rounding up and we will be using it in multiple files, the name should be clear.

raj111samant commented on Oct 25, 2021, 5:36 PM GMT+5:30

Hi @pakmarkthub actually ROUND_UP does support arbitrary rounding. We provide a second argument as GPU_PAGE_SIZE.
const size_t size = ROUND_UP(_size, GPU_PAGE_SIZE);

I feel second argument is clear indication we are ROUNDING_UP to GPU_PAGE_SIZE.

Are you suggesting to remove the second argument and make the MACRO round up to only GPU_PAGE_SIZE ?

#define GPU_PAGE_ROUND_UP(x) (((x) + ((GPU_PAGE_SIZE) - 1)) & ~((GPU_PAGE_SIZE) - 1))
const size_t size = GPU_PAGE_ROUND_UP(_size);

I would prefer keeping ROUND_UP as it is and taking second argument explicitly, it allows flexibility to even specify jumbo page sizes in future.

pakmarkthub commented on Oct 26, 2021, 7:21 AM GMT+5:30

Hi @raj111samant,

This is the definition of that ROUND_UP: #define ROUND_UP(x, n) (((x) + ((n) - 1)) & ~((n) - 1)). If it supports arbitrary number, then ROUND_UP(5, 7) should return 7, for example. However, you get 9 instead.

Strictly speaking, that ROUND_UP requires n to be in a power of 2 to work properly. I suggested that we rename it to PAGE_ROUND_UP(x, n) to indicate its use case and this limitation. I am ok with other name that clearly marks the intention of this function.

[pakmarkthub]

I don't see your PR anymore. PR #213 has been closed without merge. It contains no commit. Can you fix it or create a new PR?