* [RFC PATCH] macvlan: add FDB bridge ops @ 2012-03-21 0:26 John Fastabend 2012-03-21 8:34 ` Michael S. Tsirkin 2012-03-28 15:43 ` Roopa Prabhu 0 siblings, 2 replies; 7+ messages in thread From: John Fastabend @ 2012-03-21 0:26 UTC (permalink / raw) To: roprabhu; +Cc: netdev, mst Add support to add/del and dump the forwarding database for macvlan passthru mode. The macvlan driver acts like a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru case so adding forwarding rules is just adding the addr to the uc or mc lists. By default the passthru mode puts the lowerdev into a promiscuous mode to receive all packets. This behavior is not changed by this patch. This is a bit problematic and needs to be solved without IMHO breaking existing mechanics. Maybe on the first add_fdb we can decrement the promisc mode? That seems to work reasonable well and keep existing functionality in place... but requires an initial add to set things up which is a bit annoying so maybe a flag is better. I haven't thought too hard about it yet so any ideas welcome This patch is a result of Roopa Prabhu's work. Follow up patches are needed for VEPA and VEB macvlan modes. Only lightly touch tested at this point. CC: Roopa Prabhu <roprabhu@cisco.com> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- drivers/net/macvlan.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-) diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index f975afd..86af56b 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -349,6 +349,7 @@ static int macvlan_stop(struct net_device *dev) goto hash_del; } + dev_uc_unsync(lowerdev, dev); dev_mc_unsync(lowerdev, dev); if (dev->flags & IFF_ALLMULTI) dev_set_allmulti(lowerdev, -1); @@ -403,6 +404,7 @@ static void macvlan_set_multicast_list(struct net_device *dev) { struct macvlan_dev *vlan = netdev_priv(dev); + dev_uc_sync(vlan->lowerdev, dev); dev_mc_sync(vlan->lowerdev, dev); } @@ -542,6 +544,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev, return 0; } +static int macvlan_fdb_add(struct ndmsg *ndm, + struct net_device *dev, + unsigned char *addr, + u16 flags) +{ + struct macvlan_dev *vlan = netdev_priv(dev); + int err = -EINVAL; + + if (!vlan->port->passthru) + return -EOPNOTSUPP; + + if (is_unicast_ether_addr(addr)) + err = dev_uc_add(dev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_add(dev, addr); + + return err; +} + +static int macvlan_fdb_del(struct ndmsg *ndm, + struct net_device *dev, + unsigned char *addr) +{ + struct macvlan_dev *vlan = netdev_priv(dev); + int err = -EINVAL; + + if (!vlan->port->passthru) + return -EOPNOTSUPP; + + if (is_unicast_ether_addr(addr)) + err = dev_uc_del(dev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_del(dev, addr); + + return err; +} + static void macvlan_ethtool_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo) { @@ -577,6 +616,9 @@ static const struct net_device_ops macvlan_netdev_ops = { .ndo_validate_addr = eth_validate_addr, .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, + .ndo_fdb_add = macvlan_fdb_add, + .ndo_fdb_del = macvlan_fdb_del, + .ndo_fdb_dump = ndo_dflt_fdb_dump, }; void macvlan_common_setup(struct net_device *dev) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] macvlan: add FDB bridge ops 2012-03-21 0:26 [RFC PATCH] macvlan: add FDB bridge ops John Fastabend @ 2012-03-21 8:34 ` Michael S. Tsirkin 2012-03-28 15:43 ` Roopa Prabhu 1 sibling, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2012-03-21 8:34 UTC (permalink / raw) To: John Fastabend; +Cc: roprabhu, netdev On Tue, Mar 20, 2012 at 05:26:48PM -0700, John Fastabend wrote: > Add support to add/del and dump the forwarding database > for macvlan passthru mode. The macvlan driver acts like > a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru > case so adding forwarding rules is just adding the addr > to the uc or mc lists. > > By default the passthru mode puts the lowerdev into a > promiscuous mode to receive all packets. This behavior > is not changed by this patch. This is a bit problematic > and needs to be solved without IMHO breaking existing > mechanics. Maybe on the first add_fdb we can decrement > the promisc mode? That seems to work reasonable well and > keep existing functionality in place... but requires > an initial add to set things up which is a bit annoying > so maybe a flag is better. I think a flag is better, too. > I haven't thought too hard > about it yet so any ideas welcome > > This patch is a result of Roopa Prabhu's work. Follow up > patches are needed for VEPA and VEB macvlan modes. For bridged mode, we need to update the hash tables. What's needed for VEPA? > Only lightly touch tested at this point. > > CC: Roopa Prabhu <roprabhu@cisco.com> > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > > drivers/net/macvlan.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 42 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > index f975afd..86af56b 100644 > --- a/drivers/net/macvlan.c > +++ b/drivers/net/macvlan.c > @@ -349,6 +349,7 @@ static int macvlan_stop(struct net_device *dev) > goto hash_del; > } > > + dev_uc_unsync(lowerdev, dev); > dev_mc_unsync(lowerdev, dev); > if (dev->flags & IFF_ALLMULTI) > dev_set_allmulti(lowerdev, -1); > @@ -403,6 +404,7 @@ static void macvlan_set_multicast_list(struct net_device *dev) > { > struct macvlan_dev *vlan = netdev_priv(dev); > > + dev_uc_sync(vlan->lowerdev, dev); > dev_mc_sync(vlan->lowerdev, dev); > } > > @@ -542,6 +544,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev, > return 0; > } > > +static int macvlan_fdb_add(struct ndmsg *ndm, > + struct net_device *dev, > + unsigned char *addr, > + u16 flags) > +{ > + struct macvlan_dev *vlan = netdev_priv(dev); > + int err = -EINVAL; > + > + if (!vlan->port->passthru) > + return -EOPNOTSUPP; > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_add(dev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_add(dev, addr); > + > + return err; > +} > + > +static int macvlan_fdb_del(struct ndmsg *ndm, > + struct net_device *dev, > + unsigned char *addr) > +{ > + struct macvlan_dev *vlan = netdev_priv(dev); > + int err = -EINVAL; > + > + if (!vlan->port->passthru) > + return -EOPNOTSUPP; > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_del(dev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_del(dev, addr); > + > + return err; > +} > + > static void macvlan_ethtool_get_drvinfo(struct net_device *dev, > struct ethtool_drvinfo *drvinfo) > { > @@ -577,6 +616,9 @@ static const struct net_device_ops macvlan_netdev_ops = { > .ndo_validate_addr = eth_validate_addr, > .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, > + .ndo_fdb_add = macvlan_fdb_add, > + .ndo_fdb_del = macvlan_fdb_del, > + .ndo_fdb_dump = ndo_dflt_fdb_dump, > }; > > void macvlan_common_setup(struct net_device *dev) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] macvlan: add FDB bridge ops 2012-03-21 0:26 [RFC PATCH] macvlan: add FDB bridge ops John Fastabend 2012-03-21 8:34 ` Michael S. Tsirkin @ 2012-03-28 15:43 ` Roopa Prabhu 2012-03-28 15:52 ` Michael S. Tsirkin 1 sibling, 1 reply; 7+ messages in thread From: Roopa Prabhu @ 2012-03-28 15:43 UTC (permalink / raw) To: John Fastabend; +Cc: netdev, mst On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: > Add support to add/del and dump the forwarding database > for macvlan passthru mode. The macvlan driver acts like > a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru > case so adding forwarding rules is just adding the addr > to the uc or mc lists. > > By default the passthru mode puts the lowerdev into a > promiscuous mode to receive all packets. This behavior > is not changed by this patch. This is a bit problematic > and needs to be solved without IMHO breaking existing > mechanics. Maybe on the first add_fdb we can decrement > the promisc mode? That seems to work reasonable well and > keep existing functionality in place... but requires > an initial add to set things up which is a bit annoying > so maybe a flag is better. I haven't thought too hard > about it yet so any ideas welcome > > This patch is a result of Roopa Prabhu's work. Follow up > patches are needed for VEPA and VEB macvlan modes. > > Only lightly touch tested at this point. > > CC: Roopa Prabhu <roprabhu@cisco.com> > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > > drivers/net/macvlan.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 42 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > index f975afd..86af56b 100644 > --- a/drivers/net/macvlan.c > +++ b/drivers/net/macvlan.c > @@ -349,6 +349,7 @@ static int macvlan_stop(struct net_device *dev) > goto hash_del; > } > > + dev_uc_unsync(lowerdev, dev); > dev_mc_unsync(lowerdev, dev); > if (dev->flags & IFF_ALLMULTI) > dev_set_allmulti(lowerdev, -1); > @@ -403,6 +404,7 @@ static void macvlan_set_multicast_list(struct net_device > *dev) > { > struct macvlan_dev *vlan = netdev_priv(dev); > > + dev_uc_sync(vlan->lowerdev, dev); > dev_mc_sync(vlan->lowerdev, dev); > } > > @@ -542,6 +544,43 @@ static int macvlan_vlan_rx_kill_vid(struct net_device > *dev, > return 0; > } > > +static int macvlan_fdb_add(struct ndmsg *ndm, > + struct net_device *dev, > + unsigned char *addr, > + u16 flags) > +{ > + struct macvlan_dev *vlan = netdev_priv(dev); > + int err = -EINVAL; > + > + if (!vlan->port->passthru) > + return -EOPNOTSUPP; > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_add(dev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_add(dev, addr); > + > + return err; > +} > + > +static int macvlan_fdb_del(struct ndmsg *ndm, > + struct net_device *dev, > + unsigned char *addr) > +{ > + struct macvlan_dev *vlan = netdev_priv(dev); > + int err = -EINVAL; > + > + if (!vlan->port->passthru) > + return -EOPNOTSUPP; > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_del(dev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_del(dev, addr); > + > + return err; > +} > + > static void macvlan_ethtool_get_drvinfo(struct net_device *dev, > struct ethtool_drvinfo *drvinfo) > { > @@ -577,6 +616,9 @@ static const struct net_device_ops macvlan_netdev_ops = { > .ndo_validate_addr = eth_validate_addr, > .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, > + .ndo_fdb_add = macvlan_fdb_add, > + .ndo_fdb_del = macvlan_fdb_del, > + .ndo_fdb_dump = ndo_dflt_fdb_dump, > }; > > void macvlan_common_setup(struct net_device *dev) > Thanks John. Looks good. I added a few things to your patch below. Yes, I think the promisc check is required. Made an attempt to add a flag below (I did not get a chance to think about other approaches there too). Briefly tested it with the br command. diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index f975afd..9bc70ad 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -34,6 +34,9 @@ #define MACVLAN_HASH_SIZE (1 << BITS_PER_BYTE) +/* macvlan port flags */ +#define MACVLAN_FLAG_PROMISC 0x1 + struct macvlan_port { struct net_device *dev; struct hlist_head vlan_hash[MACVLAN_HASH_SIZE]; @@ -41,6 +44,7 @@ struct macvlan_port { struct rcu_head rcu; bool passthru; int count; + unsigned int flags; }; static void macvlan_port_destroy(struct net_device *dev); @@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev) if (vlan->port->passthru) { dev_set_promiscuity(lowerdev, 1); + vlan->port->flags |= MACVLAN_FLAG_PROMISC; goto hash_add; } @@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev) struct net_device *lowerdev = vlan->lowerdev; if (vlan->port->passthru) { - dev_set_promiscuity(lowerdev, -1); + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { + dev_set_promiscuity(lowerdev, -1); + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; + } goto hash_del; } + dev_uc_unsync(lowerdev, dev); dev_mc_unsync(lowerdev, dev); if (dev->flags & IFF_ALLMULTI) dev_set_allmulti(lowerdev, -1); @@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(struct net_device *dev) { struct macvlan_dev *vlan = netdev_priv(dev); + dev_uc_sync(vlan->lowerdev, dev); dev_mc_sync(vlan->lowerdev, dev); } @@ -542,6 +552,58 @@ static int macvlan_vlan_rx_kill_vid(struct net_device *dev, return 0; } +static int macvlan_fdb_add(struct ndmsg *ndm, + struct net_device *dev, + unsigned char *addr, + u16 flags) +{ + struct macvlan_dev *vlan = netdev_priv(dev); + struct net_device *lowerdev = vlan->lowerdev; + const struct net_device_ops *ops = lowerdev->netdev_ops; + int err = -EINVAL; + + if (!vlan->port->passthru) + return -EOPNOTSUPP; + + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { + dev_set_promiscuity (lowerdev, -1); + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; + } + + if (ops->ndo_fdb_add) + return ops->ndo_fdb_add(ndm, lowerdev, addr, flags); + + if (is_unicast_ether_addr(addr)) + err = dev_uc_add_excl(lowerdev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_add(lowerdev, addr); + + return err; +} + +static int macvlan_fdb_del(struct ndmsg *ndm, + struct net_device *dev, + unsigned char *addr) +{ + struct macvlan_dev *vlan = netdev_priv(dev); + struct net_device *lowerdev = vlan->lowerdev; + const struct net_device_ops *ops = lowerdev->netdev_ops; + int err = -EINVAL; + + if (!vlan->port->passthru) + return -EOPNOTSUPP; + + if (ops->ndo_fdb_del) + return ops->ndo_fdb_del(ndm, lowerdev, addr); + + if (is_unicast_ether_addr(addr)) + err = dev_uc_del(lowerdev, addr); + else if (is_multicast_ether_addr(addr)) + err = dev_mc_del(lowerdev, addr); + + return err; +} + static void macvlan_ethtool_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *drvinfo) { @@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops = { .ndo_validate_addr = eth_validate_addr, .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, + .ndo_fdb_add = macvlan_fdb_add, + .ndo_fdb_del = macvlan_fdb_del, + .ndo_fdb_dump = ndo_dflt_fdb_dump, }; void macvlan_common_setup(struct net_device *dev) ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] macvlan: add FDB bridge ops 2012-03-28 15:43 ` Roopa Prabhu @ 2012-03-28 15:52 ` Michael S. Tsirkin 2012-03-28 15:58 ` John Fastabend 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2012-03-28 15:52 UTC (permalink / raw) To: Roopa Prabhu; +Cc: John Fastabend, netdev On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote: > On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: > > > Add support to add/del and dump the forwarding database > > for macvlan passthru mode. The macvlan driver acts like > > a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru > > case so adding forwarding rules is just adding the addr > > to the uc or mc lists. > > > > By default the passthru mode puts the lowerdev into a > > promiscuous mode to receive all packets. This behavior > > is not changed by this patch. This is a bit problematic > > and needs to be solved without IMHO breaking existing > > mechanics. Maybe on the first add_fdb we can decrement > > the promisc mode? That seems to work reasonable well and > > keep existing functionality in place... but requires > > an initial add to set things up which is a bit annoying > > so maybe a flag is better. I haven't thought too hard > > about it yet so any ideas welcome ... > Thanks John. Looks good. > I added a few things to your patch below. Yes, I think the promisc check is > required. Made an attempt to add a flag below (I did not get a chance to > think about other approaches there too). Briefly tested it with the br > command. > > > diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c > index f975afd..9bc70ad 100644 > --- a/drivers/net/macvlan.c > +++ b/drivers/net/macvlan.c > @@ -34,6 +34,9 @@ > > #define MACVLAN_HASH_SIZE (1 << BITS_PER_BYTE) > > +/* macvlan port flags */ > +#define MACVLAN_FLAG_PROMISC 0x1 > + > struct macvlan_port { > struct net_device *dev; > struct hlist_head vlan_hash[MACVLAN_HASH_SIZE]; > @@ -41,6 +44,7 @@ struct macvlan_port { > struct rcu_head rcu; > bool passthru; > int count; > + unsigned int flags; > }; > > static void macvlan_port_destroy(struct net_device *dev); > @@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev) > > if (vlan->port->passthru) { > dev_set_promiscuity(lowerdev, 1); > + vlan->port->flags |= MACVLAN_FLAG_PROMISC; > goto hash_add; > } > > @@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev) > struct net_device *lowerdev = vlan->lowerdev; > > if (vlan->port->passthru) { > - dev_set_promiscuity(lowerdev, -1); > + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { > + dev_set_promiscuity(lowerdev, -1); > + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; > + } > goto hash_del; > } > > + dev_uc_unsync(lowerdev, dev); > dev_mc_unsync(lowerdev, dev); > if (dev->flags & IFF_ALLMULTI) > dev_set_allmulti(lowerdev, -1); > @@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(struct net_device > *dev) > { > struct macvlan_dev *vlan = netdev_priv(dev); > > + dev_uc_sync(vlan->lowerdev, dev); > dev_mc_sync(vlan->lowerdev, dev); > } > > @@ -542,6 +552,58 @@ static int macvlan_vlan_rx_kill_vid(struct net_device > *dev, > return 0; > } > > +static int macvlan_fdb_add(struct ndmsg *ndm, > + struct net_device *dev, > + unsigned char *addr, > + u16 flags) > +{ > + struct macvlan_dev *vlan = netdev_priv(dev); > + struct net_device *lowerdev = vlan->lowerdev; > + const struct net_device_ops *ops = lowerdev->netdev_ops; > + int err = -EINVAL; > + > + if (!vlan->port->passthru) > + return -EOPNOTSUPP; > + > + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { > + dev_set_promiscuity (lowerdev, -1); > + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; > + } > + > + if (ops->ndo_fdb_add) > + return ops->ndo_fdb_add(ndm, lowerdev, addr, flags); > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_add_excl(lowerdev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_add(lowerdev, addr); > + > + return err; > +} > + > +static int macvlan_fdb_del(struct ndmsg *ndm, > + struct net_device *dev, > + unsigned char *addr) > +{ > + struct macvlan_dev *vlan = netdev_priv(dev); > + struct net_device *lowerdev = vlan->lowerdev; > + const struct net_device_ops *ops = lowerdev->netdev_ops; > + int err = -EINVAL; > + > + if (!vlan->port->passthru) > + return -EOPNOTSUPP; > + > + if (ops->ndo_fdb_del) > + return ops->ndo_fdb_del(ndm, lowerdev, addr); > + > + if (is_unicast_ether_addr(addr)) > + err = dev_uc_del(lowerdev, addr); > + else if (is_multicast_ether_addr(addr)) > + err = dev_mc_del(lowerdev, addr); > + > + return err; > +} > + > static void macvlan_ethtool_get_drvinfo(struct net_device *dev, > struct ethtool_drvinfo *drvinfo) > { > @@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops = > { > .ndo_validate_addr = eth_validate_addr, > .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, > .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, > + .ndo_fdb_add = macvlan_fdb_add, > + .ndo_fdb_del = macvlan_fdb_del, > + .ndo_fdb_dump = ndo_dflt_fdb_dump, > }; > > void macvlan_common_setup(struct net_device *dev) > So this clears the promisc on the first add which is a bit annoying. How about a simple flag, set when we create the macvlan? -- MST ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] macvlan: add FDB bridge ops 2012-03-28 15:52 ` Michael S. Tsirkin @ 2012-03-28 15:58 ` John Fastabend 2012-03-29 21:26 ` Roopa Prabhu 0 siblings, 1 reply; 7+ messages in thread From: John Fastabend @ 2012-03-28 15:58 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Roopa Prabhu, netdev On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote: > On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote: >> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: >> >>> Add support to add/del and dump the forwarding database >>> for macvlan passthru mode. The macvlan driver acts like >>> a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru >>> case so adding forwarding rules is just adding the addr >>> to the uc or mc lists. >>> >>> By default the passthru mode puts the lowerdev into a >>> promiscuous mode to receive all packets. This behavior >>> is not changed by this patch. This is a bit problematic >>> and needs to be solved without IMHO breaking existing >>> mechanics. Maybe on the first add_fdb we can decrement >>> the promisc mode? That seems to work reasonable well and >>> keep existing functionality in place... but requires >>> an initial add to set things up which is a bit annoying >>> so maybe a flag is better. I haven't thought too hard >>> about it yet so any ideas welcome > > > ... > >> Thanks John. Looks good. >> I added a few things to your patch below. Yes, I think the promisc check is >> required. Made an attempt to add a flag below (I did not get a chance to >> think about other approaches there too). Briefly tested it with the br >> command. >> >> >> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c >> index f975afd..9bc70ad 100644 >> --- a/drivers/net/macvlan.c >> +++ b/drivers/net/macvlan.c >> @@ -34,6 +34,9 @@ >> >> #define MACVLAN_HASH_SIZE (1 << BITS_PER_BYTE) >> >> +/* macvlan port flags */ >> +#define MACVLAN_FLAG_PROMISC 0x1 >> + >> struct macvlan_port { >> struct net_device *dev; >> struct hlist_head vlan_hash[MACVLAN_HASH_SIZE]; >> @@ -41,6 +44,7 @@ struct macvlan_port { >> struct rcu_head rcu; >> bool passthru; >> int count; >> + unsigned int flags; >> }; >> >> static void macvlan_port_destroy(struct net_device *dev); >> @@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev) >> >> if (vlan->port->passthru) { >> dev_set_promiscuity(lowerdev, 1); >> + vlan->port->flags |= MACVLAN_FLAG_PROMISC; >> goto hash_add; >> } >> >> @@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev) >> struct net_device *lowerdev = vlan->lowerdev; >> >> if (vlan->port->passthru) { >> - dev_set_promiscuity(lowerdev, -1); >> + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { >> + dev_set_promiscuity(lowerdev, -1); >> + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; >> + } >> goto hash_del; >> } >> >> + dev_uc_unsync(lowerdev, dev); >> dev_mc_unsync(lowerdev, dev); >> if (dev->flags & IFF_ALLMULTI) >> dev_set_allmulti(lowerdev, -1); >> @@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(struct net_device >> *dev) >> { >> struct macvlan_dev *vlan = netdev_priv(dev); >> >> + dev_uc_sync(vlan->lowerdev, dev); >> dev_mc_sync(vlan->lowerdev, dev); >> } >> >> @@ -542,6 +552,58 @@ static int macvlan_vlan_rx_kill_vid(struct net_device >> *dev, >> return 0; >> } >> >> +static int macvlan_fdb_add(struct ndmsg *ndm, >> + struct net_device *dev, >> + unsigned char *addr, >> + u16 flags) >> +{ >> + struct macvlan_dev *vlan = netdev_priv(dev); >> + struct net_device *lowerdev = vlan->lowerdev; >> + const struct net_device_ops *ops = lowerdev->netdev_ops; >> + int err = -EINVAL; >> + >> + if (!vlan->port->passthru) >> + return -EOPNOTSUPP; >> + >> + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { >> + dev_set_promiscuity (lowerdev, -1); >> + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; >> + } >> + >> + if (ops->ndo_fdb_add) >> + return ops->ndo_fdb_add(ndm, lowerdev, addr, flags); >> + >> + if (is_unicast_ether_addr(addr)) >> + err = dev_uc_add_excl(lowerdev, addr); >> + else if (is_multicast_ether_addr(addr)) >> + err = dev_mc_add(lowerdev, addr); >> + >> + return err; >> +} >> + >> +static int macvlan_fdb_del(struct ndmsg *ndm, >> + struct net_device *dev, >> + unsigned char *addr) >> +{ >> + struct macvlan_dev *vlan = netdev_priv(dev); >> + struct net_device *lowerdev = vlan->lowerdev; >> + const struct net_device_ops *ops = lowerdev->netdev_ops; >> + int err = -EINVAL; >> + >> + if (!vlan->port->passthru) >> + return -EOPNOTSUPP; >> + >> + if (ops->ndo_fdb_del) >> + return ops->ndo_fdb_del(ndm, lowerdev, addr); >> + >> + if (is_unicast_ether_addr(addr)) >> + err = dev_uc_del(lowerdev, addr); >> + else if (is_multicast_ether_addr(addr)) >> + err = dev_mc_del(lowerdev, addr); >> + >> + return err; >> +} >> + >> static void macvlan_ethtool_get_drvinfo(struct net_device *dev, >> struct ethtool_drvinfo *drvinfo) >> { >> @@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops = >> { >> .ndo_validate_addr = eth_validate_addr, >> .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, >> .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, >> + .ndo_fdb_add = macvlan_fdb_add, >> + .ndo_fdb_del = macvlan_fdb_del, >> + .ndo_fdb_dump = ndo_dflt_fdb_dump, >> }; >> >> void macvlan_common_setup(struct net_device *dev) >> > > > So this clears the promisc on the first add which > is a bit annoying. How about a simple flag, set when > we create the macvlan? > Agreed. This probably needs a new attrib maybe IFLA_MACVLAN_FLAGS unless there already exists a per "kind" (rtnl_link_ops) flags field we can use. I scanned the code briefly and didn't see any such thing so likely we need the new attribute. .John ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] macvlan: add FDB bridge ops 2012-03-28 15:58 ` John Fastabend @ 2012-03-29 21:26 ` Roopa Prabhu 2012-03-30 1:06 ` John Fastabend 0 siblings, 1 reply; 7+ messages in thread From: Roopa Prabhu @ 2012-03-29 21:26 UTC (permalink / raw) To: John Fastabend, Michael S. Tsirkin; +Cc: netdev On 3/28/12 8:58 AM, "John Fastabend" <john.r.fastabend@intel.com> wrote: > On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote: >> On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote: >>> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: >>> >>>> Add support to add/del and dump the forwarding database >>>> for macvlan passthru mode. The macvlan driver acts like >>>> a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru >>>> case so adding forwarding rules is just adding the addr >>>> to the uc or mc lists. >>>> >>>> By default the passthru mode puts the lowerdev into a >>>> promiscuous mode to receive all packets. This behavior >>>> is not changed by this patch. This is a bit problematic >>>> and needs to be solved without IMHO breaking existing >>>> mechanics. Maybe on the first add_fdb we can decrement >>>> the promisc mode? That seems to work reasonable well and >>>> keep existing functionality in place... but requires >>>> an initial add to set things up which is a bit annoying >>>> so maybe a flag is better. I haven't thought too hard >>>> about it yet so any ideas welcome >> >> >> ... >> >>> Thanks John. Looks good. >>> I added a few things to your patch below. Yes, I think the promisc check is >>> required. Made an attempt to add a flag below (I did not get a chance to >>> think about other approaches there too). Briefly tested it with the br >>> command. >>> >>> >>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c >>> index f975afd..9bc70ad 100644 >>> --- a/drivers/net/macvlan.c >>> +++ b/drivers/net/macvlan.c >>> @@ -34,6 +34,9 @@ >>> >>> #define MACVLAN_HASH_SIZE (1 << BITS_PER_BYTE) >>> >>> +/* macvlan port flags */ >>> +#define MACVLAN_FLAG_PROMISC 0x1 >>> + >>> struct macvlan_port { >>> struct net_device *dev; >>> struct hlist_head vlan_hash[MACVLAN_HASH_SIZE]; >>> @@ -41,6 +44,7 @@ struct macvlan_port { >>> struct rcu_head rcu; >>> bool passthru; >>> int count; >>> + unsigned int flags; >>> }; >>> >>> static void macvlan_port_destroy(struct net_device *dev); >>> @@ -313,6 +317,7 @@ static int macvlan_open(struct net_device *dev) >>> >>> if (vlan->port->passthru) { >>> dev_set_promiscuity(lowerdev, 1); >>> + vlan->port->flags |= MACVLAN_FLAG_PROMISC; >>> goto hash_add; >>> } >>> >>> @@ -345,10 +350,14 @@ static int macvlan_stop(struct net_device *dev) >>> struct net_device *lowerdev = vlan->lowerdev; >>> >>> if (vlan->port->passthru) { >>> - dev_set_promiscuity(lowerdev, -1); >>> + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { >>> + dev_set_promiscuity(lowerdev, -1); >>> + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; >>> + } >>> goto hash_del; >>> } >>> >>> + dev_uc_unsync(lowerdev, dev); >>> dev_mc_unsync(lowerdev, dev); >>> if (dev->flags & IFF_ALLMULTI) >>> dev_set_allmulti(lowerdev, -1); >>> @@ -403,6 +412,7 @@ static void macvlan_set_multicast_list(struct net_device >>> *dev) >>> { >>> struct macvlan_dev *vlan = netdev_priv(dev); >>> >>> + dev_uc_sync(vlan->lowerdev, dev); >>> dev_mc_sync(vlan->lowerdev, dev); >>> } >>> >>> @@ -542,6 +552,58 @@ static int macvlan_vlan_rx_kill_vid(struct net_device >>> *dev, >>> return 0; >>> } >>> >>> +static int macvlan_fdb_add(struct ndmsg *ndm, >>> + struct net_device *dev, >>> + unsigned char *addr, >>> + u16 flags) >>> +{ >>> + struct macvlan_dev *vlan = netdev_priv(dev); >>> + struct net_device *lowerdev = vlan->lowerdev; >>> + const struct net_device_ops *ops = lowerdev->netdev_ops; >>> + int err = -EINVAL; >>> + >>> + if (!vlan->port->passthru) >>> + return -EOPNOTSUPP; >>> + >>> + if (vlan->port->flags & MACVLAN_FLAG_PROMISC) { >>> + dev_set_promiscuity (lowerdev, -1); >>> + vlan->port->flags &= ~MACVLAN_FLAG_PROMISC; >>> + } >>> + >>> + if (ops->ndo_fdb_add) >>> + return ops->ndo_fdb_add(ndm, lowerdev, addr, flags); >>> + >>> + if (is_unicast_ether_addr(addr)) >>> + err = dev_uc_add_excl(lowerdev, addr); >>> + else if (is_multicast_ether_addr(addr)) >>> + err = dev_mc_add(lowerdev, addr); >>> + >>> + return err; >>> +} >>> + >>> +static int macvlan_fdb_del(struct ndmsg *ndm, >>> + struct net_device *dev, >>> + unsigned char *addr) >>> +{ >>> + struct macvlan_dev *vlan = netdev_priv(dev); >>> + struct net_device *lowerdev = vlan->lowerdev; >>> + const struct net_device_ops *ops = lowerdev->netdev_ops; >>> + int err = -EINVAL; >>> + >>> + if (!vlan->port->passthru) >>> + return -EOPNOTSUPP; >>> + >>> + if (ops->ndo_fdb_del) >>> + return ops->ndo_fdb_del(ndm, lowerdev, addr); >>> + >>> + if (is_unicast_ether_addr(addr)) >>> + err = dev_uc_del(lowerdev, addr); >>> + else if (is_multicast_ether_addr(addr)) >>> + err = dev_mc_del(lowerdev, addr); >>> + >>> + return err; >>> +} >>> + >>> static void macvlan_ethtool_get_drvinfo(struct net_device *dev, >>> struct ethtool_drvinfo *drvinfo) >>> { >>> @@ -577,6 +639,9 @@ static const struct net_device_ops macvlan_netdev_ops = >>> { >>> .ndo_validate_addr = eth_validate_addr, >>> .ndo_vlan_rx_add_vid = macvlan_vlan_rx_add_vid, >>> .ndo_vlan_rx_kill_vid = macvlan_vlan_rx_kill_vid, >>> + .ndo_fdb_add = macvlan_fdb_add, >>> + .ndo_fdb_del = macvlan_fdb_del, >>> + .ndo_fdb_dump = ndo_dflt_fdb_dump, >>> }; >>> >>> void macvlan_common_setup(struct net_device *dev) >>> >> >> >> So this clears the promisc on the first add which >> is a bit annoying. How about a simple flag, set when >> we create the macvlan? >> > > Agreed. This probably needs a new attrib maybe IFLA_MACVLAN_FLAGS unless > there already exists a per "kind" (rtnl_link_ops) flags field we can use. > I scanned the code briefly and didn't see any such thing so likely we need > the new attribute. O ok. That makes sense. Unfortunately I am not sure if I will be able to get back to this anytime in the near future. If anyone wants to rework this patch please do. Thanks. Roopa ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] macvlan: add FDB bridge ops 2012-03-29 21:26 ` Roopa Prabhu @ 2012-03-30 1:06 ` John Fastabend 0 siblings, 0 replies; 7+ messages in thread From: John Fastabend @ 2012-03-30 1:06 UTC (permalink / raw) To: Roopa Prabhu; +Cc: Michael S. Tsirkin, netdev On 3/29/2012 2:26 PM, Roopa Prabhu wrote: > > > > On 3/28/12 8:58 AM, "John Fastabend" <john.r.fastabend@intel.com> wrote: > >> On 3/28/2012 8:52 AM, Michael S. Tsirkin wrote: >>> On Wed, Mar 28, 2012 at 08:43:56AM -0700, Roopa Prabhu wrote: >>>> On 3/20/12 5:26 PM, "John Fastabend" <john.r.fastabend@intel.com> wrote: >>>> >>>>> Add support to add/del and dump the forwarding database >>>>> for macvlan passthru mode. The macvlan driver acts like >>>>> a Two Port Mac Relay (TPMR 802.1Q-2011) in the passthru >>>>> case so adding forwarding rules is just adding the addr >>>>> to the uc or mc lists. >>>>> [...] >>> >>> >>> So this clears the promisc on the first add which >>> is a bit annoying. How about a simple flag, set when >>> we create the macvlan? >>> >> >> Agreed. This probably needs a new attrib maybe IFLA_MACVLAN_FLAGS unless >> there already exists a per "kind" (rtnl_link_ops) flags field we can use. >> I scanned the code briefly and didn't see any such thing so likely we need >> the new attribute. > > O ok. That makes sense. Unfortunately I am not sure if I will be able to get > back to this anytime in the near future. If anyone wants to rework this > patch please do. > > Thanks. > Roopa > > OK I'll fix it up and submit it with the rest of the FDB patches. .John ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-03-30 1:06 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-21 0:26 [RFC PATCH] macvlan: add FDB bridge ops John Fastabend 2012-03-21 8:34 ` Michael S. Tsirkin 2012-03-28 15:43 ` Roopa Prabhu 2012-03-28 15:52 ` Michael S. Tsirkin 2012-03-28 15:58 ` John Fastabend 2012-03-29 21:26 ` Roopa Prabhu 2012-03-30 1:06 ` John Fastabend
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).