Details
- Reviewers
- None
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Small fixes; the version worked in my testing.
This diff includes the test driver I used to verify the code, it is not intended for upstream.
If it isn't hard to do, it would be nice if vm_object_take_pages_for_io()
had a flag option that indicated if it should return failure if there is
no IOMMU hardware. (Or is there an easy way for the caller to check
before calling this function?)
For our purposes, there is no point in doing this if it requires a bounce
buffer or such. (The fallback is pretty good. It just ends up doing
copying in VOP_READ().)
Otherwise, it looks good to me. I'll email you and Vinicius a rough partial
patch for using it in the NFS server.
Otherwise, the interface looks
IOMMU has nothing to do with this function. It does not even look into the bus address space.
Also, any decent network card would handle the reasonably sized rdma s/g list fine, without IOMMU assistance.I think you should not worry about bounce fallbacks at all, they normally would not happen.
Oops, I confused this with what I need for the client, which is your _bus_dmamap_load_ma[] that maps pages to
configuous dma space.
Also, any decent network card would handle the reasonably sized rdma s/g list fine, without IOMMU assistance.I think you should not worry about bounce fallbacks at all, they normally would not happen.
For Mellanox, max_sg == 31 and max_sg_rd == 30 and RFC8267 recommends a limit of 16.
So, if pages aren't mapped into contiguous bus dma space, that means 64K.
For Vinicius's server code, I think he loops around doing multiple RDMA Writes to transfer
it all, so it isn't a big issue.
However, for the client, the entire transfer has to be set up before doing the RPC,
so that limits the Read RPC to 64K. I am hoping your _bus_dmamap_load_ma[]
function will allow larger RPC sizes, so it is for that that I need to know if there
is an IOMMU?
| sys/vm/vm_object.c | ||
|---|---|---|
| 2617 | Can we use an iterator here? Note that there is a vm_page_grab_valid_iter(). | |
| 2623 | Are we leaking the PIP counter here? And, why exactly is it necessary to bump the PIP counter in the first place? | |
| 2643 | Do you need to use vm_page_try_remove_{all,write}()? That is, suppose one of the pages is wired; the holder of the wiring reference may map it. Does vm_object_take_pages_for_io() provide some guarantee that this won't happen? | |
| sys/vm/vm_object.c | ||
|---|---|---|
| 2623 | What would prevent freeing and reusing of the vm_object otherwise? For instance, everything that could is dropped during vm_page_busy_sleep() call from vm_page_busy_acquire(). | |
| 2643 | We definitely need pmap_remove_write() to ensure that io op is atomic. I am not sure what troubles pmap_remove_write() would cause for wired mappings. For the VM_PROT_WRITE case, it is highly desirable to not allow userspace to observe the intermediate state during the write, I believe. For wired pages, pmap_remove_all() only causes soft faults. I think it is fine on principle, e.g. there is no guarantee that CPUs would not cause spurious page faults on the present PTEs. At least, some generations of Intel processors were known to do that. | |
| sys/vm/vm_object.c | ||
|---|---|---|
| 2623 | I would presume that the caller is responsible for this. The vm_page_grab_* helpers do not try to handle this AFAICS. | |
| 2643 |
I mean, if some kernel consumer holds a wiring of a page, then it implicitly has permission to modify the page. That is, what prevents vm_object_take_pages_for_io() from racing with a thread calling vm_fault_quick_hold_pages()? If pmap_extract_and_hold() sees a writeable mapping of the page, it will wire the page and return it, and the consumer is allowed to modify that page. To avoid this race, vm_object_take_pages_for_io() must not return wired pages. | |
| sys/vm/vm_object.c | ||
|---|---|---|
| 2623 | vm_page_grab() is somewhat internal to the VM subsystem. This interface is a convenient wrapper around the subsystem primitives. From this PoV, it is desirable to not expose consumers to such details. If you think that the interface might be useful without pip increment, I can create yet another wrapper around this function, which would e.g. take a locked vnode, lock the object, and increment the pip. | |
| 2643 | Not allowing to return wired pages is not feasible, it means that the filesystem cannot have a VMIO buffer owning the range of the page for this function to succeed. You probably pointed to vm_page_try_remove_all/write(). I do not think it is usable there due to the above. And, we do have the same issue in vfs_busy_pages(), for instance? I think this problem should be handled globally, might be by using an shared sx around vm_fault_quick(), and taking it exclusively in specific places. It might cause ordering issues, I did not thought about it hard. | |
I looked more carefully at the issue of the interaction between vm_fault_quickXXX() and map_remove_YYY(). It could be implemented using the observation that when pmap_extract_and_hold() succeeds but the resulting page is busy, we should fall back to the vm_fault_hold() path for that page. This requires some kind of interlock between the busy state and wiring.
I even started implementing it, but then I considered what would be the consequence of not implementing. Basically, to get advantage of that situation, userspace needs to construct some situation. For instance, it needs to map a file shared, and then do read(2) from pipe into the mapping. Similarly, for physio. IMO if userspace is so much dedicated to cause this inconsistency, we might allow it. The VM code is simpler, and user is causing only self-inflicted damage. I do not see a problem with it.
On the other hand, the pmap_remove_all/write() calls make the regular file io with read(2) or write(2), or normal userspace writes through the mapping, atomic with regard to the NFS over RDMA io RPCs. I believe it is good enough.
Well, if this interface is not used, the NFS server does a..
VOP_READ() with a shared vnode lock
or
VOP_WRITE() with either a shared or exclusively locked vnode
(depending on whether the fs sets MNTK_SHARED_WRITES)
No range locking.
So, ideally, this interface would provide the same semantics.
If not, someone, someday, will go "when I mount rdma this weird.. happens".
I don't see that when I do a tcp mount.
What I discussed above is not about the range locking. It is about some very specific user buffer configurations that probably cannot be created by real applications, and cannot be utilized for any useful work. Also, I believe that the issue is not limited to the proposed interface, but also exists e.g. for buffered reads. My point is that it does not corrupt the kernel state, and is irrelevant practically for correctness.
Yes, I recognized that it didn't have anything to do with range locking.
I was just stating exactly what the NFS server does now, in case VOP_READ()/VOP_WRITE()
behaviour (what you call buffered I/O) was not affected by this issue the same
way as your page based system is.
(As I think you know, I do not understand the vm side of fs caching, but I can follow
what VOP_READ()/VOP_WRITE() does.)
It is not that simple. There might be IOMMUs in the system, but dma might still use bounce (or any other platform-specific mechanism) for the given device. I think the only reliable method would be to try bus_dmamap_load_ma() and do something else if the load cannot be performed.
So, I just tried to use the helper function and the process gets
stuck waiting on"pgrbwt".
Any ideas?
Or is there more info I can try and get you?
Oh, and I should note that this attempt was for the NFS client,
which means the process/thread doing the call already holds
the buffer cache block, in case that means that the pages related
to the bufer cache block are not acquirable?
(These Netperf systems use a NFS root fs and I'm not sure if
there is a swap set up, so I'm not sure what it would take to
get a dump?)
"pgrbwt" means that the function waits for the page busy state to pass. Often the reason for this state waiting forever is that the thread that waits already owns the busy state on the page.
Does the hang occur in the buffer strategy method?
Yes. Btw, I think the main use for this helper is in the server
where the vnode won't be an NFS one, so if this isn't easily
avoided, I wouldn't worry about it. (In other words, use the
helper in the NFS server, but not the NFS client.)
So in the strategy routine you effectively have the same algorithm applied already by the buf layer. You can use b_pages[] directly.
Yes, this seems to be working fine for the client.
I do have a question w.r.t. read on the NFS server.
I am guessing that the NFS code needs to VOP_GETATTR()
and use va_size to determine where the file ends in the last
page.
Is that correct?
Perhaps yes, the VOP_GETATTR() or VOP_STAT() are the most reliable ways to get the file size.
There is also vp->v_object->un_pager.vnp.vnp_size that should be synced with the filesystem idea of the file size. But perhaps you remember the issue with NFS client and vnode_pager_setsize(), so it is not always completely reliable.
I am now testing using NFS over RDMA for read RPCs
in the NFS server and it seems to be working fine.
I see about a 5% improvement in read rate compared
with the alternate mechanism (anonymous pages in
a M_EXTPG mbuf chain with data copied into the pages
via VOP_READ()) and I think it results in a cleaner
interface.
I'm no VM guy, so I can't review this, but I am a happy user
of it.
| sys/vm/vm_object.c | ||
|---|---|---|
| 2556 | Is this assert required? For the write RPC (data being read into When that happens, no data has been written | |
So, in general, I'm confused w.r.t. how the interface
is used when growing a file via writing.
I think that calling VOP_SETATTR() to set the file's size
before using this interface to write to the file might
make sense? (I'll try that to-day.)
| sys/vm/vm_object.c | ||
|---|---|---|
| 2556 | Ok, so I guess ma_len should be all the pages, | |
| 2563 | Again, should ptoa(obj->size) be here? Should the caller be adjusting the size before | |
| 2609 | This calculation doesn't work for the write case. I am currently using if (prot == VM_PROT_READ)
end = MIN(round_page(end), ptoa(obj->size));
else
end = round_page(end);Did you assume the file's size would be adjusted before | |
Well, I was able to use these helpers for read reduction,
but I did not observe any performance improvement
compared to the mechanism already in the RDMA server,
which uses M_EXTPG mbuf lists. (It was either perfomance
neutral or slightly poorer performance.)
For write reduction, using them got messy when files
were being grown by writing. Using VOP_SETATTR(size)
was not convenient, since VOP_SETATTR() requires an
exclusively locked vnode, whereas for ZFS, it is a shared
locked vnode.
I think these helpers may be useful someday, but for now,
I don't think using them for the NFS server RDMA module
is feasible. (Their use resulted in a lot of churn for code that
Vinicius is maintaining, but without significant performance
enhancement.)
Appologies to kib@ for asking him to write these and then
not using them, at least for now.
Hm, but if ZFS does not need the exclusively locked vnode for writes, does it need the same for setattr()? May be we should lower the requirements for VOP_SETATTR() same as for VOP_WRITE()?
Did you tested if the helpers provide any improvements for UFS and esp. tmpfs?
A cleaner way to do this might be a new VOP_SETSIZE() that can have a shared
lock if MNTK_SHARED_WRITES is set for the file system. Setting of size is
really more like write than setting an attribute such as mode.
vop_stdsetsize() could just do a VOP_SETATTR() of size, so that all file systems
don't need the new VOP_SETSIZE() implemented.
What do you think?
Did you tested if the helpers provide any improvements for UFS and esp. tmpfs?
I have been testing tmpfs to avoid the VOP_SETATTR() panic, but the tmpfs on
mercat2 is so small a write happens instantly. I'll try cranking the size of /tmp
up some and see if I can get some useful measurements.
I'll see how it goes. Since we have almost a year for 16.0, we can try some stuff.