Message-ID: <ZtG4z5qPtCBNDdXO@x1>
Date: Fri, 30 Aug 2024 09:19:27 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	Nick Terrell <terrelln@...com>, Yanteng Si <siyanteng@...ngson.cn>,
	Yicong Yang <yangyicong@...ilicon.com>,
	James Clark <james.clark@...aro.org>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/8] perf header: Allow attributes to be written after
 data

On Thu, Aug 29, 2024 at 02:42:38PM -0700, Ian Rogers wrote:
> On Thu, Aug 29, 2024 at 1:58 PM Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> > On Thu, Aug 29, 2024 at 01:12:32PM -0700, Ian Rogers wrote:
> > > On Thu, Aug 29, 2024 at 12:31 PM Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> > > > On Thu, Aug 29, 2024 at 08:01:51AM -0700, Ian Rogers wrote:
> > > I'm not seeing the difference. Before:

> > You noticed the difference: before we used lseek to get the current
> > offset to use, afterwards we moved to doing plain math.

> > So I had to check if we could assume that, and with the current code
> > structure, yes, we can assume that, so seems safe, but it is different
> > and if the assumption somehow breaks, as the code in __do_write_fd()
> > guard against (unneeded at the moment as ion has even a BUG_ON for that
> > not to happen), then the offset will not be where the data is.

> > Using lseek() is more costly (syscalls) but it is the ultimate answer to
> > get where in the file the current offset is.

> > So that is the difference I noticed.

> > Doing multiple things in the same patch causes these reviewing delays,
> > doubts, its something we discussed multiple times in the past, and that
> > continue to cause these discussions.

> Right, but it is something of an unfortunate coincidence of how the
> code is structured. The fact that writing the header updates
> data_offset which is a thing that other things depend upon while
> depending on its value itself, etc. - ie the function does more than
> just a write, it also sometimes computes the layout, has inbuilt
> assumptions on the values lseek will return, and so on. To get to this

I share your frustrations, code gets complex over time, that is why I at
least try to ask these questions, encourage more granular patches, that
do just one thing, etc, to avoid having this conversation again years
from now, when some other person tries to understand the codebase do
bisects, refactor it, etc, just like you're doing now.

> final structure took a fair few iterations and I've separated this
> change out from the bulk in the next change to keep the patch size
> down. I could have done a patch switching from lseeks to math, then a
> patch to add write_attrs_after_data.

> It probably would have yielded about 4 lines of shared code, more
> lines that would have been deleted, while creating quite a bit of work
> for me.

> Ideally when these functions were created there would have been far
> more liberal use of things like immutability, so side-effects are
> minimized. Yes I could refactor everything, but time..

As I said, I think your patch is safe as-is, its just that it took more
time than needed for reviewing, i.e. it will cost more one side or the
other, and as I have to review everything I merge, doing it on my side
slows things down the overall process of collecting patches from lots of
people.

So far I collected 234 patches for v6.12, and there are way more to
process, like Howard's perf trace BTF work, that I merged partially,
but where I'll have to do work on splitting patches as agreed with him
in another thread, etc.

- Arnaldo

Powered by blists - more mailing lists