Add last downstream host tracking with quickleave enabled

[tiba222]

IGMP Leave message will now only be sent upstream when the leave is received from the last downstream host in the multicast group. This solves the issue that with quickleave enabled the multicast stream shortly stops on all other devices when one devices sends a leave while other devices on the same downstream interface are still joined on that group.

Just disabling quickleave completely is not the solution for this because that causes the uplink to get saturated when fastly changing IPTV channels for example.

[pali]

Hello @timbalmer, thank you for pointing to this problem. I briefly looked at your pull request, but it contains all changes in one commit and seems that there are some unrelated changes to this problem. So could you please split changes to logically separated commits for easier review?

[tiba222]

I'm sorry, I accidentialy had some other pfSense changes included because those were needed to make it compile on FreeBSD / pfSense. I have now splitted those to a seperate commit.

The commit with the actual changes for this pull request is this 94772b6

[pali]

When updating commits, you need to use git push --force, not git pull or git merge.

[tiba222]

I thought used git push after splittig commits I thought but things became a bit of a mess indeed... Is it better to close this and start a fresh pull request?

[pali]

No need to close pull request, just update your fork/branch. pull request just show changes between your fork/branch and upstream/main repository. Prepare commits locally and then do git push --force to update your github fork.

[tiba222]

Thanks, I think everyting should be fine now. I have two seperate commits:

• The first are the changes also made by pfSense (otherwise it cannot be compiled on FreeBSD)
• The second commit is the actual change I made to implement the last downstream host checking.

Sorry for the inconvenience, for me this is the first time to submit a pull request via github.

[pali]

ok, thank you, I will look at it.

[tiba222]

I found some issues with the changes in this pull request when watching IPTV on multiple boxes for several hours. After some time it seems to come in the situation where the multicast route is removed but there is no IGMP Leave send upstrea. Also I cannot rejoin the group then before I restart igmpproxy. This happened to me twice now but so far I'm not able to reproduce it.

I'm still trying a few things to resolve this and will update when I have news.

[pali]

Ok, so try to resolve issues and then I will look at changes.

Anyway, meanwhile I fixed setup for automated test of compilation on Travis and now should work. I pushed changed changes to master branch. So please rebase your fork/branch on upstream master and force push changes.

[tiba222]

It looks like I resolved all issues yesterday, the issue was that I marked the route for last member detection even if no leave was sent upstream yet because there were dowstream hosts left. I fixed this and since then everyting is stable and I also got rid of all warnings in the debug log,

I just rebased my fork and made a final commit to make sure the whole downstream host registration is only done in fastleave mode. Please take a look at it and let me know if you have any questions or remarks.

[pali]

Hello, below are my review comments. Main problem is with first commit and then with limit of 20 downstream hosts. Other are just cosmetic changes which should be easily fixed. First commit is independent from this issue and if it is problematic, it can be handled independently (e.g. also in separate issue / pull request). Algorithm for sending leave to upstream seems to be correct.

[pali]

So fix tracking would work only in the case there are maximally 20 downstream hosts.

Why 20? Is there any limit for 20 hosts in IGMP protocol? Or in other kernel interface which prevents usage of more than 20 downstream hosts? If not it should not be hardcoded limit to 20.

For me it looks like that 20 is just arbitrary random number which does not make sense. If not, please properly describe it why. And if there is no limit for 20, then implementation should be extended to work also with more than 20 downstream hosts.

[tiba222]

No the last host tracking still works with more than 20 downstream hosts.

Agree the exact number of 20 is a bit arbitary however I will explain the idea behind limiting it. The goal is just to avoid leaving a stream while still downstream hosts are joined, in order to do that in fact I only have to know if at least one other downstream host is watching.

If I have 20 registered downstream hosts I'm pretty sure at least more than 1 is watching, certainly because when the 20 slot's are filled I start overwriting them with new joins that come in. The chance that all registered hosts in the route have left while an overwritten downstream hosts is still watching is with 20 registered hosts almost nil.

The alternative is to register all downstream hosts indeed but in very large deployments with a lot of downstream hosts that in my opinion is a waste of resources because there is no need to know all of them, it does not add anything from my point of view.

[pali]

Thank you for explanation, I understood it. igmpproxy just needs to know if there is at least one downstream host for registered group, exact number is not important and also list of downstream host is not needed for this functionality.

But for larger deployments even this your proposed solution may cause problems. There is a space for 20 hosts and you store host into first free slot. If there is not free slot then you always overwrite host stored in slot zero. So if 40 (or mote hosts) register for one multicast group then first 20 hosts are stored into table and later all other hosts are sequentially overwriting slot zero, with result that last host is stored in slot zero. Now when hosts 40 (last one), 2, 3, 4, ..., 20 leave the group then igmpproxy would think that there is no downstream host registered. For larger deployments there can be even more downstream hosts (lets say 200 or more) and it can be possible that "problematic set" would leave group in same time. Probability increases with larger and larger deployments.

Moreover this algorithm is fully deterministic, so "bad clients" can be specially created to trigger this problem. For example if you have 40 downstream hosts and 20 would cooperate, then they can break whole multicast streaming for other hosts. This even does not need large deployments, just 20 hosts are needed.

[pali]

I was thinking a bit more about this problem. And basically quickleave mode is incompatible with IGMP protocol on a network with more then one downstream host.

According to IGMPv2 specification, RFC 2236, downstream host does not have to send join or leave message when there is another downstream host on network which is connected to same multicast group. So whole "reference counting" is broken/incompatible with IGMPv2.

But I understand needs for quickleave, specially for high bitrate IPTV streams on slow link from ISP, e.g. when link is able to receive N streams in parallel, but not N+1 (e.g. for slow DSL N can be 2 or 3). And in this case IPTV boxes, specially designed by ISP, would not do above "optimization" (send join/leave message only if there is not somebody else already joined), but rather send join/leave messages always and immediately to ensure that link is not exhausted.

So in this specific case we can expect that join/leave messages are always send by clients (or at least join messages to ensure that we did not disconnect any client). But whole downstream igmp/multicast network would be broken when there come some clients which do not do this.

Heh, by broken I mean that some mutlicast groups can be left on upstream even when there are downstream clients; but as igmpproxy is doing periodic time-based query of downstream hosts, so when timeout expires igmpproxy would know that there are downstream clients and join to those multicast groups on upstream again.

I would think more about solution for quickleave mode to fix problem which you are trying to fix with in mind that it is incompatible with IGMPv2 specification and therefore clients can broke it...

If you come up with some other ideas, please let me know.

[tiba222]

From my professional experience with telecom IGMP devices performing "proxy mode" (not snooping mode) always do exactly the thing I implemented with this pull request. So they track all downstream hosts and only send Join or Leave messages upstream when the first or the last hosts joins or leaves a stream.

But if I understand you right your problem with it is that it's not directly related to quickleave mode and that therefore it feels wrong to completely change the behavior of quickleave mode?

Maybe it's an option to make a seperate configuration option for this in the config file? Something like quickleave-hostbased? So that will mean with only quickleave in the config file we keep the current behavior and with quicleave enabled and also the quickleave-hostbased option set we use the host tracking I propose.

Because there definitely is some relation with quickleave mode, when quickleave mode is disabled you won't have this problem anyway. So host tracking without quickleave mode makes no sense.

[pali]

From my professional experience with telecom IGMP devices performing "proxy mode" (not snooping mode) always do exactly the thing I implemented with this pull request. So they track all downstream hosts and only send Join or Leave messages upstream when the first or the last hosts joins or leaves a stream.

You do this just for first 20 hosts, not for tracking all hosts.

But if I understand you right your problem with it is that it's not directly related to quickleave mode and that therefore it feels wrong to completely change the behavior of quickleave mode?

No, this is directly related to quickleave mode.

I described in previous post a problem that it is not compatible with IGMP protocol specification even if you even track all hosts in your implementation. (But your implementation does not track all hosts, but only 20).

But also I wrote that in that specific scenario it is the best what can be done for quickleave mode to track join/leave messages in downstream hosts. In case you do not have better idea how to do it. If you wrote that telecom IGMP devices are doing same thing then we probably do not come up with something better.

So the main remaining part is what to do with limitation that this proposed change tracks only (some random) 20 hosts and not all hosts.

[tiba222]

I just resolved all of your remarks and squashed all actual host tracking changes into 1 commit. Only open conversation is the limit of 20 hosts to track, please see my explanation above on that remark.

[pali]

(you have there compile errors, see travis "details" button)

[tiba222]

Reverted the size_t change, that causes more inconvenience that is resolves in my code. Will look at tracking alle downstream hosts later.

[pali]

When you want to return -1 in size_t function, you should write return (size_t)-1. And when comparing result then you should write if (result == (size_t)-1) { ... }. This is standard idiom. See e.g. standard C function wcstombs() which has this interface (returns size_t and on "error" it returns (size_t)-1)).

[tiba222]

When you want to return -1 in size_t function, you should write return (size_t)-1. And when comparing result then you should write if (result == (size_t)-1) { ... }. This is standard idiom. See e.g. standard C function wcstombs() which has this interface (returns size_t and on "error" it returns (size_t)-1)).

I understand that, but I also use -1 as return value to indicate nothing is found. This is impossible with size_t datatype. So if changing to size_t datatype I als have to work arround that. Of cours alle possible but I don't see which problem it solves in this case.

[pali]

I understand that, but I also use -1 as return value to indicate nothing is found. This is impossible with size_t datatype.

As I pointed, it is possible you can return it as return (size_t)-1. This is C pattern how to do it, indicate for you that nothing was found.

I just pointed to fact that correct type for "slot in array" is size_t, not int. It is same as correct type for malloc is size_t and not (unsigned) int.

[tiba222]

I understand that, but I also use -1 as return value to indicate nothing is found. This is impossible with size_t datatype.

As I pointed, it is possible you can return it as return (size_t)-1. This is C pattern how to do it, indicate for you that nothing was found.

I just pointed to fact that correct type for "slot in array" is size_t, not int. It is same as correct type for malloc is size_t and not (unsigned) int.

Thanks, I was not aware this easy type of casting was possible, I changed it now all back to size_t.

[tiba222]

With my last commit I made the size of the array holding the downstream hosts dynamic, so this resolves the issue with the fixed number of 20 downstream hosts. Please review this again.

/edit: Did some final fixes, compilation in travis was failing.

[pali]

I do not like current implementation for two purposes:

1. it iterates over host array always two times. First time to check if host is in array and second time to find free slot for storing new host. This is suboptimal and can be re-implemented to iterate array always only once.
2. dynamic (re)allocation of memory to arbitrary size without upper limit. Bad client on network can cause that igmpproxy exhaust all available memory on router and later could crash.

I'm still thinking about second problem and how to solve it. It must be ensured that in any case igmpproxy would not try to allocate arbitrary buffer size. Also it should be fast enough and if there is a free memory, igmpproxy may use it to handle downstream tracking host problem.

If igmpproxy is running for 10 downstream interfaces where all have /8 subnet, it is needed to use 640MB just for storing list of IPv4 addresses. It is really huge and traversing 640MB two times is slow. Plus it needs a router with enought RAM.

I think we both would agree that this approach for quickleave with allocating very big buffers does not make sense. So I would rather propose to do not store more than N IPv4 addresses, plus store it in more smarter way checking would be faster.

I'm thinking about usage of hash tables and I was told about possible solution that size of hash table could be configured by user (via config file). We can say that current igmpproxy implementation (not in this PR) uses hash table which can store one element i.e. all downstream hosts are equivalent and when one leave the multicast group, then all other hosts are also disconnected from group; igmpproxy needs to query downstream hosts and later join to that group again. And same behavior would happen also when hash collision occurre (when hash table is larger).

This has benefits: Fixed memory usage per multicast group, independently of number of downstream hosts. quickleave would work correctly when there are no hash collisions. hash collisions occures when number of downstream hosts is bigger than hash table. user can configure how much memory would be used, which also specify upper limit until "current behavior" with short stop of downstream devices may happen.

There can be also improvements like usage of just one hash table shared for all multicast groups...

Any opinion about it?

[tiba222]

I definitely agree it's better to limit te ammount of downstream hosts to be registered in a route, that was also the main motivation for the (hard) limit in my initial implementation which of course can be better made configurable.

Regarding the behavior when the array / hash table is full, I think in that case we should just not insert the new downstream hosts (and log this). What you then have is that as soon as a slot comes free the downstream host is nevertheless registered on it's next periodic group report. This also implies that the ammount of dowstream hosts that have to be stored does not have to be that large in most implementations in order to make last host tracking work correctly. You only need a large storage then in implementations where a large number of downstream hosts are joining and leaving groups very fast and also somehow timed together in batches.

What do you have in mind for the default number of downstream hosts that have to be stored on each route?

[pali]

Now I'm trying to write alternative solution with hash tables, after I finish I will push it into separate branch so you could compare and probably test it, if it would be better then current solution or your solution in this PR. For hash tables I think default size could be 32 bytes, which can store 256 non-collisions addresses, so it is approximately half of /24 subnet with a good hash function.

[pali]

I modified code from this pull request to use hash table with MurmurHash3 function and pushed code into branch hashtable. Some description is also in commit message. Can you look at it or test it, what do you think about it?

[tiba222]

I think the approach is very good, however currently it's causes a coredump on testing as soons as the first routes should be inserted.

-----------------------------------------------------
No routes in table...
-----------------------------------------------------
The IGMP message was local multicast. Ignoring.
The IGMP message was local multicast. Ignoring.
The IGMP message was local multicast. Ignoring.
RECV V2 member report   from 192.168.8.101   to 224.3.2.6
Should insert group 224.3.2.6 (from: 192.168.8.101) to route table. Vif Ix : 0
No existing route for 224.3.2.6. Create new.
Segmentation fault (core dumped)

[pali]

I have not tested it, just quickly pushed code so you can look at it...

[tiba222]

I have not tested it, just quickly pushed code so you can look at it...

I know, this is what I get from the backtrace

#0  0x0000000000208319 in zeroDownstreamHosts (conf=<optimized out>, croute=0x0) at rttable.c:107
#1  insertRoute (group=100795360, ifx=0, src=1695066304) at rttable.c:320
#2  0x00000000002068e2 in igmpProxyRun () at igmpproxy.c:326
#3  0x000000000020655d in main (ArgCn=<optimized out>, ArgVc=<optimized out>) at igmpproxy.c:153

[pali]

That is my bad, croute vs newroute. Now I fixed it in that branch.

[tiba222]

Just compiled it again (with the patches at https://github.com/pfsense/FreeBSD-ports/tree/devel/net/igmpproxy/files applied).

It is running now on my router so far without any problems, everything seems to work fine after quick testing some scenario's with my 4 settopboxes.

[pali]

Currently I cannot upgrade igmpproxy which is running on remote networks to prevent breaking somerging... so I cannot test it. But if you are fine with it after some period of testing I can push this change into master branch.

[tiba222]

Taking a closer look at the code I have only one small sugestion, I think there should be check for a hash collsion before adding the downstream host to the hashtable. In case of a colission this in my opinion should be logged as a warning to the log, this to avoid unexplainable issues with host tracking if the hash table size is too small for the deployment.

About testing, I have it running on my firewall which is on a daily basis used to watch IPTV, so I will keep a close eye on it the coming days and will let you know if everything keeps running without issues as it is right now.

[pali]

Taking a closer look at the code I have only one small sugestion, I think there should be check for a hash collsion before adding the downstream host to the hashtable. In case of a colission this in my opinion should be logged as a warning to the log, this to avoid unexplainable issues with host tracking if the hash table size is too small for the deployment.

With current implementation this is not possible. There is no way to distinguish between hash collision and trying to add same host which is already in hash table. Downstream host can send join packet more times.

[tiba222]

I understand, you would then have to store the actual dowstream addresses in the hash table instead of only bits and that's probably not worth it.

Maybe an alternative is to create a periodic check which checks the size of the hash table and generate a warning if that's nearly full?

[pali]

Full or nearly full hash table is not a problem. If there is at leat one entry in hash table it means that there is at least one downstream host and therefore no leave is send to upstream. More hosts in hash table means that there are more downstream hosts which want to receive multicast stream. And after some host which did hash collision cause erasing its bit from hash table and there is still another host in hash table then leave is not forwarded to upstream. igmpproxy is doing periodic query for downstream hosts, so other downstream host which was in hash collision with that first one, then respond to igmpproxy query and igmpproxy puts it back into the hash table. So it means that full or nearly full hash table just can prevent sending "incorrect" leave to upstream. The only problem which can happen is that all downstream hosts which are in hash table send at same time leave packet to igmpproxy. But it should be obvious that probability that such situation happen descreases with increasing number of hosts in hash table. So at the end warning for full hash table does not make sense.

[tiba222]

It would just mean theat collisions are likely to happen, but agree the chance colissions cause actual problems with host tracking are almost nil even with the default hashtable size so it's save not to warn about this.

I will test the coming days to make sure everything stays stable and let you know, thanks for all your effort so far in resolving the issue I started this push request for!

[tiba222]

I tested the new version the past days and can confirm everything is working without any issues. I think this can be safely pushed into master branch.

[pali]

Ok, thank you for testing. Have you tried changing hashtablesize config option?

[tiba222]

That's a good one, had not tested this yet because I only have 4 downstream hostss anyway. Just tested it and there seems to be an issue with parsing the config option.

My config file (relevant part):

quickleave
hashtablesize 64

But if I try to start igmpproxy then it fails on parsing the config.

igmpproxy -d -vv /tmp/igmpproxy_modified.conf
Searching for config file at '/tmp/igmpproxy_modified.conf'
Config: Quick leave mode enabled.
Config: hashtablesize for quickleave is hashtablesize.
Config: hashtablesize must be between 1 and 536870912 bytes.

/Edit: Quickly looking at the code I think the problem is that token = nextConfigToken() must be called again at line 162 of config.c to let it contain the value instead of the token itself.

[pali]

Quickly looking at the code I think the problem is that token = nextConfigToken() must be called again at line 162 of config.c to let it contain the value instead of the token itself.

You are right. I fixed it in hashtable branch.

had not tested this yet because I only have 4 downstream hostss anyway

You can e.g. set hashtablesize to 1 and see what happen. Behavior should be same as previously without this patch as it should cause that hash collision occur for every two addresses.

[tiba222]

I see, if I set it hashtablesize to 1 now still everything works, but indeed the old behavior is back then as expected.

What I meant is that I have no possibility to test with larger hashtable sizes, but with the current implementation I don't expect any issues with that either. So I think everything is fine now.

[pali]

Ok, thank you for testing, I will push this change to master branch.