linux-kernel - Re: [PATCH] perf record: mmap output file

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:	Tue, 15 Oct 2013 08:02:00 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	David Ahern <dsahern@...il.com>
Cc:	acme@...stprotocols.net, linux-kernel@...r.kernel.org,
	Frederic Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Jiri Olsa <jolsa@...hat.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Mike Galbraith <efault@....de>,
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH] perf record: mmap output file - v2

* David Ahern <dsahern@...il.com> wrote:

> +	/* for MMAP based file writes */
> +	void			*mmap_addr;
> +	u64			bytes_at_mmap_start; /* bytes in file when mmap use starts */
> +	u64			mmap_offset;    /* current location within mmap */
> +	size_t			mmap_size;      /* size of mmap segments */
> +	bool			use_mmap;

> +	if (!rec->opts.pipe_output && stat(output_name, &st) == 0) {
> +		rec->use_mmap = true;
> +		rec->bytes_at_mmap_start = st.st_size - rec->bytes_written;
> +	}

1)

I think __cmd_record() has become way too large, nearly 300 lines of code. 
It would be nice to split it into 2-3 helpers that operate on 'struct 
perf_record' or so.

2)

The stat() seems superfluous, here in __cmd_record() we've just checked 
the output_name and made sure it exists. Can that stat() call ever fail?

3)

The rec->bytes_at_mmap_start field feels a bit weird. If I read the code 
correctly, in every 'perf record' invocation, rec->bytes_written starts at 
0 - i.e. we don't have repeat invocations of cmd_record().

That means that this:

		rec->bytes_at_mmap_start = st.st_size - rec->bytes_written;

is really:

		rec->bytes_at_mmap_start = st.st_size;

furthermore, since we don't allow appends anymore, st.st_size ought to be 
zero as well.

Which means that ->bytes_at_mmap_start is always zero - and could be 
eliminated altogether.

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/