Skip to contentSkip to conversation

ipv4: igmp: hold wakelock to prevent delayed reports

PATCH In review Tejaswi Tanikella 3 participants 5 messages Jun 1, 2018 – Jun 6, 2018

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Tejaswi Tanikella

author
#0 lore raw

PATCH 1/1 ipv4: igmp: hold wakelock to prevent delayed reports

On receiving a IGMPv2/v3 query, based on max_delay set in the header a timer is started to send out a response after a random time within max_delay. If the system then moves into suspend state, Report is delayed until system wakes up.

In one reported scenario, on arm64 devices, max_delay was set to 10s, Reports were consistantly delayed if the timer is scheduled after 5 plus seconds.

Hold wakelock while starting the timer to prevent moving into suspend state.

Signed-off-by Tejaswi Tanikella <[email protected]>
---
 include/linux/igmp.h |  1 +
 net/ipv4/igmp.c      | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)
include/linux/igmp.h +1 -0
index f823185..9be1c58 100644
@@ -84,6 +84,7 @@ struct ip_mc_list {
 	};
 	struct ip_mc_list __rcu *next_hash;
 	struct timer_list	timer;
+	struct wakeup_source	*wakeup_src;
 	int			users;
 	refcount_t		refcnt;
 	spinlock_t		lock;
net/ipv4/igmp.c +18 -2
index b26a81a..c695e2c 100644
@@ -107,6 +107,9 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #endif
+#ifdef CONFIG_IP_MULTICAST
+#include <linux/pm_wakeup.h>
+#endif
 
 #ifdef CONFIG_IP_MULTICAST
 /* Parameter names and values are taken from igmp-v2-06 draft */

@@ -174,7 +177,13 @@ static int ip_mc_add_src(struct in_device *in_dev, __be32 *pmca, int sfmode,
 
 static void ip_ma_put(struct ip_mc_list *im)
 {
+#ifdef CONFIG_IP_MULTICAST
+	__pm_relax(im->wakeup_src);
+#endif
 	if (refcount_dec_and_test(&im->refcnt)) {
+#ifdef CONFIG_IP_MULTICAST
+		wakeup_source_unregister(im->wakeup_src);
+#endif
 		in_dev_put(im->interface);
 		kfree_rcu(im, rcu);
 	}

@@ -199,8 +208,10 @@ static void ip_ma_put(struct ip_mc_list *im)
 static void igmp_stop_timer(struct ip_mc_list *im)
 {
 	spin_lock_bh(&im->lock);
-	if (del_timer(&im->timer))
+	if (del_timer(&im->timer)) {
+		__pm_relax(im->wakeup_src);
 		refcount_dec(&im->refcnt);
+	}
 	im->tm_running = 0;
 	im->reporter = 0;
 	im->unsolicit_count = 0;

@@ -213,8 +224,10 @@ static void igmp_start_timer(struct ip_mc_list *im, int max_delay)
 	int tv = prandom_u32() % max_delay;
 
 	im->tm_running = 1;
-	if (!mod_timer(&im->timer, jiffies+tv+2))
+	if (!mod_timer(&im->timer, jiffies+tv+2)) {
 		refcount_inc(&im->refcnt);
+		__pm_stay_awake(im->wakeup_src);
+	}
 }
 
 static void igmp_gq_start_timer(struct in_device *in_dev)

@@ -250,6 +263,7 @@ static void igmp_mod_timer(struct ip_mc_list *im, int max_delay)
 			spin_unlock_bh(&im->lock);
 			return;
 		}
+		__pm_relax(im->wakeup_src);
 		refcount_dec(&im->refcnt);
 	}
 	igmp_start_timer(im, max_delay);

@@ -1415,6 +1429,8 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
 #ifdef CONFIG_IP_MULTICAST
 	timer_setup(&im->timer, igmp_timer_expire, 0);
 	im->unsolicit_count = net->ipv4.sysctl_igmp_qrv;
+	im->wakeup_src = wakeup_source_create("igmp_wakeup_source");
+	wakeup_source_add(im->wakeup_src);
 #endif
 
 	im->next_rcu = in_dev->mc_list;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 11 lines · Tejaswi Tanikella

On receiving a IGMPv2/v3 query, based on max_delay set in the header a timer is started to send out a response after a random time within max_delay. If the system then moves into suspend state, Report is delayed until system wakes up.

In one reported scenario, on arm64 devices, max_delay was set to 10s, Reports were consistantly delayed if the timer is scheduled after 5 plus seconds.

Hold wakelock while starting the timer to prevent moving into suspend state.

I suppose this looks fine, but are not you going to be playing whack-a-mole through the network stack wherever there are similar patterns? Is not a better solution to move this to in_dev_get()/in_dev_put() where you could create a proper wakelock per network device? For instance, I could imagine ARP suffering from the same short comings...

There is one thing that needs fixing though, see below.

Quoted 12 lines
Signed-off-by Tejaswi Tanikella <[email protected]>

---
include/linux/igmp.h | 1 +b

 net/ipv4/igmp.c      | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)
include/linux/igmp.h +0 -0
index f823185..9be1c58 100644

include/linux/igmp.h struct ip_mc_list { · line 87

Quoted 4 lines
include/linux/igmp.h +1 -0
@@ -84,6 +84,7 @@ struct ip_mc_list {
 	};
 	struct ip_mc_list __rcu *next_hash;
 	struct timer_list	timer;
+	struct wakeup_source	*wakeup_src;

Since you are using this only when CONFIG_IP_MULTICAST is defined, you might was well save a few bytes by making this enclosed within an ifdef CONFIG_IP_MULTICAST here as well?

[snip]

include/linux/igmp.h void ip_mc_inc_group(struct in_device *in_dev, __be32 addr) · line 1432

Quoted 5 lines
include/linux/igmp.h +1 -0
@@ -1415,6 +1429,8 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
 #ifdef CONFIG_IP_MULTICAST
 	timer_setup(&im->timer, igmp_timer_expire, 0);
 	im->unsolicit_count = net->ipv4.sysctl_igmp_qrv;
+	im->wakeup_src = wakeup_source_create("igmp_wakeup_source");

Missing error checking, wakeup_source_create() can return NULL here.

Signature · 1 line
Florian
author
#2 lore raw

On Fri, Jun 01, 2018 at 07:45:16AM -0700, Florian Fainelli wrote:

Thank you Florian for reviewing my patch.
I had a few questions.

Quoted 12 lines · Tejaswi Tanikella
Quoted 9 lines

On receiving a IGMPv2/v3 query, based on max_delay set in the header a timer is started to send out a response after a random time within max_delay. If the system then moves into suspend state, Report is delayed until system wakes up.

In one reported scenario, on arm64 devices, max_delay was set to 10s, Reports were consistantly delayed if the timer is scheduled after 5 plus seconds.

Quoted 8 lines
Quoted 2 lines

Hold wakelock while starting the timer to prevent moving into suspend state.

I suppose this looks fine, but are not you going to be playing whack-a-mole through the network stack wherever there are similar patterns? Is not a better solution to move this to in_dev_get()/in_dev_put() where you could create a proper wakelock per network device?

I see that in_dev_get()/in_dev_put() are being used by some drivers. won't addition of a wakelock be unnecessary for them? Will adding two independent functions per in_dev to hold and release wakelock be sufficient ?

Quoted 1 line

For instance, I could imagine ARP suffering from the same short comings...

I am not sure if ARP will be effected.

Quoted 1 line

From my understanding only systems with timers which trigger sending out

responses should be affected. For example IGMP and MLD must send reports for the router to fwd mcast packets.
In ARP for instance, only moving between states will be delayed. Is there something that I am missing ?

Quoted 2 lines

There is one thing that needs fixing though, see below.

Thanks, I'll fix them in the next patch.

Quoted 33 lines
Quoted 13 lines
Signed-off-by Tejaswi Tanikella <[email protected]>

---
include/linux/igmp.h | 1 +b

 net/ipv4/igmp.c      | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)
include/linux/igmp.h +0 -0
index f823185..9be1c58 100644

include/linux/igmp.h struct ip_mc_list { · line 87

Quoted 4 lines
include/linux/igmp.h +1 -0
@@ -84,6 +84,7 @@ struct ip_mc_list {
 	};
 	struct ip_mc_list __rcu *next_hash;
 	struct timer_list	timer;
+	struct wakeup_source	*wakeup_src;

Since you are using this only when CONFIG_IP_MULTICAST is defined, you might was well save a few bytes by making this enclosed within an ifdef CONFIG_IP_MULTICAST here as well?

[snip]

include/linux/igmp.h void ip_mc_inc_group(struct in_device *in_dev, __be32 addr) · line 1432

Quoted 5 lines
include/linux/igmp.h +1 -0
@@ -1415,6 +1429,8 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
 #ifdef CONFIG_IP_MULTICAST
 	timer_setup(&im->timer, igmp_timer_expire, 0);
 	im->unsolicit_count = net->ipv4.sysctl_igmp_qrv;
+	im->wakeup_src = wakeup_source_create("igmp_wakeup_source");

Missing error checking, wakeup_source_create() can return NULL here.

Signature · 1 line
Florian
maintainer
#3 lore raw
In reply to Tejaswi Tanikella

From: Tejaswi Tanikella <[email protected]>
Date: Fri, 1 Jun 2018 19:35:41 +0530

Quoted 5 lines

On receiving a IGMPv2/v3 query, based on max_delay set in the header a timer is started to send out a response after a random time within max_delay. If the system then moves into suspend state, Report is delayed until system wakes up.

Quoted 8 lines

In one reported scenario, on arm64 devices, max_delay was set to 10s, Reports were consistantly delayed if the timer is scheduled after 5 plus seconds.

Hold wakelock while starting the timer to prevent moving into suspend state.

Signed-off-by Tejaswi Tanikella <[email protected]>

As Florian stated, this won't be the only networking facility to hit a problem like this. So, if we go down this route, we probably want to generically solve this.

But I have a deeper concern.

Do we really want every timer based querying mechanism to prevent a system from being able to suspend?

We get to the point where external entities can generate traffic which can prevent a remote system from entering suspend state.

author
#4 lore raw
Quoted 21 lines · David Miller

From: Tejaswi Tanikella <[email protected]>
Date: Fri, 1 Jun 2018 19:35:41 +0530

Quoted 5 lines

On receiving a IGMPv2/v3 query, based on max_delay set in the header a timer is started to send out a response after a random time within max_delay. If the system then moves into suspend state, Report is delayed until system wakes up.

Quoted 8 lines

In one reported scenario, on arm64 devices, max_delay was set to 10s, Reports were consistantly delayed if the timer is scheduled after 5 plus seconds.

Hold wakelock while starting the timer to prevent moving into suspend state.

Signed-off-by Tejaswi Tanikella <[email protected]>

As Florian stated, this won't be the only networking facility to hit a problem like this. So, if we go down this route, we probably want to generically solve this.

Quoted 7 lines

But I have a deeper concern.

Do we really want every timer based querying mechanism to prevent a system from being able to suspend?

We get to the point where external entities can generate traffic which can prevent a remote system from entering suspend state.

Like you suggested maybe aquiring a wakelock will unnecessarily stop the system from suspending.
Would using alarmtimer be better? It seems similar to a hrtimer but can wake the system up from suspend.

This should allow the system to suspend till the timer expires. But might cause repeated wake-ups and suspends.