diff mbox series

[net,1/2] ipv4: igmp: use alarmtimer to prevent delayed reports

Message ID 20180611115058.GA12452@tejaswit-linux.qualcomm.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series [net,1/2] ipv4: igmp: use alarmtimer to prevent delayed reports | expand

Commit Message

Tejaswi Tanikella June 11, 2018, 11:51 a.m. UTC 
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.

Use a alarmtimer instead of using a timer. Alarmtimer will wake the
system up from suspend to send out the IGMP report.

Signed-off-by: Tejaswi Tanikella <tejaswit@codeaurora.org>
---
v2: use alarmtimer instead of wakelock.
---
If these changes are fine, I'll share similar patches for MLD and ARP.
---
 include/linux/igmp.h |  7 ++++++-
 net/ipv4/igmp.c      | 27 ++++++++++++++++-----------
 2 files changed, 22 insertions(+), 12 deletions(-)

Comments

kernel test robot June 11, 2018, 1:25 p.m. UTC | #1 
Hi Tejaswi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Tejaswi-Tanikella/ipv4-igmp-use-alarmtimer-to-prevent-delayed-reports/20180611-195615
config: x86_64-randconfig-x003-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/ipv4/igmp.c: In function 'igmp_mc_seq_show':
>> net/ipv4/igmp.c:2819:11: error: implicit declaration of function 'ktime_to_jiffies'; did you mean 'timeval_to_jiffies'? [-Werror=implicit-function-declaration]
      delta = ktime_to_jiffies(alarm_expires_remaining(&im->alarm));
              ^~~~~~~~~~~~~~~~
              timeval_to_jiffies
>> net/ipv4/igmp.c:2819:28: error: implicit declaration of function 'alarm_expires_remaining'; did you mean 'hrtimer_expires_remaining'? [-Werror=implicit-function-declaration]
      delta = ktime_to_jiffies(alarm_expires_remaining(&im->alarm));
                               ^~~~~~~~~~~~~~~~~~~~~~~
                               hrtimer_expires_remaining
>> net/ipv4/igmp.c:2819:55: error: 'struct ip_mc_list' has no member named 'alarm'
      delta = ktime_to_jiffies(alarm_expires_remaining(&im->alarm));
                                                          ^~
   cc1: some warnings being treated as errors

vim +2819 net/ipv4/igmp.c

  2813	
  2814			if (rcu_access_pointer(state->in_dev->mc_list) == im) {
  2815				seq_printf(seq, "%d\t%-10s: %5d %7s\n",
  2816					   state->dev->ifindex, state->dev->name, state->in_dev->mc_count, querier);
  2817			}
  2818	
> 2819			delta = ktime_to_jiffies(alarm_expires_remaining(&im->alarm));
  2820			seq_printf(seq,
  2821				   "\t\t\t\t%08X %5d %d:%08lX\t\t%d\n",
  2822				   im->multiaddr, im->users,
  2823				   im->tm_running,
  2824				   im->tm_running ? jiffies_delta_to_clock_t(delta) : 0,
  2825				   im->reporter);
  2826		}
  2827		return 0;
  2828	}
  2829	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot June 11, 2018, 1:25 p.m. UTC | #2 
Hi Tejaswi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Tejaswi-Tanikella/ipv4-igmp-use-alarmtimer-to-prevent-delayed-reports/20180611-195615
config: i386-randconfig-x012-201823 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the linux-review/Tejaswi-Tanikella/ipv4-igmp-use-alarmtimer-to-prevent-delayed-reports/20180611-195615 HEAD 20987eecc3144eb22578baea09ff017ddcb45163 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   net//ipv4/igmp.c: In function 'igmp_start_timer':
>> net//ipv4/igmp.c:213:19: error: implicit declaration of function 'jiffies_to_ktime'; did you mean 'jiffies_to_timeval'? [-Werror=implicit-function-declaration]
     ktime_t expiry = jiffies_to_ktime(prandom_u32() % max_delay + 2);
                      ^~~~~~~~~~~~~~~~
                      jiffies_to_timeval
   net//ipv4/igmp.c: In function 'igmp_mod_timer':
   net//ipv4/igmp.c:250:7: error: implicit declaration of function 'ktime_to_jiffies'; did you mean 'timeval_to_jiffies'? [-Werror=implicit-function-declaration]
      if (ktime_to_jiffies(expiry) < max_delay) {
          ^~~~~~~~~~~~~~~~
          timeval_to_jiffies
   cc1: some warnings being treated as errors

vim +213 net//ipv4/igmp.c

   209	
   210	/* It must be called with locked im->lock */
   211	static void igmp_start_timer(struct ip_mc_list *im, int max_delay)
   212	{
 > 213		ktime_t expiry = jiffies_to_ktime(prandom_u32() % max_delay + 2);
   214	
   215		im->tm_running = 1;
   216		alarm_start_relative(&im->alarm, expiry);
   217		refcount_inc(&im->refcnt);
   218	}
   219	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Andrew Lunn June 12, 2018, 4:28 p.m. UTC | #3 
On Mon, Jun 11, 2018 at 05:21:05PM +0530, 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.
> 
> Use a alarmtimer instead of using a timer. Alarmtimer will wake the
> system up from suspend to send out the IGMP report.

Hi Tejaswi

I think i must be missing something here. If we are suspended, we are
not receiving multicast frames. If we are not receiving frames, why do
we need to reply to the query?

Once we resume, i expect we will reply to the next query. You could
optimise restarting the flow by immediately sending a membership
report, same as when the setsockopt is used to join the group.

	Andrew
Tejaswi Tanikella June 13, 2018, 1:32 p.m. UTC | #4 
On Tue, Jun 12, 2018 at 06:28:18PM +0200, Andrew Lunn wrote:
> On Mon, Jun 11, 2018 at 05:21:05PM +0530, 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.
> > 
> > Use a alarmtimer instead of using a timer. Alarmtimer will wake the
> > system up from suspend to send out the IGMP report.
> 
> Hi Tejaswi
> 
> I think i must be missing something here. If we are suspended, we are
> not receiving multicast frames. If we are not receiving frames, why do
> we need to reply to the query?
> 
> Once we resume, i expect we will reply to the next query. You could
> optimise restarting the flow by immediately sending a membership
> report, same as when the setsockopt is used to join the group.
> 
> 	Andrew

Hi Andrew,


I'll try to explain my scenario. This was observed on a arm64 device.
An application registers for a mcast group, and just listens to mcast
packets. The connection is setup and mcast packets are being forwarded
by the router. Multicast packets are sent out every few minutes.
Not a very busy connection.

After some time the router sends out a IGMPv2 query. The max delay in
the header is set to 10s. The system starts a timer to send out the
response at 9s. But the device suspends and wakes up after 60s.
The response is sent out 50s late.

ftrace logs with boottime trace_clock and my igmp_logs:

4740693      kworker/0:3-395   [000] ..s2  4716.425695: igmp_log: skbaddr=ffffffc156fe6600 daddr=224.0.0.1, id=0, rc=4295217721
4740694      kworker/0:3-395   [000] d.s4  4716.425717: timer_start:timer=ffffffc217763140 function=igmp_timer_expire expires=4295218678 [timeout=957] flags=0x00000000
        timer set for 9.57 seconds.  957 jiffies

< device suspends >

< wakes up after ~60s >
4781289           <idle>-0     [000] .ns2  4779.170886: timer_expire_entry: timer=ffffffc217763140 function=igmp_timer_expire now=4295218678
4781290           <idle>-0     [000] .ns2  4779.171045: igmp_log: skbaddr=ffffffc1559d0e00 daddr=227.226.228.225, id=0, rc=4295218678

Since the response was delayed, mcast packets are not forwarded by the
router.

My changes use a alarmtimer, this will wake the system up if the timer
expires.

Thanks,
Tejaswi
Andrew Lunn June 13, 2018, 2:44 p.m. UTC | #5 
> I'll try to explain my scenario. This was observed on a arm64 device.
> An application registers for a mcast group, and just listens to mcast
> packets. The connection is setup and mcast packets are being forwarded
> by the router. Multicast packets are sent out every few minutes.
> Not a very busy connection.
> 
> After some time the router sends out a IGMPv2 query. The max delay in
> the header is set to 10s. The system starts a timer to send out the
> response at 9s. But the device suspends and wakes up after 60s.
>
> The response is sent out 50s late.
> 
> ftrace logs with boottime trace_clock and my igmp_logs:
> 
> 4740693      kworker/0:3-395   [000] ..s2  4716.425695: igmp_log: skbaddr=ffffffc156fe6600 daddr=224.0.0.1, id=0, rc=4295217721
> 4740694      kworker/0:3-395   [000] d.s4  4716.425717: timer_start:timer=ffffffc217763140 function=igmp_timer_expire expires=4295218678 [timeout=957] flags=0x00000000
>         timer set for 9.57 seconds.  957 jiffies
> 
> < device suspends >
> 
> < wakes up after ~60s >
> 4781289           <idle>-0     [000] .ns2  4779.170886: timer_expire_entry: timer=ffffffc217763140 function=igmp_timer_expire now=4295218678
> 4781290           <idle>-0     [000] .ns2  4779.171045: igmp_log: skbaddr=ffffffc1559d0e00 daddr=227.226.228.225, id=0, rc=4295218678
> 
> Since the response was delayed, mcast packets are not forwarded by the
> router.

While it has been asleep, it has also been dropping any multicast
traffic in the stream. So it does not really matter it has left the
group. You were not receiving the packets anyway.

Thing about this from another angle. I have an NTP client running on
my laptop, using multicast address 224.0.1.1. I suspend my laptop and
walk away for two hours. When i come back, i find that 20 seconds
after i suspended it, it resumed and send an group response
message. And an hour later, since it was still running, the battery
went flat.

It seems to me, the change you are proposing cannot be the default
behaviour.

I actually think you need to be looking at some sort of WoL feature.
You need the multicast stream data packets to wake you, and you also
need to wake up the IGMP query message. And you need to wake up to
send the group membership. Does your hardware have this sort of WoL
support? You can then explicitly enable this WoL for your application.

	Andrew
Tejaswi Tanikella June 14, 2018, 1:14 p.m. UTC | #6 
On Wed, Jun 13, 2018 at 04:44:37PM +0200, Andrew Lunn wrote:
> While it has been asleep, it has also been dropping any multicast
> traffic in the stream. So it does not really matter it has left the
> group. You were not receiving the packets anyway.
> 
> Thing about this from another angle. I have an NTP client running on
> my laptop, using multicast address 224.0.1.1. I suspend my laptop and
> walk away for two hours. When i come back, i find that 20 seconds
> after i suspended it, it resumed and send an group response
> message. And an hour later, since it was still running, the battery
> went flat.
> 
> It seems to me, the change you are proposing cannot be the default
> behaviour.
> 
> I actually think you need to be looking at some sort of WoL feature.
> You need the multicast stream data packets to wake you, and you also
> need to wake up the IGMP query message. And you need to wake up to
> send the group membership. Does your hardware have this sort of WoL
> support? You can then explicitly enable this WoL for your application.
> 
> 	Andrew

Thanks Andrew.
You are right, this should not be the default behaviour.

-Tejaswi
diff mbox series

Patch

diff --git a/include/linux/igmp.h b/include/linux/igmp.h
index f823185..45852eb 100644
--- a/include/linux/igmp.h
+++ b/include/linux/igmp.h
@@ -20,6 +20,9 @@ 
 #include <linux/in.h>
 #include <linux/refcount.h>
 #include <uapi/linux/igmp.h>
+#ifdef CONFIG_IP_MULTICAST
+#include <linux/alarmtimer.h>
+#endif
 
 static inline struct igmphdr *igmp_hdr(const struct sk_buff *skb)
 {
@@ -83,7 +86,9 @@  struct ip_mc_list {
 		struct ip_mc_list __rcu *next_rcu;
 	};
 	struct ip_mc_list __rcu *next_hash;
-	struct timer_list	timer;
+#ifdef CONFIG_IP_MULTICAST
+	struct alarm		alarm;
+#endif
 	int			users;
 	refcount_t		refcnt;
 	spinlock_t		lock;
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 85b617b..c30b5c4 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -199,7 +199,7 @@  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 (alarm_cancel(&im->alarm))
 		refcount_dec(&im->refcnt);
 	im->tm_running = 0;
 	im->reporter = 0;
@@ -210,11 +210,11 @@  static void igmp_stop_timer(struct ip_mc_list *im)
 /* It must be called with locked im->lock */
 static void igmp_start_timer(struct ip_mc_list *im, int max_delay)
 {
-	int tv = prandom_u32() % max_delay;
+	ktime_t expiry = jiffies_to_ktime(prandom_u32() % max_delay + 2);
 
 	im->tm_running = 1;
-	if (!mod_timer(&im->timer, jiffies+tv+2))
-		refcount_inc(&im->refcnt);
+	alarm_start_relative(&im->alarm, expiry);
+	refcount_inc(&im->refcnt);
 }
 
 static void igmp_gq_start_timer(struct in_device *in_dev)
@@ -241,11 +241,14 @@  static void igmp_ifc_start_timer(struct in_device *in_dev, int delay)
 
 static void igmp_mod_timer(struct ip_mc_list *im, int max_delay)
 {
+	ktime_t expiry;
+
 	spin_lock_bh(&im->lock);
 	im->unsolicit_count = 0;
-	if (del_timer(&im->timer)) {
-		if ((long)(im->timer.expires-jiffies) < max_delay) {
-			add_timer(&im->timer);
+	expiry = alarm_expires_remaining(&im->alarm);
+	if (alarm_cancel(&im->alarm)) {
+		if (ktime_to_jiffies(expiry) < max_delay) {
+			alarm_start_relative(&im->alarm, expiry);
 			im->tm_running = 1;
 			spin_unlock_bh(&im->lock);
 			return;
@@ -812,9 +815,9 @@  static void igmp_ifc_event(struct in_device *in_dev)
 }
 
 
-static void igmp_timer_expire(struct timer_list *t)
+enum alarmtimer_restart igmp_timer_expire(struct alarm *alarm, ktime_t now)
 {
-	struct ip_mc_list *im = from_timer(im, t, timer);
+	struct ip_mc_list *im = container_of(alarm, struct ip_mc_list, alarm);
 	struct in_device *in_dev = im->interface;
 
 	spin_lock(&im->lock);
@@ -835,6 +838,8 @@  static void igmp_timer_expire(struct timer_list *t)
 		igmp_send_report(in_dev, im, IGMPV3_HOST_MEMBERSHIP_REPORT);
 
 	ip_ma_put(im);
+
+	return ALARMTIMER_NORESTART;
 }
 
 /* mark EXCLUDE-mode sources */
@@ -1413,7 +1418,7 @@  void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
 	refcount_set(&im->refcnt, 1);
 	spin_lock_init(&im->lock);
 #ifdef CONFIG_IP_MULTICAST
-	timer_setup(&im->timer, igmp_timer_expire, 0);
+	alarm_init(&im->alarm, ALARM_BOOTTIME, igmp_timer_expire);
 	im->unsolicit_count = net->ipv4.sysctl_igmp_qrv;
 #endif
 
@@ -2811,7 +2816,7 @@  static int igmp_mc_seq_show(struct seq_file *seq, void *v)
 				   state->dev->ifindex, state->dev->name, state->in_dev->mc_count, querier);
 		}
 
-		delta = im->timer.expires - jiffies;
+		delta = ktime_to_jiffies(alarm_expires_remaining(&im->alarm));
 		seq_printf(seq,
 			   "\t\t\t\t%08X %5d %d:%08lX\t\t%d\n",
 			   im->multiaddr, im->users,