[PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

[PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

[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.

Signed-off-by: Tejaswi Tanikella <tejaswit@codeaurora.org>
---
 include/linux/igmp.h |  1 +
 net/ipv4/igmp.c      | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/linux/igmp.h b/include/linux/igmp.h
index f823185..9be1c58 100644
--- a/include/linux/igmp.h
+++ b/include/linux/igmp.h
@@ -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;
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index b26a81a..c695e2c 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -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;
-- 
1.9.1

Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

[Florian Fainelli]

On 06/01/2018 07:05 AM, Tejaswi Tanikella wrote:
> 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.

> 
> Signed-off-by: Tejaswi Tanikella <tejaswit@codeaurora.org>
> ---
>  include/linux/igmp.h |  1 +b
>  net/ipv4/igmp.c      | 20 ++++++++++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/igmp.h b/include/linux/igmp.h
> index f823185..9be1c58 100644
> --- a/include/linux/igmp.h
> +++ b/include/linux/igmp.h
> @@ -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]

> @@ -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.
-- 
Florian

Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

[Tejaswi Tanikella]

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.
> 
> 
> On 06/01/2018 07:05 AM, Tejaswi Tanikella wrote:
> > 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?
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 ?


> For instance, I could imagine ARP suffering from the same short comings...
I am not sure if ARP will be effected.
>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 ?


> 
> There is one thing that needs fixing though, see below.
Thanks, I'll fix them in the next patch.
> 
> > 
> > Signed-off-by: Tejaswi Tanikella <tejaswit@codeaurora.org>
> > ---
> >  include/linux/igmp.h |  1 +b
> >  net/ipv4/igmp.c      | 20 ++++++++++++++++++--
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/igmp.h b/include/linux/igmp.h
> > index f823185..9be1c58 100644
> > --- a/include/linux/igmp.h
> > +++ b/include/linux/igmp.h
> > @@ -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]
> 
> > @@ -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.
> -- 
> Florian

Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

[David Miller]

From: Tejaswi Tanikella <tejaswit@codeaurora.org>
Date: Fri, 1 Jun 2018 19:35:41 +0530

> 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 <tejaswit@codeaurora.org>

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.

Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports

[Tejaswi Tanikella]

On Mon, Jun 04, 2018 at 11:06:40AM -0400, David Miller wrote:
> From: Tejaswi Tanikella <tejaswit@codeaurora.org>
> Date: Fri, 1 Jun 2018 19:35:41 +0530
> 
> > 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 <tejaswit@codeaurora.org>
> 
> 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.

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.