Re: [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure
On Thu, Apr 09, 2009 at 12:30:57PM -0400, Gregory Haskins wrote:
+static unsigned long
+task_memctx_copy_to(struct vbus_memctx *ctx, void *dst, const void *src,
+ unsigned long n)
+{
+ struct task_memctx *tm = to_task_memctx(ctx);
+ struct task_struct *p = tm-task;
+
+ while (n) {
+ unsigned long offset = ((unsigned long)dst)%PAGE_SIZE;
+ unsigned long len = PAGE_SIZE - offset;
+ int ret;
+ struct page *pg;
+ void *maddr;
+
+ if (len n)
+ len = n;
+
+ down_read(p-mm-mmap_sem);
+ ret = get_user_pages(p, p-mm,
+ (unsigned long)dst, 1, 1, 0, pg, NULL);
+
+ if (ret != 1) {
+ up_read(p-mm-mmap_sem);
+ break;
+ }
+
+ maddr = kmap_atomic(pg, KM_USER0);
+ memcpy(maddr + offset, src, len);
+ kunmap_atomic(maddr, KM_USER0);
+ set_page_dirty_lock(pg);
+ put_page(pg);
+ up_read(p-mm-mmap_sem);
+
+ src += len;
+ dst += len;
+ n -= len;
+ }
+
+ return n;
+}
BTW, why did you decide to use get_user_pages?
Would switch_mm + copy_to_user work as well
avoiding page walk if all pages are present?
Also - if we just had vmexit because a process executed
io (or hypercall), can't we just do copy_to_user there?
Avi, I think at some point you said that we can?
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure
Michael S. Tsirkin wrote:
On Thu, Apr 09, 2009 at 12:30:57PM -0400, Gregory Haskins wrote:
+static unsigned long
+task_memctx_copy_to(struct vbus_memctx *ctx, void *dst, const void *src,
+unsigned long n)
+{
+struct task_memctx *tm = to_task_memctx(ctx);
+struct task_struct *p = tm-task;
+
+while (n) {
+unsigned long offset = ((unsigned long)dst)%PAGE_SIZE;
+unsigned long len = PAGE_SIZE - offset;
+int ret;
+struct page *pg;
+void *maddr;
+
+if (len n)
+len = n;
+
+down_read(p-mm-mmap_sem);
+ret = get_user_pages(p, p-mm,
+ (unsigned long)dst, 1, 1, 0, pg, NULL);
+
+if (ret != 1) {
+up_read(p-mm-mmap_sem);
+break;
+}
+
+maddr = kmap_atomic(pg, KM_USER0);
+memcpy(maddr + offset, src, len);
+kunmap_atomic(maddr, KM_USER0);
+set_page_dirty_lock(pg);
+put_page(pg);
+up_read(p-mm-mmap_sem);
+
+src += len;
+dst += len;
+n -= len;
+}
+
+return n;
+}
BTW, why did you decide to use get_user_pages?
Would switch_mm + copy_to_user work as well
avoiding page walk if all pages are present?
Well, basic c_t_u() won't work because its likely not current if you
are updating the ring from some other task, but I think you have already
figured that out based on the switch_mm suggestion. The simple truth is
I was not familiar with switch_mm at the time I wrote this (nor am I
now). If this is a superior method that allows you to acquire
c_t_u(some_other_ctx) like behavior, I see no problem in changing. I
will look into this, and thanks for the suggestion!
Also - if we just had vmexit because a process executed
io (or hypercall), can't we just do copy_to_user there?
Avi, I think at some point you said that we can?
Right, and yes that will work I believe. We could always do a if (p ==
current) check to test for this. To date, I don't typically do
anything mem-ops related directly in vcpu context so this wasn't an
issue...but that doesn't mean someone wont try in the future.
Therefore, I agree we should strive to optimize it if we can.
Thanks Michael,
-Greg
signature.asc
Description: OpenPGP digital signature
Re: [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure
Michael S. Tsirkin wrote: Also - if we just had vmexit because a process executed io (or hypercall), can't we just do copy_to_user there? Avi, I think at some point you said that we can? You can do copy_to_user() whereever it is legal in Linux. Almost all of kvm runs in process context, preemptible, and with interrupts enabled. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure
Gregory Haskins wrote: BTW, why did you decide to use get_user_pages? Would switch_mm + copy_to_user work as well avoiding page walk if all pages are present? Well, basic c_t_u() won't work because its likely not current if you are updating the ring from some other task, but I think you have already figured that out based on the switch_mm suggestion. The simple truth is I was not familiar with switch_mm at the time I wrote this (nor am I now). If this is a superior method that allows you to acquire c_t_u(some_other_ctx) like behavior, I see no problem in changing. I will look into this, and thanks for the suggestion! copy_to_user() is significantly faster than get_user_pages() + kmap() + memcmp() (or their variants). -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure
Avi Kivity wrote: Gregory Haskins wrote: BTW, why did you decide to use get_user_pages? Would switch_mm + copy_to_user work as well avoiding page walk if all pages are present? Well, basic c_t_u() won't work because its likely not current if you are updating the ring from some other task, but I think you have already figured that out based on the switch_mm suggestion. The simple truth is I was not familiar with switch_mm at the time I wrote this (nor am I now). If this is a superior method that allows you to acquire c_t_u(some_other_ctx) like behavior, I see no problem in changing. I will look into this, and thanks for the suggestion! copy_to_user() is significantly faster than get_user_pages() + kmap() + memcmp() (or their variants). Oh, I don't doubt that (in fact, I was pretty sure that was the case based on some of the optimizations I could see in studying the c_t_u() path). I just didn't realize there were other ways to do it if its a non current task. ;) I guess the enigma for me right now is what cost does switch_mm have? (Thats not a slam against the suggested approach...I really do not know and am curious). As an aside, note that we seem to be reviewing v2, where v3 is really the last set I pushed. I think this patch is more or less the same across both iterations, but FYI that I would recommend looking at v3 instead. -Greg signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH v2 03/19] vbus: add connection-client helper infrastructure
Gregory Haskins wrote: Oh, I don't doubt that (in fact, I was pretty sure that was the case based on some of the optimizations I could see in studying the c_t_u() path). I just didn't realize there were other ways to do it if its a non current task. ;) I guess the enigma for me right now is what cost does switch_mm have? (Thats not a slam against the suggested approach...I really do not know and am curious). switch_mm() is probably very cheap (reloads cr3), but it does dirty the current cpu's tlb. When the kernel needs to flush a process' tlb, it will have to IPI that cpu in addition to all others. This takes place, for example, after munmap() or after a page is swapped out (though significant batching is done there). It's still plenty cheaper in my estimation. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
