On Wed, 2018-01-31 at 20:08 +0000, Stephen Bates wrote: I've been mulling this over for a few days and now I believe I see a strategy that's going to simplify this a lot. First, a little bit of background. The function spdk_mem_register registers a virtual address with *all* SPDK memory translation tables. The system is set up to track an arbitrarily large number - in fact user code can create their own translation tables if they want. In practice, there are two important ones: 1) The va to pa translation table (or va to iova if IOMMU is enabled). This one is created automatically by the env layer. 2) The va to RDMA lkey translation table. This one is created if you're using NVMe-oF in some form. When spdk_mem_register is called, it loops through all existing translation tables and calls a function pointer on each to notify them that a new virtual address is registered. That function pointer is responsible for determining the appropriate mapping of that virtual address and setting the appropriate translation. The first patch in your series alters spdk_mem_register so that it has an optional second argument (paddr). But that doesn't make sense - only one of the tables knows what a physical address is. What we really want to do is change the code so that the notify function in the code managing the va to pa table can correctly identify the pa for the given va when the va is a BAR. That function is spdk_vtophys_notify() in lib/env_dpdk/vtophys.c. Right now spdk_vtophys_notify() first attempts to look up a virtual address in DPDK's list of known allocated hugepages (it has translations pre-stored in that list). If that fails (and there is no IOMMU), the code falls back to scanning /proc/self/pagemap in sysfs. Is the correct translation from va to pa for the BAR present in /proc/self/pagemap? I would have guessed that it was, but I'll assume you already tried this and it wasn't (but I'll test it myself once I get a device with CMB plugged in to my machine). If the address translation isn't available in /proc/self/pagemap, we can add a step in between looking up the va in DPDK's hugepages and scanning /proc/self/pagemap that iterates through all attached PCI devices and checks if the va is in the range of one of their BARs. There is a facility for iterating the devices already - FOREACH_DEVICE_ON_PCIBUS, which iterates over structures that contain an array named "mem_resource" where the index is the BAR number and each element is this: struct rte_mem_resource { uint64_t phys_addr; /**< Physical address, 0 if not resource. */ uint64_t len; /**< Length of the resource. */ void *addr; /**< Virtual address, NULL when not mapped. */ }; I think that's the right way to approach this. I know that when you call spdk_mem_register, you have the physical address sitting there, so on the surface this seems much less efficient. But it keeps our nice delineation between the roles and responsibilities of the software stack such that spdk_mem_register is entirely unaffected - just the code that is in charge of managing specifically the va to pa map is modified (vtophys.c). The CMB is also going to be automatically registered with RDMA NICs for NVMe-oF as well. I haven't thought as deeply about the rest of the patches, but they look fine to me right now. I think this is going to be pretty neat! Thanks, Ben

Hi Ben et al Well you suggestions simplified the series quite a bit! Our lab is getting upgraded so I have not tested the new series yet. I plan to submit after I do some testing. However if people are interested you can see the new series here [1]. Things are much simpler now and feedback now or after submission is welcome. The major change to v1 is in the first patch of the new series [2]. https://github.com/Eideticom/spdk/commits/cmb-copy-v5 BTW since there are commits that I dropped from v1 do I regenerate new Ids for this series or do I use the ones that I had plus the new Id for the new initial commit? Cheers Stephen [1] https://github.com/Eideticom/spdk/commits/cmb-copy-v5 [2] https://github.com/Eideticom/spdk/commit/ac92507f38105e9fb9cf35df20dde8bb... On 2018-01-31, 2:43 PM, "SPDK on behalf of Walker, Benjamin" <spdk-bounces(a)lists.01.org on behalf of benjamin.walker(a)intel.com&gt; wrote: On Wed, 2018-01-31 at 20:08 +0000, Stephen Bates wrote: I've been mulling this over for a few days and now I believe I see a strategy that's going to simplify this a lot. First, a little bit of background. The function spdk_mem_register registers a virtual address with *all* SPDK memory translation tables. The system is set up to track an arbitrarily large number - in fact user code can create their own translation tables if they want. In practice, there are two important ones: 1) The va to pa translation table (or va to iova if IOMMU is enabled). This one is created automatically by the env layer. 2) The va to RDMA lkey translation table. This one is created if you're using NVMe-oF in some form. When spdk_mem_register is called, it loops through all existing translation tables and calls a function pointer on each to notify them that a new virtual address is registered. That function pointer is responsible for determining the appropriate mapping of that virtual address and setting the appropriate translation. The first patch in your series alters spdk_mem_register so that it has an optional second argument (paddr). But that doesn't make sense - only one of the tables knows what a physical address is. What we really want to do is change the code so that the notify function in the code managing the va to pa table can correctly identify the pa for the given va when the va is a BAR. That function is spdk_vtophys_notify() in lib/env_dpdk/vtophys.c. Right now spdk_vtophys_notify() first attempts to look up a virtual address in DPDK's list of known allocated hugepages (it has translations pre-stored in that list). If that fails (and there is no IOMMU), the code falls back to scanning /proc/self/pagemap in sysfs. Is the correct translation from va to pa for the BAR present in /proc/self/pagemap? I would have guessed that it was, but I'll assume you already tried this and it wasn't (but I'll test it myself once I get a device with CMB plugged in to my machine). If the address translation isn't available in /proc/self/pagemap, we can add a step in between looking up the va in DPDK's hugepages and scanning /proc/self/pagemap that iterates through all attached PCI devices and checks if the va is in the range of one of their BARs. There is a facility for iterating the devices already - FOREACH_DEVICE_ON_PCIBUS, which iterates over structures that contain an array named "mem_resource" where the index is the BAR number and each element is this: struct rte_mem_resource { uint64_t phys_addr; /**< Physical address, 0 if not resource. */ uint64_t len; /**< Length of the resource. */ void *addr; /**< Virtual address, NULL when not mapped. */ }; I think that's the right way to approach this. I know that when you call spdk_mem_register, you have the physical address sitting there, so on the surface this seems much less efficient. But it keeps our nice delineation between the roles and responsibilities of the software stack such that spdk_mem_register is entirely unaffected - just the code that is in charge of managing specifically the va to pa map is modified (vtophys.c). The CMB is also going to be automatically registered with RDMA NICs for NVMe-oF as well. I haven't thought as deeply about the rest of the patches, but they look fine to me right now. I think this is going to be pretty neat! Thanks, Ben _______________________________________________ SPDK mailing list SPDK(a)lists.01.org https://lists.01.org/mailman/listinfo/spdk

Hi Stephen, Looked through your patch set on GitHub – overall it looks great! The NVMe CMB patch (#2 in your series) does some math to ensure only a 2MiB-aligned buffer is registered – with that in place, could we still enforce 2MiB alignment then in the vtophys notify code path (#1 in your series)? I’ll add this comment in your GitHub patch too – it might be clearer there what I’m talking about. Thanks, -Jim On 2/5/18, 8:06 PM, "SPDK on behalf of Stephen Bates" <spdk-bounces(a)lists.01.org on behalf of sbates(a)raithlin.com&gt; wrote: Hi Ben et al Well you suggestions simplified the series quite a bit! Our lab is getting upgraded so I have not tested the new series yet. I plan to submit after I do some testing. However if people are interested you can see the new series here [1]. Things are much simpler now and feedback now or after submission is welcome. The major change to v1 is in the first patch of the new series [2]. https://github.com/Eideticom/spdk/commits/cmb-copy-v5 BTW since there are commits that I dropped from v1 do I regenerate new Ids for this series or do I use the ones that I had plus the new Id for the new initial commit? Cheers Stephen [1] https://github.com/Eideticom/spdk/commits/cmb-copy-v5 [2] https://github.com/Eideticom/spdk/commit/ac92507f38105e9fb9cf35df20dde8bb... On 2018-01-31, 2:43 PM, "SPDK on behalf of Walker, Benjamin" <spdk-bounces(a)lists.01.org on behalf of benjamin.walker(a)intel.com&gt; wrote: On Wed, 2018-01-31 at 20:08 +0000, Stephen Bates wrote: I've been mulling this over for a few days and now I believe I see a strategy that's going to simplify this a lot. First, a little bit of background. The function spdk_mem_register registers a virtual address with *all* SPDK memory translation tables. The system is set up to track an arbitrarily large number - in fact user code can create their own translation tables if they want. In practice, there are two important ones: 1) The va to pa translation table (or va to iova if IOMMU is enabled). This one is created automatically by the env layer. 2) The va to RDMA lkey translation table. This one is created if you're using NVMe-oF in some form. When spdk_mem_register is called, it loops through all existing translation tables and calls a function pointer on each to notify them that a new virtual address is registered. That function pointer is responsible for determining the appropriate mapping of that virtual address and setting the appropriate translation. The first patch in your series alters spdk_mem_register so that it has an optional second argument (paddr). But that doesn't make sense - only one of the tables knows what a physical address is. What we really want to do is change the code so that the notify function in the code managing the va to pa table can correctly identify the pa for the given va when the va is a BAR. That function is spdk_vtophys_notify() in lib/env_dpdk/vtophys.c. Right now spdk_vtophys_notify() first attempts to look up a virtual address in DPDK's list of known allocated hugepages (it has translations pre-stored in that list). If that fails (and there is no IOMMU), the code falls back to scanning /proc/self/pagemap in sysfs. Is the correct translation from va to pa for the BAR present in /proc/self/pagemap? I would have guessed that it was, but I'll assume you already tried this and it wasn't (but I'll test it myself once I get a device with CMB plugged in to my machine). If the address translation isn't available in /proc/self/pagemap, we can add a step in between looking up the va in DPDK's hugepages and scanning /proc/self/pagemap that iterates through all attached PCI devices and checks if the va is in the range of one of their BARs. There is a facility for iterating the devices already - FOREACH_DEVICE_ON_PCIBUS, which iterates over structures that contain an array named "mem_resource" where the index is the BAR number and each element is this: struct rte_mem_resource { uint64_t phys_addr; /**< Physical address, 0 if not resource. */ uint64_t len; /**< Length of the resource. */ void *addr; /**< Virtual address, NULL when not mapped. */ }; I think that's the right way to approach this. I know that when you call spdk_mem_register, you have the physical address sitting there, so on the surface this seems much less efficient. But it keeps our nice delineation between the roles and responsibilities of the software stack such that spdk_mem_register is entirely unaffected - just the code that is in charge of managing specifically the va to pa map is modified (vtophys.c). The CMB is also going to be automatically registered with RDMA NICs for NVMe-oF as well. I haven't thought as deeply about the rest of the patches, but they look fine to me right now. I think this is going to be pretty neat! Thanks, Ben _______________________________________________ SPDK mailing list SPDK(a)lists.01.org https://lists.01.org/mailman/listinfo/spdk _______________________________________________ SPDK mailing list SPDK(a)lists.01.org https://lists.01.org/mailman/listinfo/spdk

Hi All My v2 for the NVMe CMB WDS/RDS enablement is now up for review. Thanks to Jim, Daniel, Ben and Darius for their very useful input. This v2 is a lot cleaner than v1 thanks to input from Darius and Ben that cleaned up how we do the vtophys lookups for CMBs. I have tested this new series on our hardware and it works great. Any more input or testing appreciated! The new code is mostly in this patch: https://review.gerrithub.io/#/c/398872/ Stephen

Hi Stephen, I removed the -1 label, will start a new round test soon.

No worries. That's what CI is for ;-). Thanks for the review Jim! I saw them and responded. I think they all make sense and will address them in a v3. Got to have an example ;-)... I'll probably give it another day or two for others to review/comment and then spin v3. Stephen On 2/7/18, 8:05 PM, "SPDK on behalf of Liu, Changpeng" <spdk-bounces(a)lists.01.org on behalf of changpeng.liu(a)intel.com&gt; wrote: Hi Stephen, I removed the -1 label, will start a new round test soon. _______________________________________________ SPDK mailing list SPDK(a)lists.01.org https://lists.01.org/mailman/listinfo/spdk _______________________________________________ SPDK mailing list SPDK(a)lists.01.org https://lists.01.org/mailman/listinfo/spdk

On 2/8/18, 4:36 PM, "SPDK on behalf of Stephen Bates" <spdk-bounces(a)lists.01.org on behalf of sbates(a)raithlin.com&gt; wrote: Hi All. V3 of this series is now posted. Jim I addressed all your concerns bar one. I would like to keep the cmb_copy example for copying between two distinct controllers for now. We can either add another example or extend this one in the future for copies within one controller. Thanks Stephen – v3 is running through the test pool now. For cmb_copy – I’m OK with that limitation. If user does specify the same controller for read and write – what would happen? -Jim

Oh I see the issue. We are still doing the + when the returned value is SPDK_VTOPHYS_ERROR. That is not going to fly. We are going to need to test for that case and not do the addition. I will push a fix. Sorry for the churn. Stephen