Skip to contentSkip to conversation

Linux 4.1.33-rt38-rc1

PATCH In review Steven Rostedt 1 participant 11 messages Sep 22, 2016 1 message not in the archive

Subsystems (8)
Networking x86 VFS SCSI +4 more subsystems
lore
Share & tools
lore

Steven Rostedt

author reviewer
#0 lore raw

cover Linux 4.1.33-rt38-rc1

Dear RT Folks,

This is the RT stable review cycle of patch 4.1.33-rt38-rc1.

Please scream at me if I messed something up. Please test the patches too.

The -rc release will be uploaded to kernel.org and will be deleted when the final release is out. This is just a review release (or release candidate).

The pre-releases will not be pushed to the git repository, only the final release is.

If all goes well, this patch will be converted to the next main release on 9/25/2016.

Enjoy,

-- Steve

To build 4.1.33-rt38-rc1 directly, the following patches should be applied:

http://www.kernel.org/pub/linux/kernel/v4.x/linux-4.1.tar.xz

http://www.kernel.org/pub/linux/kernel/v4.x/patch-4.1.33.xz

http://www.kernel.org/pub/linux/kernel/projects/rt/4.1/patch-4.1.33-rt38-rc1.patch.xz

You can also build from 4.1.33-rt37 by applying the incremental patch:

http://www.kernel.org/pub/linux/kernel/projects/rt/4.1/incr/patch-4.1.33-rt37-rt38-rc1.patch.xz

Changes from 4.1.33-rt37:

---

Mike Galbraith (1):
scsi/fcoe: Fix get_cpu()/put_cpu_light() imbalance in fcoe_recv_frame()

Sebastian Andrzej Siewior (8):
timers: wakeup all timer waiters
timers: wakeup all timer waiters without holding the base lock
sched: lazy_preempt: avoid a warning in the !RT case
net: add back the missing serialization in ip_send_unicast_reply()
net: add a lock around icmp_sk()
fs/dcache: resched/chill only if we make no progress x86/preempt-lazy: fixup should_resched()
fs/dcache: incremental fixup of the retry routine

Steven Rostedt (Red Hat) (1):

      Linux 4.1.33-rt38-rc1

----

 arch/x86/include/asm/preempt.h | 17 +++++++++++++++--
 drivers/scsi/fcoe/fcoe.c       |  2 +-
 fs/dcache.c                    | 17 ++++++++++++-----
 kernel/sched/core.c            |  2 +-
 kernel/time/timer.c            |  4 ++--
 localversion-rt                |  2 +-
 net/ipv4/icmp.c                |  8 ++++++++
 net/ipv4/tcp_ipv4.c            |  7 +++++++
 8 files changed, 47 insertions(+), 12 deletions(-)
author reviewer
#1 lore raw

PATCH 1/10 timers: wakeup all timer waiters

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

The base lock is dropped during the invocation if the timer. That means it is possible that we have one waiter while timer1 is running and once this one finished, we get another waiter while timer2 is running. Since we wake up only one waiter it is possible that we miss the other one. This will probably heal itself over time because most of the time we complete timers without an active wake up.
To avoid the scenario where we don't wake up all waiters at once, wake_up_all() is used.

---
 kernel/time/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/timer.c +1 -1
index c68ba873da3c..5a45162ae924 100644
@@ -1019,7 +1019,7 @@ static void wait_for_running_timer(struct timer_list *timer)
 			   base->running_timer != timer);
 }
 
-# define wakeup_timer_waiters(b)	wake_up(&(b)->wait_for_running_timer)
+# define wakeup_timer_waiters(b)	wake_up_all(&(b)->wait_for_running_timer)
 #else
 static inline void wait_for_running_timer(struct timer_list *timer)
 {
Signature · 1 line
2.8.1
author reviewer
#2 lore raw

PATCH 2/10 timers: wakeup all timer waiters without holding the base lock

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

There should be no need to hold the base lock during the wakeup. There should be no boosting involved, the wakeup list has its own lock so it should be safe to do this without the lock.

---
 kernel/time/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/timer.c +1 -1
index 5a45162ae924..b1f9e6c5bec4 100644
@@ -1279,8 +1279,8 @@ static inline void __run_timers(struct tvec_base *base)
 			}
 		}
 	}
-	wakeup_timer_waiters(base);
 	spin_unlock_irq(&base->lock);
+	wakeup_timer_waiters(base);
 }
 
 #ifdef CONFIG_NO_HZ_COMMON
Signature · 1 line
2.8.1
author reviewer
#3 lore raw

PATCH 3/10 sched: lazy_preempt: avoid a warning in the !RT case

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/sched/core.c +1 -1
index 5396888893da..9f05a3dacd16 100644
@@ -3123,7 +3123,7 @@ static __always_inline int preemptible_lazy(void)
 
 #else
 
-static int preemptible_lazy(void)
+static inline int preemptible_lazy(void)
 {
 	return 1;
 }
Signature · 1 line
2.8.1
author reviewer
#4 lore raw

PATCH 4/10 scsi/fcoe: Fix get_cpu()/put_cpu_light() imbalance in fcoe_recv_frame()

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Mike Galbraith <[email protected]>

During master->rt merge, I stumbled across the buglet below.

Fix get_cpu()/put_cpu_light() imbalance.

---
 drivers/scsi/fcoe/fcoe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/fcoe/fcoe.c +1 -1
index d6b6dde64fb9..455bf9c67b16 100644
@@ -1815,7 +1815,7 @@ static void fcoe_recv_frame(struct sk_buff *skb)
 	 */
 	hp = (struct fcoe_hdr *) skb_network_header(skb);
 
-	stats = per_cpu_ptr(lport->stats, get_cpu());
+	stats = per_cpu_ptr(lport->stats, get_cpu_light());
 	if (unlikely(FC_FCOE_DECAPS_VER(hp) != FC_FCOE_VER)) {
 		if (stats->ErrorFrames < 5)
 			printk(KERN_WARNING "fcoe: FCoE version "
Signature · 1 line
2.8.1
author reviewer
#5 lore raw

PATCH 6/10 net: add a lock around icmp_sk()

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

It looks like the this_cpu_ptr() access in icmp_sk() is protected with local_bh_disable(). To avoid missing serialization in -RT I am adding here a local lock. No crash has been observed, this is just precaution.

---
 net/ipv4/icmp.c | 8 ++++++++
 1 file changed, 8 insertions(+)
net/ipv4/icmp.c +8 -0
index be5fd9b81292..d7358c1ac63c 100644
@@ -78,6 +78,7 @@
 #include <linux/string.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/slab.h>
+#include <linux/locallock.h>
 #include <net/snmp.h>
 #include <net/ip.h>
 #include <net/route.h>
@@ -204,6 +205,8 @@ static const struct icmp_control icmp_pointers[NR_ICMP_TYPES+1];
  *
  *	On SMP we have one ICMP socket per-cpu.
  */
+static DEFINE_LOCAL_IRQ_LOCK(icmp_sk_lock);
+
 static struct sock *icmp_sk(struct net *net)
 {
 	return *this_cpu_ptr(net->ipv4.icmp_sk);
@@ -215,12 +218,14 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
 
 	local_bh_disable();
 
+	local_lock(icmp_sk_lock);
 	sk = icmp_sk(net);
 
 	if (unlikely(!spin_trylock(&sk->sk_lock.slock))) {
 		/* This can happen if the output path signals a
 		 * dst_link_failure() for an outgoing ICMP packet.
 		 */
+		local_unlock(icmp_sk_lock);
 		local_bh_enable();
 		return NULL;
 	}
@@ -230,6 +235,7 @@ static inline struct sock *icmp_xmit_lock(struct net *net)
 static inline void icmp_xmit_unlock(struct sock *sk)
 {
 	spin_unlock_bh(&sk->sk_lock.slock);
+	local_unlock(icmp_sk_lock);
 }
 
 int sysctl_icmp_msgs_per_sec __read_mostly = 1000;
@@ -357,6 +363,7 @@ static void icmp_push_reply(struct icmp_bxm *icmp_param,
 	struct sock *sk;
 	struct sk_buff *skb;
 
+	local_lock(icmp_sk_lock);
 	sk = icmp_sk(dev_net((*rt)->dst.dev));
 	if (ip_append_data(sk, fl4, icmp_glue_bits, icmp_param,
 			   icmp_param->data_len+icmp_param->head_len,
@@ -379,6 +386,7 @@ static void icmp_push_reply(struct icmp_bxm *icmp_param,
 		skb->ip_summed = CHECKSUM_NONE;
 		ip_push_pending_frames(sk, fl4);
 	}
+	local_unlock(icmp_sk_lock);
 }
 
 /*
Signature · 1 line
2.8.1
author reviewer
#6 lore raw

PATCH 7/10 fs/dcache: resched/chill only if we make no progress

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

Upstream commit 47be61845c77 ("fs/dcache.c: avoid soft-lockup in dput()") changed the condition _when_ cpu_relax() / cond_resched() was invoked. This change was adapted in -RT into mostly the same thing except that if cond_resched() did nothing we had to do cpu_chill() to force the task off CPU for a tiny little bit in case the task had RT priority and did not want to leave the CPU.
This change resulted in a performance regression (in my testcase the build time on /dev/shm increased from 19min to 24min). The reason is that with this change cpu_chill() was invoked even dput() made progress (dentry_kill() returned a different dentry) instead only if we were trying this operation on the same dentry over and over again.

This patch brings back to the old behavior back to cond_resched() & chill if we make no progress. A little improvement is to invoke cpu_chill() only if we are a RT task (and avoid the sleep otherwise). Otherwise the scheduler should remove us from the CPU if we make no progress.

---
 fs/dcache.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
fs/dcache.c +13 -5
index d96330db7f80..9a6c0a5ec1a3 100644
@@ -40,6 +40,8 @@
 #include <linux/ratelimit.h>
 #include <linux/list_lru.h>
 #include <linux/kasan.h>
+#include <linux/sched/rt.h>
+#include <linux/sched/deadline.h>
 
 #include "internal.h"
 #include "mount.h"
@@ -748,6 +750,8 @@ static inline bool fast_dput(struct dentry *dentry)
  */
 void dput(struct dentry *dentry)
 {
+	struct dentry *parent;
+
 	if (unlikely(!dentry))
 		return;
 
@@ -784,13 +788,17 @@ repeat:
 	return;
 
 kill_it:
-	dentry = dentry_kill(dentry);
-	if (dentry) {
+	parent = dentry_kill(dentry);
+	if (parent) {
 		int r;
 
-		r = cond_resched();
-		if (!r)
-			cpu_chill();
+		if (parent == dentry) {
+			/* the task with the highest priority won't schedule */
+			r = cond_resched();
+			if (!r && (rt_task(current) || dl_task(current)))
+				cpu_chill();
+		} else
+			dentry = parent;
 		goto repeat;
 	}
 }
Signature · 1 line
2.8.1
author reviewer
#7 lore raw

PATCH 8/10 x86/preempt-lazy: fixup should_resched()

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

should_resched() returns true if NEED_RESCHED is set and the preempt_count is 0 _or_ if NEED_RESCHED_LAZY is set ignoring the preempt counter. Ignoring the preemp counter is wrong. This patch adds this into account.
While at it, __preempt_count_dec_and_test() ignores preempt_lazy_count while checking TIF_NEED_RESCHED_LAZY so we this check, too.

---
 arch/x86/include/asm/preempt.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
arch/x86/include/asm/preempt.h +15 -2
index c08949b0314d..eff1b8609f77 100644
@@ -92,6 +92,8 @@ static __always_inline bool __preempt_count_dec_and_test(void)
 	if (____preempt_count_dec_and_test())
 		return true;
 #ifdef CONFIG_PREEMPT_LAZY
+	if (current_thread_info()->preempt_lazy_count)
+		return false;
 	return test_thread_flag(TIF_NEED_RESCHED_LAZY);
 #else
 	return false;
@@ -104,8 +106,19 @@ static __always_inline bool __preempt_count_dec_and_test(void)
 static __always_inline bool should_resched(int preempt_offset)
 {
 #ifdef CONFIG_PREEMPT_LAZY
-	return unlikely(raw_cpu_read_4(__preempt_count) == preempt_offset ||
-			test_thread_flag(TIF_NEED_RESCHED_LAZY));
+	u32 tmp;
+
+	tmp = raw_cpu_read_4(__preempt_count);
+	if (tmp == preempt_offset)
+		return true;
+
+	/* preempt count == 0 ? */
+	tmp &= ~PREEMPT_NEED_RESCHED;
+	if (tmp)
+		return false;
+	if (current_thread_info()->preempt_lazy_count)
+		return false;
+	return test_thread_flag(TIF_NEED_RESCHED_LAZY);
 #else
 	return unlikely(raw_cpu_read_4(__preempt_count) == preempt_offset);
 #endif
Signature · 1 line
2.8.1
author reviewer
#8 lore raw

PATCH 9/10 fs/dcache: incremental fixup of the retry routine

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

It has been pointed out by tglx that on UP the non-RT task could spin its entire time slice because the lock owner is preempted. This won't happen on !RT. So we back to "chill" if we can't cond_resched() did not work.

---
 fs/dcache.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
fs/dcache.c +3 -4
index 9a6c0a5ec1a3..c790b2b070ab 100644
@@ -40,8 +40,6 @@
 #include <linux/ratelimit.h>
 #include <linux/list_lru.h>
 #include <linux/kasan.h>
-#include <linux/sched/rt.h>
-#include <linux/sched/deadline.h>
 
 #include "internal.h"
 #include "mount.h"
@@ -795,10 +793,11 @@ kill_it:
 		if (parent == dentry) {
 			/* the task with the highest priority won't schedule */
 			r = cond_resched();
-			if (!r && (rt_task(current) || dl_task(current)))
+			if (!r)
 				cpu_chill();
-		} else
+		} else {
 			dentry = parent;
+		}
 		goto repeat;
 	}
 }
Signature · 1 line
2.8.1
author reviewer
#9 lore raw

PATCH 10/10 Linux 4.1.33-rt38-rc1

In reply to Steven Rostedt

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: "Steven Rostedt (Red Hat)" <[email protected]>

---
 localversion-rt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
localversion-rt +1 -1
index a3b2408c1da6..625367387621 100644
@@ -1 +1 @@
--rt37
+-rt38-rc1
Signature · 1 line
2.8.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

author reviewer
#11 lore raw

Re: [PATCH RT 05/10] net: add back the missing serialization in ip_send_unicast_reply()

And again. I need to fix quilt mail to handle this.

-- Steve

Quoted 89 lines · On Thu, 22 Sep 2016 17:57:52 -0400 Steven Rostedt <[email protected]> wrote:

4.1.33-rt38-rc1 stable review patch.
If anyone has any objections, please let me know.

------------------

From: Sebastian Andrzej Siewior <[email protected]>

Some time ago Sami PietikÀinen reported a crash on -RT in ip_send_unicast_reply() which was later fixed by Nicholas Mc Guire (v3.12.8-rt11). Later (v3.18.8) the code was reworked and I dropped the patch. As it turns out it was mistake.
I have reports that the same crash is possible with a similar backtrace. It seems that vanilla protects access to this_cpu_ptr() via local_bh_disable(). This does not work the on -RT since we can have NET_RX and NET_TX running in parallel on the same CPU.
This is brings back the old locks.

|Unable to handle kernel NULL pointer dereference at virtual address 00000010
|PC is at __ip_make_skb+0x198/0x3e8
|[<c04e39d8>] (__ip_make_skb) from [<c04e3ca8>] (ip_push_pending_frames+0x20/0x40)
|[<c04e3ca8>] (ip_push_pending_frames) from [<c04e3ff0>] (ip_send_unicast_reply+0x210/0x22c)
|[<c04e3ff0>] (ip_send_unicast_reply) from [<c04fbb54>] (tcp_v4_send_reset+0x190/0x1c0)
|[<c04fbb54>] (tcp_v4_send_reset) from [<c04fcc1c>] (tcp_v4_do_rcv+0x22c/0x288)
|[<c04fcc1c>] (tcp_v4_do_rcv) from [<c0474364>] (release_sock+0xb4/0x150)
|[<c0474364>] (release_sock) from [<c04ed904>] (tcp_close+0x240/0x454)
|[<c04ed904>] (tcp_close) from [<c0511408>] (inet_release+0x74/0x7c)
|[<c0511408>] (inet_release) from [<c0470728>] (sock_release+0x30/0xb0)
|[<c0470728>] (sock_release) from [<c0470abc>] (sock_close+0x1c/0x24)
|[<c0470abc>] (sock_close) from [<c0115ec4>] (__fput+0xe8/0x20c)
|[<c0115ec4>] (__fput) from [<c0116050>] (____fput+0x18/0x1c)
|[<c0116050>] (____fput) from [<c0058138>] (task_work_run+0xa4/0xb8)
|[<c0058138>] (task_work_run) from [<c0011478>] (do_work_pending+0xd0/0xe4)
|[<c0011478>] (do_work_pending) from [<c000e740>] (work_pending+0xc/0x20)
|Code: e3530001 8a000001 e3a00040 ea000011 (e5973010)
---
 net/ipv4/tcp_ipv4.c | 7 +++++++
 1 file changed, 7 insertions(+)
net/ipv4/tcp_ipv4.c +7 -0
index 13b92d595138..6bfa68fb5f21 100644
@@ -62,6 +62,7 @@
 #include <linux/init.h>
 #include <linux/times.h>
 #include <linux/slab.h>
+#include <linux/locallock.h>
 
 #include <net/net_namespace.h>
 #include <net/icmp.h>
@@ -563,6 +564,7 @@ void tcp_v4_send_check(struct sock *sk, struct sk_buff *skb)
 }
 EXPORT_SYMBOL(tcp_v4_send_check);
 
+static DEFINE_LOCAL_IRQ_LOCK(tcp_sk_lock);
 /*
  *	This routine will send an RST to the other tcp.
  *
@@ -684,10 +686,13 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb)
 		arg.bound_dev_if = sk->sk_bound_dev_if;
 
 	arg.tos = ip_hdr(skb)->tos;
+
+	local_lock(tcp_sk_lock);
 	ip_send_unicast_reply(*this_cpu_ptr(net->ipv4.tcp_sk),
 			      skb, &TCP_SKB_CB(skb)->header.h4.opt,
 			      ip_hdr(skb)->saddr, ip_hdr(skb)->daddr,
 			      &arg, arg.iov[0].iov_len);
+	local_unlock(tcp_sk_lock);
 
 	TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS);
 	TCP_INC_STATS_BH(net, TCP_MIB_OUTRSTS);
@@ -769,10 +774,12 @@ static void tcp_v4_send_ack(struct net *net,
 	if (oif)
 		arg.bound_dev_if = oif;
 	arg.tos = tos;
+	local_lock(tcp_sk_lock);
 	ip_send_unicast_reply(*this_cpu_ptr(net->ipv4.tcp_sk),
 			      skb, &TCP_SKB_CB(skb)->header.h4.opt,
 			      ip_hdr(skb)->saddr, ip_hdr(skb)->daddr,
 			      &arg, arg.iov[0].iov_len);
+	local_unlock(tcp_sk_lock);
 
 	TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS);
 }