Message-ID: <20200804145714.rcz6vg3bo6chsuoh@wittgenstein>
Date:   Tue, 4 Aug 2020 16:57:14 +0200
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Kirill Tkhai <ktkhai@...tuozzo.com>, akpm@...ux-foundation.org,
        viro@...iv.linux.org.uk, adobriyan@...il.com, davem@...emloft.net,
        linux-kernel@...r.kernel.org, keescook@...omium.org,
        avagin@...il.com, serge@...lyn.com, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 0/8] namespaces: Introduce generic refcount

On Tue, Aug 04, 2020 at 08:21:51AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner@...ntu.com> writes:
> 
> > On Tue, Aug 04, 2020 at 07:11:59AM -0500, Eric W. Biederman wrote:
> >> Christian Brauner <christian.brauner@...ntu.com> writes:
> >> 
> >> > On Mon, Aug 03, 2020 at 01:16:10PM +0300, Kirill Tkhai wrote:
> >> >> Every namespace type has its own counter. Some of them are
> >> >> of refcount_t, some of them are of kref.
> >> >> 
> >> >> This patchset introduces generic ns_common::count for any
> >> >> type of namespaces instead of them.
> >> >> 
> >> >> ---
> >> >
> >> > I was wondering why that series never made it to me turns out there's
> >> > some weird bug in my (neo)mutt where it sometimes marks messages as read
> >> > when I'm deleting completely unrelated messages. That has already cost
> >> > me a talk slot for an event I really wanted to attend and now it seems
> >> > to start costing me patches... I need to figure this out.
> >> >
> >> > Anyway, thanks for sending this. I pulled this into my tree now.
> >> 
> >> Actually why in the world should the reference count be generic?
> >> 
> >> What is the point of this patchset?
> >> 
> >> What problem does it solve.  Name spaces are not the same, and
> >> their refcounting needs are not the same so I don't have a clue how it
> >> helps anything to have a reference count in ns_common.
> >
> > What is the point of this opposition to this cleanup?
> >
> > It unifies reference counting across namespaces and gets rid of
> > inconsistencices. Over the years none of the namespaces seem to have
> > deviated enough from each that they really have needed separate
> > reference counting mechanisms.
> 
> First this posting is the first I have seen of it, unless it was a
> subset of the weird /proc/namespaces/ patchset that has design problems.
> In which case I never got this far.
> 
> Second I don't see a motivation for this.  The only point to place a
> reference count in ns_common is if it makes something easier.  What
> does it make easier and what does it make harder?
> 
> For a pure cleanup the questions are what are the trade offs.
> There are potential performance differences between refcount_t and
> kfref.
> 
> From a practical matter it makes absolutely no sense in the least to
> talk about the reference count, when some of the namespaces have more
> than one reference count, with difference semantics and they interrelate
> in somewhat subtle ways.
> 
> Further depending on what is happening sharing code that does not
> have a fundamental reason to be shared, can make maintenance more
> difficult as the entire generic infrastructure will need to be updated
> instead of just that the part that focuses on the one thing.
> 
> So I am opposed because the patchset does not explain at all why it
> makes sense to do, nor what tradeoffs were considered, nor what
> testing was done.
> 
> This change is not as trivial as a spelling change so it is not ok to
> say it is just a cleanup and move on.  A change in the reference
> counting can be noticable.  This needs at least to be acknowledged in
> the change log and at a minimum a hand wavy reason put forth why it is
> ok.
> 
> 
> Instead what I am seeing as justification is this is a trivial cleanup
> and no one will notice or care.   And it is not that trivial so I
> object to the patchset.

The "not seeing a motivation for this" is a suprising argument to me to
which I honestly can only respond that I don't see a motivation for not
doing this. It unifies and simplifies code and removes variance.

There can't be performance differences between kref and refcount_t since
kref is implemented on top of refcount_t. All functions that Kirill is
replacing are static inline wrappers around refcount_t helpers.

I have no idea why it is wrong talking about reference counts just
because something can have multiple counters or reference counters.

Another way of looking at this is that this patchset removes maintenance
by moving the shared parts of reference counting into a single place.
They are literally shared across all namespaces. I don't see a specific
worry other than the very hand wavy "if we ever have to change something
we have to update the whole generic infrastructure". That could be used
to shoot down 30 cleanup patchsets each development cycle that
significantly contribute to code legibility and maintenance. This is
also how it should work and one of the great benefits of the namespace
infra along with other parts of the kernel is that it is nice and
generic.

If you have issues with the patch descriptions than I'm sure Kirill will
be happy to adapt them to include more detail. If he can't I'm happy to.

Since past experience tells me that we're unlikely to settle this
dispute let's bring in a few more people to take a look at this.

Christian

Powered by blists - more mailing lists