Re: [PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns

From: Kirill Tkhai <ktkhai@virtuozzo.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: christian.brauner@ubuntu.com, akpm@linux-foundation.org,
	viro@zeniv.linux.org.uk, adobriyan@gmail.com,
	davem@davemloft.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/8] ns: Add common refcount into ns_common add use it as counter for net_ns
Date: Tue, 4 Aug 2020 17:36:52 +0300	[thread overview]
Message-ID: <6f6f9989-9cde-f93a-ad0a-311f264398f0@virtuozzo.com> (raw)
In-Reply-To: <87sgd2o6vt.fsf@x220.int.ebiederm.org>

On 04.08.2020 16:52, Eric W. Biederman wrote:
> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
> 
>> On 04.08.2020 15:21, Eric W. Biederman wrote:
>>> Kirill Tkhai <ktkhai@virtuozzo.com> writes:
>>>
>>>> Currently, every type of namespaces has its own counter,
>>>> which is stored in ns-specific part. Say, @net has
>>>> struct net::count, @pid has struct pid_namespace::kref, etc.
>>>>
>>>> This patchset introduces unified counter for all types
>>>> of namespaces, and converts net namespace to use it first.
>>>
>>> And the other refcounts on struct net?
>>>
>>> How do they play into what you are trying to do?
>>
>> I just don't understand you. Different refcounters are related to different
>> problems, they are introduced to solve. This patchset changes only one refcounter,
>> and it does not touch other of them. What do you want to know about others?
>>
> 
> Why net::count not net::passive?  What problem are you trying to solve?

net::count is a counter, which indicates whether net is alive and still
initilized. net::passive does not guarantees that net has not been
deinitialized yet.

The same with another namespaces. All of merged counters indicate
that a namespace is alive and initialized. So, we merge them by this property.

> They both are reference counts on the network namespace.
> 
> I don't understand what you are trying to do, other than take a bunch of
> things that look similar and squash them all together.
> 
> What semantics does this magical common reference count have?
> Why is it better for the count to live in ns_common rather than it
> it's own dedicated field of struct net?

The only visible current effect is better readability and better
debugability with, say, crash on kernel dump.

> Given that decrementing still requires code per namespace to handle
> the count going to zero.  How does this benefit anyone?

Since namespaces are different by type, they require different destructors.
Generic counter can't solve any problem, the namespaces can address.

> Has the effect of cache line placement of the move of the reference
> count been considered?

I don't see any performance impact during namespace creation/destruction
test.

> All I see in the patch in question is switching a location that the
> count lives.  Which does not seem useful to me.
> 
> I am not fundamentally oppossed but I don't see the point.  At this
> point it looks like you are making things harder to maintain by making
> things common that are merely similar