linux-kernel - Re: [RFC PATCH 2/2] trace: splice support for tracing

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: <20090209090433.GB31890@elte.hu>
Date:	Mon, 9 Feb 2009 10:04:33 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>
Cc:	fweisbec@...il.com, penberg@...helsinki.fi, rostedt@...dmis.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe


* Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro> wrote:

> Added and implemented tracing_pipe_fops->splice_read(). This allows
> userspace programs to get tracing data more efficiently.

Cool!

The structure of the splice state machine looks good - havent tested it yet.

A few comments:

> +static struct pipe_buf_operations tracing_pipe_buf_ops = {
> +	.can_merge = 0,
> +	.map = generic_pipe_buf_map,
> +	.unmap = generic_pipe_buf_unmap,
> +	.confirm = generic_pipe_buf_confirm,
> +	.release = tracing_pipe_buf_release,
> +	.steal = generic_pipe_buf_steal,
> +	.get = generic_pipe_buf_get,
> +};

this looks nicer:

 static struct pipe_buf_operations tracing_pipe_buf_ops = {
	.can_merge	= 0,
	.map		= generic_pipe_buf_map,
	.unmap		= generic_pipe_buf_unmap,
	.confirm	= generic_pipe_buf_confirm,
	.release	= tracing_pipe_buf_release,
	.steal		= generic_pipe_buf_steal,
	.get		= generic_pipe_buf_get,
 };

(and this is how we do these things elsewhere in the tracing code)

ditto:

> +	struct splice_pipe_desc spd = {
> +		.pages = pages,
> +		.partial = partial,
> +		.nr_pages = 0, /* This gets updated below. */
> +		.flags = flags,
> +		.ops = &tracing_pipe_buf_ops,
> +		.spd_release = tracing_spd_release_pipe,
> +	};

+       for (i = 0, rem = len; i < PIPE_BUFFERS && rem; i++) {
+               pages[i] = alloc_page(GFP_KERNEL);
+
+               /* Seq buffer is page-sized, exactly what we need. */
+               for (;;) {

Ok, that's a minor bug: should check for NULL alloc_page() return.

> +out:
> +	mutex_unlock(&trace_types_lock);
> +
> +	return ret;

please name this label as 'out_err' - which it really is. We generally use the 
'out:' label for error-less exits.

Btw., this is how we do structure initializations:

>  	.open		= tracing_open_pipe,
>  	.poll		= tracing_poll_pipe,
>  	.read		= tracing_read_pipe,
> +	.splice_read	= tracing_splice_read_pipe,
>  	.release	= tracing_release_pipe,
>  };

One more thing, i'd suggest to split this loop (which is an iterator in a larger 
loop):

+               /* Seq buffer is page-sized, exactly what we need. */
+               for (;;) {
+                       count = iter->seq.len;
+                       ret = print_trace_line(iter);
[...]
+		}

Out into a separate helper inline function. This really is a "fill in pipe page" 
logic and having it separate makes it easier to review the splice-loop code pattern 
itself.

Thanks,

	Ingo
--
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/