Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Wed, 12 Nov 2008 15:24:48 +0300

> +static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
> +{
> +#ifdef CONFIG_NET_NS
> +	ib->ib_net = net;
> +#endif
> +}
> +

It's basically read_pnet() hidden behind another name.
And you'll add new "aliases" for read_pnet() over and over
again.

That makes no sense to me.

I like read_pnet() much better, and the only thing that
can be improved is read_pnet()'d name (along with the
name of every other similar interface already in
net_namespace.h)

[PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Wed, Nov 12, 2008 at 11:50:29AM +0100, Eric Dumazet wrote:
> I have no problem you take care of this.

OK, here is somethiing that hopefully satisfies everyone.
It depends on pnet stuff being dropped.



[PATCH v3] net: #ifdef inet_bind_bucket::ib_net

Save one pointer in every inet_bind_bucket in NET_NS=n case.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/net/inet_hashtables.h   |   18 ++++++++++++++++++
 net/ipv4/inet_connection_sock.c |    4 ++--
 net/ipv4/inet_hashtables.c      |    6 +++---
 3 files changed, 23 insertions(+), 5 deletions(-)

--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -77,13 +77,31 @@ struct inet_ehash_bucket {
  * ports are created in O(1) time?  I thought so. ;-)	-DaveM
  */
 struct inet_bind_bucket {
+#ifdef CONFIG_NET_NS
 	struct net		*ib_net;
+#endif
 	unsigned short		port;
 	signed short		fastreuse;
 	struct hlist_node	node;
 	struct hlist_head	owners;
 };
 
+static inline struct net *ib_net(struct inet_bind_bucket *ib)
+{
+#ifdef CONFIG_NET_NS
+	return ib->ib_net;
+#else
+	return &init_net;
+#endif
+}
+
+static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
+{
+#ifdef CONFIG_NET_NS
+	ib->ib_net = net;
+#endif
+}
+
 #define inet_bind_bucket_for_each(tb, node, head) \
 	hlist_for_each_entry(tb, node, head, node)
 
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -109,7 +109,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 					hashinfo->bhash_size)];
 			spin_lock(&head->lock);
 			inet_bind_bucket_for_each(tb, node, &head->chain)
-				if (tb->ib_net == net && tb->port == rover)
+				if (ib_net(tb) == net && tb->port == rover)
 					goto next;
 			break;
 		next:
@@ -137,7 +137,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 				hashinfo->bhash_size)];
 		spin_lock(&head->lock);
 		inet_bind_bucket_for_each(tb, node, &head->chain)
-			if (tb->ib_net == net && tb->port == snum)
+			if (ib_net(tb) == net && tb->port == snum)
 				goto tb_found;
 	}
 	tb = NULL;
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -35,7 +35,7 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
 	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
 
 	if (tb != NULL) {
-		tb->ib_net       = hold_net(net);
+		ib_net_set(tb, hold_net(net));
 		tb->port      = snum;
 		tb->fastreuse = 0;
 		INIT_HLIST_HEAD(&tb->owners);
@@ -51,7 +51,7 @@ void inet_bind_bucket_destroy(struct kmem_cache *cachep, struct inet_bind_bucket
 {
 	if (hlist_empty(&tb->owners)) {
 		__hlist_del(&tb->node);
-		release_net(tb->ib_net);
+		release_net(ib_net(tb));
 		kmem_cache_free(cachep, tb);
 	}
 }
@@ -449,7 +449,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
 			 * unique enough.
 			 */
 			inet_bind_bucket_for_each(tb, node, &head->chain) {
-				if (tb->ib_net == net && tb->port == port) {
+				if (ib_net(tb) == net && tb->port == port) {
 					WARN_ON(hlist_empty(&tb->owners));
 					if (tb->fastreuse >= 0)
 						goto next_port;

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Wed, Nov 12, 2008 at 04:24:23AM -0800, David Miller wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Wed, 12 Nov 2008 15:24:48 +0300
>
> > +static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
> > +{
> > +#ifdef CONFIG_NET_NS
> > +   ib->ib_net = net;
> > +#endif
> > +}
> > +
>
> It's basically read_pnet() hidden behind another name.
> And you'll add new "aliases" for read_pnet() over and over
> again.
>
> That makes no sense to me.

It also make no sense to expose write_pnet() for one(!) user and
simultaneously hide read_pnet() under ib_net() as committed patches do.

Something is wrong with read_pnet() as nobody suggested to mass use it
or send a patch doing it.


        #ifdef CONFIG_NET_NS
                ib->ib_net = net;
        #endif

It's _obvious_ from this code that it's a C assignment or nop. It's also
obvious depending on what config option.

        write_pnet(&ib->ib_net, net);

What is & operator doing here? Is it important? '&' is syntaxic noise.

And netns assignments are exactly this: assigment or a nop.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: "Alexey Dobriyan" <adobriyan@gmail.com>
Date: Fri, 14 Nov 2008 01:53:26 +0300

> Something is wrong with read_pnet() as nobody suggested to mass use it
> or send a patch doing it.
> 
>         #ifdef CONFIG_NET_NS
>                 ib->ib_net = net;
>         #endif
> 
> It's _obvious_ from this code that it's a C assignment or nop. It's also
> obvious depending on what config option.

obvious, but ugly

>         write_pnet(&ib->ib_net, net);
> 
> What is & operator doing here? Is it important? '&' is syntaxic noise.

'&' is necessary for the assignment.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
> On Wed, Nov 12, 2008 at 04:24:23AM -0800, David Miller wrote:
>> From: Alexey Dobriyan <adobriyan@gmail.com>
>> Date: Wed, 12 Nov 2008 15:24:48 +0300
>>
>>> +static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
>>> +{
>>> +#ifdef CONFIG_NET_NS
>>> +   ib->ib_net = net;
>>> +#endif
>>> +}
>>> +
>> It's basically read_pnet() hidden behind another name.
>> And you'll add new "aliases" for read_pnet() over and over
>> again.
>>
>> That makes no sense to me.
> 
> It also make no sense to expose write_pnet() for one(!) user and
> simultaneously hide read_pnet() under ib_net() as committed patches do.
> 
> Something is wrong with read_pnet() as nobody suggested to mass use it
> or send a patch doing it.

I did. My plan is to zap all superflous #ifdef CONFIG_NET_NS if possible.


> 
> 
>         #ifdef CONFIG_NET_NS
>                 ib->ib_net = net;
>         #endif
> 
> It's _obvious_ from this code that it's a C assignment or nop. It's also
> obvious depending on what config option.
> 
>         write_pnet(&ib->ib_net, net);
> 
> What is & operator doing here? Is it important? '&' is syntaxic noise.
> 
> And netns assignments are exactly this: assigment or a nop.

You obviously didnt read my patches.

How do you want to implement a C function that can write to ib->ib_net ?

Yes, I prefer a function over a macro, as most kernel developpers,
for obvious reasons.

Only sane way is :

static inline void write_pnet(struct net **pnet, struct net *net)
{
	*pnet = net;
}

and call write_pnet(&ip->ib_net, net);

Strange, this is what I did.

Take the time to read the patch, please.

Thank you

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

> > Something is wrong with read_pnet() as nobody suggested to mass use it
> > or send a patch doing it.
>
> I did. My plan is to zap all superflous #ifdef CONFIG_NET_NS if possible.

That's not mass usage.

Mass usage is, say, s/dev_net/read_pnet/.

Do you want to do this too?

> How do you want to implement a C function that can write to ib->ib_net ?

I've sent ib_net_set().

> Take the time to read the patch, please.

You too.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
>>> Something is wrong with read_pnet() as nobody suggested to mass use it
>>> or send a patch doing it.
>> I did. My plan is to zap all superflous #ifdef CONFIG_NET_NS if possible.
> 
> That's not mass usage.
> 
> Mass usage is, say, s/dev_net/read_pnet/.
> 
> Do you want to do this too?

Yes, it was present on my original first patch.
I said I was going to split the big patch.
How many mails will be necessary until you get the point ?

I was waiting *you* change "read_pnet()/write_pnet()" names as you
intended, *before* submitting new patches, in order not to
duplicate work.

For instance, I dont like :

static inline
struct net *dev_net(const struct net_device *dev)
{
#ifdef CONFIG_NET_NS
        return dev->nd_net;
#else
        return &init_net;
#endif
}


I prefer :

static inline
struct net *dev_net(const struct net_device *dev)
{
	return read_pnet(&dev->nd_net);
}

This is better because :

1) No #ifdef CONFIG_NET_NS

2) The magic about &init_net is not duplicated in ten different include files, but
   centralized in the right file : include/net/net_namespace.h

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 14 Nov 2008 05:36:15 +0100

> This is better because :
> 
> 1) No #ifdef CONFIG_NET_NS
> 
> 2) The magic about &init_net is not duplicated in ten different include files, but
>    centralized in the right file : include/net/net_namespace.h

I %100 agree.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Fri, 14 Nov 2008 05:36:15 +0100
> 
>> This is better because :
>>
>> 1) No #ifdef CONFIG_NET_NS
>>
>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>    centralized in the right file : include/net/net_namespace.h
> 
> I %100 agree.

Speaking of those functions, what do you think of this one ?

static inline
void dev_net_set(struct net_device *dev, struct net *net)
{
#ifdef CONFIG_NET_NS
        release_net(dev->nd_net);
        dev->nd_net = hold_net(net);
#endif
}

I believe that its safer to hold a reference on "new" *before*
releasing reference on "old" object.

Also, release_net() and hold_net() can be defined to do
the use_count refcounting regardless of CONFIG_NET_NS
(Its a different NETNS_REFCNT_DEBUG #ifdef)

Yet another example where read_pnet() and write_pnet()
are the right answer : Its cleaner and fixes *bugs*.

static inline
void dev_net_set(struct net_device *dev, struct net *net)
{
	hold_net(net);
	release_net(read_pnet(&dev->nd_net);
        write_pnet(&dev->nd_net, net);
}

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
> David Miller a écrit :
>> From: Eric Dumazet <dada1@cosmosbay.com>
>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>
>>> This is better because :
>>>
>>> 1) No #ifdef CONFIG_NET_NS
>>>
>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>    centralized in the right file : include/net/net_namespace.h
>>
>> I %100 agree.
>
> Speaking of those functions, what do you think of this one ?
>
> static inline
> void dev_net_set(struct net_device *dev, struct net *net)
> {
> #ifdef CONFIG_NET_NS
>        release_net(dev->nd_net);
>        dev->nd_net = hold_net(net);
> #endif
> }
>
> I believe that its safer to hold a reference on "new" *before*
> releasing reference on "old" object.
>
> Also, release_net() and hold_net() can be defined to do
> the use_count refcounting regardless of CONFIG_NET_NS
> (Its a different NETNS_REFCNT_DEBUG #ifdef)

NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
is never freed.

> Yet another example where read_pnet() and write_pnet()
> are the right answer : Its cleaner and fixes *bugs*.

pnet stuff by definition can't fix bugs :-)

Ask from where new net comes?

If from current, nsproxy refcount is at least 1, so netns refcount is at least 1,
so shutdown sequence can't start.

If from userspace socket, there is task in netns -- see #1

If from netdevice on which ioctl is done, some task did ioctl -- see #1.

And so on.

But this is peanuts, because your race matters only when netns is almost freed
(in kmem_cache_free sense), so you're stashing dangling pointer somewhere else
which is a bug by itself.

> static inline
> void dev_net_set(struct net_device *dev, struct net *net)
> {
> 	hold_net(net);
> 	release_net(read_pnet(&dev->nd_net);
>        write_pnet(&dev->nd_net, net);
> }

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
> On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
>> David Miller a écrit :
>>> From: Eric Dumazet <dada1@cosmosbay.com>
>>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>>
>>>> This is better because :
>>>>
>>>> 1) No #ifdef CONFIG_NET_NS
>>>>
>>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>>    centralized in the right file : include/net/net_namespace.h
>>> I %100 agree.
>> Speaking of those functions, what do you think of this one ?
>>
>> static inline
>> void dev_net_set(struct net_device *dev, struct net *net)
>> {
>> #ifdef CONFIG_NET_NS
>>        release_net(dev->nd_net);
>>        dev->nd_net = hold_net(net);
>> #endif
>> }
>>
>> I believe that its safer to hold a reference on "new" *before*
>> releasing reference on "old" object.
>>
>> Also, release_net() and hold_net() can be defined to do
>> the use_count refcounting regardless of CONFIG_NET_NS
>> (Its a different NETNS_REFCNT_DEBUG #ifdef)
> 
> NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
> is never freed.

It makes sense when you want to debug things, without full NET_NS

Check CONFIG_PROVE_LOCKING or other debugging stuff

CONFIG_SMP can be unset, still we are able to check spinlocks are
lock()/unlock() are correctly paired

> 
>> Yet another example where read_pnet() and write_pnet()
>> are the right answer : Its cleaner and fixes *bugs*.
> 
> pnet stuff by definition can't fix bugs :-)

It will, I bet it.

Cant you see difference between

#ifdef CONFIG_NET_NS
	ptr->net = expression;
#endif

and

write_pnet(&ptr->net, expression);

Answer : In the 2nd case, (expression) is evaluated.

In case its evaluation has side effects (like... refcounting), it makes a huge difference.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Fri, Nov 14, 2008 at 07:21:13AM +0100, Eric Dumazet wrote:
> Alexey Dobriyan a écrit :
>> On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
>>> David Miller a écrit :
>>>> From: Eric Dumazet <dada1@cosmosbay.com>
>>>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>>>
>>>>> This is better because :
>>>>>
>>>>> 1) No #ifdef CONFIG_NET_NS
>>>>>
>>>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>>>    centralized in the right file : include/net/net_namespace.h
>>>> I %100 agree.
>>> Speaking of those functions, what do you think of this one ?
>>>
>>> static inline
>>> void dev_net_set(struct net_device *dev, struct net *net)
>>> {
>>> #ifdef CONFIG_NET_NS
>>>        release_net(dev->nd_net);
>>>        dev->nd_net = hold_net(net);
>>> #endif
>>> }
>>>
>>> I believe that its safer to hold a reference on "new" *before*
>>> releasing reference on "old" object.
>>>
>>> Also, release_net() and hold_net() can be defined to do
>>> the use_count refcounting regardless of CONFIG_NET_NS
>>> (Its a different NETNS_REFCNT_DEBUG #ifdef)
>>
>> NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
>> is never freed.
>
> It makes sense when you want to debug things, without full NET_NS

It doesn't work without NET_NS at all.

Check triggers right before netns is freed, and init_net is never freed.
You can move the check (interesting, to where?) or watch usecount live
somehow.

> Check CONFIG_PROVE_LOCKING or other debugging stuff
>
> CONFIG_SMP can be unset, still we are able to check spinlocks are
> lock()/unlock() are correctly paired
>
>>
>>> Yet another example where read_pnet() and write_pnet()
>>> are the right answer : Its cleaner and fixes *bugs*.
>>
>> pnet stuff by definition can't fix bugs :-)
>
> It will, I bet it.
>
> Cant you see difference between
>
> #ifdef CONFIG_NET_NS
> 	ptr->net = expression;
> #endif
>
> and
>
> write_pnet(&ptr->net, expression);
>
> Answer : In the 2nd case, (expression) is evaluated.
>
> In case its evaluation has side effects (like... refcounting), it makes a huge difference.

We don't do such tricky thing with netns pointers. It either comes from somewhere (netdevice)
and you use it as is or it's &init_net.

It's almost a cookie, nobody evaluates cookies. Such code should rejected.