linux-kernel - Re: 2.6.22 -mm merge plans

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]

Date:	Thu, 3 May 2007 16:12:01 +0100
From:	Christoph Hellwig <hch@...radead.org>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Christoph Hellwig <hch@...radead.org>,
	Andi Kleen <andi@...stfloor.org>, linux-kernel@...r.kernel.org,
	rusty@...tcorp.com.au, wfg@...c.edu
Subject: Re: 2.6.22 -mm merge plans

On Thu, May 03, 2007 at 11:04:15AM -0400, Mathieu Desnoyers wrote:
> -	blk_add_trace_rq(q, rq, BLK_TA_INSERT);
> +	MARK(blk_request_insert, "%p %p", q, rq);

I don't really like the shouting MARK name very much.  Can we have
a less-generic, less shouting name, e.g. trace_marker?  The aboe would then
be:

	trace_mark(blk_request_insert, "%p %p", q, rq);

> +#define NUM_PROBES ARRAY_SIZE(probe_array)

just get rid of this and use ARRAY_SIZE diretly below.

> +int blk_probe_connect(void)
> +{
> +	int result;
> +	uint8_t i;

just use an int for for loops.  it's easy to read and probably faster
on most systems (it the compiler isn't smart enough and promotes it
to int anyway during code generation)

> +void blk_probe_disconnect(void)
> +{
> +	uint8_t i;
> +
> +	for (i = 0; i < NUM_PROBES; i++) {
> +		marker_remove_probe(probe_array[i].name);
> +	}
> +	synchronize_sched();	/* Wait for probes to finish */

kprobes does this kind of synchronization internally, so the marker
wrapper should probabl aswell.

> +static int blk_probes_ref = 0;

no need to initialize this.

>  /*
>   * Send out a notify message.
>   */
> @@ -229,6 +233,12 @@
>  	blk_remove_tree(bt->dir);
>  	free_percpu(bt->sequence);
>  	kfree(bt);
> +	mutex_lock(&blk_probe_mutex);
> +	if (blk_probes_ref == 1) {
> +		blk_probe_disconnect();
> +		blk_probes_ref--;
> +	}

	if (--blk_probes_ref == 0)
		blk_probe_disconnect();

would probably be a tad cleaner.

> +	if (!blk_probes_ref) {
> +		blk_probe_connect();
> +		blk_probes_ref++;
> +	}

Dito here with a:

	if (!blk_probes_ref++)
		blk_probe_connect();

also the connect in the name seems rather add, what about arm/disarm instead?

>  static __init int blk_trace_init(void)
>  {
>  	mutex_init(&blk_tree_mutex);
> +	mutex_init(&blk_probe_mutex);

both should use DEFINE_MUTEX for compile-time initialization isntead.

Also it's probably better to put the trace points into blktrace.c,
that means all blktrace code can be static and self-contained.  And we
can probably do some additional cleanups by simplifying things later on.
-
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/