linux-kernel - Re: [RFC local_t removal V1 2/4] Replace local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1263437767.28171.3844.camel@gandalf.stny.rr.com>
Date:	Wed, 13 Jan 2010 21:56:07 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Christoph Lameter <cl@...ux-foundation.org>
Cc:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [RFC local_t removal V1 2/4] Replace local_t use in trace
 subsystem

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