linux-kernel - Re: [PATCH v1 4/6] perf threads: Move threads to its own files

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: <CAP-5=fWKdp7rf+v7t_T_0tU0OxQO9R2g+ZH7Ag7HgyBbGT3-nQ@mail.gmail.com>
Date: Tue, 27 Feb 2024 23:24:05 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	Yang Jihong <yangjihong1@...wei.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH v1 4/6] perf threads: Move threads to its own files

On Tue, Feb 27, 2024 at 10:40 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Tue, Feb 27, 2024 at 1:42 PM Ian Rogers <irogers@...gle.com> wrote:
> >
> > On Tue, Feb 27, 2024 at 11:17 AM Arnaldo Carvalho de Melo
> > <acme@...nel.org> wrote:
> > >
> > > On Tue, Feb 27, 2024 at 09:31:33AM -0800, Namhyung Kim wrote:
> > > > I can see some other differences like machine__findnew_thread()
> > > > which I think is due to the locking change.  Maybe we can fix the
> > > > problem before moving the code and let the code move simple.
> > >
> > > I was going to suggest that, agreed.
> > >
> > > We may start doing a refactoring, then find a bug, at that point we
> > > first fix the problem them go back to refactoring.
> >
> > Sure I do this all the time. Your typical complaint on the v+1 patch
> > set is to move the bug fixes to the front of the changes. On the v+2
> > patch set the bug fixes get applied but not the rest of the patch
> > series, etc.
> >
> > Here we are refactoring code for an rb-tree implementation of threads
> > and worrying about its correctness. There's no indication it's not
> > correct, it is largely copy and paste, there is also good evidence in
> > the locking disciple it is more correct. The next patch deletes that
> > implementation, replacing it with a hash table. Were I not trying to
> > break things apart I could squash those 2 patches together, but I've
> > tried to do the right thing. Now we're trying to micro correct, break
> > apart, etc. a state that gets deleted. A reviewer could equally
> > criticise this being 2 changes rather than 1, and the cognitive load
> > of having to look at code that gets deleted. At some point it is a
> > judgement call, and I think this patch is actually the right size. I
> > think what is missing here is some motivation in the commit message to
> > the findnew refactoring and so I'll add that.
>
> I'm not against your approach and actually appreciate your effort
> to split rb-tree refactoring and hash table introduction.  What I'm
> asking is just to separate out the code moving.  I think you can do
> whatever you want in the current file.  Once you have the final code
> you can move it to its own file exactly the same.  When I look at this
> commit, say a few years later, I won't expect a commit that says
> moving something to a new file has other changes.

The problem is that the code in machine treats the threads lock as if
it is a lock in machine. So there is __machine__findnew_thread which
implies the thread lock is held. This change is making threads its own
separate concept/collection and the lock belongs with that collection.
Most of the implementation of threads__findnew matches
__machine__findnew_thread, so we may be able to engineer a smaller
line diff by moving "__machine__findnew_thread" code into threads.c,
then renaming it to build the collection, etc. We could also build the
threads collection inside of machine and then in a separate change
move it to threads.[ch].  In the commit history this seems muddier
than just splitting out threads as a collection. Also, some of the API
design choices are motivated more by the hash table implementation of
the next patch than trying to have a good rbtree abstracted collection
of threads. Essentially it'd be engineering a collection of threads
but only with a view to delete it in the next patch. I don't think it
would be for the best and the commit history for deleted code is
unlikely to be looked upon.

Thanks,
Ian

> Thanks,
> Namhyung