notaz
Certified Guru
Yeah that should be better, as hugetlb mmap doesn't guarantee those 2M blocks will be contiguous in memory, only one block itself is.
[announce] c64_tools (DSP loader and IPC)
- Thread starter bsp
- Start date
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
..and on third thought I will have to do it properly after all because the cache ioctls in c64.ko validate the address range. Can't allow them to invalidate arbitrary memory regions.
..and because the hugetlb memory is not necessarily physically contiguous,I have to change the API so that the dsp_cache_*() functions expect virtual addresses now.
I implemented the HUGETLB feature by adding a new cache type DSP_CACHE_HUGETLB. At first I wanted to implement that as a flag but it does not make sense to allocate/map uncached huge pages, so these will be read/write cached by definition.
What I don't feel quite comfortable with is that the huge pages are allocated in userspace (resp. not via c64.ko), then the virtual address is passed to c64.ko.
At least the API will not have to be changed in case this is rectified later on.
As a precaution, before invalidating/writing back any cachelines, I'll check whether the specified virtual address range actually maps to hugetlb pages. Guess these things are not used for kernel internal things anyway, given that they have to be allocated via special cmdlines resp. /proc writes, so the worst thing that can happen is that there are two apps, both using hugetlb pages, that invalidate each others memory.
EDIT: instead of changing the dsp_cache_*() API, I'll add new functions/ioctls that handle virtual instead of physical addresses. This way the old binaries will still work.
..and because the hugetlb memory is not necessarily physically contiguous,
I implemented the HUGETLB feature by adding a new cache type DSP_CACHE_HUGETLB. At first I wanted to implement that as a flag but it does not make sense to allocate/map uncached huge pages, so these will be read/write cached by definition.
What I don't feel quite comfortable with is that the huge pages are allocated in userspace (resp. not via c64.ko), then the virtual address is passed to c64.ko.
At least the API will not have to be changed in case this is rectified later on.
As a precaution, before invalidating/writing back any cachelines, I'll check whether the specified virtual address range actually maps to hugetlb pages. Guess these things are not used for kernel internal things anyway, given that they have to be allocated via special cmdlines resp. /proc writes, so the worst thing that can happen is that there are two apps, both using hugetlb pages, that invalidate each others memory.
EDIT: instead of changing the dsp_cache_*() API, I'll add new functions/ioctls that handle virtual instead of physical addresses. This way the old binaries will still work.
Last edited by a moderator:
notaz
Certified Guru
While at it, maybe change 'virt_addr' in dsp_mem_region_t to be a void* pointer so that casts are not needed?
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
I know.. but ironically this would require me to add a lot of new casts in the existing code.
You may have noticed that dsp_phys_to_virt() already returns a void* and I figured that real-world applications would do sth like this, anyway:
/* Allocate physically contiguous shared memory */
dsp_mem_region_t shm = dsp_shm_alloc(DSP_CACHE_R);
/* Create memory allocator instance */
dsp_mspace_handle_t msp = dsp_mspace_create(shm.virt_addr, shm.size);
/* Use memory allocator to allocate some bytes */
unsigned char *mybuf = dsp_mspace_malloc(msp, MY_BUF_SIZE);
/* Optional, automatically done during dsp_close(): */
dsp_mspace_free(msp, mybuf);
dsp_mspace_destroy(msp);
dsp_shm_free(shm);
The new dsp_cache_*_virt() functions already use a void*, btw.
You may have noticed that dsp_phys_to_virt() already returns a void* and I figured that real-world applications would do sth like this, anyway:
/* Allocate physically contiguous shared memory */
dsp_mem_region_t shm = dsp_shm_alloc(DSP_CACHE_R);
/* Create memory allocator instance */
dsp_mspace_handle_t msp = dsp_mspace_create(shm.virt_addr, shm.size);
/* Use memory allocator to allocate some bytes */
unsigned char *mybuf = dsp_mspace_malloc(msp, MY_BUF_SIZE);
/* Optional, automatically done during dsp_close(): */
dsp_mspace_free(msp, mybuf);
dsp_mspace_destroy(msp);
dsp_shm_free(shm);
The new dsp_cache_*_virt() functions already use a void*, btw.
Last edited by a moderator:
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
@notaz:
Your HUGETLB virt_to_phys() function you posted on the previous page does not work -- it always returns 0.
I also noticed that there's no MAP_HUGETLB define in the Pandora kernel includes.
I tried with MAP_HUGETLB = (2 * 1024 * 1024) and MAP_HUGETLB = 0x40000. In both cases, mmap() returns a virtual address but your virt_to_phys() either does not work or the returned memory does not map to (a) HUGETLB page(s).
Any idea what's going wrong ?
Here's the (first) update for today:
+ add DSP_PAGESHIFT define to dsp_config.h
+ add DSP_HUGETLB_PAGESIZE/PAGESHIFT defines to dsp_config.h
+ add DSP_CACHE_HUGETLB cache type to dsp_common.h
+ add dsp_cache_wb_virt() API function
+ add dsp_cache_inv_virt() API function
+ add dsp_cache_wbinv_virt() API function
+ add C64_IOCTL_CACHE_VIRT to c64_dev.h
+ add loc_ioctl_cache_virt() to kmod.c
+ add C64_IOCTL_HUGETLB_V2P to c64_dev.h
+ add loc_ioctl_hugetlb_v2p() to kmod.c
+ add notaz' HUGETLB virt_to_phys() function to kmod/dev.c (currently does not work)
+ add HUGETLB support in dsp_virt_to_phys()
+ add HUGETLB support in dsp_phys_to_virt()
+ add HUGETLB support in dsp_shm_alloc()
+ add HUGETLB support in kmod.c:loc_shm_alloc()
+ add HUGETLB support in kmod.c:loc_shm_free()
+ add USE_FORCED_LOWPOWER_BYPASS config option to kmod.h
+ add new c64_tc testcase TC_SHM_CACHE_NONE_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_RW_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_R_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_W_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_NONE_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_RW_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_R_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_W_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_HUGETLB (currently fails)
+ release http://tkscript.de/c64_tools/c64_tools-02Nov2013_snapshot_1.tar.gz
Your HUGETLB virt_to_phys() function you posted on the previous page does not work -- it always returns 0.
I also noticed that there's no MAP_HUGETLB define in the Pandora kernel includes.
I tried with MAP_HUGETLB = (2 * 1024 * 1024) and MAP_HUGETLB = 0x40000. In both cases, mmap() returns a virtual address but your virt_to_phys() either does not work or the returned memory does not map to (a) HUGETLB page(s).
Any idea what's going wrong ?
Here's the (first) update for today:
+ add DSP_PAGESHIFT define to dsp_config.h
+ add DSP_HUGETLB_PAGESIZE/PAGESHIFT defines to dsp_config.h
+ add DSP_CACHE_HUGETLB cache type to dsp_common.h
+ add dsp_cache_wb_virt() API function
+ add dsp_cache_inv_virt() API function
+ add dsp_cache_wbinv_virt() API function
+ add C64_IOCTL_CACHE_VIRT to c64_dev.h
+ add loc_ioctl_cache_virt() to kmod.c
+ add C64_IOCTL_HUGETLB_V2P to c64_dev.h
+ add loc_ioctl_hugetlb_v2p() to kmod.c
+ add notaz' HUGETLB virt_to_phys() function to kmod/dev.c (currently does not work)
+ add HUGETLB support in dsp_virt_to_phys()
+ add HUGETLB support in dsp_phys_to_virt()
+ add HUGETLB support in dsp_shm_alloc()
+ add HUGETLB support in kmod.c:loc_shm_alloc()
+ add HUGETLB support in kmod.c:loc_shm_free()
+ add USE_FORCED_LOWPOWER_BYPASS config option to kmod.h
+ add new c64_tc testcase TC_SHM_CACHE_NONE_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_RW_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_R_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_W_RAND_DSP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_NONE_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_RW_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_R_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_W_RAND_GPP_VIRT
+ add new c64_tc testcase TC_SHM_CACHE_HUGETLB (currently fails)
+ release http://tkscript.de/c64_tools/c64_tools-02Nov2013_snapshot_1.tar.gz
notaz
Certified Guru
I've re-tested the code in minimal module + map_hugetlb.c from prev page and it works. MAP_HUGETLB should be 0x40000, also must specify MAP_PRIVATE | MAP_ANONYMOUS. On successful map the region should appear on /proc/<pid>/maps as "/anon_hugepage (deleted)". Also you need to reserve some pages before using them, like mentioned there is /usr/pandora/scripts/op_hugetlb.sh for that on pandora.
The kernel code must execute in context of process that has that memory mapped (should be the case during ioctl processing).
The kernel code must execute in context of process that has that memory mapped (should be the case during ioctl processing).
Last edited by a moderator:
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
I just double-checked, anon_hugepage (deleted) appears in /proc/<pid>/maps/ after the mmap() call (and I have reserved the pages using the script, of course) but your virt_to_phys() function fails.
Just did a little debugging and it's the first check in that function
if ((l1_entry & 3) == 2) { /* section a.k.a. huge page */
that fails because l1_entry is always 0.
Maybe you care to take a look ? I've posted the updated sources.
You have to uncomment the #define MAP_HUGETLB 0x40000 line in libc64/dsp.c before you compile them.
p.s.: I cannot confirm whether it works in your minimal module since I don't have the sources for it.
Just did a little debugging and it's the first check in that function
if ((l1_entry & 3) == 2) { /* section a.k.a. huge page */
that fails because l1_entry is always 0.
Maybe you care to take a look ? I've posted the updated sources.
You have to uncomment the #define MAP_HUGETLB 0x40000 line in libc64/dsp.c before you compile them.
p.s.: I cannot confirm whether it works in your minimal module since I don't have the sources for it.
notaz
Certified Guru
Hmm maybe you don't touch the memory before trying to do this? I'll take a look at it later.
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
ehem.. that was actually it. added a memset(), then virt_to_phys() worked.
what I don't understand though: First of all I added a lot of debug output to virt_to_phys() (e.g. print mm/vma/pgd/pud/pmd infos).
Then I tried to use do_mmap() in the kernel module to allocate the memory. For whatever reason the returned memory does not have the VM_HUGETLB flag set but the memory allocated in userspace via mmap() does. Hmm.
anyways, now I can continue with debugging/testing the new code. Will upload a new release later.
what I don't understand though: First of all I added a lot of debug output to virt_to_phys() (e.g. print mm/vma/pgd/pud/pmd infos).
Then I tried to use do_mmap() in the kernel module to allocate the memory. For whatever reason the returned memory does not have the VM_HUGETLB flag set but the memory allocated in userspace via mmap() does. Hmm.
anyways, now I can continue with debugging/testing the new code. Will upload a new release later.
notaz
Certified Guru
MAP_HUGETLB is handled in syscall handler before calling do_mmap_pgoff(), see mmap_pgoff() in mm/mmap.c
Hugetlb setup code is not available to modules.. But if you really want it, I could arrange some hacks.
Hugetlb setup code is not available to modules.. But if you really want it, I could arrange some hacks.
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
Thanks for the offer but I think we can do without that. It is more a theoretical / security related thing that probably has no practical implications, especially not since the DSP MMU is still not enabled which we should not do until we really know what addresses it needs to access. Maybe someone needs to write a hardware driver for whatever reason.
EDIT: plus it complicates things a lot, especially with the new hugetlb feature (non-contiguous memory..)
However, the hugetlb feature seems to work fine, I just released a new "dist" package:
(remember to define USE_DSP_POWER_NOTIFY when you build the kernel module for the Pandora!)
Changelog:
02-Nov-2013
- added pnd_src/ folder
- added -nops cmdline option to c64_dsprite
(executes 800 million NOPs on DSP side, can be used to calibrate the DSP clock rate)
- added DSP_CACHE_W (cached writes / uncached reads) cache type
- added dsp_cache_inv_virt() API function
- added dsp_cache_wb_virt() API function
- added dsp_cache_wbinv_virt() API function
- added USE_DSP_POWER_NOTIFY build option to kmod.h
(if defined, call dsp_power_notify() before/after DSP is resumed/suspended)
- added USE_FORCED_LOWPOWER_BYPASS build option to kmod.h
- fixed: kmod/dev.c cache actions (inv/wb/wbinv) now use customized versions of v7_dma_map_area()
instead of falling back to (the slow) cache_flush_all()
- added support for huge pages (DSP_CACHE_HUGETLB cache type)
- added (a lot of) new c64_tc testcases (run "./c64_tc" to see a list)
Download: http://tkscript.de/c64_tools/c64_tools-02Nov2013_dist.tar.gz
EDIT: plus it complicates things a lot, especially with the new hugetlb feature (non-contiguous memory..)
However, the hugetlb feature seems to work fine, I just released a new "dist" package:
(remember to define USE_DSP_POWER_NOTIFY when you build the kernel module for the Pandora!)
Changelog:
02-Nov-2013
- added pnd_src/ folder
- added -nops cmdline option to c64_dsprite
(executes 800 million NOPs on DSP side, can be used to calibrate the DSP clock rate)
- added DSP_CACHE_W (cached writes / uncached reads) cache type
- added dsp_cache_inv_virt() API function
- added dsp_cache_wb_virt() API function
- added dsp_cache_wbinv_virt() API function
- added USE_DSP_POWER_NOTIFY build option to kmod.h
(if defined, call dsp_power_notify() before/after DSP is resumed/suspended)
- added USE_FORCED_LOWPOWER_BYPASS build option to kmod.h
- fixed: kmod/dev.c cache actions (inv/wb/wbinv) now use customized versions of v7_dma_map_area()
instead of falling back to (the slow) cache_flush_all()
- added support for huge pages (DSP_CACHE_HUGETLB cache type)
- added (a lot of) new c64_tc testcases (run "./c64_tc" to see a list)
Download: http://tkscript.de/c64_tools/c64_tools-02Nov2013_dist.tar.gz
Last edited by a moderator:
notaz
Certified Guru
OS update published, thanks for your work!
(random note) filling 1MB buffer (memset) takes just a bit longer than invalidating it on ARM, so it's not worth offloading that to DSP if the buffer is cached. Doing dsp_cache_wbinvall() is ~5 times faster in such case, but that doesn't account for performance losses for losing all program's data from the caches.
(random note) filling 1MB buffer (memset) takes just a bit longer than invalidating it on ARM, so it's not worth offloading that to DSP if the buffer is cached. Doing dsp_cache_wbinvall() is ~5 times faster in such case, but that doesn't account for performance losses for losing all program's data from the caches.
Last edited by a moderator:
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
Ok, thanks for the OS update!
A couple of (minor) change requests, though:
About the cache invalidation speed: Have not benchmarked that myself but I think I've seen that dsp_cache_inv() is now ~20 times faster than the old implementation (flush_cache_all()), at least with one of the c64 testcases. Maybe it's worth a dedicated testcase (although there is probably not much room for improvements, anyway).
After testing the hugetlb memory I noticed that there's the same speed discrepancy as with contiguous/CMA memory. memcpy() on regular 4k-paged memory performs at ~1 GB/sec (sometimes 1.33 GB/s), memcpy() on hugetlb memory (or contiguous memory) is not faster than ~322 MB/s. Still an unsolved mystery..
Nevertheless, the "copy32" benchmark clearly shows a performance improvement with hugetlb memory: ~213 MB/sec with hugetlb, ~153 MB/sec with regular shared mem (DSP_CACHE_RW), but ~870 MB/sec with malloc()'d memory.
I still would really like to understand why memcpy() on 'regular' memory is that much faster in these tests (so do you, I guess) but then again, this is all unpaid work and analyzing this is probably quite an ungrateful task.
p.s.: I just updated the c64_tools_test.pnd, for completeness' sake: http://repo.openpandora.org/?page=detail&app=c64_tools_tests (for some reason it is not flagged as an "update", no idea why)
A couple of (minor) change requests, though:
- The default DSP clock rate is too low. This should be 800Mhz instead of 660Mhz on a DM3730 Pandora. The CPU clock rate is 1000Mhz, so that does not match the DSP clock rate at that OPP (4).
- There should be an XFCE menu entry "System"->"DSP-Speed". The script (/usr/pandora/scripts/op_dspspeed.sh) already exists and works fine, so it should just be a matter of creating the desktop link.
About the cache invalidation speed: Have not benchmarked that myself but I think I've seen that dsp_cache_inv() is now ~20 times faster than the old implementation (flush_cache_all()), at least with one of the c64 testcases. Maybe it's worth a dedicated testcase (although there is probably not much room for improvements, anyway).
After testing the hugetlb memory I noticed that there's the same speed discrepancy as with contiguous/CMA memory. memcpy() on regular 4k-paged memory performs at ~1 GB/sec (sometimes 1.33 GB/s), memcpy() on hugetlb memory (or contiguous memory) is not faster than ~322 MB/s. Still an unsolved mystery..
Nevertheless, the "copy32" benchmark clearly shows a performance improvement with hugetlb memory: ~213 MB/sec with hugetlb, ~153 MB/sec with regular shared mem (DSP_CACHE_RW), but ~870 MB/sec with malloc()'d memory.
I still would really like to understand why memcpy() on 'regular' memory is that much faster in these tests (so do you, I guess) but then again, this is all unpaid work and analyzing this is probably quite an ungrateful task.
p.s.: I just updated the c64_tools_test.pnd, for completeness' sake: http://repo.openpandora.org/?page=detail&app=c64_tools_tests (for some reason it is not flagged as an "update", no idea why)
Last edited by a moderator:
milkshake
Advanced Member
It is flagged as an update 
notaz
Certified Guru
Well, it sure makes sense, so if you let me know how to set the rate, I'll implement that into the scripts.
notaz
Certified Guru
It's all done similar as we have it for ARM, op_cpuspeed.sh -> op_dspspeed.sh, cpu.conf -> dsp.conf . op_dspspeed.sh is not in sudoers right now as I don't know if it's good allowing random PNDs to be messing around with DSP clock or not.
Last edited by a moderator:
bsp
Very Active Member
- Joined
- Dec 2, 2006
- Messages
- 314
yep, exactly. good job on that (again), notaz, and thanks for all your all help!
I think the limits you've chosen are quite reasonable. ED should just set the default clock rate to 800Mhz on DM3730 resp. 430Mhz on OMAP3530.
same thing as with op_cpuspeed.sh, right ? You must be root to change the CPU ("GPP") clock rate, too. ("scaling_governor: permission denied" otherwise..)
notaz
Certified Guru
Yeah but you can run op_cpuspeed.sh with sudo and it won't ask for a password (even if it's not cached for 5 minutes by sudo) while for op_dspspeed.sh it currently will.
Okay, added default speeds to the startup.
