[PATCH net-next-2.6] netns: call ops_free right after ops_exit

[PATCH net-next-2.6] netns: call ops_free right after ops_exit

[Jiri Pirko]

There's no need to iterate this twice. We can free net generic variables right
after exit is called.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>

diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index bd8c471..16217bc 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -82,7 +82,7 @@ static void ops_free_list(const struct pernet_operations *ops,
 static __net_init int setup_net(struct net *net)
 {
 	/* Must be called with net_mutex held */
-	const struct pernet_operations *ops, *saved_ops;
+	const struct pernet_operations *ops;
 	int error = 0;
 	LIST_HEAD(net_exit_list);
 
@@ -105,13 +105,10 @@ out_undo:
 	 * for the pernet modules whose init functions did not fail.
 	 */
 	list_add(&net->exit_list, &net_exit_list);
-	saved_ops = ops;
-	list_for_each_entry_continue_reverse(ops, &pernet_list, list)
+	list_for_each_entry_continue_reverse(ops, &pernet_list, list) {
 		ops_exit_list(ops, &net_exit_list);
-
-	ops = saved_ops;
-	list_for_each_entry_continue_reverse(ops, &pernet_list, list)
 		ops_free_list(ops, &net_exit_list);
+	}
 
 	rcu_barrier();
 	goto out;
@@ -231,13 +228,14 @@ static void cleanup_net(struct work_struct *work)
 	 */
 	synchronize_rcu();
 
-	/* Run all of the network namespace exit methods */
-	list_for_each_entry_reverse(ops, &pernet_list, list)
+	/*
+	 * Run all of the network namespace exit methods and free
+	 * the net generic variables
+	 */
+	list_for_each_entry_reverse(ops, &pernet_list, list) {
 		ops_exit_list(ops, &net_exit_list);
-
-	/* Free the net generic variables */
-	list_for_each_entry_reverse(ops, &pernet_list, list)
 		ops_free_list(ops, &net_exit_list);
+	}
 
 	mutex_unlock(&net_mutex);
 

Re: [PATCH net-next-2.6] netns: call ops_free right after ops_exit

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Sun, 25 Apr 2010 11:26:01 +0200

> There's no need to iterate this twice. We can free net generic
> variables right after exit is called.
>
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Are you sure there are no problems with doing this?

What if there are inter-net variable reference dependencies
or something like that?

I really suspect it is being done this way on purpose, but
in the end I defer to experts like Eric B. :-)

Re: [PATCH net-next-2.6] netns: call ops_free right after ops_exit

[Eric W. Biederman]

David Miller <davem@davemloft.net> writes:

> From: Jiri Pirko <jpirko@redhat.com>
> Date: Sun, 25 Apr 2010 11:26:01 +0200
>
>> There's no need to iterate this twice. We can free net generic
>> variables right after exit is called.
>>
>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>
> Are you sure there are no problems with doing this?
>
> What if there are inter-net variable reference dependencies
> or something like that?
>
> I really suspect it is being done this way on purpose, but
> in the end I defer to experts like Eric B. :-)

I am pretty certain there is a problem.  My memory is fuzzy this
morning but I believe we can have rcu references between various
pieces of the networking stack for a single network namespace.  So we
need to cause all of the network namespace to exit before it is safe
to free those pieces.

Eric

Re: [PATCH net-next-2.6] netns: call ops_free right after ops_exit

[Jiri Pirko]

Sun, Apr 25, 2010 at 04:50:34PM CEST, ebiederm@xmission.com wrote:
>David Miller <davem@davemloft.net> writes:
>
>> From: Jiri Pirko <jpirko@redhat.com>
>> Date: Sun, 25 Apr 2010 11:26:01 +0200
>>
>>> There's no need to iterate this twice. We can free net generic
>>> variables right after exit is called.
>>>
>>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>>
>> Are you sure there are no problems with doing this?
>>
>> What if there are inter-net variable reference dependencies
>> or something like that?
>>
>> I really suspect it is being done this way on purpose, but
>> in the end I defer to experts like Eric B. :-)
>
>I am pretty certain there is a problem.  My memory is fuzzy this
>morning but I believe we can have rcu references between various
>pieces of the networking stack for a single network namespace.  So we
>need to cause all of the network namespace to exit before it is safe
>to free those pieces.

Hmm, that doesn't make much sense to me. Since the allocated memory in question
is used locally, after exit() is called, the memory chunk should not be used by
anyone and if it is, I think it's a bug.

Earlier, when the memory wasn't allocated automatically (by filling .size)
memory was individually freed in exit(). From what I understood from your reply,
you are telling this was buggy?

Jirka

>
>Eric
>
>

Re: [PATCH net-next-2.6] netns: call ops_free right after ops_exit

[Eric W. Biederman]

Jiri Pirko <jpirko@redhat.com> writes:

> Sun, Apr 25, 2010 at 04:50:34PM CEST, ebiederm@xmission.com wrote:
>>David Miller <davem@davemloft.net> writes:
>>
>>> From: Jiri Pirko <jpirko@redhat.com>
>>> Date: Sun, 25 Apr 2010 11:26:01 +0200
>>>
>>>> There's no need to iterate this twice. We can free net generic
>>>> variables right after exit is called.
>>>>
>>>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>>>
>>> Are you sure there are no problems with doing this?
>>>
>>> What if there are inter-net variable reference dependencies
>>> or something like that?
>>>
>>> I really suspect it is being done this way on purpose, but
>>> in the end I defer to experts like Eric B. :-)
>>
>>I am pretty certain there is a problem.  My memory is fuzzy this
>>morning but I believe we can have rcu references between various
>>pieces of the networking stack for a single network namespace.  So we
>>need to cause all of the network namespace to exit before it is safe
>>to free those pieces.
>
> Hmm, that doesn't make much sense to me. Since the allocated memory in question
> is used locally, after exit() is called, the memory chunk should not be used by
> anyone and if it is, I think it's a bug.
>
> Earlier, when the memory wasn't allocated automatically (by filling .size)
> memory was individually freed in exit(). From what I understood from your reply,
> you are telling this was buggy?

Now I remember clearly.  The use case for not freeing memory
immediately is the delayed freeing of network devices.  Ideally we
delay unregistering all of the network devices into
default_device_exit_batch, when we terminate one or namespaces.

Since network devices are freed outside of their exit routines we need
to keep their per net memory around until they are freed.

Ultimately all of this is much easier to think about if these chunks of
memory can be logically thought of as living on struct net and have the
same lifetime rules.

Eric

Re: [PATCH net-next-2.6] netns: call ops_free right after ops_exit

[Jiri Pirko]

Mon, Apr 26, 2010 at 05:06:07AM CEST, ebiederm@xmission.com wrote:
>Jiri Pirko <jpirko@redhat.com> writes:
>
>> Sun, Apr 25, 2010 at 04:50:34PM CEST, ebiederm@xmission.com wrote:
>>>David Miller <davem@davemloft.net> writes:
>>>
>>>> From: Jiri Pirko <jpirko@redhat.com>
>>>> Date: Sun, 25 Apr 2010 11:26:01 +0200
>>>>
>>>>> There's no need to iterate this twice. We can free net generic
>>>>> variables right after exit is called.
>>>>>
>>>>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>>>>
>>>> Are you sure there are no problems with doing this?
>>>>
>>>> What if there are inter-net variable reference dependencies
>>>> or something like that?
>>>>
>>>> I really suspect it is being done this way on purpose, but
>>>> in the end I defer to experts like Eric B. :-)
>>>
>>>I am pretty certain there is a problem.  My memory is fuzzy this
>>>morning but I believe we can have rcu references between various
>>>pieces of the networking stack for a single network namespace.  So we
>>>need to cause all of the network namespace to exit before it is safe
>>>to free those pieces.
>>
>> Hmm, that doesn't make much sense to me. Since the allocated memory in question
>> is used locally, after exit() is called, the memory chunk should not be used by
>> anyone and if it is, I think it's a bug.
>>
>> Earlier, when the memory wasn't allocated automatically (by filling .size)
>> memory was individually freed in exit(). From what I understood from your reply,
>> you are telling this was buggy?
>
>Now I remember clearly.  The use case for not freeing memory
>immediately is the delayed freeing of network devices.  Ideally we
>delay unregistering all of the network devices into
>default_device_exit_batch, when we terminate one or namespaces.
>
>Since network devices are freed outside of their exit routines we need
>to keep their per net memory around until they are freed.
>
>Ultimately all of this is much easier to think about if these chunks of
>memory can be logically thought of as living on struct net and have the
>same lifetime rules.

Ok, I see your point. So scratch this.

Thanks,

	Jirka
>
>Eric