Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-11 13:15, IgorMitsyanko wrote: > On 01/11/2017 02:30 PM, Felix Fietkau wrote: >> On 2017-01-11 12:26, IgorMitsyanko wrote: >>> On 01/11/2017 12:27 AM, Felix Fietkau wrote: On 2017-01-10 11:56, Johannes Berg wrote: > On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote: >> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: >>> I wonder if MAC80211 should be doing IGMP snooping and not bridge >>> in this environment. >> In the long term, yes. For now, not quite sure. > There's no "for now" in the kernel. Code added now will have to be > maintained essentially forever. I'm not sure that putting the IGMP snooping code in mac80211 is a good idea, that would be quite a bit of code duplication. This implementation works, it's very simple, and it's quite flexible for a number of use cases. Is there any remaining objection to merging this in principle (aside from potential issues with the code)? - Felix >>> >>> Hi Felix, can we consider two examples configurations with multicast >>> traffic: >>> >>> 1. AP is a source of multicast traffic itself, no bridge on AP. For >>> example, wireless video server streaming to several clients. >>> In this situation, we can not make use of possible advantages given by >>> mc-to-uc conversion? >> You could simply put the AP interface in a bridge, no need to have any >> other bridge members present. >> >>> 2. A configuration with AP + STA + 3 client devices behind STA. >>> |client 1| >>> | >>> | mc ||AP||STA|---|---|client 2| >>> |server|| >>> |client 3| >>> >>> Multicast server behind AP streams MC video traffic. All 3 clients >>> behind the STA have joined the multicast group. >>> I'm not sure if this case will be handled correctly with mc-to-uc >>> conversion in bridge on AP? >> What do you mean by "3 client devices behind STA"? Are you using a >> 4-addr STA, multicast routing, or some kind of vendor specific "client >> bridge" hackery? > > 3 client devices connected by backbone Ethernet network. Generic > case is probably STA/AP operating in 4-addr mode (more or less standard > solution as far as I know). If the AP is running in 4-addr mode, it will need to have a bridge interface anyway, because the link to the STA will be split out into a separate virtual interface (AP_VLAN iftype). In this case you don't actually need any multicast-to-unicast conversion, because the multicast traffic will be unicast on 802.11 already (due to use of 4-addr mode). - Felix
Re: [PATCH net-next] bridge: multicast to unicast
On 01/11/2017 02:30 PM, Felix Fietkau wrote: On 2017-01-11 12:26, IgorMitsyanko wrote: On 01/11/2017 12:27 AM, Felix Fietkau wrote: On 2017-01-10 11:56, Johannes Berg wrote: On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote: On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: I wonder if MAC80211 should be doing IGMP snooping and not bridge in this environment. In the long term, yes. For now, not quite sure. There's no "for now" in the kernel. Code added now will have to be maintained essentially forever. I'm not sure that putting the IGMP snooping code in mac80211 is a good idea, that would be quite a bit of code duplication. This implementation works, it's very simple, and it's quite flexible for a number of use cases. Is there any remaining objection to merging this in principle (aside from potential issues with the code)? - Felix Hi Felix, can we consider two examples configurations with multicast traffic: 1. AP is a source of multicast traffic itself, no bridge on AP. For example, wireless video server streaming to several clients. In this situation, we can not make use of possible advantages given by mc-to-uc conversion? You could simply put the AP interface in a bridge, no need to have any other bridge members present. 2. A configuration with AP + STA + 3 client devices behind STA. |client 1| | | mc ||AP||STA|---|---|client 2| |server|| |client 3| Multicast server behind AP streams MC video traffic. All 3 clients behind the STA have joined the multicast group. I'm not sure if this case will be handled correctly with mc-to-uc conversion in bridge on AP? What do you mean by "3 client devices behind STA"? Are you using a 4-addr STA, multicast routing, or some kind of vendor specific "client bridge" hackery? 3 client devices connected by backbone Ethernet network. Generic case is probably STA/AP operating in 4-addr mode (more or less standard solution as far as I know). "Client bridge" approach should not concern us here I think, it will seem to AP and AP's bridge as a single client. - Felix
Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-11 12:26, IgorMitsyanko wrote: > On 01/11/2017 12:27 AM, Felix Fietkau wrote: >> On 2017-01-10 11:56, Johannes Berg wrote: >>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote: On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: > I wonder if MAC80211 should be doing IGMP snooping and not bridge > in this environment. In the long term, yes. For now, not quite sure. >>> >>> There's no "for now" in the kernel. Code added now will have to be >>> maintained essentially forever. >> I'm not sure that putting the IGMP snooping code in mac80211 is a good >> idea, that would be quite a bit of code duplication. >> This implementation works, it's very simple, and it's quite flexible for >> a number of use cases. >> >> Is there any remaining objection to merging this in principle (aside >> from potential issues with the code)? >> >> - Felix >> > > > Hi Felix, can we consider two examples configurations with multicast > traffic: > > 1. AP is a source of multicast traffic itself, no bridge on AP. For > example, wireless video server streaming to several clients. > In this situation, we can not make use of possible advantages given by > mc-to-uc conversion? You could simply put the AP interface in a bridge, no need to have any other bridge members present. > 2. A configuration with AP + STA + 3 client devices behind STA. > |client 1| > | > | mc ||AP||STA|---|---|client 2| > |server|| > |client 3| > > Multicast server behind AP streams MC video traffic. All 3 clients > behind the STA have joined the multicast group. > I'm not sure if this case will be handled correctly with mc-to-uc > conversion in bridge on AP? What do you mean by "3 client devices behind STA"? Are you using a 4-addr STA, multicast routing, or some kind of vendor specific "client bridge" hackery? - Felix
Re: [PATCH net-next] bridge: multicast to unicast
On 01/11/2017 12:27 AM, Felix Fietkau wrote: On 2017-01-10 11:56, Johannes Berg wrote: On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote: On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: I wonder if MAC80211 should be doing IGMP snooping and not bridge in this environment. In the long term, yes. For now, not quite sure. There's no "for now" in the kernel. Code added now will have to be maintained essentially forever. I'm not sure that putting the IGMP snooping code in mac80211 is a good idea, that would be quite a bit of code duplication. This implementation works, it's very simple, and it's quite flexible for a number of use cases. Is there any remaining objection to merging this in principle (aside from potential issues with the code)? - Felix Hi Felix, can we consider two examples configurations with multicast traffic: 1. AP is a source of multicast traffic itself, no bridge on AP. For example, wireless video server streaming to several clients. In this situation, we can not make use of possible advantages given by mc-to-uc conversion? 2. A configuration with AP + STA + 3 client devices behind STA. |client 1| | | mc ||AP||STA|---|---|client 2| |server|| |client 3| Multicast server behind AP streams MC video traffic. All 3 clients behind the STA have joined the multicast group. I'm not sure if this case will be handled correctly with mc-to-uc conversion in bridge on AP?
Re: [PATCH net-next] bridge: multicast to unicast
> > Exactly. My point is that this is breaking the expectation that > > hosts are actually able to drop such packets. > > [readding CCs I removed earlier] > > Ah! Thanks. I was worried about creating packetloss :D. Ah, well, no - at least not in this case. > Hm, for this other other way round, I think it does not apply for > the bridge multicast-to-unicast patch if I'm not misreading the > bridge code: > > For a packet with a link-layer multicast address but a unicast IP > destination, the bridge MDB lookup will fail. > (http://lxr.free-electrons.com/source/net/bridge/br_multicast.c?v=4.8 > #L178 > returns NULL) > > Case A): No multicast router on port: > -> bridge, br_multicast_flood(), will drop the packet already > (no matter if multicast-to-unicast is enabled or not) > > Case B): Multicast router present on port: > -> The new patch does not apply multicast-to-unicast but just floods > packet unaltered > ("else { port = rport; addr = NULL; }" branch) Ah, interesting. This is different then - the mac80211 code is not L3 aware at all. johannes
Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-10 11:56, Johannes Berg wrote: > On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote: >> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: >> > I wonder if MAC80211 should be doing IGMP snooping and not bridge >> > in this environment. >> >> In the long term, yes. For now, not quite sure. > > There's no "for now" in the kernel. Code added now will have to be > maintained essentially forever. I'm not sure that putting the IGMP snooping code in mac80211 is a good idea, that would be quite a bit of code duplication. This implementation works, it's very simple, and it's quite flexible for a number of use cases. Is there any remaining objection to merging this in principle (aside from potential issues with the code)? - Felix
Re: [PATCH net-next] bridge: multicast to unicast
On Tue, Jan 10, 2017 at 9:23 AM, Felix Fietkau wrote: > On 2017-01-10 18:17, Dave Taht wrote: >> In the case of wifi I have 3 issues with this line of thought. >> >> multicast in wifi has generally supposed to be unreliable. This makes >> it reliable. reliability comes at a cost - >> >> multicast is typically set at a fixed low rate today. unicast is >> retried at different rates until it succeeds - for every station >> listening. If one station is already at the lowest rate, the total >> cost of the transmit increases, rather than decreases. >> >> unicast gets block acks until it succeeds. Again, more delay. >> >> I think there is something like 31 soft-retries in the ath9k driver > If I remember correctly, hardware retries are counted here as well. I chopped this to something more reasonable but never got around to quantifying it, so never pushed the patch. I figured I'd measure ATF in a noisy environment (which I'd be doing now if it weren't for https://bugs.lede-project.org/index.php?do=details&task_id=368 ) first. >> what happens to diffserv markings here? for unicast CS1 goes into the >> BE queue, CS6, the VO queue. Do we go from one flat queue for all of >> multicast to punching it through one of the hardware queues based on >> the diffserv mark now with this patch? I meant CS1=BK here. Tracing the path through the bridge code made my head hurt, I can go look at some aircaps to see if the mcast->unicast conversion respects those markings or not (my vote is *not*). >> I would like it if there was a way to preserve the unreliability >> (which multiple mesh protocols depend on), send stuff with QoSNoack, >> etc - or dynamically choose (based on the rates of the stations) >> between conventional multicast and unicast. >> >> Or - better, IMHO, keep sending multicast as is but pick the best of >> the rates available to all the listening stations for it. > The advantage of the multicast-to-unicast conversion goes beyond simply > selecting a better rate - aggregation matters a lot as well, and that is > simply incompatible with normal multicast. Except for the VO queue which cannot aggregate. And for that matter, using any other hardware queue than BE tends to eat a txop that would otherwise possibly be combined with an aggregate. (and the VI queue has always misbehaved, long on my todo list) > Some multicast streams use lots of small-ish packets, the airtime impact > of those is vastly reduced, even if the transmission has to be > duplicated for a few stations. The question was basically how far up does it scale. Arguably, for a very few, well connected stations, this patch would help. For a network with more - and more badly connected stations, I think it would hurt. What sorts of multicast traffic are being observed that flood the network sufficiently to be worth optimizing out? arp? nd? upnp? mdns? uftp? tv? (my questions above are related to basically trying to setup a sane a/b test, I've been building up a new testbed in noisy environment to match the one I have in a quiet one, and don't have any "good" mcast tests defined. Has anyone done an a/b test of this code with some repeatable test already?) (In my observations... The only truly heavy creator of a multicast "burp" has tended to be upnp and mdns on smaller networks. Things like nd and arp get more problematic as the number of stations go up also. I can try things like abusing vlc or uftp to see what happens?) I certainly agree multicast is a "problem" (I've seen 20-80% or more of a given wifi network eaten by multicast) but I'm not convinced that making it reliable, aggregatable unicast scales much past basement-level testing of a few "good" stations, and don't know which protocols are making it worse, the worst, in typical environments. Certainly apple gear puts out a lot of multicast. ... As best as I recall a recommendation in the 802.11-2012 standard was that multicast packets be rate-limited so that you'd have a fixed amount of crap after each beacon sufficient to keep the rest of the unicast traffic flowing rapidly, instead of dumping everything into a given beacon transmit. That, combined with (maybe) picking the "best" union of known rates per station, was essentially the strategy I'd intended[1] to pursue for tackling the currently infinite wifi multicast queue - fq the entries, have a fairly short queue (codel is not the best choice here) drop from head, and limit the number of packets transmitted per beacon to spread them out. That would solve the issue for sparse multicast (dhcp etc), and smooth out the burps from bigger chunks while impacting conventional unicast minimally. There's also the pursuit of less multicast overall at least in some protocols https://tools.ietf.org/html/draft-ietf-dnssd-hybrid-05 > > - Felix [1] but make-wifi-fast has been out of funding since august -- Dave Täht Let's go make home routers and wifi faster! With better software! http://blog.cerowrt.org
Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-10 18:17, Dave Taht wrote: > In the case of wifi I have 3 issues with this line of thought. > > multicast in wifi has generally supposed to be unreliable. This makes > it reliable. reliability comes at a cost - > > multicast is typically set at a fixed low rate today. unicast is > retried at different rates until it succeeds - for every station > listening. If one station is already at the lowest rate, the total > cost of the transmit increases, rather than decreases. > > unicast gets block acks until it succeeds. Again, more delay. > > I think there is something like 31 soft-retries in the ath9k driver If I remember correctly, hardware retries are counted here as well. > what happens to diffserv markings here? for unicast CS1 goes into the > BE queue, CS6, the VO queue. Do we go from one flat queue for all of > multicast to punching it through one of the hardware queues based on > the diffserv mark now with this patch? > > I would like it if there was a way to preserve the unreliability > (which multiple mesh protocols depend on), send stuff with QoSNoack, > etc - or dynamically choose (based on the rates of the stations) > between conventional multicast and unicast. > > Or - better, IMHO, keep sending multicast as is but pick the best of > the rates available to all the listening stations for it. The advantage of the multicast-to-unicast conversion goes beyond simply selecting a better rate - aggregation matters a lot as well, and that is simply incompatible with normal multicast. Some multicast streams use lots of small-ish packets, the airtime impact of those is vastly reduced, even if the transmission has to be duplicated for a few stations. - Felix
Re: [PATCH net-next] bridge: multicast to unicast
In the case of wifi I have 3 issues with this line of thought. multicast in wifi has generally supposed to be unreliable. This makes it reliable. reliability comes at a cost - multicast is typically set at a fixed low rate today. unicast is retried at different rates until it succeeds - for every station listening. If one station is already at the lowest rate, the total cost of the transmit increases, rather than decreases. unicast gets block acks until it succeeds. Again, more delay. I think there is something like 31 soft-retries in the ath9k driver what happens to diffserv markings here? for unicast CS1 goes into the BE queue, CS6, the VO queue. Do we go from one flat queue for all of multicast to punching it through one of the hardware queues based on the diffserv mark now with this patch? I would like it if there was a way to preserve the unreliability (which multiple mesh protocols depend on), send stuff with QoSNoack, etc - or dynamically choose (based on the rates of the stations) between conventional multicast and unicast. Or - better, IMHO, keep sending multicast as is but pick the best of the rates available to all the listening stations for it. Has anyone actually looked at the effects of this with, say, 5-10 stations at middlin to poor quality (longer distance)? using something to measure the real effect of the multicast conversion? (uftp, mdns?)
Re: [PATCH net-next] bridge: multicast to unicast
On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote: > On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: > > I wonder if MAC80211 should be doing IGMP snooping and not bridge > > in this environment. > > In the long term, yes. For now, not quite sure. There's no "for now" in the kernel. Code added now will have to be maintained essentially forever. johannes
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote: > I wonder if MAC80211 should be doing IGMP snooping and not bridge > in this environment. In the long term, yes. For now, not quite sure. I personally like to go for simple solutions first :).
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, Jan 09, 2017 at 10:42:46PM +0100, Johannes Berg wrote: > On Mon, 2017-01-09 at 22:33 +0100, Linus Lüssing wrote: > > On Mon, Jan 09, 2017 at 01:44:03PM +0100, Johannes Berg wrote: > > > > > > > > A host SHOULD silently discard a datagram that is > > > > > received via > > > > > a link-layer broadcast (see Section 2.4) but does not > > > > > specify > > > > > an IP multicast or broadcast destination address. > > > > > > > > This example is the other way round. It specifies how the IP > > > > destination should look like in case of link-layer broadcast. Not > > > > how the link-layer destination should look like in case of a > > > > multicast/broadcast IP destination. > > > > > > You stopped reading too early - snipped the context part for you :) > > > > Sorry for writing to you directly, but I still have some > > difficulties. In pseudo-code that line says: > > > > - > > if ll_dst(pkt) == bcast AND ip_dst(pkt) != mcast/bcast: > > -> drop(pkt) > > - > > > > But after multicast-to-unicast conversion, we have: > > > > - > > ll_dst(pkt) == ucast AND ip_dst(pkt) == mcast > > - > > > > So none of the two requirements for dropping are matched? > > > > Exactly. My point is that this is breaking the expectation that hosts > are actually able to drop such packets. [readding CCs I removed earlier] Ah! Thanks. I was worried about creating packetloss :D. Hm, for this other other way round, I think it does not apply for the bridge multicast-to-unicast patch if I'm not misreading the bridge code: For a packet with a link-layer multicast address but a unicast IP destination, the bridge MDB lookup will fail. (http://lxr.free-electrons.com/source/net/bridge/br_multicast.c?v=4.8#L178 returns NULL) Case A): No multicast router on port: -> bridge, br_multicast_flood(), will drop the packet already (no matter if multicast-to-unicast is enabled or not) Case B): Multicast router present on port: -> The new patch does not apply multicast-to-unicast but just floods packet unaltered ("else { port = rport; addr = NULL; }" branch) Regards, Linus
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, 9 Jan 2017 22:23:45 +0100 Linus Lüssing wrote: > On Mon, Jan 09, 2017 at 12:44:19PM +0100, M. Braun wrote: > > Am 09.01.2017 um 09:08 schrieb Johannes Berg: > > > Does it make sense to implement the two in separate layers though? > > > > > > Clearly, this part needs to be implemented in the bridge layer due to > > > the snooping knowledge, but the code is very similar to what mac80211 > > > has now. > > > > Does the bridge always know about all stations connected? > > The bridge does not always know about all stations, especially the > silent ones like in your DVB-T example. > > However, concerning IP multicast, there is IGMP/MLD. So the bridge > does know about all stations which are interested in a specific IP > multicast stream. > > (As long as there is a querier on the link, which periodically > queriers for IGMP/MLD reports from any listener. If there is no > querier then the bridge multicast snooping, including the bridge > multicast-to-unicast will fall back to flooding) > > > So if your television example uses IP multicast properly, it is > completely doable with the bridge multicast-to-unicast, thanks to > IGMP/MLD. I wonder if MAC80211 should be doing IGMP snooping and not bridge in this environment.
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, Jan 09, 2017 at 12:44:19PM +0100, M. Braun wrote: > Am 09.01.2017 um 09:08 schrieb Johannes Berg: > > Does it make sense to implement the two in separate layers though? > > > > Clearly, this part needs to be implemented in the bridge layer due to > > the snooping knowledge, but the code is very similar to what mac80211 > > has now. > > Does the bridge always know about all stations connected? The bridge does not always know about all stations, especially the silent ones like in your DVB-T example. However, concerning IP multicast, there is IGMP/MLD. So the bridge does know about all stations which are interested in a specific IP multicast stream. (As long as there is a querier on the link, which periodically queriers for IGMP/MLD reports from any listener. If there is no querier then the bridge multicast snooping, including the bridge multicast-to-unicast will fall back to flooding) So if your television example uses IP multicast properly, it is completely doable with the bridge multicast-to-unicast, thanks to IGMP/MLD.
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, 2017-01-09 at 16:25 +0100, michael-dev wrote: > Am 09.01.2017 13:15, schrieb Johannes Berg: > > > That is bridge fdb entries (need to) expire so the bridge might > > > "forget" a still-connected station not sending but only consuming > > > broadcast traffic. > > > > Ok, that I don't know. Somehow if you address a unicast packet > > there > > the bridge has to make a decision - so it really should know? > > If the bridge has not learned the unicast destination mac address on > any port, it will flood the packet on all ports except the port it > received the packet on. Ok, so this really needs to be done in mac80211. I'll send out the pull request soon then. johannes
Re: [PATCH net-next] bridge: multicast to unicast
Am 09.01.2017 13:15, schrieb Johannes Berg: That is bridge fdb entries (need to) expire so the bridge might "forget" a still-connected station not sending but only consuming broadcast traffic. Ok, that I don't know. Somehow if you address a unicast packet there the bridge has to make a decision - so it really should know? If the bridge has not learned the unicast destination mac address on any port, it will flood the packet on all ports except the port it received the packet on. Regards, M. Braun
Re: [PATCH net-next] bridge: multicast to unicast
> > A host SHOULD silently discard a datagram that is received via > > a link-layer broadcast (see Section 2.4) but does not specify > > an IP multicast or broadcast destination address. > > This example is the other way round. It specifies how the IP > destination should look like in case of link-layer broadcast. Not > how the link-layer destination should look like in case of a > multicast/broadcast IP destination. You stopped reading too early - snipped the context part for you :) johannes
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, Jan 09, 2017 at 09:05:49AM +0100, Johannes Berg wrote: > On Sat, 2017-01-07 at 16:15 +0100, Linus Lüssing wrote: > > > Actually, I do not quite understand that remark in the mac80211 > > multicast-to-unicast patch. IP should not care about the ethernet > > header? > > But it does, for example RFC 1122 states: > > When a host sends a datagram to a link-layer broadcast address, > the IP destination address MUST be a legal IP broadcast or IP > multicast address. > > A host SHOULD silently discard a datagram that is received via > a link-layer broadcast (see Section 2.4) but does not specify > an IP multicast or broadcast destination address. This example is the other way round. It specifies how the IP destination should look like in case of link-layer broadcast. Not how the link-layer destination should look like in case of a multicast/broadcast IP destination. Any other examples?
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, 2017-01-09 at 12:44 +0100, M. Braun wrote: > Am 09.01.2017 um 09:08 schrieb Johannes Berg: > > Does it make sense to implement the two in separate layers though? > > > > Clearly, this part needs to be implemented in the bridge layer due > > to > > the snooping knowledge, but the code is very similar to what > > mac80211 > > has now. > > Does the bridge always know about all stations connected? > > That is bridge fdb entries (need to) expire so the bridge might > "forget" a still-connected station not sending but only consuming > broadcast traffic. > > E.g. there is a television broadcast station here that receives a > video stream (via wifi, udp packets) and then airs it (dvb-t) but (on > its own) would not send any data packet on wifi (static ip, etc.). Ok, that I don't know. Somehow if you address a unicast packet there the bridge has to make a decision - so it really should know? Would it query the port somehow to see if the device is behind it, if getting a packet for a station it forgot about? > An other reason to implement this in mac80211 initially was that > mac80211 could encapsulate broacast/multicast ethernet packtes in > unicast A-MSDU packets in a way, so that the receiver would still see > process ethernet packets (after conversion) but have unicast wifi > frames. This cannot be done in bridge easily but one might want to > add this later to mac80211. Yes, DMG would have to be done in mac80211, but that's a lot clearer case too since it requires negotiation functionality etc. johannes
Re: [PATCH net-next] bridge: multicast to unicast
Am 09.01.2017 um 09:08 schrieb Johannes Berg: > Does it make sense to implement the two in separate layers though? > > Clearly, this part needs to be implemented in the bridge layer due to > the snooping knowledge, but the code is very similar to what mac80211 > has now. Does the bridge always know about all stations connected? That is bridge fdb entries (need to) expire so the bridge might "forget" a still-connected station not sending but only consuming broadcast traffic. E.g. there is a television broadcast station here that receives a video stream (via wifi, udp packets) and then airs it (dvb-t) but (on its own) would not send any data packet on wifi (static ip, etc.). An other reason to implement this in mac80211 initially was that mac80211 could encapsulate broacast/multicast ethernet packtes in unicast A-MSDU packets in a way, so that the receiver would still see process ethernet packets (after conversion) but have unicast wifi frames. This cannot be done in bridge easily but one might want to add this later to mac80211. Michael
RE: [PATCH net-next] bridge: multicast to unicast
> -Message d'origine- > De : linux-wireless-ow...@vger.kernel.org [mailto:linux-wireless- > ow...@vger.kernel.org] De la part de Stephen Hemminger > Envoyé : samedi 7 janvier 2017 04:14 > À : Linus Lüssing > Cc : netdev@vger.kernel.org; David S . Miller; bridge@lists.linux- > foundation.org; linux-ker...@vger.kernel.org; linux- > wirel...@vger.kernel.org; Felix Fietkau > Objet : Re: [PATCH net-next] bridge: multicast to unicast > > On Mon, 2 Jan 2017 20:32:14 +0100 > Linus Lüssing wrote: > > > This feature is intended for interface types which have a more > > reliable and/or efficient way to deliver unicast packets than > > broadcast ones (e.g. wifi). > > > Why is this not done in MAC80211 rather than bridge? OTOH mac80211 has more information to decide whether it is more economic to send one multicast or several unicast. It depends on the bitrate of each station, number of stations and the (not necessarily slower) bitrate of multicasts.
Re: [PATCH net-next] bridge: multicast to unicast
On Sat, 2017-01-07 at 15:55 +0100, Linus Lüssing wrote: > On Sat, Jan 07, 2017 at 11:32:57AM +0100, M. Braun wrote: > > Am 06.01.2017 um 14:54 schrieb Johannes Berg: > > > > > > > The bridge layer can use IGMP snooping to ensure that the > > > > multicast > > > > stream is only transmitted to clients that are actually a > > > > member of > > > > the group. Can the mac80211 feature do the same? > > > > > > No, it'll convert the packet for all clients that are behind that > > > netdev. But that's an argument for dropping the mac80211 feature, > > > which > > > hasn't been merged upstream yet, no? > > > > But there is multicast/broadcast traffic like e.g. ARP and some IP > > multicast groups that are not covered by IGMP snooping. The > > mac80211 > > patch converts this to unicast as well, which the bridge cannot do. > > > > That way, these features both complement and overlap each other. > > Right, I'd agree with that. Ok. > I didn't write it explicitly in the commit message, but yes, the > like anything concerning bridge multicast snooping, bridge > multicast-to-unicast can only affect packets as noted in > RFC4541 ("Considerations for Internet Group Management Protocol (IGMP) > and Multicast Listener Discovery (MLD) Snooping Switches"), too. > > So it is only working for IPv4 multicast, excluding link-local > (224.0.0.0/24), and IPv6 multicast, excluding all-host-multicast > (ff02::1). > > And does not concern ARP in any way. > > > The nice complementary effect is, that the bridge can first sieve > out those IP packets thanks to IGMP/MLD snooping knowledge and for > anything else, like ARP, 224.0.0.x or ff02::1, the mac80211 > multicast-to-unicast could do its job. > > > For APs with a small number of STAs (like your private home AP), > you might want to enable both bridge multicast-to-unicast and > mac80211 multicast-to-unicast for this complementary effect. While > on public APs with 30 to 50 STAs with varying distances and bitrates, > you might only one to enable the bridge one, because sending an ARP > packet 50x might actually reduce performance and airtime > significantly. Does it make sense to implement the two in separate layers though? Clearly, this part needs to be implemented in the bridge layer due to the snooping knowledge, but the code is very similar to what mac80211 has now. It would probably not make sense to combine the two options into one, but it seems relatively simple for bridge to also implement the one mac80211 tentatively has now, with multiple benefits: * single place for configuration, leading to less possible confusion * single implementation for all wireless devices, including ones with Full-MAC firmware that don't use mac80211 * code sharing for the duplication, although admittedly not so much Thoughts? johannes
Re: [PATCH net-next] bridge: multicast to unicast
On Sat, 2017-01-07 at 16:15 +0100, Linus Lüssing wrote: > Actually, I do not quite understand that remark in the mac80211 > multicast-to-unicast patch. IP should not care about the ethernet > header? But it does, for example RFC 1122 states: When a host sends a datagram to a link-layer broadcast address, the IP destination address MUST be a legal IP broadcast or IP multicast address. A host SHOULD silently discard a datagram that is received via a link-layer broadcast (see Section 2.4) but does not specify an IP multicast or broadcast destination address. You can probably find other examples too. johannes
Re: [PATCH net-next] bridge: multicast to unicast
On Fri, Jan 06, 2017 at 01:47:52PM +0100, Johannes Berg wrote: > How does this compare and/or relate to the multicast-to-unicast feature > we were going to add to the wifi stack, particularly mac80211? Do we > perhaps not need that feature at all, if bridging will have it? > > I suppose that the feature there could apply also to locally generated > traffic when the AP interface isn't in a bridge, but I think I could > live with requiring the AP to be put into a bridge to achieve a similar > configuration? > > Additionally, on an unrelated note, this seems to apply generically to > all kinds of frames, losing information by replacing the address. > Shouldn't it have similar limitations as the wifi stack feature has > then, like only applying to ARP, IPv4, IPv6 and not general protocols? (should all three be answered with Michael's and my reply to Michael's mail, I think) > > Also, it should probably come with the same caveat as we documented for > the wifi feature: > > Note that this may break certain expectations of the receiver, > such as the ability to drop unicast IP packets received within > multicast L2 frames, or the ability to not send ICMP destination > unreachable messages for packets received in L2 multicast (which > is required, but the receiver can't tell the difference if this > new option is enabled.) Actually, I do not quite understand that remark in the mac80211 multicast-to-unicast patch. IP should not care about the ethernet header?
Re: [PATCH net-next] bridge: multicast to unicast
On Fri, Jan 06, 2017 at 07:13:56PM -0800, Stephen Hemminger wrote: > On Mon, 2 Jan 2017 20:32:14 +0100 > Linus Lüssing wrote: > > > This feature is intended for interface types which have a more reliable > > and/or efficient way to deliver unicast packets than broadcast ones > > (e.g. wifi). > > > Why is this not done in MAC80211 rather than bridge? Because mac80211 does not have the IGMP/MLD snooping code as the bridge has? Reimplementing the snooping in mac80211 does not make sense because of duplicating code. Moving the snooping code from the bridge to mac80211 does not make sense either, because we want multicast snooping, software based, (virtually) wired switches, too. The "best way" (TM) would probably be to migrate the IGMP/MLD snooping from the bridge code to the net device code on the long run to make such a database usable for any kind of device, without needing this bridge hack. But such a migration would also need a way more invasive patchset. While Felix's idea might look a little "ugly" due it's hacky nature, I think it is also quite beautiful thanks to it's simplicity.
Re: [PATCH net-next] bridge: multicast to unicast
On Sat, Jan 07, 2017 at 11:32:57AM +0100, M. Braun wrote: > Am 06.01.2017 um 14:54 schrieb Johannes Berg: > > > >> The bridge layer can use IGMP snooping to ensure that the multicast > >> stream is only transmitted to clients that are actually a member of > >> the group. Can the mac80211 feature do the same? > > > > No, it'll convert the packet for all clients that are behind that > > netdev. But that's an argument for dropping the mac80211 feature, which > > hasn't been merged upstream yet, no? > > But there is multicast/broadcast traffic like e.g. ARP and some IP > multicast groups that are not covered by IGMP snooping. The mac80211 > patch converts this to unicast as well, which the bridge cannot do. > > That way, these features both complement and overlap each other. Right, I'd agree with that. I didn't write it explicitly in the commit message, but yes, the like anything concerning bridge multicast snooping, bridge multicast-to-unicast can only affect packets as noted in RFC4541 ("Considerations for Internet Group Management Protocol (IGMP) and Multicast Listener Discovery (MLD) Snooping Switches"), too. So it is only working for IPv4 multicast, excluding link-local (224.0.0.0/24), and IPv6 multicast, excluding all-host-multicast (ff02::1). And does not concern ARP in any way. The nice complementary effect is, that the bridge can first sieve out those IP packets thanks to IGMP/MLD snooping knowledge and for anything else, like ARP, 224.0.0.x or ff02::1, the mac80211 multicast-to-unicast could do its job. For APs with a small number of STAs (like your private home AP), you might want to enable both bridge multicast-to-unicast and mac80211 multicast-to-unicast for this complementary effect. While on public APs with 30 to 50 STAs with varying distances and bitrates, you might only one to enable the bridge one, because sending an ARP packet 50x might actually reduce performance and airtime significantly.
Re: [PATCH net-next] bridge: multicast to unicast
Am 06.01.2017 um 14:54 schrieb Johannes Berg: > >> The bridge layer can use IGMP snooping to ensure that the multicast >> stream is only transmitted to clients that are actually a member of >> the group. Can the mac80211 feature do the same? > > No, it'll convert the packet for all clients that are behind that > netdev. But that's an argument for dropping the mac80211 feature, which > hasn't been merged upstream yet, no? But there is multicast/broadcast traffic like e.g. ARP and some IP multicast groups that are not covered by IGMP snooping. The mac80211 patch converts this to unicast as well, which the bridge cannot do. That way, these features both complement and overlap each other. Regards, Michael
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, 2 Jan 2017 20:32:14 +0100 Linus Lüssing wrote: > This feature is intended for interface types which have a more reliable > and/or efficient way to deliver unicast packets than broadcast ones > (e.g. wifi). Why is this not done in MAC80211 rather than bridge?
Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-06 14:54, Johannes Berg wrote: > >> The bridge layer can use IGMP snooping to ensure that the multicast >> stream is only transmitted to clients that are actually a member of >> the group. Can the mac80211 feature do the same? > > No, it'll convert the packet for all clients that are behind that > netdev. But that's an argument for dropping the mac80211 feature, which > hasn't been merged upstream yet, no? Right. - Felix
Re: [PATCH net-next] bridge: multicast to unicast
> The bridge layer can use IGMP snooping to ensure that the multicast > stream is only transmitted to clients that are actually a member of > the group. Can the mac80211 feature do the same? No, it'll convert the packet for all clients that are behind that netdev. But that's an argument for dropping the mac80211 feature, which hasn't been merged upstream yet, no? johannes
Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-06 13:47, Johannes Berg wrote: > On Mon, 2017-01-02 at 20:32 +0100, Linus Lüssing wrote: >> Implements an optional, per bridge port flag and feature to deliver >> multicast packets to any host on the according port via unicast >> individually. This is done by copying the packet per host and >> changing the multicast destination MAC to a unicast one accordingly. > > How does this compare and/or relate to the multicast-to-unicast feature > we were going to add to the wifi stack, particularly mac80211? Do we > perhaps not need that feature at all, if bridging will have it? > > I suppose that the feature there could apply also to locally generated > traffic when the AP interface isn't in a bridge, but I think I could > live with requiring the AP to be put into a bridge to achieve a similar > configuration? > > Additionally, on an unrelated note, this seems to apply generically to > all kinds of frames, losing information by replacing the address. > Shouldn't it have similar limitations as the wifi stack feature has > then, like only applying to ARP, IPv4, IPv6 and not general protocols? > > Also, it should probably come with the same caveat as we documented for > the wifi feature: > > Note that this may break certain expectations of the receiver, > such as the ability to drop unicast IP packets received within > multicast L2 frames, or the ability to not send ICMP destination > unreachable messages for packets received in L2 multicast (which > is required, but the receiver can't tell the difference if this > new option is enabled.) > > > I'll hold off sending my tree in until we see that we really need both > features, or decide that we want the wifi feature *instead* of the > bridge feature. The bridge layer can use IGMP snooping to ensure that the multicast stream is only transmitted to clients that are actually a member of the group. Can the mac80211 feature do the same? - Felix
Re: [PATCH net-next] bridge: multicast to unicast
On Mon, 2017-01-02 at 20:32 +0100, Linus Lüssing wrote: > Implements an optional, per bridge port flag and feature to deliver > multicast packets to any host on the according port via unicast > individually. This is done by copying the packet per host and > changing the multicast destination MAC to a unicast one accordingly. How does this compare and/or relate to the multicast-to-unicast feature we were going to add to the wifi stack, particularly mac80211? Do we perhaps not need that feature at all, if bridging will have it? I suppose that the feature there could apply also to locally generated traffic when the AP interface isn't in a bridge, but I think I could live with requiring the AP to be put into a bridge to achieve a similar configuration? Additionally, on an unrelated note, this seems to apply generically to all kinds of frames, losing information by replacing the address. Shouldn't it have similar limitations as the wifi stack feature has then, like only applying to ARP, IPv4, IPv6 and not general protocols? Also, it should probably come with the same caveat as we documented for the wifi feature: Note that this may break certain expectations of the receiver, such as the ability to drop unicast IP packets received within multicast L2 frames, or the ability to not send ICMP destination unreachable messages for packets received in L2 multicast (which is required, but the receiver can't tell the difference if this new option is enabled.) I'll hold off sending my tree in until we see that we really need both features, or decide that we want the wifi feature *instead* of the bridge feature. johannes
Re: [PATCH net-next] bridge: multicast to unicast
On 2017-01-02 20:32, Linus Lüssing wrote: > Implements an optional, per bridge port flag and feature to deliver > multicast packets to any host on the according port via unicast > individually. This is done by copying the packet per host and > changing the multicast destination MAC to a unicast one accordingly. > > multicast-to-unicast works on top of the multicast snooping feature of > the bridge. Which means unicast copies are only delivered to hosts which > are interested in it and signalized this via IGMP/MLD reports > previously. > > This feature is intended for interface types which have a more reliable > and/or efficient way to deliver unicast packets than broadcast ones > (e.g. wifi). > > However, it should only be enabled on interfaces where no IGMPv2/MLDv1 > report suppression takes place. This feature is disabled by default. > > The initial patch and idea is from Felix Fietkau. > > Cc: Felix Fietkau > Signed-off-by: Linus Lüssing Please add Signed-off-by: Felix Fietkau in the next version, and maybe also From: Thanks, - Felix
Re: [PATCH net-next] bridge: multicast to unicast
On 02/01/17 20:32, Linus Lüssing wrote: Implements an optional, per bridge port flag and feature to deliver multicast packets to any host on the according port via unicast individually. This is done by copying the packet per host and changing the multicast destination MAC to a unicast one accordingly. multicast-to-unicast works on top of the multicast snooping feature of the bridge. Which means unicast copies are only delivered to hosts which are interested in it and signalized this via IGMP/MLD reports previously. This feature is intended for interface types which have a more reliable and/or efficient way to deliver unicast packets than broadcast ones (e.g. wifi). However, it should only be enabled on interfaces where no IGMPv2/MLDv1 report suppression takes place. This feature is disabled by default. The initial patch and idea is from Felix Fietkau. Cc: Felix Fietkau Signed-off-by: Linus Lüssing --- Hi Linus, A few comments below, in general I have 2 concerns: the new mcast fast-path tests and cache line ref, and adding netlink support for the new flag. This feature is used and enabled by default in OpenWRT and LEDE for AP interfaces for more than a year now to allow both a more robust multicast delivery and multicast at higher rates (e.g. multicast streaming). In OpenWRT/LEDE the IGMP/MLD report suppression issue is overcome by the network daemon enabling AP isolation and by that separating all STAs. Delivery of STA-to-STA IP mulitcast is made possible again by enabling and utilizing the bridge hairpin mode, which considers the incoming port as a potential outgoing port, too. Hairpin-mode is performed after multicast snooping, therefore leading to only deliver reports to STAs running a multicast router. --- include/linux/if_bridge.h | 1 + net/bridge/br_forward.c | 44 +-- net/bridge/br_mdb.c | 2 +- net/bridge/br_multicast.c | 92 ++- net/bridge/br_private.h | 4 ++- net/bridge/br_sysfs_if.c | 2 ++ 6 files changed, 115 insertions(+), 30 deletions(-) diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h index c6587c0..f1b0d78 100644 --- a/include/linux/if_bridge.h +++ b/include/linux/if_bridge.h @@ -46,6 +46,7 @@ struct br_ip_list { #define BR_LEARNING_SYNC BIT(9) #define BR_PROXYARP_WIFI BIT(10) #define BR_MCAST_FLOOD BIT(11) +#define BR_MULTICAST_TO_UCAST BIT(12) #define BR_DEFAULT_AGEING_TIME (300 * HZ) diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c index 7cb41ae..49d742d 100644 --- a/net/bridge/br_forward.c +++ b/net/bridge/br_forward.c @@ -174,6 +174,33 @@ static struct net_bridge_port *maybe_deliver( return p; } +static struct net_bridge_port *maybe_deliver_addr( + struct net_bridge_port *prev, struct net_bridge_port *p, + struct sk_buff *skb, const unsigned char *addr, + bool local_orig) +{ + struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev; + const unsigned char *src = eth_hdr(skb)->h_source; + + if (!should_deliver(p, skb)) + return prev; + + /* Even with hairpin, no soliloquies - prevent breaking IPv6 DAD */ + if (skb->dev == p->dev && ether_addr_equal(src, addr)) + return prev; + + skb = skb_copy(skb, GFP_ATOMIC); + if (!skb) { + dev->stats.tx_dropped++; + return prev; + } + + memcpy(eth_hdr(skb)->h_dest, addr, ETH_ALEN); + __br_forward(p, skb, local_orig); + + return prev; +} + /* called under rcu_read_lock */ void br_flood(struct net_bridge *br, struct sk_buff *skb, enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) @@ -231,6 +258,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, struct net_bridge_port *prev = NULL; struct net_bridge_port_group *p; struct hlist_node *rp; + const unsigned char *addr; nit: please arrange these into reverse christmas tree rp = rcu_dereference(hlist_first_rcu(&br->router_list)); p = mdst ? rcu_dereference(mdst->ports) : NULL; @@ -241,10 +269,20 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) : NULL; - port = (unsigned long)lport > (unsigned long)rport ? - lport : rport; + if ((unsigned long)lport > (unsigned long)rport) { + port = lport; + addr = p->unicast ? p->eth_addr : NULL; + } else { + port = rport; + addr = NULL; + } + + if (addr) + prev = maybe_deliver_addr(prev, port, skb, addr, + local_orig); + else + prev = maybe_deliver(prev, port, skb, local_orig); This hunk
[PATCH net-next] bridge: multicast to unicast
Implements an optional, per bridge port flag and feature to deliver multicast packets to any host on the according port via unicast individually. This is done by copying the packet per host and changing the multicast destination MAC to a unicast one accordingly. multicast-to-unicast works on top of the multicast snooping feature of the bridge. Which means unicast copies are only delivered to hosts which are interested in it and signalized this via IGMP/MLD reports previously. This feature is intended for interface types which have a more reliable and/or efficient way to deliver unicast packets than broadcast ones (e.g. wifi). However, it should only be enabled on interfaces where no IGMPv2/MLDv1 report suppression takes place. This feature is disabled by default. The initial patch and idea is from Felix Fietkau. Cc: Felix Fietkau Signed-off-by: Linus Lüssing --- This feature is used and enabled by default in OpenWRT and LEDE for AP interfaces for more than a year now to allow both a more robust multicast delivery and multicast at higher rates (e.g. multicast streaming). In OpenWRT/LEDE the IGMP/MLD report suppression issue is overcome by the network daemon enabling AP isolation and by that separating all STAs. Delivery of STA-to-STA IP mulitcast is made possible again by enabling and utilizing the bridge hairpin mode, which considers the incoming port as a potential outgoing port, too. Hairpin-mode is performed after multicast snooping, therefore leading to only deliver reports to STAs running a multicast router. --- include/linux/if_bridge.h | 1 + net/bridge/br_forward.c | 44 +-- net/bridge/br_mdb.c | 2 +- net/bridge/br_multicast.c | 92 ++- net/bridge/br_private.h | 4 ++- net/bridge/br_sysfs_if.c | 2 ++ 6 files changed, 115 insertions(+), 30 deletions(-) diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h index c6587c0..f1b0d78 100644 --- a/include/linux/if_bridge.h +++ b/include/linux/if_bridge.h @@ -46,6 +46,7 @@ struct br_ip_list { #define BR_LEARNING_SYNC BIT(9) #define BR_PROXYARP_WIFI BIT(10) #define BR_MCAST_FLOOD BIT(11) +#define BR_MULTICAST_TO_UCAST BIT(12) #define BR_DEFAULT_AGEING_TIME (300 * HZ) diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c index 7cb41ae..49d742d 100644 --- a/net/bridge/br_forward.c +++ b/net/bridge/br_forward.c @@ -174,6 +174,33 @@ static struct net_bridge_port *maybe_deliver( return p; } +static struct net_bridge_port *maybe_deliver_addr( + struct net_bridge_port *prev, struct net_bridge_port *p, + struct sk_buff *skb, const unsigned char *addr, + bool local_orig) +{ + struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev; + const unsigned char *src = eth_hdr(skb)->h_source; + + if (!should_deliver(p, skb)) + return prev; + + /* Even with hairpin, no soliloquies - prevent breaking IPv6 DAD */ + if (skb->dev == p->dev && ether_addr_equal(src, addr)) + return prev; + + skb = skb_copy(skb, GFP_ATOMIC); + if (!skb) { + dev->stats.tx_dropped++; + return prev; + } + + memcpy(eth_hdr(skb)->h_dest, addr, ETH_ALEN); + __br_forward(p, skb, local_orig); + + return prev; +} + /* called under rcu_read_lock */ void br_flood(struct net_bridge *br, struct sk_buff *skb, enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) @@ -231,6 +258,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, struct net_bridge_port *prev = NULL; struct net_bridge_port_group *p; struct hlist_node *rp; + const unsigned char *addr; rp = rcu_dereference(hlist_first_rcu(&br->router_list)); p = mdst ? rcu_dereference(mdst->ports) : NULL; @@ -241,10 +269,20 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) : NULL; - port = (unsigned long)lport > (unsigned long)rport ? - lport : rport; + if ((unsigned long)lport > (unsigned long)rport) { + port = lport; + addr = p->unicast ? p->eth_addr : NULL; + } else { + port = rport; + addr = NULL; + } + + if (addr) + prev = maybe_deliver_addr(prev, port, skb, addr, + local_orig); + else + prev = maybe_deliver(prev, port, skb, local_orig); - prev = maybe_deliver(prev, port, skb, local_orig); if (IS_ERR(prev)) goto out; if (prev == port) diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c index 7dbc80d..056e6ac 100644 --- a/net/b