Arnd Bergmann wrote: > On Thursday 16 July 2009, Gregory Haskins wrote: > >> Arnd Bergmann wrote: >> > > >>> Your approach allows passing the vmid from a process that does >>> not own the kvm context. This looks like an intentional feature, >>> but I can't see what this gains us. >>> >> This work is towards the implementation of lockless-shared-memory >> subsystems, which includes ring constructs such as virtio-ring, >> VJ-netchannels, and vbus-ioq. I find that these designs perform >> optimally when you allow two distinct contexts (producer + consumer) to >> process the ring concurrently, which implies a disparate context from >> the guest in question. Note that the infrastructure we are discussing >> does not impose a requirement for the contexts to be unique: it will >> work equally well from the same or a different process. >> >> For an example of this "producer/consumer" dynamic over shared memory in >> action, please refer to my previous posting re: "vbus" >> >> http://lkml.org/lkml/2009/4/21/408 >> >> I am working on v4 now, and this patch is part of the required support. >> > > Ok. I can see how your approach gives you more flexibility in this > regard, but it does not seem critical. > Yeah, and I think I missed your point the first time around. I thought you were asking about how the resulting memory API was used, but now I see you were referring to the scope of the vmid namespace (vs file-descriptors). On that topic, yes the vmid implementation here differs from file-descriptors in the sense that the namespace is global to the kernel, instead of per-process. And yes, I also agree with you that this is not critical to the design, per se. For instance, the use cases I have in mind would work fine with the per-task fd namespace as well since I will be within the same QEMU instance. So ultimately, I think that means the fd idea you presented would work equally well. > >> But to your point, I suppose the dependency lifetime thing is not a huge >> deal. I could therefore modify the patch to simply link xinterface.o >> into kvm.ko and still achieve the primary objective by retaining ops->owner. >> > > Right. And even if it's a separate module, holding an extra reference > on kvm.ko will not cause any harm. > Yep, agreed. > >>> Can't you simply provide a function call to lookup the kvm context >>> pointer from the file descriptor to achieve the same functionality? >>> >>> >> You mean so have: struct kvm_xinterface *kvm_xinterface_find(int fd) >> >> (instead of creating our own vmid namespace) ? >> >> Or are you suggesting using fget() instead of kvm_xinterface_find()? >> > > I guess they are roughly equivalent. Either you pass a fd to > kvm_xinterface_find, or pass the struct file pointer you get > from fget. The latter is probably more convenient because it > allows you to pass around the struct file in kernel contexts > that don't have that file descriptor open. > Interesting. I like it. > >>> To take that thought further, maybe the dependency can be turned >>> around: If every user (pci-uio, virtio-net, ...) exposes a file >>> descriptor based interface to user space, you can have a kvm >>> ioctl to register the object behind that file descriptor with >>> an existing kvm context to associate it with a guest. >>> >> FWIW: We do that already for the signaling path (see irqfd and ioeventfd >> in kvm.git). Each side exposes interfaces that accept eventfds, and the >> fds are passed around that way. >> >> However, for the functions we are talking about now, I don't think it >> really works well to go the other way. I could be misunderstanding what >> you mean, though. What I mean is that it's KVM that is providing a >> service to the other modules (in this case, translating memory >> pointers), so what would an inverse interface look like for that? And >> even if you came up with one, it seems to me that its just "6 of one, >> half-dozen of the other" kind of thing. >> > > I mean something like > > int kvm_ioctl_register_service(struct file *filp, unsigned long arg) > { > struct file *service = fget(arg); > struct kvm *kvm = filp->private_data; > > if (!service->f_ops->new_xinterface_register) > return -EINVAL; > > return service->f_ops->new_xinterface_register(service, (void*)kvm, > &kvm_xinterface_ops); > } > > This would assume that we define a new file_operation specifically for this, > which would simplify the code, but there are other ways to achieve the same. > It would even mean that you don't need any static code as an interface layer. > I suspect I will have a much harder time getting that type of modification accepted in mainline ;) I think I will go with your suggestion for the fd/file interface, tho. I can get rid of the rbtree logic and re-use the lookup via fget, which is a nice win. Thanks Arnd! -Greg
Attachment:
signature.asc
Description: OpenPGP digital signature
