Re: [RFC PATCH 2/2] trace: splice support for tracing_pipe

[Ingo Molnar]

* Eduard - Gabriel Munteanu <eduard.munteanu@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/