| Message ID | 20200124161828.12206-10-horatiu.vultur@microchip.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: bridge: mrp: Add support for Media Redundancy Protocol (MRP) | expand |
On Fri, Jan 24, 2020 at 05:18:27PM +0100, Horatiu Vultur wrote: > To integrate MRP into the bridge, the bridge needs to do the following: > - initialized and destroy the generic netlink used by MRP > - detect if the MRP frame was received on a port that is part of a MRP ring. In > case it was not, then forward the frame as usual, otherwise redirect the frame > to the upper layer. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > net/bridge/br.c | 11 +++++++++++ > net/bridge/br_device.c | 3 +++ > net/bridge/br_if.c | 6 ++++++ > net/bridge/br_input.c | 14 ++++++++++++++ > net/bridge/br_private.h | 14 ++++++++++++++ > 5 files changed, 48 insertions(+) > > diff --git a/net/bridge/br.c b/net/bridge/br.c > index b6fe30e3768f..d5e556eed4ba 100644 > --- a/net/bridge/br.c > +++ b/net/bridge/br.c > @@ -344,6 +344,12 @@ static int __init br_init(void) > if (err) > goto err_out5; > > +#ifdef CONFIG_BRIDGE_MRP > + err = br_mrp_netlink_init(); > + if (err) > + goto err_out6; > +#endif Please try to avoid #ifdef's like this in C code. Add a stub function to br_private_mrp.h. If you really cannot avoid #ifdef, please use #if IS_ENABLED(CONFIG_BRIDGE_MRP). That expands to if (0) { } So the compiler will compile it and then optimize it out. That gives us added benefit of build testing, we don't suddenly find the code no longer compiles when we enable the option. > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -21,6 +21,9 @@ > #include <linux/rculist.h> > #include "br_private.h" > #include "br_private_tunnel.h" > +#ifdef CONFIG_BRIDGE_MRP > +#include "br_private_mrp.h" > +#endif It should always be safe to include a header file. Andrew
> br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb) > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > return RX_HANDLER_CONSUMED; > } > } > +#ifdef CONFIG_BRIDGE_MRP > + /* If there is no MRP instance do normal forwarding */ > + if (!p->mrp_aware) > + goto forward; > + > + if (skb->protocol == htons(ETH_P_MRP)) > + return RX_HANDLER_PASS; What MAC address is used for these MRP frames? It would make sense to use a L2 link local destination address, since i assume they are not supposed to be forwarded by the bridge. If so, you could extend the if (unlikely(is_link_local_ether_addr(dest))) condition. > + > + if (p->state == BR_STATE_BLOCKING) > + goto drop; > +#endif Is this needed? The next block of code is a switch statement on p->state. The default case, which BR_STATE_BLOCKING should hit, is drop. This function is on the hot path. So we should try to optimize it as much as possible. Andrew
The 01/25/2020 16:42, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, Jan 24, 2020 at 05:18:27PM +0100, Horatiu Vultur wrote: > > To integrate MRP into the bridge, the bridge needs to do the following: > > - initialized and destroy the generic netlink used by MRP > > - detect if the MRP frame was received on a port that is part of a MRP ring. In > > case it was not, then forward the frame as usual, otherwise redirect the frame > > to the upper layer. > > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > net/bridge/br.c | 11 +++++++++++ > > net/bridge/br_device.c | 3 +++ > > net/bridge/br_if.c | 6 ++++++ > > net/bridge/br_input.c | 14 ++++++++++++++ > > net/bridge/br_private.h | 14 ++++++++++++++ > > 5 files changed, 48 insertions(+) > > > > diff --git a/net/bridge/br.c b/net/bridge/br.c > > index b6fe30e3768f..d5e556eed4ba 100644 > > --- a/net/bridge/br.c > > +++ b/net/bridge/br.c > > @@ -344,6 +344,12 @@ static int __init br_init(void) > > if (err) > > goto err_out5; > > > > +#ifdef CONFIG_BRIDGE_MRP > > + err = br_mrp_netlink_init(); > > + if (err) > > + goto err_out6; > > +#endif > > Please try to avoid #ifdef's like this in C code. Add a stub function > to br_private_mrp.h. > > If you really cannot avoid #ifdef, please use #if IS_ENABLED(CONFIG_BRIDGE_MRP). > That expands to > > if (0) { > > } > > So the compiler will compile it and then optimize it out. That gives > us added benefit of build testing, we don't suddenly find the code no > longer compiles when we enable the option. > > > --- a/net/bridge/br_input.c > > +++ b/net/bridge/br_input.c > > @@ -21,6 +21,9 @@ > > #include <linux/rculist.h> > > #include "br_private.h" > > #include "br_private_tunnel.h" > > +#ifdef CONFIG_BRIDGE_MRP > > +#include "br_private_mrp.h" > > +#endif > > It should always be safe to include a header file. > > Andrew Thanks for pointing out these mistakes. I will try to avoid all these #ifdef's in the next patch series.
The 01/25/2020 17:16, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb) > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > > return RX_HANDLER_CONSUMED; > > } > > } > > +#ifdef CONFIG_BRIDGE_MRP > > + /* If there is no MRP instance do normal forwarding */ > > + if (!p->mrp_aware) > > + goto forward; > > + > > + if (skb->protocol == htons(ETH_P_MRP)) > > + return RX_HANDLER_PASS; > > What MAC address is used for these MRP frames? It would make sense to > use a L2 link local destination address, since i assume they are not > supposed to be forwarded by the bridge. If so, you could extend the > if (unlikely(is_link_local_ether_addr(dest))) condition. The MAC addresses used by MRP frames are: 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames. If we will add support also for MIM/MIC. These requires 2 more MAC addresses: 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames. 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect frames. Then maybe I shoukd change the check to be something like: if (unlikely(skb->protocol == htons(ETH_P_MRP))) > > > + > > + if (p->state == BR_STATE_BLOCKING) > > + goto drop; > > +#endif > > Is this needed? The next block of code is a switch statement on > p->state. The default case, which BR_STATE_BLOCKING should hit, is > drop. Yes you are rigth, it is not needed anymore. > > This function is on the hot path. So we should try to optimize it as > much as possible. > > Andrew
On Sun, Jan 26, 2020 at 02:01:11PM +0100, Horatiu Vultur wrote: > The 01/25/2020 17:16, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb) > > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > > > return RX_HANDLER_CONSUMED; > > > } > > > } > > > +#ifdef CONFIG_BRIDGE_MRP > > > + /* If there is no MRP instance do normal forwarding */ > > > + if (!p->mrp_aware) > > > + goto forward; > > > + > > > + if (skb->protocol == htons(ETH_P_MRP)) > > > + return RX_HANDLER_PASS; > > > > What MAC address is used for these MRP frames? It would make sense to > > use a L2 link local destination address, since i assume they are not > > supposed to be forwarded by the bridge. If so, you could extend the > > if (unlikely(is_link_local_ether_addr(dest))) condition. > > The MAC addresses used by MRP frames are: > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames. > > If we will add support also for MIM/MIC. These requires 2 more MAC > addresses: > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames. > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect > frames. Hi Horatiu I made the wrong guess about how this protocol worked when i said L2 link local. These MAC addresses are L2 multicast. And you are using a raw socket to receive them into userspace when needed. 'Thinking allowed' here. +------------------------------------------+ | | +-->|H1|<---------->|H2|<---------->|H3|<--+ eth0 eth1 eth0 eth1 eth0 eth1 ^ | Blocked There are three major classes of user case here: 1) Pure software solution You need the software bridge in the client to forward these frames from the left side to the right side. (Does the standard give these two ports names)? In the master, the left port is blocked, so the bridge drops them anyway. You have a RAW socket open on both eth0 and eth1, so you get to see the frames, even if the bridge drops them. 2) Hardware offload to an MRP unaware switch. I'm thinking about a plain switch supported by DSA, Marvell, Broadcom, etc. It has no special knowledge of MRP. Ideally, you want the switch to forward MRP_Test frames left to right for a client. In a master, i think you have a problem, since the port is blocked. The hardware is unlikely to recognise these frames as special, since they are not in the 01-80-C2-XX-XX-XX block, and let them through. So your raw socket is never going to see them, and you cannot detect open/closed ring. I don't know how realistic it is to support MRP in this case, and i also don't think you can fall back to a pure software solution, because the software bridge is going to offload the basic bridge operation to the hardware. It would be nice if you could detect this, and return -EOPNOTSUPP. 3) Hardware offload to an MRP aware switch. For a client, you tell it which port is left, which is right, and assume it forwards the frames. For a master, you again tell it which is left, which is right, and ask it send MRP_Test frames out right, and report changes in open/closed on the right port. You don't need the CPU to see the MRP_Test frames, so the switch has no need to forward them to the CPU. We should think about the general case of a bridge with many ports, and many pairs of ports using MRP. This makes the forwarding of these frames interesting. Given that they are multicast, the default action of the software bridge is that it will flood them. Does the protocol handle seeing MRP_Test from some other loop? Do we need to avoid this? You could avoid this by adding MDB entries to the bridge. However, this does not scale to more then one ring. I don't think an MDB is associated to an ingress port. So you cannot say 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port1 egress port2 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port3 egress port4 The best you can say is 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 egress port2, port4 I'm sure there are other issues i'm missing, but it is interesting to think about all this. Andrew
On 26.01.2020 18:12, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Sun, Jan 26, 2020 at 02:01:11PM +0100, Horatiu Vultur wrote: > > The 01/25/2020 17:16, Andrew Lunn wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > > br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb) > > > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > > > > return RX_HANDLER_CONSUMED; > > > > } > > > > } > > > > +#ifdef CONFIG_BRIDGE_MRP > > > > + /* If there is no MRP instance do normal forwarding */ > > > > + if (!p->mrp_aware) > > > > + goto forward; > > > > + > > > > + if (skb->protocol == htons(ETH_P_MRP)) > > > > + return RX_HANDLER_PASS; > > > > > > What MAC address is used for these MRP frames? It would make sense to > > > use a L2 link local destination address, since i assume they are not > > > supposed to be forwarded by the bridge. If so, you could extend the > > > if (unlikely(is_link_local_ether_addr(dest))) condition. > > > > The MAC addresses used by MRP frames are: > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames. > > > > If we will add support also for MIM/MIC. These requires 2 more MAC > > addresses: > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames. > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect > > frames. > > Hi Horatiu > > I made the wrong guess about how this protocol worked when i said L2 > link local. These MAC addresses are L2 multicast. > > And you are using a raw socket to receive them into userspace when > needed. > > 'Thinking allowed' here. > > +------------------------------------------+ > | | > +-->|H1|<---------->|H2|<---------->|H3|<--+ > eth0 eth1 eth0 eth1 eth0 eth1 > ^ > | > Blocked > > > There are three major classes of user case here: > > 1) Pure software solution > You need the software bridge in the client to forward these frames > from the left side to the right side. As far as I understand it is not the bridge which forward these frames - it is the user-space tool. This was to put as much functionality in user-space and only use the kernel to configure the HW. We can (and should) discuss if this is the right decision. > (Does the standard give these two ports names)? Horatiu? > In the master, the left port is blocked, so the bridge drops them > anyway. You have a RAW socket open on both eth0 and eth1, so you get > to see the frames, even if the bridge drops them. Yes, in the current patch-set such frames are forwarded by the user-space daemon. We would properly have better performance if we do this in kernel-space. > 2) Hardware offload to an MRP unaware switch. > > I'm thinking about a plain switch supported by DSA, Marvell, Broadcom, > etc. It has no special knowledge of MRP. We have implemented this on Ocelot - which is not MRP aware at all. Not sure what facilities Marvell and Broadcom has, but it is not a lot which is needed. > Ideally, you want the switch to forward MRP_Test frames left to right > for a client. Yes. If we have only 1 ring, then we can do that with a MAC table entry. If we have more than 1 ring, then we will need a TCAM rule of some kind. In the what we have today on Ocelot, we do not do this is HW, we do the forwarding in SW. BTW: It is not only from left to right, it is also from right to left. The MRM will inject packets on both ring ports, and monitor both. This is to detect asymmetrical link down or similar. The two ports are treated the same. But you can set a priority (the primary/secondary) to state your preference on what port to use if both are up and the ring is closed. > In a master, i think you have a problem, since the port > is blocked. The hardware is unlikely to recognise these frames as > special, since they are not in the 01-80-C2-XX-XX-XX block, and let > them through. So your raw socket is never going to see them, and you > cannot detect open/closed ring. Again, I do not know how other HW is designed, but all the SOC's we are working with, does allow us to add a TCAM rule which can redirect these frames to the CPU even on a blocked port. > I don't know how realistic it is to support MRP in this case, and i > also don't think you can fall back to a pure software solution, > because the software bridge is going to offload the basic bridge > operation to the hardware. It would be nice if you could detect this, > and return -EOPNOTSUPP. We do want to support this on Ocelot, but you are right, if the current running bridge, cannot block a port, and still get the MRP frames on that port, then it cannot support MRM. And this we need to detect in some way. > 3) Hardware offload to an MRP aware switch. > > For a client, you tell it which port is left, which is right, and > assume it forwards the frames. For a master, you again tell it which > is left, which is right, and ask it send MRP_Test frames out right, > and report changes in open/closed on the right port. You don't need > the CPU to see the MRP_Test frames, so the switch has no need to > forward them to the CPU. > > We should think about the general case of a bridge with many ports, > and many pairs of ports using MRP. This makes the forwarding of these > frames interesting. Given that they are multicast, the default action > of the software bridge is that it will flood them. Does the protocol > handle seeing MRP_Test from some other loop? Do we need to avoid this? Yes, we need to avoid. We cannot "just" do normal flooding. > You could avoid this by adding MDB entries to the bridge. However, > this does not scale to more then one ring. I would prefer a solution where the individual drivers can do what is best on the given HW. - If we have a 2 ported switch, then flooding seems like a perfect valid approach. There will be only 1 ring. - If we have a many ported switch, then we could use MAC-table entry - if the user only configure 1 ring. - When adding more rings, it either needs to return error, or use other HW facilities. > I don't think an MDB is associated to an ingress port. So you cannot > say Agree. > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port1 egress port2 > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port3 egress port4 > > The best you can say is > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 egress port2, port4 > > I'm sure there are other issues i'm missing, but it is interesting to > think about all this. Yes, the solution Horatiu has chosen, is not to forward MRP frames, received in MRP ring ports at all. This is done by the user-space tool. Again, not sure if this is the right way to do it, but it is what patch v3 does. The alternative to this would be to learn the bridge how to forward MRP frames when it is a MRC. The user-space tool then never needs to do this, it know that the kernel will take care of this part (either in SW or in HW). /Allan
The 01/27/2020 11:57, Allan W. Nielsen wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 26.01.2020 18:12, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Sun, Jan 26, 2020 at 02:01:11PM +0100, Horatiu Vultur wrote: > > > The 01/25/2020 17:16, Andrew Lunn wrote: > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > > > > br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb) > > > > > @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) > > > > > return RX_HANDLER_CONSUMED; > > > > > } > > > > > } > > > > > +#ifdef CONFIG_BRIDGE_MRP > > > > > + /* If there is no MRP instance do normal forwarding */ > > > > > + if (!p->mrp_aware) > > > > > + goto forward; > > > > > + > > > > > + if (skb->protocol == htons(ETH_P_MRP)) > > > > > + return RX_HANDLER_PASS; > > > > > > > > What MAC address is used for these MRP frames? It would make sense to > > > > use a L2 link local destination address, since i assume they are not > > > > supposed to be forwarded by the bridge. If so, you could extend the > > > > if (unlikely(is_link_local_ether_addr(dest))) condition. > > > > > > The MAC addresses used by MRP frames are: > > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 - used by MRP_Test frames > > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x2 - used by the rest of MRP frames. > > > > > > If we will add support also for MIM/MIC. These requires 2 more MAC > > > addresses: > > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x3 - used by MRP_InTest frames. > > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x4 - used by the other MRP interconnect > > > frames. > > > > Hi Horatiu > > > > I made the wrong guess about how this protocol worked when i said L2 > > link local. These MAC addresses are L2 multicast. > > > > And you are using a raw socket to receive them into userspace when > > needed. > > > > 'Thinking allowed' here. > > > > +------------------------------------------+ > > | | > > +-->|H1|<---------->|H2|<---------->|H3|<--+ > > eth0 eth1 eth0 eth1 eth0 eth1 > > ^ > > | > > Blocked > > > > > > There are three major classes of user case here: > > > > 1) Pure software solution > > You need the software bridge in the client to forward these frames > > from the left side to the right side. > As far as I understand it is not the bridge which forward these frames - > it is the user-space tool. This was to put as much functionality in > user-space and only use the kernel to configure the HW. We can (and > should) discuss if this is the right decision. > > > (Does the standard give these two ports names)? > Horatiu? They don't have a specific name, the standard names them as "ring ports". And to differentiate between them, they have roles: primary or secondary. These roles are used to know which port needs to be blocked(the secondary) and which needs to forward the frames. One observation, these roles are not fix for the entire time. The ring ports can interchange their roles. For example if eth0 is the primary port and eth1 is the secondary port and then the eth0 link goes down then eth0 will have the secondary role and eth1 will become primary port. > > > In the master, the left port is blocked, so the bridge drops them > > anyway. You have a RAW socket open on both eth0 and eth1, so you get > > to see the frames, even if the bridge drops them. > Yes, in the current patch-set such frames are forwarded by the > user-space daemon. > > We would properly have better performance if we do this in kernel-space. > > > > 2) Hardware offload to an MRP unaware switch. > > > > I'm thinking about a plain switch supported by DSA, Marvell, Broadcom, > > etc. It has no special knowledge of MRP. > We have implemented this on Ocelot - which is not MRP aware at all. Not > sure what facilities Marvell and Broadcom has, but it is not a lot which > is needed. Here is a small confusion. The implementation that we have done on Ocelot doesn't have at all HW offload(I have hacked the network driver to remove this support, so basically is just 4 NICs). Therefor all the non-MRP frames switching were done by the SW bridge and forwarding of MRP frames were done in the userspace. > > > Ideally, you want the switch to forward MRP_Test frames left to right > > for a client. > Yes. If we have only 1 ring, then we can do that with a MAC table entry. > If we have more than 1 ring, then we will need a TCAM rule of some kind. > > In the what we have today on Ocelot, we do not do this is HW, we do the > forwarding in SW. > > BTW: It is not only from left to right, it is also from right to left. > The MRM will inject packets on both ring ports, and monitor both. This > is to detect asymmetrical link down or similar. The two ports are > treated the same. But you can set a priority (the primary/secondary) to > state your preference on what port to use if both are up and the ring is > closed. A small observation, the primary/secondary are defined in the standard as roles and not priority. And yes it uses this role(primary/secondary) to decide which port to block. > > > In a master, i think you have a problem, since the port > > is blocked. The hardware is unlikely to recognise these frames as > > special, since they are not in the 01-80-C2-XX-XX-XX block, and let > > them through. So your raw socket is never going to see them, and you > > cannot detect open/closed ring. > Again, I do not know how other HW is designed, but all the SOC's we are > working with, does allow us to add a TCAM rule which can redirect these > frames to the CPU even on a blocked port. > > > I don't know how realistic it is to support MRP in this case, and i > > also don't think you can fall back to a pure software solution, > > because the software bridge is going to offload the basic bridge > > operation to the hardware. It would be nice if you could detect this, > > and return -EOPNOTSUPP. > We do want to support this on Ocelot, but you are right, if the current > running bridge, cannot block a port, and still get the MRP frames on > that port, then it cannot support MRM. And this we need to detect in > some way. > > > 3) Hardware offload to an MRP aware switch. > > > > For a client, you tell it which port is left, which is right, and > > assume it forwards the frames. For a master, you again tell it which > > is left, which is right, and ask it send MRP_Test frames out right, > > and report changes in open/closed on the right port. You don't need > > the CPU to see the MRP_Test frames, so the switch has no need to > > forward them to the CPU. > > > > We should think about the general case of a bridge with many ports, > > and many pairs of ports using MRP. This makes the forwarding of these > > frames interesting. Given that they are multicast, the default action > > of the software bridge is that it will flood them. Does the protocol > > handle seeing MRP_Test from some other loop? Do we need to avoid this? > Yes, we need to avoid. We cannot "just" do normal flooding. > > > You could avoid this by adding MDB entries to the bridge. However, > > this does not scale to more then one ring. > I would prefer a solution where the individual drivers can do what is > best on the given HW. > > - If we have a 2 ported switch, then flooding seems like a perfect valid > approach. There will be only 1 ring. > - If we have a many ported switch, then we could use MAC-table entry - > if the user only configure 1 ring. > - When adding more rings, it either needs to return error, or use > other HW facilities. > > > I don't think an MDB is associated to an ingress port. So you cannot > > say > Agree. > > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port1 egress port2 > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 ingress port3 egress port4 > > > > The best you can say is > > > > 0x1, 0x15, 0x4e, 0x0, 0x0, 0x1 egress port2, port4 > > > > I'm sure there are other issues i'm missing, but it is interesting to > > think about all this. > Yes, the solution Horatiu has chosen, is not to forward MRP frames, > received in MRP ring ports at all. This is done by the user-space tool. > > Again, not sure if this is the right way to do it, but it is what patch > v3 does. > > The alternative to this would be to learn the bridge how to forward MRP > frames when it is a MRC. The user-space tool then never needs to do > this, it know that the kernel will take care of this part (either in SW > or in HW). > > /Allan
> > 'Thinking allowed' here. > > > > +------------------------------------------+ > > | | > > +-->|H1|<---------->|H2|<---------->|H3|<--+ > > eth0 eth1 eth0 eth1 eth0 eth1 > > ^ > > | > > Blocked > > > > > > There are three major classes of user case here: > > > > 1) Pure software solution > > You need the software bridge in the client to forward these frames > > from the left side to the right side. > As far as I understand it is not the bridge which forward these frames - > it is the user-space tool. This was to put as much functionality in > user-space and only use the kernel to configure the HW. We can (and > should) discuss if this is the right decision. So i need to flip the point around. How does the software switch know not to forward the frames? Are you adding an MDB? > We would properly have better performance if we do this in kernel-space. Yes, that is what i think. And if you can do it without any additional code, using the forwarding tables, so much the better. > BTW: It is not only from left to right, it is also from right to left. > The MRM will inject packets on both ring ports, and monitor both. Using the same MAC address in both directions? I need to think what that implies for MDB entries. It probably just works, since you never flood back out the ingress port. > Again, I do not know how other HW is designed, but all the SOC's we are > working with, does allow us to add a TCAM rule which can redirect these > frames to the CPU even on a blocked port. It is not in scope for what you are doing, but i wonder how we describe this in a generic Linux way? And then how we push it down to the hardware? For the Marvell Switches, it might be possible to do this without the TCAM. You can add forwarding DB entries marked as Management. It is unclear if this overrides the blocked state, but it would be a bit odd if it did not. > > You could avoid this by adding MDB entries to the bridge. However, > > this does not scale to more then one ring. > I would prefer a solution where the individual drivers can do what is > best on the given HW. The nice thing about adding MDB is that it is making use of the software bridge facilities. In general, the software bridge and hardware bridges are pretty similar. If you can solve the problem using generic software bridge features, not additional special cases in code, you have good chance of being able to offload it to a hardware bridge which is not MRP aware. The switchdev API for MRP specific features should then allow you to make use of any additional features the hardware might have. > Yes, the solution Horatiu has chosen, is not to forward MRP frames, > received in MRP ring ports at all. This is done by the user-space tool. > > Again, not sure if this is the right way to do it, but it is what patch > v3 does. > > The alternative to this would be to learn the bridge how to forward MRP > frames when it is a MRC. The user-space tool then never needs to do > this, it know that the kernel will take care of this part (either in SW > or in HW). I think that should be considered. I'm not saying it is the best way, just that some thought should be put into it to figure out what it actually implies. Andrew
On 1/27/20 2:40 PM, Andrew Lunn wrote: >> Again, I do not know how other HW is designed, but all the SOC's we are >> working with, does allow us to add a TCAM rule which can redirect these >> frames to the CPU even on a blocked port. > It is not in scope for what you are doing, but i wonder how we > describe this in a generic Linux way? And then how we push it down to > the hardware? > > For the Marvell Switches, it might be possible to do this without the > TCAM. You can add forwarding DB entries marked as Management. It is > unclear if this overrides the blocked state, but it would be a bit odd > if it did not. A MGMT frame does override the blocked state according the the datasheet. And any MAC address can be loaded, not only 01:80:C2:00:00:0x (802.1D) and 01:80:C2:00:00:2x (GARP). Then the ATU is used instead of something specialized. (referring to Andrew's email of 20200126 6:12 PM) (I only checked again 88E6250/88E6220/88E6071/88E6070/88E6020 Functional Specification) Kind regards, Jürgen
On 27.01.2020 14:40, Andrew Lunn wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> > 'Thinking allowed' here. >> > >> > +------------------------------------------+ >> > | | >> > +-->|H1|<---------->|H2|<---------->|H3|<--+ >> > eth0 eth1 eth0 eth1 eth0 eth1 >> > ^ >> > | >> > Blocked >> > >> > >> > There are three major classes of user case here: >> > >> > 1) Pure software solution >> > You need the software bridge in the client to forward these frames >> > from the left side to the right side. > >> As far as I understand it is not the bridge which forward these frames - >> it is the user-space tool. This was to put as much functionality in >> user-space and only use the kernel to configure the HW. We can (and >> should) discuss if this is the right decision. > >So i need to flip the point around. How does the software switch know >not to forward the frames? Are you adding an MDB? In the current implementation (patch v3) this is done here: https://github.com/microchip-ung/mrp/blob/patch-v3/kernel-patches/v3-0009-net-bridge-mrp-Integrate-MRP-into-the-bridge.patch#L112 We simply ask the bridge not to forward any MRP frames, on MRP enabled ports, and let "someone" else do that. >> We would properly have better performance if we do this in kernel-space. > >Yes, that is what i think. And if you can do it without any additional >code, using the forwarding tables, so much the better. I understand the motivation of using the existing forwarding mechanism, but I do not think we have all the hooks needed. But we can certainly limit the impact on the existing code as much as possible. >> BTW: It is not only from left to right, it is also from right to left. >> The MRM will inject packets on both ring ports, and monitor both. > >Using the same MAC address in both directions? I need to think what >that implies for MDB entries. It probably just works, since you never >flood back out the ingress port. Seems to work fine :-D >> Again, I do not know how other HW is designed, but all the SOC's we are >> working with, does allow us to add a TCAM rule which can redirect these >> frames to the CPU even on a blocked port. > >It is not in scope for what you are doing, but i wonder how we >describe this in a generic Linux way? And then how we push it down to >the hardware? > >For the Marvell Switches, it might be possible to do this without the >TCAM. You can add forwarding DB entries marked as Management. It is >unclear if this overrides the blocked state, but it would be a bit odd >if it did not. Based on this, and also on the input from Jürgen, I think there is a good chnage we can make this work for existing silicon from several vendors. >> > You could avoid this by adding MDB entries to the bridge. However, >> > this does not scale to more then one ring. >> I would prefer a solution where the individual drivers can do what is >> best on the given HW. >The nice thing about adding MDB is that it is making use of the >software bridge facilities. In general, the software bridge and >hardware bridges are pretty similar. If you can solve the problem >using generic software bridge features, not additional special cases >in code, you have good chance of being able to offload it to a >hardware bridge which is not MRP aware. The switchdev API for MRP >specific features should then allow you to make use of any additional >features the hardware might have. Yes, but the issues in using the MDB API for this is that it does not allow to look at source ports, and it does not allow to update the priority of the frames. >> Yes, the solution Horatiu has chosen, is not to forward MRP frames, >> received in MRP ring ports at all. This is done by the user-space tool. >> >> Again, not sure if this is the right way to do it, but it is what patch >> v3 does. >> >> The alternative to this would be to learn the bridge how to forward MRP >> frames when it is a MRC. The user-space tool then never needs to do >> this, it know that the kernel will take care of this part (either in SW >> or in HW). >I think that should be considered. I'm not saying it is the best way, >just that some thought should be put into it to figure out what it >actually implies. Sounds good - I will try to explain and illustrate this a bit better, such that we all have the same understanding of the problem we need to solve. /Allan
diff --git a/net/bridge/br.c b/net/bridge/br.c index b6fe30e3768f..d5e556eed4ba 100644 --- a/net/bridge/br.c +++ b/net/bridge/br.c @@ -344,6 +344,12 @@ static int __init br_init(void) if (err) goto err_out5; +#ifdef CONFIG_BRIDGE_MRP + err = br_mrp_netlink_init(); + if (err) + goto err_out6; +#endif + brioctl_set(br_ioctl_deviceless_stub); #if IS_ENABLED(CONFIG_ATM_LANE) @@ -358,6 +364,11 @@ static int __init br_init(void) return 0; +#ifdef CONFIG_BRIDGE_MRP +err_out6: + br_netlink_fini(); +#endif + err_out5: unregister_switchdev_notifier(&br_switchdev_notifier); err_out4: diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index fb38add21b37..29966754d86a 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -464,6 +464,9 @@ void br_dev_setup(struct net_device *dev) spin_lock_init(&br->lock); INIT_LIST_HEAD(&br->port_list); INIT_HLIST_HEAD(&br->fdb_list); +#ifdef CONFIG_BRIDGE_MRP + INIT_LIST_HEAD(&br->mrp_list); +#endif spin_lock_init(&br->hash_lock); br->bridge_id.prio[0] = 0x80; diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index 4fe30b182ee7..9b8bb41c0574 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -331,6 +331,9 @@ static void del_nbp(struct net_bridge_port *p) spin_lock_bh(&br->lock); br_stp_disable_port(p); +#ifdef CONFIG_BRIDGE_MRP + p->mrp_aware = false; +#endif spin_unlock_bh(&br->lock); br_ifinfo_notify(RTM_DELLINK, NULL, p); @@ -427,6 +430,9 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br, p->port_no = index; p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; br_init_port(p); +#ifdef CONFIG_BRIDGE_MRP + p->mrp_aware = false; +#endif br_set_state(p, BR_STATE_DISABLED); br_stp_port_timer_init(p); err = br_multicast_add_port(p); diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index 8944ceb47fe9..de7066b077e2 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -21,6 +21,9 @@ #include <linux/rculist.h> #include "br_private.h" #include "br_private_tunnel.h" +#ifdef CONFIG_BRIDGE_MRP +#include "br_private_mrp.h" +#endif static int br_netif_receive_skb(struct net *net, struct sock *sk, struct sk_buff *skb) @@ -338,6 +341,17 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb) return RX_HANDLER_CONSUMED; } } +#ifdef CONFIG_BRIDGE_MRP + /* If there is no MRP instance do normal forwarding */ + if (!p->mrp_aware) + goto forward; + + if (skb->protocol == htons(ETH_P_MRP)) + return RX_HANDLER_PASS; + + if (p->state == BR_STATE_BLOCKING) + goto drop; +#endif forward: switch (p->state) { diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index f540f3bdf294..a5d01a394f54 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -285,6 +285,10 @@ struct net_bridge_port { u16 backup_redirected_cnt; struct bridge_stp_xstats stp_xstats; + +#ifdef CONFIG_BRIDGE_MRP + bool mrp_aware; +#endif }; #define kobj_to_brport(obj) container_of(obj, struct net_bridge_port, kobj) @@ -424,6 +428,10 @@ struct net_bridge { int offload_fwd_mark; #endif struct hlist_head fdb_list; + +#ifdef CONFIG_BRIDGE_MRP + struct list_head mrp_list; +#endif }; struct br_input_skb_cb { @@ -1165,6 +1173,12 @@ unsigned long br_timer_value(const struct timer_list *timer); extern int (*br_fdb_test_addr_hook)(struct net_device *dev, unsigned char *addr); #endif +/* br_mrp.c */ +#ifdef CONFIG_BRIDGE_MRP +int br_mrp_netlink_init(void); +void br_mrp_netlink_uninit(void); +#endif + /* br_netlink.c */ extern struct rtnl_link_ops br_link_ops; int br_netlink_init(void);
To integrate MRP into the bridge, the bridge needs to do the following: - initialized and destroy the generic netlink used by MRP - detect if the MRP frame was received on a port that is part of a MRP ring. In case it was not, then forward the frame as usual, otherwise redirect the frame to the upper layer. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- net/bridge/br.c | 11 +++++++++++ net/bridge/br_device.c | 3 +++ net/bridge/br_if.c | 6 ++++++ net/bridge/br_input.c | 14 ++++++++++++++ net/bridge/br_private.h | 14 ++++++++++++++ 5 files changed, 48 insertions(+)