linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
@ 2018-07-22  1:31 Phil Karn
  2018-07-22  5:03 ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Phil Karn @ 2018-07-22  1:31 UTC (permalink / raw)
  To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
	linux-kernel

I'm running pimd (protocol independent multicast routing) and found that
on busy networks with lots of unresolved multicast routing entries, the
creation of new multicast group routes can be extremely slow and
unreliable, especially when the group in question has little traffic.

A google search revealed the following conversation about the problem
from the fall of 2015:

https://github.com/troglobit/pimd/issues/58

Note especially the comment by kopren on Sep 13, 2016.

The writer traced the problem to function ipmr_cache_unresolved() in
file net/ipmr.c, in the following block of code:

		/* Create a new entry if allowable */
		if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
		    (c = ipmr_cache_alloc_unres()) == NULL) {
			spin_unlock_bh(&mfc_unres_lock);

			kfree_skb(skb);
			return -ENOBUFS;
		}

This imposes a hard-wired limit of 10 multicast route entries with
unresolved source addresses and upstream interfaces. My problem system
sits on a busy subnet at UC San Diego, and when I run the command 'ip
mroute show' there are almost always exactly 10 unresolved multicast
routes. The authors reported that removing this limit solved their
problem, but I still see the test in the just-released kernel version
4.17.8.

I don't have this problem on my home network or on a small network at a
local high school, both networks having fewer active multicast groups.
The problem only shows up at UCSD.

The problem is most acute with a multicast group that generates only one
packet (decoded ham radio location tracking packets) every 10 seconds or
so. The multicast route *never* resolves and traffic never gets through.

The problem is also severe with a multicast group generating
intermittent bursts of traffic with seconds of idle time between bursts
(audio PCM from a software defined FM receiver with a squelch that stops
the traffic when there's no signal). However, when the receiver is tuned
to NOAA Weather Radio (which generates a continuous stream of traffic)
multicast routing generally worked.

The *only* difference between these three cases was the intensity of
traffic in the multicast groups.

Does this hard-coded limit serve any purpose? Can it be safely increased
to a much larger value, or better yet, removed altogether? If it can't
be removed, can it at least be made configurable through a /proc entry?

Thanks,

Phil Karn, KA9Q


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
  2018-07-22  1:31 hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks Phil Karn
@ 2018-07-22  5:03 ` David Miller
  2018-07-22  5:32   ` Phil Karn
  2018-11-20  9:23   ` Hangbin Liu
  0 siblings, 2 replies; 9+ messages in thread
From: David Miller @ 2018-07-22  5:03 UTC (permalink / raw)
  To: karn; +Cc: kuznet, yoshfuji, netdev, linux-kernel

From: Phil Karn <karn@ka9q.net>
Date: Sat, 21 Jul 2018 18:31:22 -0700

> I'm running pimd (protocol independent multicast routing) and found that
> on busy networks with lots of unresolved multicast routing entries, the
> creation of new multicast group routes can be extremely slow and
> unreliable, especially when the group in question has little traffic.
> 
> A google search revealed the following conversation about the problem
> from the fall of 2015:
> 
> https://github.com/troglobit/pimd/issues/58
> 
> Note especially the comment by kopren on Sep 13, 2016.
> 
> The writer traced the problem to function ipmr_cache_unresolved() in
> file net/ipmr.c, in the following block of code:
> 
> 		/* Create a new entry if allowable */
> 		if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
> 		    (c = ipmr_cache_alloc_unres()) == NULL) {
> 			spin_unlock_bh(&mfc_unres_lock);
> 
> 			kfree_skb(skb);
> 			return -ENOBUFS;
> 		}
 ...
> Does this hard-coded limit serve any purpose? Can it be safely increased
> to a much larger value, or better yet, removed altogether? If it can't
> be removed, can it at least be made configurable through a /proc entry?

Yeah that limit is bogus for several reasons.

One, it's too low.

Two, it's not configurable.

There does have to be some limit, because we are depending upon a user
process (mrouted or whatever) to receive the netlink message, resolve
the cache entry, and update the kernel.

If the user process gets stuck, or processes entries very slowly, the
backlog could grow infinitely.  So we do indeed need some kind of limit.

But, we essentially already do have such a limit, and that's the
socket receive queue limits of the mrouted socket.  And indeed, we
fail the cache creation if we cannot queue up the netlink message to
the user process successfully.

Therefore, it probably is safe and correct to remove this
cache_resolve_queue_len altogether.

Something like this:

diff --git a/include/linux/mroute_base.h b/include/linux/mroute_base.h
index d633f737b3c6..b166465d7c05 100644
--- a/include/linux/mroute_base.h
+++ b/include/linux/mroute_base.h
@@ -234,7 +234,6 @@ struct mr_table_ops {
  * @mfc_hash: Hash table of all resolved routes for easy lookup
  * @mfc_cache_list: list of resovled routes for possible traversal
  * @maxvif: Identifier of highest value vif currently in use
- * @cache_resolve_queue_len: current size of unresolved queue
  * @mroute_do_assert: Whether to inform userspace on wrong ingress
  * @mroute_do_pim: Whether to receive IGMP PIMv1
  * @mroute_reg_vif_num: PIM-device vif index
@@ -251,7 +250,6 @@ struct mr_table {
 	struct rhltable		mfc_hash;
 	struct list_head	mfc_cache_list;
 	int			maxvif;
-	atomic_t		cache_resolve_queue_len;
 	bool			mroute_do_assert;
 	bool			mroute_do_pim;
 	int			mroute_reg_vif_num;
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 9f79b9803a16..c007cf9bfe82 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -747,8 +747,6 @@ static void ipmr_destroy_unres(struct mr_table *mrt, struct mfc_cache *c)
 	struct sk_buff *skb;
 	struct nlmsgerr *e;
 
-	atomic_dec(&mrt->cache_resolve_queue_len);
-
 	while ((skb = skb_dequeue(&c->_c.mfc_un.unres.unresolved))) {
 		if (ip_hdr(skb)->version == 0) {
 			struct nlmsghdr *nlh = skb_pull(skb,
@@ -1135,9 +1133,11 @@ static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
 	}
 
 	if (!found) {
+		bool was_empty;
+
 		/* Create a new entry if allowable */
-		if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
-		    (c = ipmr_cache_alloc_unres()) == NULL) {
+		c = ipmr_cache_alloc_unres();
+		if (!c) {
 			spin_unlock_bh(&mfc_unres_lock);
 
 			kfree_skb(skb);
@@ -1163,11 +1163,11 @@ static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
 			return err;
 		}
 
-		atomic_inc(&mrt->cache_resolve_queue_len);
+		was_empty = list_empty(&mrt->mfc_unres_queue);
 		list_add(&c->_c.list, &mrt->mfc_unres_queue);
 		mroute_netlink_event(mrt, c, RTM_NEWROUTE);
 
-		if (atomic_read(&mrt->cache_resolve_queue_len) == 1)
+		if (was_empty)
 			mod_timer(&mrt->ipmr_expire_timer,
 				  c->_c.mfc_un.unres.expires);
 	}
@@ -1274,7 +1274,6 @@ static int ipmr_mfc_add(struct net *net, struct mr_table *mrt,
 		if (uc->mfc_origin == c->mfc_origin &&
 		    uc->mfc_mcastgrp == c->mfc_mcastgrp) {
 			list_del(&_uc->list);
-			atomic_dec(&mrt->cache_resolve_queue_len);
 			found = true;
 			break;
 		}
@@ -1322,7 +1321,7 @@ static void mroute_clean_tables(struct mr_table *mrt, bool all)
 		mr_cache_put(c);
 	}
 
-	if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
+	if (!list_empty(&mrt->mfc_unres_queue)) {
 		spin_lock_bh(&mfc_unres_lock);
 		list_for_each_entry_safe(c, tmp, &mrt->mfc_unres_queue, list) {
 			list_del(&c->list);
@@ -2648,9 +2647,19 @@ static int ipmr_rtm_route(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return ipmr_mfc_delete(tbl, &mfcc, parent);
 }
 
+static int queue_count(struct mr_table *mrt)
+{
+	struct list_head *pos;
+	int count = 0;
+	
+	list_for_each(pos, &mrt->mfc_unres_queue)
+		count++;
+	return count;
+}
+
 static bool ipmr_fill_table(struct mr_table *mrt, struct sk_buff *skb)
 {
-	u32 queue_len = atomic_read(&mrt->cache_resolve_queue_len);
+	u32 queue_len = queue_count(mrt);
 
 	if (nla_put_u32(skb, IPMRA_TABLE_ID, mrt->id) ||
 	    nla_put_u32(skb, IPMRA_TABLE_CACHE_RES_QUEUE_LEN, queue_len) ||
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 0d0f0053bb11..75e9c5a3e7ea 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -759,8 +759,6 @@ static void ip6mr_destroy_unres(struct mr_table *mrt, struct mfc6_cache *c)
 	struct net *net = read_pnet(&mrt->net);
 	struct sk_buff *skb;
 
-	atomic_dec(&mrt->cache_resolve_queue_len);
-
 	while ((skb = skb_dequeue(&c->_c.mfc_un.unres.unresolved)) != NULL) {
 		if (ipv6_hdr(skb)->version == 0) {
 			struct nlmsghdr *nlh = skb_pull(skb,
@@ -1139,8 +1137,8 @@ static int ip6mr_cache_unresolved(struct mr_table *mrt, mifi_t mifi,
 		 *	Create a new entry if allowable
 		 */
 
-		if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
-		    (c = ip6mr_cache_alloc_unres()) == NULL) {
+		c = ip6mr_cache_alloc_unres();
+		if (!c) {
 			spin_unlock_bh(&mfc_unres_lock);
 
 			kfree_skb(skb);
@@ -1167,7 +1165,6 @@ static int ip6mr_cache_unresolved(struct mr_table *mrt, mifi_t mifi,
 			return err;
 		}
 
-		atomic_inc(&mrt->cache_resolve_queue_len);
 		list_add(&c->_c.list, &mrt->mfc_unres_queue);
 		mr6_netlink_event(mrt, c, RTM_NEWROUTE);
 
@@ -1455,7 +1452,6 @@ static int ip6mr_mfc_add(struct net *net, struct mr_table *mrt,
 		if (ipv6_addr_equal(&uc->mf6c_origin, &c->mf6c_origin) &&
 		    ipv6_addr_equal(&uc->mf6c_mcastgrp, &c->mf6c_mcastgrp)) {
 			list_del(&_uc->list);
-			atomic_dec(&mrt->cache_resolve_queue_len);
 			found = true;
 			break;
 		}
@@ -1502,7 +1498,7 @@ static void mroute_clean_tables(struct mr_table *mrt, bool all)
 		mr_cache_put(c);
 	}
 
-	if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
+	if (!list_empty(&mrt->mfc_unres_queue)) {
 		spin_lock_bh(&mfc_unres_lock);
 		list_for_each_entry_safe(c, tmp, &mrt->mfc_unres_queue, list) {
 			list_del(&c->list);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
  2018-07-22  5:03 ` David Miller
@ 2018-07-22  5:32   ` Phil Karn
  2018-11-20  9:23   ` Hangbin Liu
  1 sibling, 0 replies; 9+ messages in thread
From: Phil Karn @ 2018-07-22  5:32 UTC (permalink / raw)
  To: David Miller; +Cc: kuznet, yoshfuji, netdev, linux-kernel

On 07/21/2018 10:03 PM, David Miller wrote:

> There does have to be some limit, because we are depending upon a user
> process (mrouted or whatever) to receive the netlink message, resolve
> the cache entry, and update the kernel.

Wow, thanks for the quick response.

I notice that an "unresolved" entry seems to be created whenever the
router sees multicast traffic from a particular source to a particular
group. The entry seems to "resolve" when an IGMP membership message is
also seen for that particular group. There are a whole bunch of active
multicast streams on my UCSD network segment for which there are
apparently no listeners, and that seems to be why I always see a mroute
cache full of "unresolved" entries. (If I run a local application that
joins a group, the entry "resolves".) What puzzles me is that the Iif
field is given as "unresolved" even though we know where the multicast
traffic is coming from. We only don't know who might want it.

I'm intimately familiar with unicast IP but I'm still learning about
multicast routing so I am probably missing something important.

I tried a workaround by using iptables to block the unwanted multicast
traffic (the senders are on one well-defined non-local subnet). I
created drop entries in both the INPUT and FORWARD chains but the hit
counts remain zero. I guess the mroute table is populated before the
packets reach netfilter. Is that how it should work?

Phil

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
  2018-07-22  5:03 ` David Miller
  2018-07-22  5:32   ` Phil Karn
@ 2018-11-20  9:23   ` Hangbin Liu
       [not found]     ` <CADiZnkSy=rFq5xLs6RcgJDihQ1Vwo2WBBY9Fi_5jOHr8XupukQ@mail.gmail.com>
  1 sibling, 1 reply; 9+ messages in thread
From: Hangbin Liu @ 2018-11-20  9:23 UTC (permalink / raw)
  To: David Miller; +Cc: karn, kuznet, yoshfuji, netdev, linux-kernel

Hi David,

On Sat, Jul 21, 2018 at 10:03:09PM -0700, David Miller wrote:
> Yeah that limit is bogus for several reasons.
...
> 
> Therefore, it probably is safe and correct to remove this
> cache_resolve_queue_len altogether.
> 
> Something like this:
> 
> diff --git a/include/linux/mroute_base.h b/include/linux/mroute_base.h
> index d633f737b3c6..b166465d7c05 100644
> --- a/include/linux/mroute_base.h
> +++ b/include/linux/mroute_base.h
> @@ -234,7 +234,6 @@ struct mr_table_ops {
>   * @mfc_hash: Hash table of all resolved routes for easy lookup
>   * @mfc_cache_list: list of resovled routes for possible traversal
>   * @maxvif: Identifier of highest value vif currently in use
> - * @cache_resolve_queue_len: current size of unresolved queue
>   * @mroute_do_assert: Whether to inform userspace on wrong ingress
>   * @mroute_do_pim: Whether to receive IGMP PIMv1
>   * @mroute_reg_vif_num: PIM-device vif index
> @@ -251,7 +250,6 @@ struct mr_table {
>  	struct rhltable		mfc_hash;
>  	struct list_head	mfc_cache_list;
>  	int			maxvif;
> -	atomic_t		cache_resolve_queue_len;
>  	bool			mroute_do_assert;
>  	bool			mroute_do_pim;
>  	int			mroute_reg_vif_num;
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 9f79b9803a16..c007cf9bfe82 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -747,8 +747,6 @@ static void ipmr_destroy_unres(struct mr_table *mrt, struct mfc_cache *c)
>  	struct sk_buff *skb;
>  	struct nlmsgerr *e;
>  
> -	atomic_dec(&mrt->cache_resolve_queue_len);
> -
>  	while ((skb = skb_dequeue(&c->_c.mfc_un.unres.unresolved))) {
>  		if (ip_hdr(skb)->version == 0) {
>  			struct nlmsghdr *nlh = skb_pull(skb,
> @@ -1135,9 +1133,11 @@ static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
>  	}
>  
>  	if (!found) {
> +		bool was_empty;
> +
>  		/* Create a new entry if allowable */
> -		if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
> -		    (c = ipmr_cache_alloc_unres()) == NULL) {
> +		c = ipmr_cache_alloc_unres();
> +		if (!c) {
>  			spin_unlock_bh(&mfc_unres_lock);
>  
>  			kfree_skb(skb);
> @@ -1163,11 +1163,11 @@ static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
>  			return err;
>  		}
>  
> -		atomic_inc(&mrt->cache_resolve_queue_len);
> +		was_empty = list_empty(&mrt->mfc_unres_queue);
>  		list_add(&c->_c.list, &mrt->mfc_unres_queue);
>  		mroute_netlink_event(mrt, c, RTM_NEWROUTE);
>  
> -		if (atomic_read(&mrt->cache_resolve_queue_len) == 1)
> +		if (was_empty)
>  			mod_timer(&mrt->ipmr_expire_timer,
>  				  c->_c.mfc_un.unres.expires);

In ipmr_expire_process() and ipmr_do_expire_process(), they start mod_timer
when !list_empty(&mrt->mfc_unres_queue), should here also be !was_empty?

BTW, do you have any plan to apply this patch in kernel?

Regards
Hangbin

>  	}
> @@ -1274,7 +1274,6 @@ static int ipmr_mfc_add(struct net *net, struct mr_table *mrt,
>  		if (uc->mfc_origin == c->mfc_origin &&
>  		    uc->mfc_mcastgrp == c->mfc_mcastgrp) {
>  			list_del(&_uc->list);
> -			atomic_dec(&mrt->cache_resolve_queue_len);
>  			found = true;
>  			break;
>  		}
> @@ -1322,7 +1321,7 @@ static void mroute_clean_tables(struct mr_table *mrt, bool all)
>  		mr_cache_put(c);
>  	}
>  
> -	if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
> +	if (!list_empty(&mrt->mfc_unres_queue)) {
>  		spin_lock_bh(&mfc_unres_lock);
>  		list_for_each_entry_safe(c, tmp, &mrt->mfc_unres_queue, list) {
>  			list_del(&c->list);
> @@ -2648,9 +2647,19 @@ static int ipmr_rtm_route(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		return ipmr_mfc_delete(tbl, &mfcc, parent);
>  }
>  
> +static int queue_count(struct mr_table *mrt)
> +{
> +	struct list_head *pos;
> +	int count = 0;
> +	
> +	list_for_each(pos, &mrt->mfc_unres_queue)
> +		count++;
> +	return count;
> +}
> +
>  static bool ipmr_fill_table(struct mr_table *mrt, struct sk_buff *skb)
>  {
> -	u32 queue_len = atomic_read(&mrt->cache_resolve_queue_len);
> +	u32 queue_len = queue_count(mrt);
>  
>  	if (nla_put_u32(skb, IPMRA_TABLE_ID, mrt->id) ||
>  	    nla_put_u32(skb, IPMRA_TABLE_CACHE_RES_QUEUE_LEN, queue_len) ||
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 0d0f0053bb11..75e9c5a3e7ea 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -759,8 +759,6 @@ static void ip6mr_destroy_unres(struct mr_table *mrt, struct mfc6_cache *c)
>  	struct net *net = read_pnet(&mrt->net);
>  	struct sk_buff *skb;
>  
> -	atomic_dec(&mrt->cache_resolve_queue_len);
> -
>  	while ((skb = skb_dequeue(&c->_c.mfc_un.unres.unresolved)) != NULL) {
>  		if (ipv6_hdr(skb)->version == 0) {
>  			struct nlmsghdr *nlh = skb_pull(skb,
> @@ -1139,8 +1137,8 @@ static int ip6mr_cache_unresolved(struct mr_table *mrt, mifi_t mifi,
>  		 *	Create a new entry if allowable
>  		 */
>  
> -		if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
> -		    (c = ip6mr_cache_alloc_unres()) == NULL) {
> +		c = ip6mr_cache_alloc_unres();
> +		if (!c) {
>  			spin_unlock_bh(&mfc_unres_lock);
>  
>  			kfree_skb(skb);
> @@ -1167,7 +1165,6 @@ static int ip6mr_cache_unresolved(struct mr_table *mrt, mifi_t mifi,
>  			return err;
>  		}
>  
> -		atomic_inc(&mrt->cache_resolve_queue_len);
>  		list_add(&c->_c.list, &mrt->mfc_unres_queue);
>  		mr6_netlink_event(mrt, c, RTM_NEWROUTE);
>  
> @@ -1455,7 +1452,6 @@ static int ip6mr_mfc_add(struct net *net, struct mr_table *mrt,
>  		if (ipv6_addr_equal(&uc->mf6c_origin, &c->mf6c_origin) &&
>  		    ipv6_addr_equal(&uc->mf6c_mcastgrp, &c->mf6c_mcastgrp)) {
>  			list_del(&_uc->list);
> -			atomic_dec(&mrt->cache_resolve_queue_len);
>  			found = true;
>  			break;
>  		}
> @@ -1502,7 +1498,7 @@ static void mroute_clean_tables(struct mr_table *mrt, bool all)
>  		mr_cache_put(c);
>  	}
>  
> -	if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
> +	if (!list_empty(&mrt->mfc_unres_queue)) {
>  		spin_lock_bh(&mfc_unres_lock);
>  		list_for_each_entry_safe(c, tmp, &mrt->mfc_unres_queue, list) {
>  			list_del(&c->list);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
       [not found]     ` <CADiZnkSy=rFq5xLs6RcgJDihQ1Vwo2WBBY9Fi_5jOHr8XupukQ@mail.gmail.com>
@ 2018-11-26  6:58       ` Hangbin Liu
  2018-12-04  6:51       ` Hangbin Liu
  1 sibling, 0 replies; 9+ messages in thread
From: Hangbin Liu @ 2018-11-26  6:58 UTC (permalink / raw)
  To: Sukumar Gopalakrishnan
  Cc: davem, karn, kuznet, yoshfuji, netdev, linux-kernel

On Mon, Nov 26, 2018 at 10:44:49AM +0530, Sukumar Gopalakrishnan wrote:
> Hi,
> 
>  There is a patch to make this queue len configurable. Is below mentioned going
> to be applied ?
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1810.3/02344.html

Thanks for this reminder, I saw Stephen has gave some comments. But I'm not sure
which patch David would apply.

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
       [not found]     ` <CADiZnkSy=rFq5xLs6RcgJDihQ1Vwo2WBBY9Fi_5jOHr8XupukQ@mail.gmail.com>
  2018-11-26  6:58       ` Hangbin Liu
@ 2018-12-04  6:51       ` Hangbin Liu
       [not found]         ` <CADiZnkTm3UMdZ+ivPXFeTJS+_2ZaiQh7D8wWnsw0BNGfxa0C4w@mail.gmail.com>
  1 sibling, 1 reply; 9+ messages in thread
From: Hangbin Liu @ 2018-12-04  6:51 UTC (permalink / raw)
  To: Sukumar Gopalakrishnan
  Cc: davem, karn, kuznet, yoshfuji, netdev, linux-kernel

On Mon, Nov 26, 2018 at 10:44:49AM +0530, Sukumar Gopalakrishnan wrote:
> Hi,
> 
>  There is a patch to make this queue len configurable. Is below mentioned going
> to be applied ?
> 
> http://lkml.iu.edu/hypermail/linux/kernel/1810.3/02344.html

It looks this topic stuckd again..

> 
> Regards,
> Sukumar
> 
> On Tue, Nov 20, 2018 at 2:55 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
>     Hi David,
> 
>     On Sat, Jul 21, 2018 at 10:03:09PM -0700, David Miller wrote:
>     > Yeah that limit is bogus for several reasons.
>     ...
>     >
>     > Therefore, it probably is safe and correct to remove this
>     > cache_resolve_queue_len altogether.
>     >
>     > Something like this:
>     >
>     > diff --git a/include/linux/mroute_base.h b/include/linux/mroute_base.h
>     > index d633f737b3c6..b166465d7c05 100644
>     > --- a/include/linux/mroute_base.h
>     > +++ b/include/linux/mroute_base.h
>     > @@ -234,7 +234,6 @@ struct mr_table_ops {
>     >   * @mfc_hash: Hash table of all resolved routes for easy lookup
>     >   * @mfc_cache_list: list of resovled routes for possible traversal
>     >   * @maxvif: Identifier of highest value vif currently in use
>     > - * @cache_resolve_queue_len: current size of unresolved queue
>     >   * @mroute_do_assert: Whether to inform userspace on wrong ingress
>     >   * @mroute_do_pim: Whether to receive IGMP PIMv1
>     >   * @mroute_reg_vif_num: PIM-device vif index
>     > @@ -251,7 +250,6 @@ struct mr_table {
>     >       struct rhltable         mfc_hash;
>     >       struct list_head        mfc_cache_list;
>     >       int                     maxvif;
>     > -     atomic_t                cache_resolve_queue_len;
>     >       bool                    mroute_do_assert;
>     >       bool                    mroute_do_pim;
>     >       int                     mroute_reg_vif_num;
>     > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
>     > index 9f79b9803a16..c007cf9bfe82 100644
>     > --- a/net/ipv4/ipmr.c
>     > +++ b/net/ipv4/ipmr.c
>     > @@ -747,8 +747,6 @@ static void ipmr_destroy_unres(struct mr_table *mrt,
>     struct mfc_cache *c)
>     >       struct sk_buff *skb;
>     >       struct nlmsgerr *e;
>     > 
>     > -     atomic_dec(&mrt->cache_resolve_queue_len);
>     > -
>     >       while ((skb = skb_dequeue(&c->_c.mfc_un.unres.unresolved))) {
>     >               if (ip_hdr(skb)->version == 0) {
>     >                       struct nlmsghdr *nlh = skb_pull(skb,
>     > @@ -1135,9 +1133,11 @@ static int ipmr_cache_unresolved(struct mr_table
>     *mrt, vifi_t vifi,
>     >       }
>     > 
>     >       if (!found) {
>     > +             bool was_empty;
>     > +
>     >               /* Create a new entry if allowable */
>     > -             if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
>     > -                 (c = ipmr_cache_alloc_unres()) == NULL) {
>     > +             c = ipmr_cache_alloc_unres();
>     > +             if (!c) {
>     >                       spin_unlock_bh(&mfc_unres_lock);
>     > 
>     >                       kfree_skb(skb);
>     > @@ -1163,11 +1163,11 @@ static int ipmr_cache_unresolved(struct mr_table
>     *mrt, vifi_t vifi,
>     >                       return err;
>     >               }
>     > 
>     > -             atomic_inc(&mrt->cache_resolve_queue_len);
>     > +             was_empty = list_empty(&mrt->mfc_unres_queue);
>     >               list_add(&c->_c.list, &mrt->mfc_unres_queue);
>     >               mroute_netlink_event(mrt, c, RTM_NEWROUTE);
>     > 
>     > -             if (atomic_read(&mrt->cache_resolve_queue_len) == 1)
>     > +             if (was_empty)
>     >                       mod_timer(&mrt->ipmr_expire_timer,
>     >                                 c->_c.mfc_un.unres.expires);
> 
>     In ipmr_expire_process() and ipmr_do_expire_process(), they start mod_timer
>     when !list_empty(&mrt->mfc_unres_queue), should here also be !was_empty?
> 
>     BTW, do you have any plan to apply this patch in kernel?
> 
>     Regards
>     Hangbin
> 
>     >       }
>     > @@ -1274,7 +1274,6 @@ static int ipmr_mfc_add(struct net *net, struct
>     mr_table *mrt,
>     >               if (uc->mfc_origin == c->mfc_origin &&
>     >                   uc->mfc_mcastgrp == c->mfc_mcastgrp) {
>     >                       list_del(&_uc->list);
>     > -                     atomic_dec(&mrt->cache_resolve_queue_len);
>     >                       found = true;
>     >                       break;
>     >               }
>     > @@ -1322,7 +1321,7 @@ static void mroute_clean_tables(struct mr_table
>     *mrt, bool all)
>     >               mr_cache_put(c);
>     >       }
>     > 
>     > -     if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
>     > +     if (!list_empty(&mrt->mfc_unres_queue)) {
>     >               spin_lock_bh(&mfc_unres_lock);
>     >               list_for_each_entry_safe(c, tmp, &mrt->mfc_unres_queue,
>     list) {
>     >                       list_del(&c->list);
>     > @@ -2648,9 +2647,19 @@ static int ipmr_rtm_route(struct sk_buff *skb,
>     struct nlmsghdr *nlh,
>     >               return ipmr_mfc_delete(tbl, &mfcc, parent);
>     >  }
>     > 
>     > +static int queue_count(struct mr_table *mrt)
>     > +{
>     > +     struct list_head *pos;
>     > +     int count = 0;
>     > +     
>     > +     list_for_each(pos, &mrt->mfc_unres_queue)
>     > +             count++;
>     > +     return count;
>     > +}
>     > +
>     >  static bool ipmr_fill_table(struct mr_table *mrt, struct sk_buff *skb)
>     >  {
>     > -     u32 queue_len = atomic_read(&mrt->cache_resolve_queue_len);
>     > +     u32 queue_len = queue_count(mrt);
>     > 
>     >       if (nla_put_u32(skb, IPMRA_TABLE_ID, mrt->id) ||
>     >           nla_put_u32(skb, IPMRA_TABLE_CACHE_RES_QUEUE_LEN, queue_len) ||
>     > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
>     > index 0d0f0053bb11..75e9c5a3e7ea 100644
>     > --- a/net/ipv6/ip6mr.c
>     > +++ b/net/ipv6/ip6mr.c
>     > @@ -759,8 +759,6 @@ static void ip6mr_destroy_unres(struct mr_table *mrt,
>     struct mfc6_cache *c)
>     >       struct net *net = read_pnet(&mrt->net);
>     >       struct sk_buff *skb;
>     > 
>     > -     atomic_dec(&mrt->cache_resolve_queue_len);
>     > -
>     >       while ((skb = skb_dequeue(&c->_c.mfc_un.unres.unresolved)) != NULL)
>     {
>     >               if (ipv6_hdr(skb)->version == 0) {
>     >                       struct nlmsghdr *nlh = skb_pull(skb,
>     > @@ -1139,8 +1137,8 @@ static int ip6mr_cache_unresolved(struct mr_table
>     *mrt, mifi_t mifi,
>     >                *      Create a new entry if allowable
>     >                */
>     > 
>     > -             if (atomic_read(&mrt->cache_resolve_queue_len) >= 10 ||
>     > -                 (c = ip6mr_cache_alloc_unres()) == NULL) {
>     > +             c = ip6mr_cache_alloc_unres();
>     > +             if (!c) {
>     >                       spin_unlock_bh(&mfc_unres_lock);
>     > 
>     >                       kfree_skb(skb);
>     > @@ -1167,7 +1165,6 @@ static int ip6mr_cache_unresolved(struct mr_table
>     *mrt, mifi_t mifi,
>     >                       return err;
>     >               }
>     > 
>     > -             atomic_inc(&mrt->cache_resolve_queue_len);
>     >               list_add(&c->_c.list, &mrt->mfc_unres_queue);
>     >               mr6_netlink_event(mrt, c, RTM_NEWROUTE);
>     > 
>     > @@ -1455,7 +1452,6 @@ static int ip6mr_mfc_add(struct net *net, struct
>     mr_table *mrt,
>     >               if (ipv6_addr_equal(&uc->mf6c_origin, &c->mf6c_origin) &&
>     >                   ipv6_addr_equal(&uc->mf6c_mcastgrp, &c->mf6c_mcastgrp))
>     {
>     >                       list_del(&_uc->list);
>     > -                     atomic_dec(&mrt->cache_resolve_queue_len);
>     >                       found = true;
>     >                       break;
>     >               }
>     > @@ -1502,7 +1498,7 @@ static void mroute_clean_tables(struct mr_table
>     *mrt, bool all)
>     >               mr_cache_put(c);
>     >       }
>     > 
>     > -     if (atomic_read(&mrt->cache_resolve_queue_len) != 0) {
>     > +     if (!list_empty(&mrt->mfc_unres_queue)) {
>     >               spin_lock_bh(&mfc_unres_lock);
>     >               list_for_each_entry_safe(c, tmp, &mrt->mfc_unres_queue,
>     list) {
>     >                       list_del(&c->list);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
       [not found]         ` <CADiZnkTm3UMdZ+ivPXFeTJS+_2ZaiQh7D8wWnsw0BNGfxa0C4w@mail.gmail.com>
@ 2018-12-19  5:55           ` David Miller
  2019-06-25  6:15             ` Hangbin Liu
  0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2018-12-19  5:55 UTC (permalink / raw)
  To: sukumarg1973; +Cc: liuhangbin, karn, kuznet, yoshfuji, netdev, linux-kernel

From: Sukumar Gopalakrishnan <sukumarg1973@gmail.com>
Date: Wed, 19 Dec 2018 10:57:02 +0530

> Hi David,
> 
>   There are two patch for this issue:
>    1) Your changes which removes cache_resolve_queue_len
>     2) Hangbin's changes which make cache_resolve_queue_len configurable.
> 
> Which one will be chosen for this issue ?

I do plan to look into this, sorry for taking so long.

Right now I am overwhelmed preparing for the next merge window and
synchronizing with other developers for that.

Please be patient.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
  2018-12-19  5:55           ` David Miller
@ 2019-06-25  6:15             ` Hangbin Liu
  2019-06-25 20:03               ` David Miller
  0 siblings, 1 reply; 9+ messages in thread
From: Hangbin Liu @ 2019-06-25  6:15 UTC (permalink / raw)
  To: David Miller; +Cc: sukumarg1973, karn, kuznet, yoshfuji, netdev, linux-kernel

On Tue, Dec 18, 2018 at 09:55:45PM -0800, David Miller wrote:
> From: Sukumar Gopalakrishnan <sukumarg1973@gmail.com>
> Date: Wed, 19 Dec 2018 10:57:02 +0530
> 
> > Hi David,
> > 
> >   There are two patch for this issue:
> >    1) Your changes which removes cache_resolve_queue_len
> >     2) Hangbin's changes which make cache_resolve_queue_len configurable.
> > 
> > Which one will be chosen for this issue ?
> 
> I do plan to look into this, sorry for taking so long.
> 
> Right now I am overwhelmed preparing for the next merge window and
> synchronizing with other developers for that.
> 
> Please be patient.

Hi David,

Any progress for this issue?

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
  2019-06-25  6:15             ` Hangbin Liu
@ 2019-06-25 20:03               ` David Miller
  0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2019-06-25 20:03 UTC (permalink / raw)
  To: liuhangbin; +Cc: sukumarg1973, karn, kuznet, yoshfuji, netdev, linux-kernel

From: Hangbin Liu <liuhangbin@gmail.com>
Date: Tue, 25 Jun 2019 14:15:07 +0800

> On Tue, Dec 18, 2018 at 09:55:45PM -0800, David Miller wrote:
>> From: Sukumar Gopalakrishnan <sukumarg1973@gmail.com>
>> Date: Wed, 19 Dec 2018 10:57:02 +0530
>> 
>> > Hi David,
>> > 
>> >   There are two patch for this issue:
>> >    1) Your changes which removes cache_resolve_queue_len
>> >     2) Hangbin's changes which make cache_resolve_queue_len configurable.
>> > 
>> > Which one will be chosen for this issue ?
>> 
>> I do plan to look into this, sorry for taking so long.
>> 
>> Right now I am overwhelmed preparing for the next merge window and
>> synchronizing with other developers for that.
>> 
>> Please be patient.
> 
> Hi David,
> 
> Any progress for this issue?

I have absolutely no context from a discussion that happened back in Dec 2018

If it is important to you, please restart the discussion with a new mailing list
posting restating the problem from the beginning and reiterating all of the
points and arguments that have been made thus far.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-06-25 20:03 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-22  1:31 hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks Phil Karn
2018-07-22  5:03 ` David Miller
2018-07-22  5:32   ` Phil Karn
2018-11-20  9:23   ` Hangbin Liu
     [not found]     ` <CADiZnkSy=rFq5xLs6RcgJDihQ1Vwo2WBBY9Fi_5jOHr8XupukQ@mail.gmail.com>
2018-11-26  6:58       ` Hangbin Liu
2018-12-04  6:51       ` Hangbin Liu
     [not found]         ` <CADiZnkTm3UMdZ+ivPXFeTJS+_2ZaiQh7D8wWnsw0BNGfxa0C4w@mail.gmail.com>
2018-12-19  5:55           ` David Miller
2019-06-25  6:15             ` Hangbin Liu
2019-06-25 20:03               ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).