Patchwork [1/2] net: igmp: Reduce Unsolicited report interval to 1s when using IGMPv3

login
register
mail settings
Submitter William Manley
Date July 25, 2013, 12:14 p.m.
Message ID <1374754445-5608-1-git-send-email-william.manley@youview.com>
Download mbox | patch
Permalink /patch/261679/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

William Manley - July 25, 2013, 12:14 p.m.
If an IGMP join packet is lost you will not receive data sent to the
multicast group so if no data arrives from that multicast group in a
period of time after the IGMP join a second IGMP join will be sent.  The
delay between joins is the "IGMP Unsolicited Report Interval".

Previously this value was hard coded to be chosen randomly between 0-10s.
This can be too long for some use-cases, such as IPTV as it can cause
channel change to be slow in the presence of packet loss.

The value 10s has come from IGMPv2 RFC2236, which was reduced to 1s in
IGMPv3 RFC3376.  This patch makes the kernel use the 1s value from the
later RFC if we are operating in IGMPv3 mode.  IGMPv2 behaviour is
unaffected.

Tested with Wireshark and a simple program to join a (non-existent)
multicast group.  The distribution of timings for the second join differ
based upon setting /proc/sys/net/ipv4/conf/eth0/force_igmp_version.

Signed-off-by: William Manley <william.manley@youview.com>
---
 net/ipv4/igmp.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Hannes Frederic Sowa - July 26, 2013, 4:32 p.m.
On Thu, Jul 25, 2013 at 01:14:04PM +0100, William Manley wrote:
> If an IGMP join packet is lost you will not receive data sent to the
> multicast group so if no data arrives from that multicast group in a
> period of time after the IGMP join a second IGMP join will be sent.  The
> delay between joins is the "IGMP Unsolicited Report Interval".
> 
> Previously this value was hard coded to be chosen randomly between 0-10s.
> This can be too long for some use-cases, such as IPTV as it can cause
> channel change to be slow in the presence of packet loss.
> 
> The value 10s has come from IGMPv2 RFC2236, which was reduced to 1s in
> IGMPv3 RFC3376.  This patch makes the kernel use the 1s value from the
> later RFC if we are operating in IGMPv3 mode.  IGMPv2 behaviour is
> unaffected.
> 
> Tested with Wireshark and a simple program to join a (non-existent)
> multicast group.  The distribution of timings for the second join differ
> based upon setting /proc/sys/net/ipv4/conf/eth0/force_igmp_version.
> 
> Signed-off-by: William Manley <william.manley@youview.com>
> ---
>  net/ipv4/igmp.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
> index d8c2327..12ffc2b 100644
> --- a/net/ipv4/igmp.c
> +++ b/net/ipv4/igmp.c
> @@ -113,7 +113,8 @@
>  
>  #define IGMP_V1_Router_Present_Timeout		(400*HZ)
>  #define IGMP_V2_Router_Present_Timeout		(400*HZ)
> -#define IGMP_Unsolicited_Report_Interval	(10*HZ)
> +#define IGMP_V2_Unsolicited_Report_Interval	(10*HZ)
> +#define IGMP_V3_Unsolicited_Report_Interval	(1*HZ)
>  #define IGMP_Query_Response_Interval		(10*HZ)
>  #define IGMP_Unsolicited_Report_Count		2
>  
> @@ -138,6 +139,14 @@
>  	 ((in_dev)->mr_v2_seen && \
>  	  time_before(jiffies, (in_dev)->mr_v2_seen)))
>  
> +static int unsolicited_report_interval(struct in_device *in_dev)
> +{
> +	if (IGMP_V2_SEEN(in_dev))
> +		return IGMP_V2_Unsolicited_Report_Interval;
> +	else /* v3 */
> +		return IGMP_V3_Unsolicited_Report_Interval;
> +}

if (IGMP_V1_SEEN(in_dev) || IGMP_V2_SEEN(in_dev))

Otherwise I am fine with it.

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - July 26, 2013, 4:39 p.m.
On Fri, Jul 26, 2013 at 06:32:39PM +0200, Hannes Frederic Sowa wrote:
> On Thu, Jul 25, 2013 at 01:14:04PM +0100, William Manley wrote:
> > If an IGMP join packet is lost you will not receive data sent to the
> > multicast group so if no data arrives from that multicast group in a
> > period of time after the IGMP join a second IGMP join will be sent.  The
> > delay between joins is the "IGMP Unsolicited Report Interval".
> > 
> > Previously this value was hard coded to be chosen randomly between 0-10s.
> > This can be too long for some use-cases, such as IPTV as it can cause
> > channel change to be slow in the presence of packet loss.
> > 
> > The value 10s has come from IGMPv2 RFC2236, which was reduced to 1s in
> > IGMPv3 RFC3376.  This patch makes the kernel use the 1s value from the
> > later RFC if we are operating in IGMPv3 mode.  IGMPv2 behaviour is
> > unaffected.
> > 
> > Tested with Wireshark and a simple program to join a (non-existent)
> > multicast group.  The distribution of timings for the second join differ
> > based upon setting /proc/sys/net/ipv4/conf/eth0/force_igmp_version.
> >
> > [...]
> >
>
> [...]
> 
> Otherwise I am fine with it.

Also, could you have a look at IPv6, too? We currently use a hardcoded
IGMP6_UNSOLICITED_IVAL = 10*HZ there.

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
William Manley - July 29, 2013, 2:39 p.m.
On 26/07/13 17:39, Hannes Frederic Sowa wrote:
> On Fri, Jul 26, 2013 at 06:32:39PM +0200, Hannes Frederic Sowa wrote:
>> On Thu, Jul 25, 2013 at 01:14:04PM +0100, William Manley wrote:
>>> If an IGMP join packet is lost you will not receive data sent to the
>>> multicast group so if no data arrives from that multicast group in a
>>> period of time after the IGMP join a second IGMP join will be sent.  The
>>> delay between joins is the "IGMP Unsolicited Report Interval".
>>>
>>> Previously this value was hard coded to be chosen randomly between 0-10s.
>>> This can be too long for some use-cases, such as IPTV as it can cause
>>> channel change to be slow in the presence of packet loss.
>>>
>>> The value 10s has come from IGMPv2 RFC2236, which was reduced to 1s in
>>> IGMPv3 RFC3376.  This patch makes the kernel use the 1s value from the
>>> later RFC if we are operating in IGMPv3 mode.  IGMPv2 behaviour is
>>> unaffected.
>>>
>>> Tested with Wireshark and a simple program to join a (non-existent)
>>> multicast group.  The distribution of timings for the second join differ
>>> based upon setting /proc/sys/net/ipv4/conf/eth0/force_igmp_version.
>>>
>>> [...]
>>>
>>
>> [...]
>>
>> Otherwise I am fine with it.
>
> Also, could you have a look at IPv6, too? We currently use a hardcoded
> IGMP6_UNSOLICITED_IVAL = 10*HZ there.

I'll have a look, but I have to admit my ignorance on how multicast 
works with IPv6 and I may have difficulty setting up a test environment.

It seems that IGMP has been replaced with "Multicast Listener Discovery" 
on top of ICMPv6 and much like IGMP there is more than one version:

1. RFC2710 - MLD - Unsolicited Report Interval = 10s
2. RFC3810 - MLDv2 - Unsolicited Report Interval = 1s

There also seems to be a /proc/sys/net/ipv6/conf/all/force_mld_version. 
  Ok, so maybe it's not so difficult.  I'll see if I can come up with 
some additional patches but I'd prefer to keep them separate from the 
IPv4 ones which I hope are now good-to-go.

Thanks

Will


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - July 29, 2013, 2:56 p.m.
On Mon, Jul 29, 2013 at 03:39:04PM +0100, William Manley wrote:
> I'll have a look, but I have to admit my ignorance on how multicast 
> works with IPv6 and I may have difficulty setting up a test environment.
> 
> It seems that IGMP has been replaced with "Multicast Listener Discovery" 
> on top of ICMPv6 and much like IGMP there is more than one version:
> 
> 1. RFC2710 - MLD - Unsolicited Report Interval = 10s
> 2. RFC3810 - MLDv2 - Unsolicited Report Interval = 1s

Correct.

> There also seems to be a /proc/sys/net/ipv6/conf/all/force_mld_version. 
>  Ok, so maybe it's not so difficult.  I'll see if I can come up with 
> some additional patches but I'd prefer to keep them separate from the 
> IPv4 ones which I hope are now good-to-go.

Of course, separate patches for ipv6 are favorable. The code is actually
pretty similar to IPv4. Let me know if you need help.

I'll review your IPv4 patches now.

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index d8c2327..12ffc2b 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -113,7 +113,8 @@ 
 
 #define IGMP_V1_Router_Present_Timeout		(400*HZ)
 #define IGMP_V2_Router_Present_Timeout		(400*HZ)
-#define IGMP_Unsolicited_Report_Interval	(10*HZ)
+#define IGMP_V2_Unsolicited_Report_Interval	(10*HZ)
+#define IGMP_V3_Unsolicited_Report_Interval	(1*HZ)
 #define IGMP_Query_Response_Interval		(10*HZ)
 #define IGMP_Unsolicited_Report_Count		2
 
@@ -138,6 +139,14 @@ 
 	 ((in_dev)->mr_v2_seen && \
 	  time_before(jiffies, (in_dev)->mr_v2_seen)))
 
+static int unsolicited_report_interval(struct in_device *in_dev)
+{
+	if (IGMP_V2_SEEN(in_dev))
+		return IGMP_V2_Unsolicited_Report_Interval;
+	else /* v3 */
+		return IGMP_V3_Unsolicited_Report_Interval;
+}
+
 static void igmpv3_add_delrec(struct in_device *in_dev, struct ip_mc_list *im);
 static void igmpv3_del_delrec(struct in_device *in_dev, __be32 multiaddr);
 static void igmpv3_clear_delrec(struct in_device *in_dev);
@@ -719,7 +728,8 @@  static void igmp_ifc_timer_expire(unsigned long data)
 	igmpv3_send_cr(in_dev);
 	if (in_dev->mr_ifc_count) {
 		in_dev->mr_ifc_count--;
-		igmp_ifc_start_timer(in_dev, IGMP_Unsolicited_Report_Interval);
+		igmp_ifc_start_timer(in_dev,
+				     unsolicited_report_interval(in_dev));
 	}
 	__in_dev_put(in_dev);
 }
@@ -744,7 +754,7 @@  static void igmp_timer_expire(unsigned long data)
 
 	if (im->unsolicit_count) {
 		im->unsolicit_count--;
-		igmp_start_timer(im, IGMP_Unsolicited_Report_Interval);
+		igmp_start_timer(im, unsolicited_report_interval(in_dev));
 	}
 	im->reporter = 1;
 	spin_unlock(&im->lock);