From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wu Fengguang Subject: Re: [PATCH] markers: comment usage of marker_synchronize_unregister() Date: Wed, 26 Nov 2008 21:08:48 +0800 Message-ID: <20081126130847.GA20988@localhost> References: <20081126123652.GA9446@localhost> <20081126124608.GA22504@Krystal> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: LKML , Ingo Molnar , Avi Kivity , "kvm@vger.kernel.org" , Lai Jiangshan , "Paul E. McKenney" , Andrew Morton , Nick Piggin To: Mathieu Desnoyers Return-path: Received: from mga14.intel.com ([143.182.124.37]:42347 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753884AbYKZNJN (ORCPT ); Wed, 26 Nov 2008 08:09:13 -0500 Content-Disposition: inline In-Reply-To: <20081126124608.GA22504@Krystal> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Nov 26, 2008 at 02:46:08PM +0200, Mathieu Desnoyers wrote: > * Wu Fengguang (fengguang.wu@intel.com) wrote: > > Add more comments to marker_synchronize_unregister() in order to > > reduce the chance of misusing. > > > > Based on comments from Lai Jiangshan . > > > > Cc: Lai Jiangshan > > Cc: Mathieu Desnoyers > > Signed-off-by: Wu Fengguang > > --- > > > > 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 Cc: Mathieu Desnoyers Signed-off-by: Wu Fengguang --- 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()