Hi Nikolay, Thanks for the feedback, see comments below.
On Tue, Sep 03, 2019 at 12:15:34PM +0300, Nikolay Aleksandrov wrote: > On 03/09/2019 11:43, Hangbin Liu wrote: > > This is a re-post of previous patch wrote by David Miller[1]. > > > > Phil Karn reported[2] that on busy networks with lots of unresolved > > multicast routing entries, the creation of new multicast group routes > > can be extremely slow and unreliable. > > > > The reason is we hard-coded multicast route entries with unresolved source > > addresses(cache_resolve_queue_len) to 10. If some multicast route never > > resolves and the unresolved source addresses increased, there will > > be no ability to create new multicast route cache. > > > > To resolve this issue, we need either add a sysctl entry to make the > > cache_resolve_queue_len configurable, or just remove cache_resolve_queue_len > > directly, as we already have the socket receive queue limits of mrouted > > socket, pointed by David. > > > > From my side, I'd perfer to remove the cache_resolve_queue_len instead > > of creating two more(IPv4 and IPv6 version) sysctl entry. > > > > [1] https://lkml.org/lkml/2018/7/22/11 > > [2] https://lkml.org/lkml/2018/7/21/343 > > > > Reported-by: Phil Karn <k...@ka9q.net> > > Signed-off-by: Hangbin Liu <liuhang...@gmail.com> > > --- > > include/linux/mroute_base.h | 2 -- > > net/ipv4/ipmr.c | 27 ++++++++++++++++++--------- > > net/ipv6/ip6mr.c | 10 +++------- > > 3 files changed, 21 insertions(+), 18 deletions(-) > > > > Hi, > IMO this is definitely net-next material. A few more comments below. I thoug this is a bug fix. But it looks more suitable to net-next as you said. > > Note that hosts will automatically have this limit lifted to about 270 > entries with current defaults, some might be surprised if they have > higher limits set and could be left with queues allowing for thousands > of entries in a linked list. I think this is just a cache list and should be expired soon. The cache creation would also failed if there is no buffer. But if you like, I can write a patch with sysctl parameter. > > > +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; > > +} > > I don't think we hold the mfc_unres_lock here while walking > the unresolved list below in ipmr_fill_table(). ah, yes, I will fix this. Thanks Hangbin
