add missing local serialization in ip_output.c

This is what I am going to apply. It also dropped the get_cpu_light() call which was added in a patch to remove the get_cpu_var() and is now no longer required since we have the get_locked_var() thingy now.

From: Nicholas Mc Guire <[email protected]>
Date: Sun, 29 Dec 2013 18:11:54 +0100
Subject: [PATCH] net: ip_send_unicast_reply: add missing local serialization

in response to the oops in ip_output.c:ip_send_unicast_reply under high network load with CONFIG_PREEMPT_RT_FULL=y, reported by Sami Pietikainen <[email protected]>, this patch adds local serialization in ip_send_unicast_reply.

from ip_output.c:
/*
* Generic function to send a packet as reply to another packet.
* Used to send some TCP resets/acks so far.
*
* Use a fake percpu inet socket to avoid false sharing and contention.
*/
static DEFINE_PER_CPU(struct inet_sock, unicast_sock) = {
...

which was added in commit be9f4a44 in linux-stable. The git log, wich introduced the PER_CPU unicast_sock, states:
<snip>
commit be9f4a44e7d41cee50ddb5f038fc2391cbbb4046

Author: Eric Dumazet <[email protected]>
Date:   Thu Jul 19 07:34:03 2012 +0000

ipv4: tcp: remove per net tcp_sock

tcp_v4_send_reset() and tcp_v4_send_ack() use a single socket per network namespace.

This leads to bad behavior on multiqueue NICS, because many cpus contend for the socket lock and once socket lock is acquired, extra false sharing on various socket fields slow down the operations.

To better resist to attacks, we use a percpu socket. Each cpu can run without contention, using appropriate memory (local node)
<snip>

The per-cpu here thus is assuming exclusivity serializing per cpu - so the use of get_cpu_ligh introduced in net-use-cpu-light-in-ip-send-unicast-reply.patch, which droped the preempt_disable in favor of a migrate_disable is probably wrong as this only handles the referencial consistency but not the serialization. To evade a preempt_disable here a local lock would be needed.

Therapie:
* add local lock:
* and re-introduce local serialization:

Tested on x86 with high network load using the testcase from Sami Pietikainen
while : ; do wget -O - ftp://LOCAL_SERVER/empty_file > /dev/null 2>&1; done

Link http://www.spinics.net/lists/linux-rt-users/msg11007.html

Cc [email protected]

Signed-off-by Nicholas Mc Guire <[email protected]>

Signed-off-by Sebastian Andrzej Siewior <[email protected]>

---
 net/ipv4/ip_output.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

net/ipv4/ip_output.c +6 -3

index e9fa68c..8bb3b4a 100644
@@ -79,6 +79,7 @@
 #include <linux/mroute.h>
 #include <linux/netlink.h>
 #include <linux/tcp.h>
+#include <linux/locallock.h>
 
 int sysctl_ip_default_ttl __read_mostly = IPDEFTTL;
 EXPORT_SYMBOL(sysctl_ip_default_ttl);

@@ -1468,6 +1469,9 @@ static DEFINE_PER_CPU(struct inet_sock, unicast_sock) = {
 	.uc_ttl		= -1,
 };
 
+/* serialize concurrent calls on the same CPU to ip_send_unicast_reply */
+static DEFINE_LOCAL_IRQ_LOCK(unicast_lock);
+
 void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
 			   __be32 saddr, const struct ip_reply_arg *arg,
 			   unsigned int len)

@@ -1505,8 +1509,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
 	if (IS_ERR(rt))
 		return;
 
-	get_cpu_light();
-	inet = &__get_cpu_var(unicast_sock);
+	inet = &get_locked_var(unicast_lock, unicast_sock);
 
 	inet->tos = arg->tos;
 	sk = &inet->sk;

@@ -1530,7 +1533,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
 		ip_push_pending_frames(sk, &fl4);
 	}
 
-	put_cpu_light();
+	put_locked_var(unicast_lock, unicast_sock);
 
 	ip_rt_put(rt);
 }

Signature · 1 line

1.8.5.2

Quoted 4 lines · Sebastian Andrzej Siewior

This is what I am going to apply. It also dropped the get_cpu_light() call which was added in a patch to remove the get_cpu_var() and is now no longer required since we have the get_locked_var() thingy now.

I do not think you can drop that - what is preventing migration now ?

#define get_locked_var(lvar, var)                                       \
        (*({                                                            \
                local_lock(lvar);                                       \
                &__get_cpu_var(var);                                    \
        }))

No migrate_disable here - so how is this protected against migration ?

Note that I did send out mail on this because I believe get_locked_var should actually be doing a a migrate_disable/enable but got no feedback on that yet.

So for now I think you need to retain the get_cpu_light/put_cpu_light

thx!
hofrat

Quoted 8 lines · Nicholas Mc Guire

Quoted 4 lines · Sebastian Andrzej Siewior

This is what I am going to apply. It also dropped the get_cpu_light() call which was added in a patch to remove the get_cpu_var() and is now no longer required since we have the get_locked_var() thingy now.

I do not think you can drop that - what is preventing migration now ?

Nothing but I do not see the need for it.

Quoted 7 lines

#define get_locked_var(lvar, var)                                       \
        (*({                                                            \
                local_lock(lvar);                                       \
                &__get_cpu_var(var);                                    \
        }))

It does not. If you get here on CPU0, you the variable from CPU0. If you get migrated to CPU1 you still use the variable from CPU0. If another task is active on CPU0 then it will be blocked until the other now running on CPU1 completes and releases the lock.

Quoted 3 lines

Note that I did send out mail on this because I believe get_locked_var should actually be doing a a migrate_disable/enable but got no feedback on that yet.

I don't see a reason why you should not leave the CPU on which you got access to the variable as long as you do not do any further assumption regarding the CPU number. I don't see that this happens here.

Quoted 1 line

So for now I think you need to retain the get_cpu_light/put_cpu_light

Are you still sure?

Quoted 13 lines · Sebastian Andrzej Siewior

Quoted 8 lines · Nicholas Mc Guire

Quoted 4 lines · Sebastian Andrzej Siewior

This is what I am going to apply. It also dropped the get_cpu_light() call which was added in a patch to remove the get_cpu_var() and is now no longer required since we have the get_locked_var() thingy now.

I do not think you can drop that - what is preventing migration now ?

Nothing but I do not see the need for it.

Quoted 1 line

Quoted 6 lines

Quoted 6 lines

#define get_locked_var(lvar, var)                                       \
        (*({                                                            \
                local_lock(lvar);                                       \
                &__get_cpu_var(var);                                    \
        }))

q> > No migrate_disable here - so how is this protected against migration ?

Quoted 10 lines

It does not. If you get here on CPU0, you the variable from CPU0. If you get migrated to CPU1 you still use the variable from CPU0. If another task is active on CPU0 then it will be blocked until the other now running on CPU1 completes and releases the lock.

Quoted 3 lines

Note that I did send out mail on this because I believe get_locked_var should actually be doing a a migrate_disable/enable but got no feedback on that yet.

Quoted 8 lines

I don't see a reason why you should not leave the CPU on which you got access to the variable as long as you do not do any further assumption regarding the CPU number. I don't see that this happens here.

Quoted 1 line

So for now I think you need to retain the get_cpu_light/put_cpu_light

Are you still sure?

yes and no - it is needed I believe but it is actually already provided. what I overlooked is that (actually my path-diagram was wrong - so thanks for the catch):

#define get_locked_var(lvar, var)                                       \
        (*({                                                            \
                local_lock(lvar);                                       \
                &__get_cpu_var(var);                                    \
        }))
      ->#define local_lock(lvar)                                        \
        	do { __local_lock(&get_local_var(lvar)); } while (0)

                -> # define get_local_var(var) (*({                        \
        	       migrate_disable();                              \
        	       &__get_cpu_var(var); }))
                       -> #define __get_cpu_var(var) (*this_cpu_ptr(&(var)))

so its fine to drop the get_cpu_light/put_cpu_light as migration is in fact already disabled at this point. the access to the local spinlock object here is via this_cpu_ptr so if we would allow migration I think you would end up unlocking the wrong lock.

thx!
hofrat

Quoted 21 lines · On Fri, 17 Jan 2014 16:33:23 +0100 Nicholas Mc Guire <[email protected]> wrote:

Quoted 19 lines · Sebastian Andrzej Siewior

Quoted 8 lines · Nicholas Mc Guire

Quoted 4 lines · Sebastian Andrzej Siewior

This is what I am going to apply. It also dropped the get_cpu_light() call which was added in a patch to remove the get_cpu_var() and is now no longer required since we have the get_locked_var() thingy now.

I do not think you can drop that - what is preventing migration now ?

Nothing but I do not see the need for it.

Quoted 7 lines

#define get_locked_var(lvar, var)                                       \
        (*({                                                            \
                local_lock(lvar);                                       \
                &__get_cpu_var(var);                                    \
        }))

Quoted 1 line

q> > No migrate_disable here - so how is this protected against migration ?

I was just about to reply to this, that local_lock() grabs a spinlock which does do a migrate disable. But you also noticed that the get_local_var() does a migrate disable too. We now have double the protection, so we are safe as Sebastion has done it.

Drop recursive call to migrate_disabel/enable for local_*lock* api reported by Steven Rostedt.

trivial API cleanup - kernel/softirq.c was mimiking local_lock.