[PATCH] trace: module: Maintain a valid user count

[PATCH] trace: module: Maintain a valid user count

[Romain Izard]

The replacement of the 'count' variable by two variables 'incs' and
'decs' to resolve some race conditions during module unloading was done
in parallel with some cleanup in the trace subsystem, and was integrated
as a merge.

Unfortunately, the formula for this replacement was wrong in the tracing
code, and the refcount in the traces was not usable as a result.

Use 'count = incs - decs' to compute the user count.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 include/trace/events/module.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 161932737416..1228d963b6d1 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -78,7 +78,8 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
 	TP_fast_assign(
 		__entry->ip	= ip;
-		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) + __this_cpu_read(mod->refptr->decs);
+		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) -
+			__this_cpu_read(mod->refptr->decs);
 		__assign_str(name, mod->name);
 	),
 
-- 
1.8.3.2

Re: [PATCH] trace: module: Maintain a valid user count

[Steven Rostedt]

On Mon,  3 Mar 2014 19:17:32 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> The replacement of the 'count' variable by two variables 'incs' and
> 'decs' to resolve some race conditions during module unloading was done
> in parallel with some cleanup in the trace subsystem, and was integrated
> as a merge.
> 
> Unfortunately, the formula for this replacement was wrong in the tracing
> code, and the refcount in the traces was not usable as a result.
> 
> Use 'count = incs - decs' to compute the user count.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  include/trace/events/module.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/module.h b/include/trace/events/module.h
> index 161932737416..1228d963b6d1 100644
> --- a/include/trace/events/module.h
> +++ b/include/trace/events/module.h
> @@ -78,7 +78,8 @@ DECLARE_EVENT_CLASS(module_refcnt,
>  
>  	TP_fast_assign(
>  		__entry->ip	= ip;
> -		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) + __this_cpu_read(mod->refptr->decs);
> +		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) -
> +			__this_cpu_read(mod->refptr->decs);

I know this breaks the 80 column rule, but please keep it on one line.
Look at the above. The line you are removing is much easier to read
than the one you replaced it with.

Or if it is too big, what about:

		__entry->refcnt	=
		  __this_cpu_read(mod->refptr->incs) - __this_cpu_read(mod->refptr->decs);

Thanks,

-- Steve

>  		__assign_str(name, mod->name);
>  	),
>  

[PATCH v2] trace: module: Maintain a valid user count

[Romain Izard]

The replacement of the 'count' variable by two variables 'incs' and
'decs' to resolve some race conditions during module unloading was done
in parallel with some cleanup in the trace subsystem, and was integrated
as a merge.

Unfortunately, the formula for this replacement was wrong in the tracing
code, and the refcount in the traces was not usable as a result.

Use 'count = incs - decs' to compute the user count.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 include/trace/events/module.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index 161932737416..ca298c7157ae 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
 	TP_fast_assign(
 		__entry->ip	= ip;
-		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) + __this_cpu_read(mod->refptr->decs);
+		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) - __this_cpu_read(mod->refptr->decs);
 		__assign_str(name, mod->name);
 	),
 
-- 
1.8.3.2

Re: [PATCH v2] trace: module: Maintain a valid user count

[Steven Rostedt]

Ingo,

You want to push this to Linus?

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve


On Tue,  4 Mar 2014 10:09:39 +0100
Romain Izard <romain.izard.pro@gmail.com> wrote:

> The replacement of the 'count' variable by two variables 'incs' and
> 'decs' to resolve some race conditions during module unloading was done
> in parallel with some cleanup in the trace subsystem, and was integrated
> as a merge.
> 
> Unfortunately, the formula for this replacement was wrong in the tracing
> code, and the refcount in the traces was not usable as a result.
> 
> Use 'count = incs - decs' to compute the user count.
> 
> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
> ---
>  include/trace/events/module.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/module.h b/include/trace/events/module.h
> index 161932737416..ca298c7157ae 100644
> --- a/include/trace/events/module.h
> +++ b/include/trace/events/module.h
> @@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
>  
>  	TP_fast_assign(
>  		__entry->ip	= ip;
> -		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) + __this_cpu_read(mod->refptr->decs);
> +		__entry->refcnt	= __this_cpu_read(mod->refptr->incs) - __this_cpu_read(mod->refptr->decs);
>  		__assign_str(name, mod->name);
>  	),
>  

Re: [PATCH v2] trace: module: Maintain a valid user count

[Romain Izard]

2014-03-06 18:34 GMT+01:00 Steven Rostedt <rostedt@goodmis.org>:
> Ingo,
>
> You want to push this to Linus?
>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
>
> -- Steve
>
>
> On Tue,  4 Mar 2014 10:09:39 +0100
> Romain Izard <romain.izard.pro@gmail.com> wrote:
>
>> The replacement of the 'count' variable by two variables 'incs' and
>> 'decs' to resolve some race conditions during module unloading was done
>> in parallel with some cleanup in the trace subsystem, and was integrated
>> as a merge.
>>
>> Unfortunately, the formula for this replacement was wrong in the tracing
>> code, and the refcount in the traces was not usable as a result.
>>
>> Use 'count = incs - decs' to compute the user count.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
>> ---
>>  include/trace/events/module.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/trace/events/module.h b/include/trace/events/module.h
>> index 161932737416..ca298c7157ae 100644
>> --- a/include/trace/events/module.h
>> +++ b/include/trace/events/module.h
>> @@ -78,7 +78,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
>>
>>       TP_fast_assign(
>>               __entry->ip     = ip;
>> -             __entry->refcnt = __this_cpu_read(mod->refptr->incs) + __this_cpu_read(mod->refptr->decs);
>> +             __entry->refcnt = __this_cpu_read(mod->refptr->incs) - __this_cpu_read(mod->refptr->decs);
>>               __assign_str(name, mod->name);
>>       ),
>>
>

Ping ?

Re: [PATCH v2] trace: module: Maintain a valid user count

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Ingo,
> 
> You want to push this to Linus?
> 
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

I think it would be more natural through the tracing tree, through 
which most tracepoint fixes go, agreed?

Thanks,

	Ingo

Re: [PATCH v2] trace: module: Maintain a valid user count

[Steven Rostedt]

On Wed, 7 May 2014 21:23:36 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Ingo,
> > 
> > You want to push this to Linus?
> > 
> > Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> 
> I think it would be more natural through the tracing tree, through 
> which most tracepoint fixes go, agreed?
> 

Hmm, yeah. Not sure why I pushed it off to you :-/

Looks like this should be marked as stable and incorporated in 3.15 as
well.

Thanks,

-- Steve

Re: [PATCH v2] trace: module: Maintain a valid user count

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 7 May 2014 21:23:36 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > Ingo,
> > > 
> > > You want to push this to Linus?
> > > 
> > > Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> > 
> > I think it would be more natural through the tracing tree, through 
> > which most tracepoint fixes go, agreed?
> > 
> 
> Hmm, yeah. Not sure why I pushed it off to you :-/

To check whether I'm awake, I'm sure!

> Looks like this should be marked as stable and incorporated in 3.15 
> as well.

Yeah.

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo