pussuw commented on code in PR #12041: URL: https://github.com/apache/nuttx/pull/12041#discussion_r1571233557
########## include/nuttx/addrenv.h: ########## @@ -193,7 +193,9 @@ # define ARCH_SHM_MAXPAGES (CONFIG_ARCH_SHM_NPAGES * CONFIG_ARCH_SHM_MAXREGIONS) # define ARCH_SHM_REGIONSIZE (CONFIG_ARCH_SHM_NPAGES * CONFIG_MM_PGSIZE) # define ARCH_SHM_SIZE (CONFIG_ARCH_SHM_MAXREGIONS * ARCH_SHM_REGIONSIZE) Review Comment: Changing the name of the SHM region is unnecessary, what would be a better name. Doing so would also create a regression for downstream projects, so this is not preferred by me. Why not just use CONFIG_ARCH_SHM_NPAGES for the UDEV/UMAP area as well ? There is no distinction between the two from memory segmentation point of view. The SHM area has a confusing name I agree, but it is really just a general "memory mappings region". Anything that is mapped by mmap can go here; shm objects, shared libraries, udev mappings, whatever. Here is a good description of a process's memory map: https://i.stack.imgur.com/epGfE.png. Semantically, the SHM region in that figure is the "Memory Mapping Region". You define `# define ARCH_SHM_VEND (CONFIG_ARCH_SHM_VBASE + ARCH_UMAP_SIZE - 1)` which proves that the UMAP region is just a region of SHM. If your goal is to remove naming confusion, this does not help in my opinion. ########## include/nuttx/addrenv.h: ########## @@ -193,7 +193,9 @@ # define ARCH_SHM_MAXPAGES (CONFIG_ARCH_SHM_NPAGES * CONFIG_ARCH_SHM_MAXREGIONS) # define ARCH_SHM_REGIONSIZE (CONFIG_ARCH_SHM_NPAGES * CONFIG_MM_PGSIZE) # define ARCH_SHM_SIZE (CONFIG_ARCH_SHM_MAXREGIONS * ARCH_SHM_REGIONSIZE) Review Comment: Changing the name of the SHM region is unnecessary, what would be a better name? Doing so would also create a regression for downstream projects, so this is not preferred by me. Why not just use CONFIG_ARCH_SHM_NPAGES for the UDEV/UMAP area as well ? There is no distinction between the two from memory segmentation point of view. The SHM area has a confusing name I agree, but it is really just a general "memory mappings region". Anything that is mapped by mmap can go here; shm objects, shared libraries, udev mappings, whatever. Here is a good description of a process's memory map: https://i.stack.imgur.com/epGfE.png. Semantically, the SHM region in that figure is the "Memory Mapping Region". You define `# define ARCH_SHM_VEND (CONFIG_ARCH_SHM_VBASE + ARCH_UMAP_SIZE - 1)` which proves that the UMAP region is just a region of SHM. If your goal is to remove naming confusion, this does not help in my opinion. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
