[PATCH] KVM: trivial document fixes
Signed-off-by: Wu Fengguang
---
Documentation/kvm/api.txt | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
--- linux.orig/Documentation/kvm/api.txt2009-12-12 09:54:19.0
+0800
+++ linux/Documentation/kvm/api.txt 2009-12-12 10:48:02.0 +0800
@@ -23,12 +23,12 @@ of a virtual machine. The ioctls belong
Only run vcpu ioctls from the same thread that was used to create the
vcpu.
-2. File descritpors
+2. File descriptors
The kvm API is centered around file descriptors. An initial
open("/dev/kvm") obtains a handle to the kvm subsystem; this handle
can be used to issue system ioctls. A KVM_CREATE_VM ioctl on this
-handle will create a VM file descripror which can be used to issue VM
+handle will create a VM file descriptor which can be used to issue VM
ioctls. A KVM_CREATE_VCPU ioctl on a VM fd will create a virtual cpu
and return a file descriptor pointing to it. Finally, ioctls on a vcpu
fd can be used to control the vcpu, including the important task of
@@ -643,7 +643,7 @@ Type: vm ioctl
Parameters: struct kvm_clock_data (in)
Returns: 0 on success, -1 on error
-Sets the current timestamp of kvmclock to the valued specific in its parameter.
+Sets the current timestamp of kvmclock to the value specified in its parameter.
In conjunction with KVM_GET_CLOCK, it is used to ensure monotonicity on
scenarios
such as migration.
@@ -787,11 +787,11 @@ Unused.
__u64 data_offset; /* relative to kvm_run start */
} io;
-If exit_reason is KVM_EXIT_IO_IN or KVM_EXIT_IO_OUT, then the vcpu has
+If exit_reason is KVM_EXIT_IO, then the vcpu has
executed a port I/O instruction which could not be satisfied by kvm.
data_offset describes where the data is located (KVM_EXIT_IO_OUT) or
where kvm expects application code to place the data for the next
-KVM_RUN invocation (KVM_EXIT_IO_IN). Data format is a patcked array.
+KVM_RUN invocation (KVM_EXIT_IO_IN). Data format is a packed array.
struct {
struct kvm_debug_exit_arch arch;
@@ -807,7 +807,7 @@ Unused.
__u8 is_write;
} mmio;
-If exit_reason is KVM_EXIT_MMIO or KVM_EXIT_IO_OUT, then the vcpu has
+If exit_reason is KVM_EXIT_MMIO, then the vcpu has
executed a memory-mapped I/O instruction which could not be satisfied
by kvm. The 'data' member contains the written data if 'is_write' is
true, and should be filled by application code otherwise.
--
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: [PATCH] markers: comment marker_synchronize_unregister() on data dependency
On Thu, Nov 27, 2008 at 10:00:03AM +0200, Mathieu Desnoyers wrote: > * Wu Fengguang ([EMAIL PROTECTED]) wrote: > > + > > +marker_synchronize_unregister() must be called before the first occurrence > > of > > > You should probably say > > ".. must be called between probe unregistration and the first occurence > of..." > > Mathieu That's much better! With your comments I'd assume you reviewed this patch ;-) Thanks, Fengguang --- markers: comment marker_synchronize_unregister() on data dependency Add document and comments on marker_synchronize_unregister(): it should be called before freeing resources that the probes depend on. Based on comments from Lai Jiangshan and Mathieu Desnoyers. Reviewed-by: Mathieu Desnoyers <[EMAIL PROTECTED]> Reviewed-by: Lai Jiangshan <[EMAIL PROTECTED]> Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- diff --git a/Documentation/markers.txt b/Documentation/markers.txt index 089f613..d9569a3 100644 --- a/Documentation/markers.txt +++ b/Documentation/markers.txt @@ -51,11 +51,16 @@ to call) for the specific marker through marker_probe_register() and can be activated by calling marker_arm(). Marker deactivation can be done by calling marker_disarm() as many times as marker_arm() has been called. Removing a probe is done through marker_probe_unregister(); it will disarm the probe. -marker_synchronize_unregister() must be called before the end of the module exit -function to make sure there is no caller left using the probe. This, and the -fact that preemption is disabled around the probe call, make sure that probe -removal and module unload are safe. See the "Probe example" section below for a -sample probe module. + +marker_synchronize_unregister() must be called between probe unregistration and +the first occurrence of +- the end of module exit function, + to make sure there is no caller left using the probe; +- the free of any resource used by the probes, + to make sure the probes wont be accessing invalid data. +This, and the fact that preemption is disabled around the probe call, make sure +that probe removal and module unload are safe. See the "Probe example" section +below for a sample probe module. The marker mechanism supports inserting multiple instances of the same marker. Markers can be put in inline functions, inlined static functions, and diff --git a/include/linux/marker.h b/include/linux/marker.h index 889196c..5f12d1b 100644 --- a/include/linux/marker.h +++ b/include/linux/marker.h @@ -162,8 +162,10 @@ extern void *marker_get_private_data(const char *name, marker_probe_func *probe, /* * marker_synchronize_unregister must be called between the last marker probe - * unregistration and the end of module exit to make sure there is no caller - * executing a probe when it is freed. + * unregistration and the first one of + * - the end of module exit function + * - the free of any resource used by the probes + * to ensure the code and data are valid for any possibly running probes. */ #define marker_synchronize_unregister() synchronize_sched() -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] markers: comment marker_synchronize_unregister() on data dependency
On Thu, Nov 27, 2008 at 03:23:06AM +0200, Lai Jiangshan wrote: > Wu Fengguang wrote: > > [updated patch to include Documentation/markers.txt changes] > > > > Add document and comments on marker_synchronize_unregister(): it > > should be called before freeing resources that the probes depend on. > > > > Based on comments from Lai Jiangshan and Mathieu Desnoyers. > > > > Cc: Lai Jiangshan <[EMAIL PROTECTED]> > > Cc: Mathieu Desnoyers <[EMAIL PROTECTED]> > > Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> > > --- > > diff --git a/Documentation/markers.txt b/Documentation/markers.txt > > index 089f613..8bf6afe 100644 > > --- a/Documentation/markers.txt > > +++ b/Documentation/markers.txt > > @@ -51,11 +51,15 @@ to call) for the specific marker through > > marker_probe_register() and can be > > activated by calling marker_arm(). Marker deactivation can be done by > > calling > > marker_disarm() as many times as marker_arm() has been called. Removing a > > probe > > is done through marker_probe_unregister(); it will disarm the probe. > > -marker_synchronize_unregister() must be called before the end of the > > module exit > > -function to make sure there is no caller left using the probe. This, and > > the > > -fact that preemption is disabled around the probe call, make sure that > > probe > > -removal and module unload are safe. See the "Probe example" section below > > for a > > -sample probe module. > > + > > +marker_synchronize_unregister() must be called before the first occurrence > > of > > +- the end of the module exit function, > > + to make sure there is no caller left using the probe; > > +- the free of any resource used by the probes, > > + to make sure the probes wont be accessing destructed data. > > +This, and the fact that preemption is disabled around the probe call, make > > sure > > +that probe removal and module unload are safe. See the "Probe example" > > section > > +below for a sample probe module. > > > > The marker mechanism supports inserting multiple instances of the same > > marker. > > Markers can be put in inline functions, inlined static functions, and > > diff --git a/include/linux/marker.h b/include/linux/marker.h > > index 889196c..32ce4f2 100644 > > --- a/include/linux/marker.h > > +++ b/include/linux/marker.h > > @@ -162,8 +162,10 @@ extern void *marker_get_private_data(const char *name, > > marker_probe_func *probe, > > > > /* > > * marker_synchronize_unregister must be called between the last marker > > probe > > - * unregistration and the end of module exit to make sure there is no > > caller > > - * executing a probe when it is freed. > > + * unregistration and the first one of > > + * - the end of module exit function > > + * - the free of any resource used by the probes > > Does "destruction" contain the meaning of "free" and other destruction > behavior? Ahh, better to use "invalid data" instead of "destructed data"? > It's every good job, thank you. > > Reviewed-by: Lai Jiangshan <[EMAIL PROTECTED]> Thank you, Fengguang --- markers: comment marker_synchronize_unregister() on data dependency Add document and comments on marker_synchronize_unregister(): it should be called before freeing resources that the probes depend on. Based on comments from Lai Jiangshan and Mathieu Desnoyers. Cc: Mathieu Desnoyers <[EMAIL PROTECTED]> Reviewed-by: Lai Jiangshan <[EMAIL PROTECTED]> Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- diff --git a/Documentation/markers.txt b/Documentation/markers.txt index 089f613..8bf6afe 100644 --- a/Documentation/markers.txt +++ b/Documentation/markers.txt @@ -51,11 +51,15 @@ to call) for the specific marker through marker_probe_register() and can be activated by calling marker_arm(). Marker deactivation can be done by calling marker_disarm() as many times as marker_arm() has been called. Removing a probe is done through marker_probe_unregister(); it will disarm the probe. -marker_synchronize_unregister() must be called before the end of the module exit -function to make sure there is no caller left using the probe. This, and the -fact that preemption is disabled around the probe call, make sure that probe -removal and module unload are safe. See the "Probe example" section below for a -sample probe module. + +marker_synchronize_unregister() must be called before the first occurrence of +- the end of the module exit function, + to make sure there is no caller left using the probe; +- the free o
[PATCH] markers: comment marker_synchronize_unregister() on data dependency
[updated patch to include Documentation/markers.txt changes] Add document and comments on marker_synchronize_unregister(): it should be called before freeing resources that the probes depend on. Based on comments from Lai Jiangshan and Mathieu Desnoyers. Cc: Lai Jiangshan <[EMAIL PROTECTED]> Cc: Mathieu Desnoyers <[EMAIL PROTECTED]> Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- diff --git a/Documentation/markers.txt b/Documentation/markers.txt index 089f613..8bf6afe 100644 --- a/Documentation/markers.txt +++ b/Documentation/markers.txt @@ -51,11 +51,15 @@ to call) for the specific marker through marker_probe_register() and can be activated by calling marker_arm(). Marker deactivation can be done by calling marker_disarm() as many times as marker_arm() has been called. Removing a probe is done through marker_probe_unregister(); it will disarm the probe. -marker_synchronize_unregister() must be called before the end of the module exit -function to make sure there is no caller left using the probe. This, and the -fact that preemption is disabled around the probe call, make sure that probe -removal and module unload are safe. See the "Probe example" section below for a -sample probe module. + +marker_synchronize_unregister() must be called before the first occurrence of +- the end of the module exit function, + to make sure there is no caller left using the probe; +- the free of any resource used by the probes, + to make sure the probes wont be accessing destructed data. +This, and the fact that preemption is disabled around the probe call, make sure +that probe removal and module unload are safe. See the "Probe example" section +below for a sample probe module. The marker mechanism supports inserting multiple instances of the same marker. Markers can be put in inline functions, inlined static functions, and diff --git a/include/linux/marker.h b/include/linux/marker.h index 889196c..32ce4f2 100644 --- a/include/linux/marker.h +++ b/include/linux/marker.h @@ -162,8 +162,10 @@ extern void *marker_get_private_data(const char *name, marker_probe_func *probe, /* * marker_synchronize_unregister must be called between the last marker probe - * unregistration and the end of module exit to make sure there is no caller - * executing a probe when it is freed. + * unregistration and the first one of + * - the end of module exit function + * - the free of any resource used by the probes + * to ensure the code and data are valid for any possibly running probes. */ #define marker_synchronize_unregister() synchronize_sched() -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] markers: comment usage of marker_synchronize_unregister()
On Wed, Nov 26, 2008 at 02:46:08PM +0200, Mathieu Desnoyers wrote: > * Wu Fengguang ([EMAIL PROTECTED]) wrote: > > Add more comments to marker_synchronize_unregister() in order to > > reduce the chance of misusing. > > > > Based on comments from Lai Jiangshan <[EMAIL PROTECTED]>. > > > > Cc: Lai Jiangshan <[EMAIL PROTECTED]> > > Cc: Mathieu Desnoyers <[EMAIL PROTECTED]> > > Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> > > --- > > > > I'm still not sure about the last sentence. Can anyone clarify on > > this? Thanks! > > > > diff --git a/include/linux/marker.h b/include/linux/marker.h > > index 889196c..89ce1b8 100644 > > --- a/include/linux/marker.h > > +++ b/include/linux/marker.h > > @@ -164,6 +164,12 @@ extern void *marker_get_private_data(const char *name, > > marker_probe_func *probe, > > * marker_synchronize_unregister must be called between the last marker > > probe > > * unregistration and the end of module exit to make sure there is no > > caller > > * executing a probe when it is freed. > > + * > > + * It must be called _also_ between unregistration and destruction the data > > + * that unregistration-ed probes need to make sure there is no caller > > executing > > + * a probe when it's data is destroyed. > > it's -> its > > And the way it's written, this last sentence is a bit misleading. One > might think that the synchronize_unregister has to be called two, when > in fact it just has to be called once, but it must be called at a moment > in time between unregister and free of any resource used by the probes, > including the code which is removed by module unload. > > > + * > > + * It works reliably only when all probe routines do not sleep and > > reschedule. > > Per definition, preemption is disabled around marker probe execution, so > I don't see why we should add this last sentence ? Thanks, your reminder dismissed my confusion on this last sentence :-) Updated patch according to your helpful comments. Thank you, Fengguang --- markers: comment usage of marker_synchronize_unregister() Add more comments to marker_synchronize_unregister() in order to reduce the chance of misusing. Based on comments from Lai Jiangshan and Mathieu Desnoyers. Cc: Lai Jiangshan <[EMAIL PROTECTED]> Cc: Mathieu Desnoyers <[EMAIL PROTECTED]> Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- diff --git a/include/linux/marker.h b/include/linux/marker.h index 889196c..32ce4f2 100644 --- a/include/linux/marker.h +++ b/include/linux/marker.h @@ -162,8 +162,10 @@ extern void *marker_get_private_data(const char *name, marker_probe_func *probe, /* * marker_synchronize_unregister must be called between the last marker probe - * unregistration and the end of module exit to make sure there is no caller - * executing a probe when it is freed. + * unregistration and the first one of + * - the end of module exit + * - the free of any resource used by the probes + * to ensure the code and data are all valid for any possibly running probes. */ #define marker_synchronize_unregister() synchronize_sched() -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] markers: comment usage of marker_synchronize_unregister()
Add more comments to marker_synchronize_unregister() in order to reduce the chance of misusing. Based on comments from Lai Jiangshan <[EMAIL PROTECTED]>. Cc: Lai Jiangshan <[EMAIL PROTECTED]> Cc: Mathieu Desnoyers <[EMAIL PROTECTED]> Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- I'm still not sure about the last sentence. Can anyone clarify on this? Thanks! diff --git a/include/linux/marker.h b/include/linux/marker.h index 889196c..89ce1b8 100644 --- a/include/linux/marker.h +++ b/include/linux/marker.h @@ -164,6 +164,12 @@ extern void *marker_get_private_data(const char *name, marker_probe_func *probe, * marker_synchronize_unregister must be called between the last marker probe * unregistration and the end of module exit to make sure there is no caller * executing a probe when it is freed. + * + * It must be called _also_ between unregistration and destruction the data + * that unregistration-ed probes need to make sure there is no caller executing + * a probe when it's data is destroyed. + * + * It works reliably only when all probe routines do not sleep and reschedule. */ #define marker_synchronize_unregister() synchronize_sched() -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Prevent trace call into unloaded module text
On Wed, Nov 26, 2008 at 07:46:19PM +0800, Wu Fengguang wrote: > On Wed, Nov 26, 2008 at 01:17:55PM +0200, Avi Kivity wrote: > > Wu Fengguang wrote: > > > Add marker_synchronize_unregister() before module unloading. > > > This prevents possible trace calls into unloaded module text. > > > > > > Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> > > > --- > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index a87f45e..64f38b3 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -2102,5 +2102,6 @@ void kvm_exit(void) > > > kvm_arch_exit(); > > > kvm_exit_debug(); > > > __free_page(bad_page); > > > + marker_synchronize_unregister(); > > > } > > > EXPORT_SYMBOL_GPL(kvm_exit); > > > > > > > What about kvm-intel.ko and kvm-amd.ko? They also contain markers. > > vmx_exit and svm_exit() all calls into kvm_exit(), so they have been > handled in a common way. > > > (and, why doesn't module unload do this automatically?) > > Maybe most modules don't need it for now? OK I got a better answer: because marker_synchronize_unregister() must be called after marker_probe_unregister() calls and the tear down of any private data the trace functions rely on. So there are no automatic way. Below is the updated patch. Thanks, Fengguang --- Prevent trace call into torn down text or data Add marker_synchronize_unregister() immediately after probe unregisters. This prevents possible trace calls into unloaded module text, or the trace functions accessing invalidated data. Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- diff --git a/virt/kvm/kvm_trace.c b/virt/kvm/kvm_trace.c index 41dcc84..f598744 100644 --- a/virt/kvm/kvm_trace.c +++ b/virt/kvm/kvm_trace.c @@ -252,6 +252,7 @@ void kvm_trace_cleanup(void) struct kvm_trace_probe *p = &kvm_trace_probes[i]; marker_probe_unregister(p->name, p->probe_func, p); } + marker_synchronize_unregister(); relay_close(kt->rchan); debugfs_remove(kt->lost_file); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Prevent trace call into unloaded module text
On Wed, Nov 26, 2008 at 01:17:55PM +0200, Avi Kivity wrote: > Wu Fengguang wrote: > > Add marker_synchronize_unregister() before module unloading. > > This prevents possible trace calls into unloaded module text. > > > > Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> > > --- > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index a87f45e..64f38b3 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -2102,5 +2102,6 @@ void kvm_exit(void) > > kvm_arch_exit(); > > kvm_exit_debug(); > > __free_page(bad_page); > > + marker_synchronize_unregister(); > > } > > EXPORT_SYMBOL_GPL(kvm_exit); > > > > What about kvm-intel.ko and kvm-amd.ko? They also contain markers. vmx_exit and svm_exit() all calls into kvm_exit(), so they have been handled in a common way. > (and, why doesn't module unload do this automatically?) Maybe most modules don't need it for now? Thanks, Fengguang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Prevent trace call into unloaded module text
Add marker_synchronize_unregister() before module unloading. This prevents possible trace calls into unloaded module text. Signed-off-by: Wu Fengguang <[EMAIL PROTECTED]> --- diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index a87f45e..64f38b3 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2102,5 +2102,6 @@ void kvm_exit(void) kvm_arch_exit(); kvm_exit_debug(); __free_page(bad_page); + marker_synchronize_unregister(); } EXPORT_SYMBOL_GPL(kvm_exit); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
