Re: [RFC local_t removal V1 2/4] Replace local_t use in tracesubsystem

[Steven Rostedt]

Please Cc me on changes to the ring buffer.

On Tue, 2010-01-05 at 16:04 -0600, Christoph Lameter wrote:
> plain text document attachment (remove_local_t_ringbuffer_convert)
> Replace the local_t use with longs in the trace subsystem. The longs can then be
> updated with the required level of concurrency protection through cmpxchg_local()
> and add_local().
> 
> Signed-off-by: Christoph Lameter <cl@xxxxxxxxxxxxxxxxxxxx>
> 


> @@ -1741,7 +1741,7 @@ rb_reset_tail(struct ring_buffer_per_cpu
>  	 * must fill the old tail_page with padding.
>  	 */
>  	if (tail >= BUF_PAGE_SIZE) {
> -		local_sub(length, &tail_page->write);
> +		add_local(&tail_page->write, -length);

Can't you have a helper function or macro that does:

#define sub_local(local, val)	add_local(local, -(val))

>  		return;
>  	}
>  


>  static struct ring_buffer_event *
> @@ -1801,7 +1801,7 @@ rb_move_tail(struct ring_buffer_per_cpu 
>  	 * about it.
>  	 */
>  	if (unlikely(next_page == commit_page)) {
> -		local_inc(&cpu_buffer->commit_overrun);
> +		add_local(&cpu_buffer->commit_overrun, 1);

As well as a:

#define inc_local(local)	add_local(local, 1)

>  		goto out_reset;
>  	}
>  


>   again:
> -	commits = local_read(&cpu_buffer->commits);
> +	commits = cpu_buffer->commits;
>  	/* synchronize with interrupts */
>  	barrier();
> -	if (local_read(&cpu_buffer->committing) == 1)
> +	if (cpu_buffer->committing == 1)
>  		rb_set_commit_to_write(cpu_buffer);
>  
> -	local_dec(&cpu_buffer->committing);
> +	add_local(&cpu_buffer->committing, -1);

As well as a:

#define dec_local(local)	add_local(local, -1)


>  
>  	/* synchronize with interrupts */
>  	barrier();
> @@ -2059,9 +2059,9 @@ static void rb_end_commit(struct ring_bu
>  	 * updating of the commit page and the clearing of the
>  	 * committing counter.
>  	 */

The reason I ask for the above is because it sticks out much better. The
subtracting and inc/dec pairs I need to match. So I usually end up
scanning a matching "dec" for a "inc", or a matching "sub" for an "add",
but having "-1" to match "1" is not going to stand out, and I envision a
lot of stupid bugs happening because of this.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/