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!

include/linux/marker.h +0 -0

index 889196c..89ce1b8 100644

include/linux/marker.h +0 -0

  */
 #define marker_synchronize_unregister() synchronize_sched()
 

Quoted 18 lines · * 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!

include/linux/marker.h +0 -0

index 889196c..89ce1b8 100644

include/linux/marker.h extern void *marker_get_private_data(const char *name, marker_probe_func *probe, · line 170

Quoted 7 lines

include/linux/marker.h +4 -0

@@ -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.

include/linux/marker.h extern void *marker_get_private_data(const char *name, marker_probe_func *probe, · line 172

Quoted 2 lines

include/linux/marker.h +2 -0

+ *
+ * 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 ?

[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]>

---

Documentation/markers.txt +9 -5

index 089f613..8bf6afe 100644
@@ -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

include/linux/marker.h +0 -0

index 889196c..32ce4f2 100644

include/linux/marker.h +1 -0

+ * to ensure the code and data are valid for any possibly running probes.
  */
 #define marker_synchronize_unregister() synchronize_sched()
 

Quoted 42 lines · Wu Fengguang

[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]>

---

Documentation/markers.txt +9 -5

index 089f613..8bf6afe 100644
@@ -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

include/linux/marker.h +0 -0

index 889196c..32ce4f2 100644
@@ -162,8 +162,10 @@ extern void *marker_get_private_data(const char *name, marker_probe_func *probe,
 

include/linux/marker.h extern void *marker_get_private_data(const char *name, marker_probe_func *probe, · line 167

Quoted 7 lines

include/linux/marker.h +3 -2

 /*
  * 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?

It's every good job, thank you.

Reviewed-by Lai Jiangshan <[email protected]>

Quoted 43 lines · Lai Jiangshan

Quoted 42 lines · Wu Fengguang

[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]>

---

Documentation/markers.txt +9 -5

index 089f613..8bf6afe 100644
@@ -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

include/linux/marker.h +0 -0

index 889196c..32ce4f2 100644
@@ -162,8 +162,10 @@ extern void *marker_get_private_data(const char *name, marker_probe_func *probe,
 

Quoted 9 lines

include/linux/marker.h extern void *marker_get_private_data(const char *name, marker_probe_func *probe, · line 167

Quoted 7 lines

include/linux/marker.h +3 -2

 /*
  * 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"?

Quoted 3 lines

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]>

---

Documentation/markers.txt +0 -0

index 089f613..8bf6afe 100644

Documentation/markers.txt +7 -0

+- 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 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

include/linux/marker.h +4 -2

index 889196c..32ce4f2 100644
@@ -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()
 

Review

Quoted 2 lines · * Wu Fengguang ([email protected]) wrote:

(quoted diff) +2 -0

+
+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

Quoted 11 lines · Mathieu Desnoyers

Review

Quoted 2 lines · * Wu Fengguang ([email protected]) wrote:

(quoted diff) +2 -0

+
+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]>

---

Documentation/markers.txt +10 -5

index 089f613..d9569a3 100644
@@ -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

include/linux/marker.h +4 -2

index 889196c..5f12d1b 100644
@@ -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()
 

Quoted 27 lines · * Wu Fengguang ([email protected]) wrote:

Quoted 11 lines · Mathieu Desnoyers

Review

Quoted 2 lines · * Wu Fengguang ([email protected]) wrote:

(quoted diff) +2 -0

+
+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.

Quoted 3 lines

Reviewed-by Mathieu Desnoyers <[email protected]>

Reviewed-by Lai Jiangshan <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

It looks good to me, thanks!

Acked-by Mathieu Desnoyers <[email protected]>