* [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode @ 2015-07-09 4:25 Simon Horman 2015-07-09 5:38 ` John Fastabend 2015-07-14 6:37 ` Scott Feldman 0 siblings, 2 replies; 10+ messages in thread From: Simon Horman @ 2015-07-09 4:25 UTC (permalink / raw) To: Scott Feldman, Jiri Pirko; +Cc: netdev, Simon Horman This change allows the CPU to see all packets seen by a port when the netdev associated with the port is in promiscuous mode. This change was previously posted as part of a larger patch and in turn patchset which also aimed to allow rocker interfaces to receive packets when not bridged. That problem has subsequently been addressed in a different way by Scott Feldman. When this change was previously posted Scott indicated that he had some reservations about sending all packets from a switch to the CPU. The purpose of posting this patch is to start discussion of weather this approach is appropriate and if not how else we might move forwards. In my opinion if host doesn't want all packets its shouldn't put a port in promiscuous mode. But perhaps that is an overly naïve view to take. My main motivation for this change at this time is to allow rocker to work with Open vSwitch and it appears that this change is sufficient to reach that goal. Another approach might be to teach rocker_port_master_changed() about Open vSwitch. In the longer term I believe Open vSwitch should be able to program flows into rocker 'hardware' and thus not all packets would reach the CPU. Signed-off-by: Simon Horman <simon.horman@netronome.com> --- drivers/net/ethernet/rocker/rocker.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c index 2d8578cade03..9e812b85b421 100644 --- a/drivers/net/ethernet/rocker/rocker.c +++ b/drivers/net/ethernet/rocker/rocker.c @@ -199,6 +199,7 @@ struct rocker; enum { ROCKER_CTRL_LINK_LOCAL_MCAST, ROCKER_CTRL_LOCAL_ARP, + ROCKER_CTRL_PROMISC, ROCKER_CTRL_IPV4_MCAST, ROCKER_CTRL_IPV6_MCAST, ROCKER_CTRL_DFLT_BRIDGING, @@ -3222,6 +3223,12 @@ static struct rocker_ctrl { .eth_type = htons(ETH_P_ARP), .acl = true, }, + [ROCKER_CTRL_PROMISC] = { + /* pass all pkts up to CPU */ + .eth_dst = zero_mac, + .eth_dst_mask = zero_mac, + .acl = true, + }, [ROCKER_CTRL_IPV4_MCAST] = { /* pass IPv4 mcast pkts up to CPU, RFC 1112 */ .eth_dst = ipv4_mcast, @@ -3755,13 +3762,19 @@ static int rocker_port_stp_update(struct rocker_port *rocker_port, break; case BR_STATE_LEARNING: case BR_STATE_FORWARDING: - want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true; + if (!(rocker_port->dev->flags & IFF_PROMISC)) + want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true; want[ROCKER_CTRL_IPV4_MCAST] = true; want[ROCKER_CTRL_IPV6_MCAST] = true; - if (rocker_port_is_bridged(rocker_port)) + if (rocker_port_is_bridged(rocker_port)) { want[ROCKER_CTRL_DFLT_BRIDGING] = true; - else - want[ROCKER_CTRL_LOCAL_ARP] = true; + } else { + if (rocker_port->dev->flags & IFF_PROMISC) { + want[ROCKER_CTRL_PROMISC] = true; + } else { + want[ROCKER_CTRL_LOCAL_ARP] = true; + } + } break; } @@ -4152,6 +4165,17 @@ static int rocker_port_set_mac_address(struct net_device *dev, void *p) return 0; } +static void rocker_port_change_rx_flags(struct net_device *dev, int change) +{ + struct rocker_port *rocker_port = netdev_priv(dev); + + if (change & IFF_PROMISC && rocker_port->dev->flags & IFF_UP) + if (!rocker_port_fwd_disable(rocker_port, + SWITCHDEV_TRANS_NONE, 0)) + rocker_port_fwd_enable(rocker_port, + SWITCHDEV_TRANS_NONE, 0); +} + static int rocker_port_get_phys_port_name(struct net_device *dev, char *buf, size_t len) { @@ -4171,6 +4195,7 @@ static const struct net_device_ops rocker_port_netdev_ops = { .ndo_open = rocker_port_open, .ndo_stop = rocker_port_stop, .ndo_start_xmit = rocker_port_xmit, + .ndo_change_rx_flags = rocker_port_change_rx_flags, .ndo_set_mac_address = rocker_port_set_mac_address, .ndo_bridge_getlink = switchdev_port_bridge_getlink, .ndo_bridge_setlink = switchdev_port_bridge_setlink, -- 2.1.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-09 4:25 [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode Simon Horman @ 2015-07-09 5:38 ` John Fastabend 2015-07-14 6:37 ` Scott Feldman 1 sibling, 0 replies; 10+ messages in thread From: John Fastabend @ 2015-07-09 5:38 UTC (permalink / raw) To: Simon Horman, Scott Feldman, Jiri Pirko; +Cc: netdev On 15-07-08 09:25 PM, Simon Horman wrote: > This change allows the CPU to see all packets seen by a port when the > netdev associated with the port is in promiscuous mode. > > This change was previously posted as part of a larger patch and in turn > patchset which also aimed to allow rocker interfaces to receive packets > when not bridged. That problem has subsequently been addressed in a > different way by Scott Feldman. > > When this change was previously posted Scott indicated that he had some > reservations about sending all packets from a switch to the CPU. The > purpose of posting this patch is to start discussion of weather this > approach is appropriate and if not how else we might move forwards. > > In my opinion if host doesn't want all packets its shouldn't put a port > in promiscuous mode. But perhaps that is an overly naïve view to take. > > My main motivation for this change at this time is to allow rocker to > work with Open vSwitch and it appears that this change is sufficient to > reach that goal. Another approach might be to teach > rocker_port_master_changed() about Open vSwitch. > My opinion would be to not confuse a host concept 'promiscuous mode' and allow it to impact how the forwarding is done by the hardware. I think a better approach would be to put a default low priority rule in one of the hardware match action tables to mirror all traffic to the port you want to mirror traffic to. And if it happens to be a CPU port then it will be pushed to the host. My problem with the semantics here is it seems overload how we use promiscuous mode in the NIC today. For example I don't see how to use it in an SR-IOV case or multi-function device. How would you handle this if more than one port was put in promiscuous mode? Mirror to both ports? Today when we put a device in a promiscuous mode behind a switch we don't expect to start receiving all traffic. I think at very least we need to be very specific about how we define it and when it applies if we overload promisc field. Also wouldn't it break any 'bridge/vswitch' software that puts the port in promisc mode by default. > In the longer term I believe Open vSwitch should be able to program > flows into rocker 'hardware' and thus not all packets would reach the CPU. Agree with this point. > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > --- > drivers/net/ethernet/rocker/rocker.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-09 4:25 [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode Simon Horman 2015-07-09 5:38 ` John Fastabend @ 2015-07-14 6:37 ` Scott Feldman 2015-07-15 4:45 ` Simon Horman 1 sibling, 1 reply; 10+ messages in thread From: Scott Feldman @ 2015-07-14 6:37 UTC (permalink / raw) To: Simon Horman; +Cc: Jiri Pirko, Netdev, john fastabend On Wed, Jul 8, 2015 at 9:25 PM, Simon Horman <simon.horman@netronome.com> wrote: > This change allows the CPU to see all packets seen by a port when the > netdev associated with the port is in promiscuous mode. > > This change was previously posted as part of a larger patch and in turn > patchset which also aimed to allow rocker interfaces to receive packets > when not bridged. That problem has subsequently been addressed in a > different way by Scott Feldman. > > When this change was previously posted Scott indicated that he had some > reservations about sending all packets from a switch to the CPU. The > purpose of posting this patch is to start discussion of weather this > approach is appropriate and if not how else we might move forwards. > > In my opinion if host doesn't want all packets its shouldn't put a port > in promiscuous mode. But perhaps that is an overly naïve view to take. > > My main motivation for this change at this time is to allow rocker to > work with Open vSwitch and it appears that this change is sufficient to > reach that goal. Another approach might be to teach > rocker_port_master_changed() about Open vSwitch. > > In the longer term I believe Open vSwitch should be able to program > flows into rocker 'hardware' and thus not all packets would reach the CPU. Hi Simon, I like your alternate approach to teach rocker about Open vSwitch using rocker_port_master_change() and only when port is captured by OVS would we install the "promisc" filter to pass all traffic up. (Maybe call it ROCKER_CTRL_DFLT_OVS rule?). Putting a non-bridged, non-ovs port into promisc is kind of weird for a switch port. I think of the port in L3 mode by default, where the port is locked down for all but some selective mcasts, and only opened up by installing explicit routes. (Unlike a bridged port where we flood everything L2 we don't understand). So maybe first pass is to pass up everything when port is captured by OVS, and then later refine what's passed up per ovs flows on that port. -scott ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-14 6:37 ` Scott Feldman @ 2015-07-15 4:45 ` Simon Horman 2015-07-15 5:32 ` Scott Feldman 0 siblings, 1 reply; 10+ messages in thread From: Simon Horman @ 2015-07-15 4:45 UTC (permalink / raw) To: Scott Feldman; +Cc: Jiri Pirko, Netdev, john fastabend Hi Scott, On Mon, Jul 13, 2015 at 11:37:59PM -0700, Scott Feldman wrote: > On Wed, Jul 8, 2015 at 9:25 PM, Simon Horman <simon.horman@netronome.com> wrote: > > This change allows the CPU to see all packets seen by a port when the > > netdev associated with the port is in promiscuous mode. > > > > This change was previously posted as part of a larger patch and in turn > > patchset which also aimed to allow rocker interfaces to receive packets > > when not bridged. That problem has subsequently been addressed in a > > different way by Scott Feldman. > > > > When this change was previously posted Scott indicated that he had some > > reservations about sending all packets from a switch to the CPU. The > > purpose of posting this patch is to start discussion of weather this > > approach is appropriate and if not how else we might move forwards. > > > > In my opinion if host doesn't want all packets its shouldn't put a port > > in promiscuous mode. But perhaps that is an overly naïve view to take. > > > > My main motivation for this change at this time is to allow rocker to > > work with Open vSwitch and it appears that this change is sufficient to > > reach that goal. Another approach might be to teach > > rocker_port_master_changed() about Open vSwitch. > > > > In the longer term I believe Open vSwitch should be able to program > > flows into rocker 'hardware' and thus not all packets would reach the CPU. > > Hi Simon, > > I like your alternate approach to teach rocker about Open vSwitch > using rocker_port_master_change() and only when port is captured by > OVS would we install the "promisc" filter to pass all traffic up. > (Maybe call it ROCKER_CTRL_DFLT_OVS rule?). > > Putting a non-bridged, non-ovs port into promisc is kind of weird for > a switch port. I think of the port in L3 mode by default, where the > port is locked down for all but some selective mcasts, and only opened > up by installing explicit routes. (Unlike a bridged port where we > flood everything L2 we don't understand). > > So maybe first pass is to pass up everything when port is captured by > OVS, and then later refine what's passed up per ovs flows on that > port. That sounds reasonable to me. Its pretty clear to me from the responses from John and yourself that my approach to the promiscuous flag isn't as clean-cut as I had hoped. And it seems that we have a nice way to move forwards on supporting Open vSwitch. How about this? -- >8 -- From: Simon Horman <simon.horman@netronome.com> Subject: rocker: forward packets to CPU when port is joined to openvswitch Teach rocker to forward packets to CPU when a port is joined to Open vSwitch. There is scope to later refine what is passed up as per Open vSwitch flows on a port. This does not change the behaviour of rocker ports that are not joined to Open vSwitch. Signed-off-by: Simon Horman <simon.horman@netronome.com> --- drivers/net/ethernet/rocker/rocker.c | 55 ++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c index c0051673c9fa..8c53d6839260 100644 --- a/drivers/net/ethernet/rocker/rocker.c +++ b/drivers/net/ethernet/rocker/rocker.c @@ -202,6 +202,7 @@ enum { ROCKER_CTRL_IPV4_MCAST, ROCKER_CTRL_IPV6_MCAST, ROCKER_CTRL_DFLT_BRIDGING, + ROCKER_CTRL_DFLT_OVS, ROCKER_CTRL_MAX, }; @@ -321,9 +322,21 @@ static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port, return ntohs(vlan_id); } +static bool rocker_port_is_bridged__(const struct rocker_port *rocker_port, + const char *kind) +{ + return rocker_port->bridge_dev && + !strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind); +} + static bool rocker_port_is_bridged(const struct rocker_port *rocker_port) { - return !!rocker_port->bridge_dev; + return rocker_port_is_bridged__(rocker_port, "bridge"); +} + +static bool rocker_port_is_ovs(const struct rocker_port *rocker_port) +{ + return rocker_port_is_bridged__(rocker_port, "openvswitch"); } #define ROCKER_OP_FLAG_REMOVE BIT(0) @@ -3275,6 +3288,12 @@ static struct rocker_ctrl { .bridge = true, .copy_to_cpu = true, }, + [ROCKER_CTRL_DFLT_OVS] = { + /* pass all pkts up to CPU */ + .eth_dst = zero_mac, + .eth_dst_mask = zero_mac, + .acl = true, + }, }; static int rocker_port_ctrl_vlan_acl(struct rocker_port *rocker_port, @@ -3787,11 +3806,14 @@ static int rocker_port_stp_update(struct rocker_port *rocker_port, break; case BR_STATE_LEARNING: case BR_STATE_FORWARDING: - want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true; + if (!rocker_port_is_ovs(rocker_port)) + want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true; want[ROCKER_CTRL_IPV4_MCAST] = true; want[ROCKER_CTRL_IPV6_MCAST] = true; if (rocker_port_is_bridged(rocker_port)) want[ROCKER_CTRL_DFLT_BRIDGING] = true; + else if (rocker_port_is_ovs(rocker_port)) + want[ROCKER_CTRL_DFLT_OVS] = true; else want[ROCKER_CTRL_LOCAL_ARP] = true; break; @@ -5251,6 +5273,22 @@ static int rocker_port_bridge_leave(struct rocker_port *rocker_port) return err; } + +static int rocker_port_ovs_changed(struct rocker_port *rocker_port, + struct net_device *master) +{ + int err; + + rocker_port->bridge_dev = master; + + err = rocker_port_fwd_disable(rocker_port, SWITCHDEV_TRANS_NONE, 0); + if (err) + return err; + err = rocker_port_fwd_enable(rocker_port, SWITCHDEV_TRANS_NONE, 0); + + return err; +} + static int rocker_port_master_changed(struct net_device *dev) { struct rocker_port *rocker_port = netdev_priv(dev); @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) * 3. Other, e.g. being added to or removed from a bond or openvswitch, * in which case nothing is done */ - if (master && master->rtnl_link_ops && - !strcmp(master->rtnl_link_ops->kind, "bridge")) - err = rocker_port_bridge_join(rocker_port, master); - else if (rocker_port_is_bridged(rocker_port)) + if (master && master->rtnl_link_ops) { + if (!strcmp(master->rtnl_link_ops->kind, "bridge")) + err = rocker_port_bridge_join(rocker_port, master); + else if (!strcmp(master->rtnl_link_ops->kind, "openvswitch")) + err = rocker_port_ovs_changed(rocker_port, master); + } else if (rocker_port_is_bridged(rocker_port)) { err = rocker_port_bridge_leave(rocker_port); + } else if (rocker_port_is_ovs(rocker_port)) { + err = rocker_port_ovs_changed(rocker_port, NULL); + } return err; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-15 4:45 ` Simon Horman @ 2015-07-15 5:32 ` Scott Feldman 2015-07-15 6:34 ` Simon Horman 0 siblings, 1 reply; 10+ messages in thread From: Scott Feldman @ 2015-07-15 5:32 UTC (permalink / raw) To: Simon Horman; +Cc: Jiri Pirko, Netdev, john fastabend On Tue, Jul 14, 2015 at 9:45 PM, Simon Horman <simon.horman@netronome.com> wrote: > Hi Scott, > > On Mon, Jul 13, 2015 at 11:37:59PM -0700, Scott Feldman wrote: >> On Wed, Jul 8, 2015 at 9:25 PM, Simon Horman <simon.horman@netronome.com> wrote: >> > This change allows the CPU to see all packets seen by a port when the >> > netdev associated with the port is in promiscuous mode. >> > >> > This change was previously posted as part of a larger patch and in turn >> > patchset which also aimed to allow rocker interfaces to receive packets >> > when not bridged. That problem has subsequently been addressed in a >> > different way by Scott Feldman. >> > >> > When this change was previously posted Scott indicated that he had some >> > reservations about sending all packets from a switch to the CPU. The >> > purpose of posting this patch is to start discussion of weather this >> > approach is appropriate and if not how else we might move forwards. >> > >> > In my opinion if host doesn't want all packets its shouldn't put a port >> > in promiscuous mode. But perhaps that is an overly naïve view to take. >> > >> > My main motivation for this change at this time is to allow rocker to >> > work with Open vSwitch and it appears that this change is sufficient to >> > reach that goal. Another approach might be to teach >> > rocker_port_master_changed() about Open vSwitch. >> > >> > In the longer term I believe Open vSwitch should be able to program >> > flows into rocker 'hardware' and thus not all packets would reach the CPU. >> >> Hi Simon, >> >> I like your alternate approach to teach rocker about Open vSwitch >> using rocker_port_master_change() and only when port is captured by >> OVS would we install the "promisc" filter to pass all traffic up. >> (Maybe call it ROCKER_CTRL_DFLT_OVS rule?). >> >> Putting a non-bridged, non-ovs port into promisc is kind of weird for >> a switch port. I think of the port in L3 mode by default, where the >> port is locked down for all but some selective mcasts, and only opened >> up by installing explicit routes. (Unlike a bridged port where we >> flood everything L2 we don't understand). >> >> So maybe first pass is to pass up everything when port is captured by >> OVS, and then later refine what's passed up per ovs flows on that >> port. > > That sounds reasonable to me. Its pretty clear to me from the responses > from John and yourself that my approach to the promiscuous flag isn't > as clean-cut as I had hoped. And it seems that we have a nice way to > move forwards on supporting Open vSwitch. > > How about this? Looks good, some inline comments... > From: Simon Horman <simon.horman@netronome.com> > > Subject: rocker: forward packets to CPU when port is joined to openvswitch > > Teach rocker to forward packets to CPU when a port is joined to Open vSwitch. > There is scope to later refine what is passed up as per Open vSwitch flows > on a port. > > This does not change the behaviour of rocker ports that are > not joined to Open vSwitch. > > Signed-off-by: Simon Horman <simon.horman@netronome.com> > --- > drivers/net/ethernet/rocker/rocker.c | 55 ++++++++++++++++++++++++++++++++---- > 1 file changed, 49 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c > index c0051673c9fa..8c53d6839260 100644 > --- a/drivers/net/ethernet/rocker/rocker.c > +++ b/drivers/net/ethernet/rocker/rocker.c > @@ -202,6 +202,7 @@ enum { > ROCKER_CTRL_IPV4_MCAST, > ROCKER_CTRL_IPV6_MCAST, > ROCKER_CTRL_DFLT_BRIDGING, > + ROCKER_CTRL_DFLT_OVS, > ROCKER_CTRL_MAX, > }; > > @@ -321,9 +322,21 @@ static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port, > return ntohs(vlan_id); > } > > +static bool rocker_port_is_bridged__(const struct rocker_port *rocker_port, > + const char *kind) Maybe use __rocker_port_is_bridged? (leading '__'; I've not seen use of trailing '__'). Or __rocker_port_is_slave(port, kind)? > +{ > + return rocker_port->bridge_dev && > + !strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind); > +} > + > static bool rocker_port_is_bridged(const struct rocker_port *rocker_port) > { > - return !!rocker_port->bridge_dev; > + return rocker_port_is_bridged__(rocker_port, "bridge"); > +} > + > +static bool rocker_port_is_ovs(const struct rocker_port *rocker_port) rocker_port_is_ovsed? Just to be consistent with is_bridged. > +{ > + return rocker_port_is_bridged__(rocker_port, "openvswitch"); > } > > #define ROCKER_OP_FLAG_REMOVE BIT(0) > @@ -3275,6 +3288,12 @@ static struct rocker_ctrl { > .bridge = true, > .copy_to_cpu = true, > }, > + [ROCKER_CTRL_DFLT_OVS] = { > + /* pass all pkts up to CPU */ > + .eth_dst = zero_mac, > + .eth_dst_mask = zero_mac, > + .acl = true, > + }, > }; > > static int rocker_port_ctrl_vlan_acl(struct rocker_port *rocker_port, > @@ -3787,11 +3806,14 @@ static int rocker_port_stp_update(struct rocker_port *rocker_port, > break; > case BR_STATE_LEARNING: > case BR_STATE_FORWARDING: > - want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true; > + if (!rocker_port_is_ovs(rocker_port)) > + want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true; > want[ROCKER_CTRL_IPV4_MCAST] = true; > want[ROCKER_CTRL_IPV6_MCAST] = true; > if (rocker_port_is_bridged(rocker_port)) > want[ROCKER_CTRL_DFLT_BRIDGING] = true; > + else if (rocker_port_is_ovs(rocker_port)) > + want[ROCKER_CTRL_DFLT_OVS] = true; > else > want[ROCKER_CTRL_LOCAL_ARP] = true; > break; > @@ -5251,6 +5273,22 @@ static int rocker_port_bridge_leave(struct rocker_port *rocker_port) > return err; > } > > + > +static int rocker_port_ovs_changed(struct rocker_port *rocker_port, > + struct net_device *master) > +{ > + int err; > + > + rocker_port->bridge_dev = master; > + > + err = rocker_port_fwd_disable(rocker_port, SWITCHDEV_TRANS_NONE, 0); > + if (err) > + return err; > + err = rocker_port_fwd_enable(rocker_port, SWITCHDEV_TRANS_NONE, 0); > + > + return err; > +} > + > static int rocker_port_master_changed(struct net_device *dev) > { > struct rocker_port *rocker_port = netdev_priv(dev); > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) > * 3. Other, e.g. being added to or removed from a bond or openvswitch, > * in which case nothing is done > */ Maybe comment above needs adjusting? > - if (master && master->rtnl_link_ops && > - !strcmp(master->rtnl_link_ops->kind, "bridge")) > - err = rocker_port_bridge_join(rocker_port, master); > - else if (rocker_port_is_bridged(rocker_port)) > + if (master && master->rtnl_link_ops) { > + if (!strcmp(master->rtnl_link_ops->kind, "bridge")) > + err = rocker_port_bridge_join(rocker_port, master); > + else if (!strcmp(master->rtnl_link_ops->kind, "openvswitch")) > + err = rocker_port_ovs_changed(rocker_port, master); > + } else if (rocker_port_is_bridged(rocker_port)) { > err = rocker_port_bridge_leave(rocker_port); > + } else if (rocker_port_is_ovs(rocker_port)) { > + err = rocker_port_ovs_changed(rocker_port, NULL); > + } > > return err; > } > -- > 2.1.4 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-15 5:32 ` Scott Feldman @ 2015-07-15 6:34 ` Simon Horman 2015-07-15 7:18 ` Scott Feldman 0 siblings, 1 reply; 10+ messages in thread From: Simon Horman @ 2015-07-15 6:34 UTC (permalink / raw) To: Scott Feldman; +Cc: Jiri Pirko, Netdev, john fastabend On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote: > On Tue, Jul 14, 2015 at 9:45 PM, Simon Horman [snip] > > How about this? > > Looks good, some inline comments... [snip] > > @@ -321,9 +322,21 @@ static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port, > > return ntohs(vlan_id); > > } > > > > +static bool rocker_port_is_bridged__(const struct rocker_port *rocker_port, > > + const char *kind) > > Maybe use __rocker_port_is_bridged? (leading '__'; I've not seen use > of trailing '__'). Or __rocker_port_is_slave(port, kind)? Perhaps rocker_port_is_slave() would be a good choice? > > +{ > > + return rocker_port->bridge_dev && > > + !strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind); > > +} > > + > > static bool rocker_port_is_bridged(const struct rocker_port *rocker_port) > > { > > - return !!rocker_port->bridge_dev; > > + return rocker_port_is_bridged__(rocker_port, "bridge"); > > +} > > + > > +static bool rocker_port_is_ovs(const struct rocker_port *rocker_port) > > rocker_port_is_ovsed? Just to be consistent with is_bridged. Sure, for the sake of consistency I'll change things as you suggest. [snip] > > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) > > * 3. Other, e.g. being added to or removed from a bond or openvswitch, > > * in which case nothing is done > > */ > > Maybe comment above needs adjusting? Indeed, sorry for missing that. How about this? /* There are currently five cases handled here: * 1. Joining a bridge * 2. Joining a Open vSwitch datapath * 3. Leaving a previously joined bridge * 4. Leaving a previously joined Open vSwitch datapath * 5. Other, e.g. being added to or removed from a bond, * in which case nothing is done */ [snip] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-15 6:34 ` Simon Horman @ 2015-07-15 7:18 ` Scott Feldman 2015-07-15 7:54 ` Simon Horman 0 siblings, 1 reply; 10+ messages in thread From: Scott Feldman @ 2015-07-15 7:18 UTC (permalink / raw) To: Simon Horman; +Cc: Jiri Pirko, Netdev, john fastabend On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman <simon.horman@netronome.com> wrote: > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote: >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) >> > * 3. Other, e.g. being added to or removed from a bond or openvswitch, >> > * in which case nothing is done >> > */ >> >> Maybe comment above needs adjusting? > > Indeed, sorry for missing that. > How about this? > > > /* There are currently five cases handled here: > * 1. Joining a bridge > * 2. Joining a Open vSwitch datapath > * 3. Leaving a previously joined bridge > * 4. Leaving a previously joined Open vSwitch datapath > * 5. Other, e.g. being added to or removed from a bond, > * in which case nothing is done > */ Seems like one of those comments that needs adjusting each time the code changes, which isn't good. Maybe just kill the comment or write in a generic way saying what code is doing. Code seems obvious enough to me to not require a comment. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-15 7:18 ` Scott Feldman @ 2015-07-15 7:54 ` Simon Horman 2015-07-15 14:50 ` Scott Feldman 0 siblings, 1 reply; 10+ messages in thread From: Simon Horman @ 2015-07-15 7:54 UTC (permalink / raw) To: Scott Feldman; +Cc: Jiri Pirko, Netdev, john fastabend On Wed, Jul 15, 2015 at 12:18:20AM -0700, Scott Feldman wrote: > On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman > <simon.horman@netronome.com> wrote: > > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote: > > >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) > >> > * 3. Other, e.g. being added to or removed from a bond or openvswitch, > >> > * in which case nothing is done > >> > */ > >> > >> Maybe comment above needs adjusting? > > > > Indeed, sorry for missing that. > > How about this? > > > > > > /* There are currently five cases handled here: > > * 1. Joining a bridge > > * 2. Joining a Open vSwitch datapath > > * 3. Leaving a previously joined bridge > > * 4. Leaving a previously joined Open vSwitch datapath > > * 5. Other, e.g. being added to or removed from a bond, > > * in which case nothing is done > > */ > > Seems like one of those comments that needs adjusting each time the > code changes, which isn't good. Maybe just kill the comment or write > in a generic way saying what code is doing. Code seems obvious enough > to me to not require a comment. My purpose in adding the comment in the first place was to document the "other" case, as it wasn't handled and thus didn't seem obvious. Perhaps something like this would work. /* N.B: Do nothing if the type of master is not supported */ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-15 7:54 ` Simon Horman @ 2015-07-15 14:50 ` Scott Feldman 2015-07-16 1:41 ` Simon Horman 0 siblings, 1 reply; 10+ messages in thread From: Scott Feldman @ 2015-07-15 14:50 UTC (permalink / raw) To: Simon Horman; +Cc: Jiri Pirko, Netdev, john fastabend On Wed, Jul 15, 2015 at 12:54 AM, Simon Horman <simon.horman@netronome.com> wrote: > On Wed, Jul 15, 2015 at 12:18:20AM -0700, Scott Feldman wrote: >> On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman >> <simon.horman@netronome.com> wrote: >> > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote: >> >> >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) >> >> > * 3. Other, e.g. being added to or removed from a bond or openvswitch, >> >> > * in which case nothing is done >> >> > */ >> >> >> >> Maybe comment above needs adjusting? >> > >> > Indeed, sorry for missing that. >> > How about this? >> > >> > >> > /* There are currently five cases handled here: >> > * 1. Joining a bridge >> > * 2. Joining a Open vSwitch datapath >> > * 3. Leaving a previously joined bridge >> > * 4. Leaving a previously joined Open vSwitch datapath >> > * 5. Other, e.g. being added to or removed from a bond, >> > * in which case nothing is done >> > */ >> >> Seems like one of those comments that needs adjusting each time the >> code changes, which isn't good. Maybe just kill the comment or write >> in a generic way saying what code is doing. Code seems obvious enough >> to me to not require a comment. > > My purpose in adding the comment in the first place was > to document the "other" case, as it wasn't handled and thus > didn't seem obvious. > > Perhaps something like this would work. > > /* N.B: Do nothing if the type of master is not supported */ Ack ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode 2015-07-15 14:50 ` Scott Feldman @ 2015-07-16 1:41 ` Simon Horman 0 siblings, 0 replies; 10+ messages in thread From: Simon Horman @ 2015-07-16 1:41 UTC (permalink / raw) To: Scott Feldman; +Cc: Jiri Pirko, Netdev, john fastabend On Wed, Jul 15, 2015 at 07:50:32AM -0700, Scott Feldman wrote: > On Wed, Jul 15, 2015 at 12:54 AM, Simon Horman > <simon.horman@netronome.com> wrote: > > On Wed, Jul 15, 2015 at 12:18:20AM -0700, Scott Feldman wrote: > >> On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman > >> <simon.horman@netronome.com> wrote: > >> > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote: > >> > >> >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev) > >> >> > * 3. Other, e.g. being added to or removed from a bond or openvswitch, > >> >> > * in which case nothing is done > >> >> > */ > >> >> > >> >> Maybe comment above needs adjusting? > >> > > >> > Indeed, sorry for missing that. > >> > How about this? > >> > > >> > > >> > /* There are currently five cases handled here: > >> > * 1. Joining a bridge > >> > * 2. Joining a Open vSwitch datapath > >> > * 3. Leaving a previously joined bridge > >> > * 4. Leaving a previously joined Open vSwitch datapath > >> > * 5. Other, e.g. being added to or removed from a bond, > >> > * in which case nothing is done > >> > */ > >> > >> Seems like one of those comments that needs adjusting each time the > >> code changes, which isn't good. Maybe just kill the comment or write > >> in a generic way saying what code is doing. Code seems obvious enough > >> to me to not require a comment. > > > > My purpose in adding the comment in the first place was > > to document the "other" case, as it wasn't handled and thus > > didn't seem obvious. > > > > Perhaps something like this would work. > > > > /* N.B: Do nothing if the type of master is not supported */ > > Ack Thanks. I've made a formal submission of the path with the changes you suggested earlier in this thread. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-07-16 1:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-09 4:25 [PATCH/RFC net-next] rocker: forward packets to CPU when a port in promiscuous mode Simon Horman 2015-07-09 5:38 ` John Fastabend 2015-07-14 6:37 ` Scott Feldman 2015-07-15 4:45 ` Simon Horman 2015-07-15 5:32 ` Scott Feldman 2015-07-15 6:34 ` Simon Horman 2015-07-15 7:18 ` Scott Feldman 2015-07-15 7:54 ` Simon Horman 2015-07-15 14:50 ` Scott Feldman 2015-07-16 1:41 ` Simon Horman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.