Subject: Re: [RFC local_t removal V1 2/4] Replace local_t use in trace
 subsystem
From: Steven Rostedt <rostedt@goodmis.org>
To: Christoph Lameter <cl@linux-foundation.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>,
       Tejun Heo <tj@kernel.org>, linux-kernel@vger.kernel.org
In-Reply-To: <20100105220438.462143211@quilx.com>
References: <20100105220417.400092933@quilx.com>
	 <20100105220438.462143211@quilx.com>
Content-Type: text/plain; charset="ISO-8859-15"
Date: Wed, 13 Jan 2010 21:56:07 -0500
Message-ID: <1263437767.28171.3844.camel@gandalf.stny.rr.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2401
Lines: 84

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@linux-foundation.org>
> 


> @@ -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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/