Message-ID: <aCHggEc3MdlL6t7j@gallifrey>
Date: Mon, 12 May 2025 11:50:24 +0000
From: "Dr. David Alan Gilbert" <linux@...blig.org>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	andriy.shevchenko@...ux.intel.com, corbet@....net,
	linux-doc@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	viro@...iv.linux.org.uk, Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH] relay: Remove unused relay_late_setup_files

* Jason Xing (kerneljasonxing@...il.com) wrote:
> On Mon, May 12, 2025 at 9:30 AM Dr. David Alan Gilbert
> <linux@...blig.org> wrote:
> >
> > * Jason Xing (kerneljasonxing@...il.com) wrote:
> > > Hi All,
> >
> > Hi Jason,
> >
> > > I noticed this patch "relay: Remove unused relay_late_setup_files"
> > > appears in the mm branch already[1], which I totally missed. Sorry for
> > > joining the party late.
> > >
> > > I have a different opinion on this. For me, I'm very cautious about
> > > what those so-called legacy interfaces are and how they can work in
> > > different cases and what the use case might be... There are still a
> > > small number of out-of-tree users like me heavily relying on relayfs
> > > mechanism. So my humble opinion is that if you want to remove
> > > so-called dead code, probably clearly state why it cannot be used
> > > anymore in the future.
> >
> > We've got lots of deadcode, why it's dead varies a lot; for example
> > people forgetting to clean it up after other patches etc - so this
> > _could_ be used but hasn't been for well over 7 years.
> >
> > > Dr. David, I appreciate your patch, but please do not simply do the
> > > random cleanup work __here__. If you take a deep look at the relayfs,
> > > you may find there are other interfaces/functions no one uses in the
> > > kernel tree.
> >
> > Actually, that was the only interface in relay that I found unused.
> 
> Not really. More than this single one, say, __relay_write() and
> subbuf_start_reserve()...

Ah, my tools only spot unused symbols, they're header inlines; I've
not found a way to spot those yet.

> > > I'm now checking this kind of patch in relayfs one by one to avoid
> > > such a thing happening. I'm trying to maintain it as much as possible
> > > since we internally use it in the networking area to output useful
> > > information in the hot paths, a little bit like blktrace. BTW, relayfs
> > > is really a wonderful one that helps kernel modules communicate with
> > > userspace very efficiently. I'm trying to revive it if I can.
> >
> > If you've got a use for that function, then I'm more than happy to suggest
> > just dropping my patch.
> >
> > However, it is a fairly chunky function that is built into distro
> > kernels - so I think it should have a little thought put to it.
> >
> > As I say, if you are using it, it's fine by me just to drop this patch.
> 
> For now, I'm not using it but still considering what the use case
> might be in the future. As I mentioned earlier, I'm trying to make
> relayfs more robust with more realistic functions.
> 
> IMHO, it's not really a dead code to me unless you can clarify why
> it's obsolete instead of claiming "no one is using it".

i'm very gentle about this; I'm not pushing back hard if someone
says actually they want to keep something.

I'd say my 'claim' is fairly good as even you say
  'I'm not using it but still considering..'

You don't need to push back quite as hard on me!

> If you insist
> on the point, then most of relayfs would be removed, which is
> apparently not what I'm wishing for.

You could forgive me for thinking this is unused;
  a) There are no callers in the tree - I can't possibly imagine all
   other trees to check; especially those on someones local disk
   or thoughts still bouncing around in your brain!

  b) There's no listed maintainer, so I can't assume anyone is actively
  working on it

  c) The only changes in years in the tree are cleanups, like strcpy
  variants.

We do have other APIs that people care about and aren't in use;
now if those are nicely thought out APIs etc I think that's fine.
(I've had others say they want to keep some because they like them or
they're part of a well thought out API)

> Probably it will be finally removed, but not at the moment. Evidence
> is still not clear to me :S
> 
> For sure, the last call would be made by Andrew and Jens. Please help
> review this patch one more time. Thanks!

Why don't you add a MAINTAINERS section with you added just as a 
reviewer?   That at least gets you told if someone dares to clean it up
in the future!

Dave

> Thanks,
> Jason
> 
> >
> > Dave
> >
> > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-everything&id=46aa76118ee365c25911806e34d28fc2aa5ef997
> > >
> > > Thanks,
> > > Jason
> > --
> >  -----Open up your eyes, open up your mind, open up your code -------
> > / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
> > \        dave @ treblig.org |                               | In Hex /
> >  \ _________________________|_____ http://www.treblig.org   |_______/
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Powered by blists - more mailing lists