netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
@ 2015-05-06 21:54 Sridhar Samudrala
  2015-05-06 23:35 ` Jamal Hadi Salim
  2015-05-12 22:20 ` David Miller
  0 siblings, 2 replies; 10+ messages in thread
From: Sridhar Samudrala @ 2015-05-06 21:54 UTC (permalink / raw)
  To: sfeldma, netdev

- introduce port fdb obj and generic switchdev_port_fdb_add/del/dump()
- use switchdev_port_fdb_add/del/dump in rocker/team/bonding ndo ops.
- add support for fdb obj in switchdev_port_obj_add/del/dump()
- switch rocker to implement fdb ops via switchdev_ops

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/bonding/bond_main.c      |   3 +
 drivers/net/ethernet/rocker/rocker.c | 183 +++++++++++++++--------------------
 drivers/net/team/team.c              |   3 +
 include/net/switchdev.h              |  50 ++++++++++
 net/switchdev/switchdev.c            | 175 +++++++++++++++++++++++++++++++++
 5 files changed, 309 insertions(+), 105 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 5215e63..f63d078 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4039,6 +4039,9 @@ static const struct net_device_ops bond_netdev_ops = {
 	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
 	.ndo_bridge_getlink	= switchdev_port_bridge_getlink,
 	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
+	.ndo_fdb_add		= switchdev_port_fdb_add,
+	.ndo_fdb_del		= switchdev_port_fdb_del,
+	.ndo_fdb_dump		= switchdev_port_fdb_dump,
 	.ndo_features_check	= passthru_features_check,
 };
 
diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index e9ee372..ba9011e 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4153,108 +4153,6 @@ static int rocker_port_vlan_rx_kill_vid(struct net_device *dev,
 	return rocker_port_vlan(rocker_port, ROCKER_OP_FLAG_REMOVE, vid);
 }
 
-static int rocker_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
-			       struct net_device *dev,
-			       const unsigned char *addr, u16 vid,
-			       u16 nlm_flags)
-{
-	struct rocker_port *rocker_port = netdev_priv(dev);
-	__be16 vlan_id = rocker_port_vid_to_vlan(rocker_port, vid, NULL);
-	int flags = 0;
-
-	if (!rocker_port_is_bridged(rocker_port))
-		return -EINVAL;
-
-	return rocker_port_fdb(rocker_port, addr, vlan_id, flags);
-}
-
-static int rocker_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
-			       struct net_device *dev,
-			       const unsigned char *addr, u16 vid)
-{
-	struct rocker_port *rocker_port = netdev_priv(dev);
-	__be16 vlan_id = rocker_port_vid_to_vlan(rocker_port, vid, NULL);
-	int flags = ROCKER_OP_FLAG_REMOVE;
-
-	if (!rocker_port_is_bridged(rocker_port))
-		return -EINVAL;
-
-	return rocker_port_fdb(rocker_port, addr, vlan_id, flags);
-}
-
-static int rocker_fdb_fill_info(struct sk_buff *skb,
-				struct rocker_port *rocker_port,
-				const unsigned char *addr, u16 vid,
-				u32 portid, u32 seq, int type,
-				unsigned int flags)
-{
-	struct nlmsghdr *nlh;
-	struct ndmsg *ndm;
-
-	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
-	if (!nlh)
-		return -EMSGSIZE;
-
-	ndm = nlmsg_data(nlh);
-	ndm->ndm_family	 = AF_BRIDGE;
-	ndm->ndm_pad1    = 0;
-	ndm->ndm_pad2    = 0;
-	ndm->ndm_flags	 = NTF_SELF;
-	ndm->ndm_type	 = 0;
-	ndm->ndm_ifindex = rocker_port->dev->ifindex;
-	ndm->ndm_state   = NUD_REACHABLE;
-
-	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, addr))
-		goto nla_put_failure;
-
-	if (vid && nla_put_u16(skb, NDA_VLAN, vid))
-		goto nla_put_failure;
-
-	nlmsg_end(skb, nlh);
-	return 0;
-
-nla_put_failure:
-	nlmsg_cancel(skb, nlh);
-	return -EMSGSIZE;
-}
-
-static int rocker_port_fdb_dump(struct sk_buff *skb,
-				struct netlink_callback *cb,
-				struct net_device *dev,
-				struct net_device *filter_dev,
-				int idx)
-{
-	struct rocker_port *rocker_port = netdev_priv(dev);
-	struct rocker *rocker = rocker_port->rocker;
-	struct rocker_fdb_tbl_entry *found;
-	struct hlist_node *tmp;
-	int bkt;
-	unsigned long lock_flags;
-	const unsigned char *addr;
-	u16 vid;
-	int err;
-
-	spin_lock_irqsave(&rocker->fdb_tbl_lock, lock_flags);
-	hash_for_each_safe(rocker->fdb_tbl, bkt, tmp, found, entry) {
-		if (found->key.pport != rocker_port->pport)
-			continue;
-		if (idx < cb->args[0])
-			goto skip;
-		addr = found->key.addr;
-		vid = rocker_port_vlan_to_vid(rocker_port, found->key.vlan_id);
-		err = rocker_fdb_fill_info(skb, rocker_port, addr, vid,
-					   NETLINK_CB(cb->skb).portid,
-					   cb->nlh->nlmsg_seq,
-					   RTM_NEWNEIGH, NLM_F_MULTI);
-		if (err < 0)
-			break;
-skip:
-		++idx;
-	}
-	spin_unlock_irqrestore(&rocker->fdb_tbl_lock, lock_flags);
-	return idx;
-}
-
 static int rocker_port_get_phys_port_name(struct net_device *dev,
 					  char *buf, size_t len)
 {
@@ -4277,12 +4175,12 @@ static const struct net_device_ops rocker_port_netdev_ops = {
 	.ndo_set_mac_address		= rocker_port_set_mac_address,
 	.ndo_vlan_rx_add_vid		= rocker_port_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid		= rocker_port_vlan_rx_kill_vid,
-	.ndo_fdb_add			= rocker_port_fdb_add,
-	.ndo_fdb_del			= rocker_port_fdb_del,
-	.ndo_fdb_dump			= rocker_port_fdb_dump,
 	.ndo_bridge_getlink		= switchdev_port_bridge_getlink,
 	.ndo_bridge_setlink		= switchdev_port_bridge_setlink,
 	.ndo_bridge_dellink		= switchdev_port_bridge_dellink,
+	.ndo_fdb_add			= switchdev_port_fdb_add,
+	.ndo_fdb_del			= switchdev_port_fdb_del,
+	.ndo_fdb_dump			= switchdev_port_fdb_dump,
 	.ndo_get_phys_port_name		= rocker_port_get_phys_port_name,
 };
 
@@ -4405,6 +4303,18 @@ static int rocker_port_vlans_add(struct rocker_port *rocker_port,
 	return 0;
 }
 
+static int rocker_port_fdb_add(struct rocker_port *rocker_port,
+			       struct switchdev_obj_fdb *fdb)
+{
+	__be16 vlan_id = rocker_port_vid_to_vlan(rocker_port, fdb->vid, NULL);
+	int flags = 0;
+
+	if (!rocker_port_is_bridged(rocker_port))
+		return -EINVAL;
+
+	return rocker_port_fdb(rocker_port, fdb->addr, vlan_id, flags);
+}
+
 static int rocker_port_obj_add(struct net_device *dev,
 			       struct switchdev_obj *obj)
 {
@@ -4435,6 +4345,9 @@ static int rocker_port_obj_add(struct net_device *dev,
 					   fib4->dst_len, fib4->fi,
 					   fib4->tb_id, 0);
 		break;
+	case SWITCHDEV_OBJ_PORT_FDB:
+		err = rocker_port_fdb_add(rocker_port, &obj->fdb);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -4473,6 +4386,18 @@ static int rocker_port_vlans_del(struct rocker_port *rocker_port,
 	return 0;
 }
 
+static int rocker_port_fdb_del(struct rocker_port *rocker_port,
+			       struct switchdev_obj_fdb *fdb)
+{
+	__be16 vlan_id = rocker_port_vid_to_vlan(rocker_port, fdb->vid, NULL);
+	int flags = ROCKER_OP_FLAG_REMOVE;
+
+	if (!rocker_port_is_bridged(rocker_port))
+		return -EINVAL;
+
+	return rocker_port_fdb(rocker_port, fdb->addr, vlan_id, flags);
+}
+
 static int rocker_port_obj_del(struct net_device *dev,
 			       struct switchdev_obj *obj)
 {
@@ -4491,6 +4416,53 @@ static int rocker_port_obj_del(struct net_device *dev,
 					   fib4->tb_id,
 					   ROCKER_OP_FLAG_REMOVE);
 		break;
+	case SWITCHDEV_OBJ_PORT_FDB:
+		err = rocker_port_fdb_del(rocker_port, &obj->fdb);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int rocker_port_fdb_dump(struct rocker_port *rocker_port,
+				struct switchdev_obj *obj)
+{
+	struct rocker *rocker = rocker_port->rocker;
+	struct rocker_fdb_tbl_entry *found;
+	struct hlist_node *tmp;
+	unsigned long lock_flags;
+	int bkt;
+	int err = 0;
+
+	spin_lock_irqsave(&rocker->fdb_tbl_lock, lock_flags);
+	hash_for_each_safe(rocker->fdb_tbl, bkt, tmp, found, entry) {
+		if (found->key.pport != rocker_port->pport)
+			continue;
+		obj->fdb.addr = found->key.addr;
+		obj->fdb.vid = rocker_port_vlan_to_vid(rocker_port,
+						       found->key.vlan_id);
+		err = obj->cb(rocker_port->dev, obj);
+		if (err)
+			break;
+	}
+	spin_unlock_irqrestore(&rocker->fdb_tbl_lock, lock_flags);
+
+	return err;
+}
+
+static int rocker_port_obj_dump(struct net_device *dev,
+				struct switchdev_obj *obj)
+{
+	struct rocker_port *rocker_port = netdev_priv(dev);
+	int err = 0;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_PORT_FDB:
+		err = rocker_port_fdb_dump(rocker_port, obj);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -4504,6 +4476,7 @@ static const struct switchdev_ops rocker_port_switchdev_ops = {
 	.switchdev_port_attr_set	= rocker_port_attr_set,
 	.switchdev_port_obj_add		= rocker_port_obj_add,
 	.switchdev_port_obj_del		= rocker_port_obj_del,
+	.switchdev_port_obj_dump	= rocker_port_obj_dump,
 };
 
 /********************
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 1ec035a..daa054b 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1980,6 +1980,9 @@ static const struct net_device_ops team_netdev_ops = {
 	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
 	.ndo_bridge_getlink	= switchdev_port_bridge_getlink,
 	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
+	.ndo_fdb_add		= switchdev_port_fdb_add,
+	.ndo_fdb_del		= switchdev_port_fdb_del,
+	.ndo_fdb_dump		= switchdev_port_fdb_dump,
 	.ndo_features_check	= passthru_features_check,
 };
 
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 3b217b4..0dba9a7 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -47,11 +47,13 @@ enum switchdev_obj_id {
 	SWITCHDEV_OBJ_UNDEFINED,
 	SWITCHDEV_OBJ_PORT_VLAN,
 	SWITCHDEV_OBJ_IPV4_FIB,
+	SWITCHDEV_OBJ_PORT_FDB,
 };
 
 struct switchdev_obj {
 	enum switchdev_obj_id id;
 	enum switchdev_trans trans;
+	int (*cb)(struct net_device *dev, struct switchdev_obj *obj);
 	union {
 		struct switchdev_obj_vlan {			/* PORT_VLAN */
 			u16 flags;
@@ -67,6 +69,10 @@ struct switchdev_obj {
 			u32 nlflags;
 			u32 tb_id;
 		} ipv4_fib;
+		struct switchdev_obj_fdb {		/* PORT_FDB */
+			const unsigned char *addr;
+			u16 vid;
+		} fdb;
 	};
 };
 
@@ -80,6 +86,8 @@ struct switchdev_obj {
  * @switchdev_port_obj_add: Add an object to port (see switchdev_obj).
  *
  * @switchdev_port_obj_del: Delete an object from port (see switchdev_obj).
+ *
+ * @switchdev_port_obj_dump: Dump port objects (see switchdev_obj).
  */
 struct switchdev_ops {
 	int	(*switchdev_port_attr_get)(struct net_device *dev,
@@ -90,6 +98,8 @@ struct switchdev_ops {
 					  struct switchdev_obj *obj);
 	int	(*switchdev_port_obj_del)(struct net_device *dev,
 					  struct switchdev_obj *obj);
+	int	(*switchdev_port_obj_dump)(struct net_device *dev,
+					  struct switchdev_obj *obj);
 };
 
 enum switchdev_notifier_type {
@@ -121,6 +131,7 @@ int switchdev_port_attr_set(struct net_device *dev,
 			    struct switchdev_attr *attr);
 int switchdev_port_obj_add(struct net_device *dev, struct switchdev_obj *obj);
 int switchdev_port_obj_del(struct net_device *dev, struct switchdev_obj *obj);
+int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj);
 int register_switchdev_notifier(struct notifier_block *nb);
 int unregister_switchdev_notifier(struct notifier_block *nb);
 int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
@@ -137,6 +148,15 @@ int switchdev_fib_ipv4_add(u32 dst, int dst_len, struct fib_info *fi,
 int switchdev_fib_ipv4_del(u32 dst, int dst_len, struct fib_info *fi,
 			   u8 tos, u8 type, u32 tb_id);
 void switchdev_fib_ipv4_abort(struct fib_info *fi);
+int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
+			   struct net_device *dev, const unsigned char *addr,
+			   u16 vid, u16 nlm_flags);
+int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
+			   struct net_device *dev, const unsigned char *addr,
+			   u16 vid);
+int switchdev_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
+			    struct net_device *dev,
+			    struct net_device *filter_dev, int idx);
 
 #else
 
@@ -164,6 +184,12 @@ static inline int switchdev_port_obj_del(struct net_device *dev,
 	return -EOPNOTSUPP;
 }
 
+static inline int switchdev_port_obj_dump(struct net_device *dev,
+					  struct switchdev_obj *obj)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int register_switchdev_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -221,6 +247,30 @@ static inline void switchdev_fib_ipv4_abort(struct fib_info *fi)
 {
 }
 
+static inline int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
+					 struct net_device *dev,
+					 const unsigned char *addr,
+					 u16 vid, u16 nlm_flags)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
+					 struct net_device *dev,
+					 const unsigned char *addr, u16 vid)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int switchdev_port_fdb_dump(struct sk_buff *skb,
+					  struct netlink_callback *cb,
+					  struct net_device *dev,
+					  struct net_device *filter_dev,
+					  int idx)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif
 
 #endif /* _LINUX_SWITCHDEV_H_ */
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 65d49d4..3f92a5e 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -295,6 +295,36 @@ int switchdev_port_obj_del(struct net_device *dev, struct switchdev_obj *obj)
 }
 EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
 
+/**
+ *	switchdev_port_obj_dump - Dump port objects
+ *
+ *	@dev: port device
+ *	@obj: object to dump
+ */
+int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj)
+{
+	const struct switchdev_ops *ops = dev->switchdev_ops;
+	struct net_device *lower_dev;
+	struct list_head *iter;
+	int err = -EOPNOTSUPP;
+
+	if (ops && ops->switchdev_port_obj_dump)
+		return ops->switchdev_port_obj_dump(dev, obj);
+
+	/* Switch device port(s) may be stacked under
+	 * bond/team/vlan dev, so recurse down to dump objects on
+	 * first port at bottom of stack.
+	 */
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter) {
+		err = switchdev_port_obj_dump(lower_dev, obj);
+		break;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(switchdev_port_obj_dump);
+
 static DEFINE_MUTEX(switchdev_mutex);
 static RAW_NOTIFIER_HEAD(switchdev_notif_chain);
 
@@ -564,6 +594,151 @@ int switchdev_port_bridge_dellink(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(switchdev_port_bridge_dellink);
 
+/**
+ *	switchdev_port_fdb_add - Add FDB (MAC/VLAN) entry to port
+ *
+ *	@ndmsg: netlink hdr
+ *	@nlattr: netlink attributes
+ *	@dev: port device
+ *	@addr: MAC address to add
+ *	@vid: VLAN to add
+ *
+ *	Add FDB entry to switch device.
+ */
+int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
+			   struct net_device *dev, const unsigned char *addr,
+			   u16 vid, u16 nlm_flags)
+{
+	struct switchdev_obj obj = {
+		.id = SWITCHDEV_OBJ_PORT_FDB,
+		.fdb = {
+			.addr = addr,
+			.vid = vid,
+		},
+	};
+
+	return switchdev_port_obj_add(dev, &obj);
+}
+EXPORT_SYMBOL_GPL(switchdev_port_fdb_add);
+
+/**
+ *	switchdev_port_fdb_del - Delete FDB (MAC/VLAN) entry from port
+ *
+ *	@ndmsg: netlink hdr
+ *	@nlattr: netlink attributes
+ *	@dev: port device
+ *	@addr: MAC address to delete
+ *	@vid: VLAN to delete
+ *
+ *	Delete FDB entry from switch device.
+ */
+int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
+			   struct net_device *dev, const unsigned char *addr,
+			   u16 vid)
+{
+	struct switchdev_obj obj = {
+		.id = SWITCHDEV_OBJ_PORT_FDB,
+		.fdb = {
+			.addr = addr,
+			.vid = vid,
+		},
+	};
+
+	return switchdev_port_obj_del(dev, &obj);
+}
+EXPORT_SYMBOL_GPL(switchdev_port_fdb_del);
+
+struct switchdev_fdb_dump {
+	struct switchdev_obj obj;
+	struct sk_buff *skb;
+	struct netlink_callback *cb;
+	struct net_device *filter_dev;
+	int idx;
+};
+
+static int switchdev_port_fdb_dump_cb(struct net_device *dev,
+				      struct switchdev_obj *obj)
+{
+	struct switchdev_fdb_dump *dump =
+		container_of(obj, struct switchdev_fdb_dump, obj);
+	u32 portid = NETLINK_CB(dump->cb->skb).portid;
+	u32 seq = dump->cb->nlh->nlmsg_seq;
+	struct nlmsghdr *nlh;
+	struct ndmsg *ndm;
+	struct net_device *master = netdev_master_upper_dev_get(dev);
+
+	if (dump->idx < dump->cb->args[0])
+		goto skip;
+
+	if (master && dump->filter_dev != master)
+		goto skip;
+
+	nlh = nlmsg_put(dump->skb, portid, seq, RTM_NEWNEIGH,
+			sizeof(*ndm), NLM_F_MULTI);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	ndm = nlmsg_data(nlh);
+	ndm->ndm_family  = AF_BRIDGE;
+	ndm->ndm_pad1    = 0;
+	ndm->ndm_pad2    = 0;
+	ndm->ndm_flags   = NTF_SELF;
+	ndm->ndm_type    = 0;
+	ndm->ndm_ifindex = dev->ifindex;
+	ndm->ndm_state   = NUD_REACHABLE;
+
+	if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, obj->fdb.addr))
+		goto nla_put_failure;
+
+	if (obj->fdb.vid && nla_put_u16(dump->skb, NDA_VLAN, obj->fdb.vid))
+		goto nla_put_failure;
+
+	nlmsg_end(dump->skb, nlh);
+
+skip:
+	dump->idx++;
+	return 0;
+
+nla_put_failure:
+	nlmsg_cancel(dump->skb, nlh);
+	return -EMSGSIZE;
+}
+
+/**
+ *	switchdev_port_fdb_dump - Dump port FDB (MAC/VLAN) entries
+ *
+ *	@skb: netlink skb
+ *	@cb: netlink callback
+ *	@dev: port device
+ *	@filter_dev: filter device
+ *	@idx:
+ *
+ *	Delete FDB entry from switch device.
+ */
+int switchdev_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
+			    struct net_device *dev,
+			    struct net_device *filter_dev, int idx)
+{
+	struct switchdev_fdb_dump dump = {
+		.obj = {
+			.id = SWITCHDEV_OBJ_PORT_FDB,
+			.cb = switchdev_port_fdb_dump_cb,
+		},
+		.skb = skb,
+		.cb = cb,
+		.filter_dev = filter_dev,
+		.idx = idx,
+	};
+	int err;
+
+	err = switchdev_port_obj_dump(dev, &dump.obj);
+	if (err)
+		return err;
+
+	return dump.idx;
+}
+EXPORT_SYMBOL_GPL(switchdev_port_fdb_dump);
+
 static struct net_device *switchdev_get_lowest_dev(struct net_device *dev)
 {
 	const struct switchdev_ops *ops = dev->switchdev_ops;
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-06 21:54 [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops Sridhar Samudrala
@ 2015-05-06 23:35 ` Jamal Hadi Salim
  2015-05-07  4:42   ` Samudrala, Sridhar
  2015-05-12 22:20 ` David Miller
  1 sibling, 1 reply; 10+ messages in thread
From: Jamal Hadi Salim @ 2015-05-06 23:35 UTC (permalink / raw)
  To: Sridhar Samudrala, sfeldma, netdev

On 05/06/15 17:54, Sridhar Samudrala wrote:
> - introduce port fdb obj and generic switchdev_port_fdb_add/del/dump()
> - use switchdev_port_fdb_add/del/dump in rocker/team/bonding ndo ops.
> - add support for fdb obj in switchdev_port_obj_add/del/dump()
> - switch rocker to implement fdb ops via switchdev_ops
>
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> ---



So i raised this earlier. DaveM also chimed in - but it seems still
in there.
i havent been following the discussion and i may have missed
the agreement to keep the new IDs. Could we not just have used netlink
  IDs (as opposed to a new SWITCHDEV_OBJ_PORT_FDB id)?

> +int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
> +			   struct net_device *dev, const unsigned char *addr,
> +			   u16 vid, u16 nlm_flags)
> +{
> +	struct switchdev_obj obj = {
> +		.id = SWITCHDEV_OBJ_PORT_FDB,
> +		.fdb = {
> +			.addr = addr,
> +			.vid = vid,
> +		},
> +	};
> +
> +	return switchdev_port_obj_add(dev, &obj);
> +}

cheers,
jamal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-06 23:35 ` Jamal Hadi Salim
@ 2015-05-07  4:42   ` Samudrala, Sridhar
  2015-05-07 12:51     ` roopa
  2015-05-08 12:05     ` Jamal Hadi Salim
  0 siblings, 2 replies; 10+ messages in thread
From: Samudrala, Sridhar @ 2015-05-07  4:42 UTC (permalink / raw)
  To: Jamal Hadi Salim, sfeldma, netdev



On 5/6/2015 4:35 PM, Jamal Hadi Salim wrote:
> On 05/06/15 17:54, Sridhar Samudrala wrote:
>> - introduce port fdb obj and generic switchdev_port_fdb_add/del/dump()
>> - use switchdev_port_fdb_add/del/dump in rocker/team/bonding ndo ops.
>> - add support for fdb obj in switchdev_port_obj_add/del/dump()
>> - switch rocker to implement fdb ops via switchdev_ops
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> ---
>
> So i raised this earlier. DaveM also chimed in - but it seems still
> in there.
> i havent been following the discussion and i may have missed
> the agreement to keep the new IDs. Could we not just have used netlink
>  IDs (as opposed to a new SWITCHDEV_OBJ_PORT_FDB id)?

I think you are referring to switch port attributes.  See Scott's 
response here on
using netlink IDs for attributes.
http://thread.gmane.org/gmane.linux.network/357694/focus=357921

This patch is adding 'fdb' as new switch port object.  It is similar to 
other
objects like 'VLAN' and 'FIB' that are added by Scott's patches.

>
>> +int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>> +               struct net_device *dev, const unsigned char *addr,
>> +               u16 vid, u16 nlm_flags)
>> +{
>> +    struct switchdev_obj obj = {
>> +        .id = SWITCHDEV_OBJ_PORT_FDB,
>> +        .fdb = {
>> +            .addr = addr,
>> +            .vid = vid,
>> +        },
>> +    };
>> +
>> +    return switchdev_port_obj_add(dev, &obj);
>> +}
>
> cheers,
> jamal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-07  4:42   ` Samudrala, Sridhar
@ 2015-05-07 12:51     ` roopa
  2015-05-08 12:05     ` Jamal Hadi Salim
  1 sibling, 0 replies; 10+ messages in thread
From: roopa @ 2015-05-07 12:51 UTC (permalink / raw)
  To: Samudrala, Sridhar; +Cc: Jamal Hadi Salim, sfeldma, netdev

On 5/6/15, 9:42 PM, Samudrala, Sridhar wrote:
>
>
> On 5/6/2015 4:35 PM, Jamal Hadi Salim wrote:
>> On 05/06/15 17:54, Sridhar Samudrala wrote:
>>> - introduce port fdb obj and generic switchdev_port_fdb_add/del/dump()
>>> - use switchdev_port_fdb_add/del/dump in rocker/team/bonding ndo ops.
>>> - add support for fdb obj in switchdev_port_obj_add/del/dump()
>>> - switch rocker to implement fdb ops via switchdev_ops
>>>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>> ---
>>
>> So i raised this earlier. DaveM also chimed in - but it seems still
>> in there.
>> i havent been following the discussion and i may have missed
>> the agreement to keep the new IDs. Could we not just have used netlink
>>  IDs (as opposed to a new SWITCHDEV_OBJ_PORT_FDB id)?
>
> I think you are referring to switch port attributes.  See Scott's 
> response here on
> using netlink IDs for attributes.
> http://thread.gmane.org/gmane.linux.network/357694/focus=357921
>
If you are using a global namespace for all switchdev objects/apis (as 
with scotts new patches), yes, using existing netlink id's and 
structures will not work as scott mentions in the link you point to 
above. But, if you were designing the api's for specific networking 
subsystems like it was initially, then netlink id's or existing 
structures will work.

And i am guessing Jamal is referring to the latter.

I did raise similar concerns earlier. Scotts patches do make the code 
way cleaner and less confusing than it was before (Thanks scott!). But, 
It does add this additional layer of mapping between kernel 
structures/objects and newly introduced switchdev structures/objects. 
The advantage of being in the kernel is access to existing kernel 
structures/objects. We lose that benefit.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-07  4:42   ` Samudrala, Sridhar
  2015-05-07 12:51     ` roopa
@ 2015-05-08 12:05     ` Jamal Hadi Salim
  2015-05-08 20:44       ` Samudrala, Sridhar
  2015-05-08 20:58       ` Scott Feldman
  1 sibling, 2 replies; 10+ messages in thread
From: Jamal Hadi Salim @ 2015-05-08 12:05 UTC (permalink / raw)
  To: Samudrala, Sridhar, sfeldma, netdev

On 05/07/15 00:42, Samudrala, Sridhar wrote:
>
>
> On 5/6/2015 4:35 PM, Jamal Hadi Salim wrote:
>> On 05/06/15 17:54, Sridhar Samudrala wrote:

>> So i raised this earlier. DaveM also chimed in - but it seems still
>> in there.
>> i havent been following the discussion and i may have missed
>> the agreement to keep the new IDs. Could we not just have used netlink
>>  IDs (as opposed to a new SWITCHDEV_OBJ_PORT_FDB id)?
>
> I think you are referring to switch port attributes.  See Scott's
> response here on
> using netlink IDs for attributes.
> http://thread.gmane.org/gmane.linux.network/357694/focus=357921
>
> This patch is adding 'fdb' as new switch port object.  It is similar to
> other
> objects like 'VLAN' and 'FIB' that are added by Scott's patches.

Sorry I missed it - but I am not making sense of Scott's answer.
The danger of adding these visible APIs is it would be too late
to take them out once they hit the wild (Dave's famous "the horse
has left the barn" or look at Jiri's netconf presentation). I hate
to do this but taking longer to discuss these issues is important.

I will agree that we need some common id to represent the grouping
of the fdb entries - i will contend using netlink verb-nouns is
sufficient. Example: RTM_NEWNEIGH is clearly refering to a
modify or create on an fdb.
The other issue is how to define optionality.
So lets start with the fdb object:
While i would agree they are common, Vlans are optional in an fdb
entry. The only two items that must be present for an fdb entry are a
dstmac address and an egress port.
some low end switches dont do vlans. Therefore there are two IDs
that must always be present:

The object id for a port is the ifindex.
The object id for a mac address is NDA_LLADDR

The object id for a vlan is NDA_VLAN
Then there are of course a gazillion other features which may
be one-of such as the fdb partition id in the netgear switch Ben was
playing.

I understand the earlier arguement of not needing to have
multiple parsings of the same thing. But providing parseable
options means never having to change what the object struct
looks like. Just provide a scatter list of pointers to the
different netlink attr-val pairs so it doesnt need to be
re-parsed.

Otherwise we are re-inventing things and start to look like
vendor SDKs.

cheers,
jamal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-08 12:05     ` Jamal Hadi Salim
@ 2015-05-08 20:44       ` Samudrala, Sridhar
  2015-05-08 20:58       ` Scott Feldman
  1 sibling, 0 replies; 10+ messages in thread
From: Samudrala, Sridhar @ 2015-05-08 20:44 UTC (permalink / raw)
  To: Jamal Hadi Salim, sfeldma, netdev

On 5/8/2015 5:05 AM, Jamal Hadi Salim wrote:
> On 05/07/15 00:42, Samudrala, Sridhar wrote:
>>
>>
>> On 5/6/2015 4:35 PM, Jamal Hadi Salim wrote:
>>> On 05/06/15 17:54, Sridhar Samudrala wrote:
>
>>> So i raised this earlier. DaveM also chimed in - but it seems still
>>> in there.
>>> i havent been following the discussion and i may have missed
>>> the agreement to keep the new IDs. Could we not just have used netlink
>>>  IDs (as opposed to a new SWITCHDEV_OBJ_PORT_FDB id)?
>>
>> I think you are referring to switch port attributes.  See Scott's
>> response here on
>> using netlink IDs for attributes.
>> http://thread.gmane.org/gmane.linux.network/357694/focus=357921
>>
>> This patch is adding 'fdb' as new switch port object.  It is similar to
>> other
>> objects like 'VLAN' and 'FIB' that are added by Scott's patches.
>
> Sorry I missed it - but I am not making sense of Scott's answer.
> The danger of adding these visible APIs is it would be too late
> to take them out once they hit the wild (Dave's famous "the horse
> has left the barn" or look at Jiri's netconf presentation). I hate
> to do this but taking longer to discuss these issues is important.
>
> I will agree that we need some common id to represent the grouping
> of the fdb entries - i will contend using netlink verb-nouns is
> sufficient. Example: RTM_NEWNEIGH is clearly refering to a
> modify or create on an fdb.
> The other issue is how to define optionality.
> So lets start with the fdb object:
> While i would agree they are common, Vlans are optional in an fdb
> entry. The only two items that must be present for an fdb entry are a
> dstmac address and an egress port.
> some low end switches dont do vlans. Therefore there are two IDs
> that must always be present:
>
> The object id for a port is the ifindex.
> The object id for a mac address is NDA_LLADDR
An fdb object is tied to a particular switch port and is associated with a
netdev. So we don't need a port in the fdb object.

>
> The object id for a vlan is NDA_VLAN
> Then there are of course a gazillion other features which may
> be one-of such as the fdb partition id in the netgear switch Ben was
> playing.
>
> I understand the earlier arguement of not needing to have
> multiple parsings of the same thing. But providing parseable
> options means never having to change what the object struct
> looks like. Just provide a scatter list of pointers to the
> different netlink attr-val pairs so it doesnt need to be
> re-parsed.

I think Scott introduced switchdev object abstraction to make the 
switchdev code simpler
and also to enable direct calls to add/delete objects without having to 
create dummy netlink
messages with attributes.
Scott should be able to provide better reasoning behind this abstraction.

>
> Otherwise we are re-inventing things and start to look like
> vendor SDKs.
>
> cheers,
> jamal
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-08 12:05     ` Jamal Hadi Salim
  2015-05-08 20:44       ` Samudrala, Sridhar
@ 2015-05-08 20:58       ` Scott Feldman
  2015-05-11 13:15         ` Jamal Hadi Salim
  1 sibling, 1 reply; 10+ messages in thread
From: Scott Feldman @ 2015-05-08 20:58 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: Samudrala, Sridhar, Netdev

On Fri, May 8, 2015 at 5:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 05/07/15 00:42, Samudrala, Sridhar wrote:
>>
>>
>>
>> On 5/6/2015 4:35 PM, Jamal Hadi Salim wrote:
>>>
>>> On 05/06/15 17:54, Sridhar Samudrala wrote:
>
>
>>> So i raised this earlier. DaveM also chimed in - but it seems still
>>> in there.
>>> i havent been following the discussion and i may have missed
>>> the agreement to keep the new IDs. Could we not just have used netlink
>>>  IDs (as opposed to a new SWITCHDEV_OBJ_PORT_FDB id)?
>>
>>
>> I think you are referring to switch port attributes.  See Scott's
>> response here on
>> using netlink IDs for attributes.
>> http://thread.gmane.org/gmane.linux.network/357694/focus=357921
>>
>> This patch is adding 'fdb' as new switch port object.  It is similar to
>> other
>> objects like 'VLAN' and 'FIB' that are added by Scott's patches.
>
>
> Sorry I missed it - but I am not making sense of Scott's answer.
> The danger of adding these visible APIs is it would be too late
> to take them out once they hit the wild (Dave's famous "the horse
> has left the barn" or look at Jiri's netconf presentation). I hate
> to do this but taking longer to discuss these issues is important.
>
> I will agree that we need some common id to represent the grouping
> of the fdb entries - i will contend using netlink verb-nouns is
> sufficient. Example: RTM_NEWNEIGH is clearly refering to a
> modify or create on an fdb.
> The other issue is how to define optionality.
> So lets start with the fdb object:
> While i would agree they are common, Vlans are optional in an fdb
> entry. The only two items that must be present for an fdb entry are a
> dstmac address and an egress port.
> some low end switches dont do vlans. Therefore there are two IDs
> that must always be present:
>
> The object id for a port is the ifindex.
> The object id for a mac address is NDA_LLADDR
>
> The object id for a vlan is NDA_VLAN
> Then there are of course a gazillion other features which may
> be one-of such as the fdb partition id in the netgear switch Ben was
> playing.
>
> I understand the earlier arguement of not needing to have
> multiple parsings of the same thing. But providing parseable
> options means never having to change what the object struct
> looks like. Just provide a scatter list of pointers to the
> different netlink attr-val pairs so it doesnt need to be
> re-parsed.
>
> Otherwise we are re-inventing things and start to look like
> vendor SDKs.

Bottom line is netlink is not great for in-kernel driver APIs.  Look
at the fdb and bridge_set/get/dellink ndo ops in netdevice.h to see
what kind of horse already left the barn.  It's a swayback nag blind
in one eye and deaf in the other eye.  The problems are:

1) Inconsistent call arguments, some strange mix of parsed and raw
netlink attrs.  This make it impossible to provide generic wrappers to
handle higher-level operations such as the prepare-commit transaction
model.

2) Netlink attr parsing/policy checks are now split/duplicated between
core code and driver code.  Parsing/policy checks should be done in
one place: the core code.

3) Driver dumps ops duplicate the netlink skb building (see recent
patch to fix MULTI flag usage and how it touched every driver).  This
skb building should be in core code, not in each driver.

4) netlink-based APIs are hard to call from kernel context where no
netlink msgs exists.  For example, consider the task of flushing FDBs
from the device.  Today, we'd need to synthesis netlink message to
call into ndo_fdb_del to delete the FDB entry.

5) In some cases, there is a lot of processing done on the netlink msg
before the call into the driver, and we don't want to duplicate all of
that processing again in each driver.  For example, consider the IPv4
fib API.  A netlink msg originates the call into the driver, but the
actual arguments of the call into the driver are packaged quite
differently than the originating netlink msg.   This call to the
driver is made inline deep down in the logic of fib processing.

6) There are some switchdev attrs that don't originate as netlink
msgs.  Example is STP state.  That's an internal call from the bridge
driver.

Don't get me wrong: I love netlink for user-space/kernel APIs.  But
for in-kernel APIs it's awkward and we want a clean API for switchdev.

-scott

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-08 20:58       ` Scott Feldman
@ 2015-05-11 13:15         ` Jamal Hadi Salim
  0 siblings, 0 replies; 10+ messages in thread
From: Jamal Hadi Salim @ 2015-05-11 13:15 UTC (permalink / raw)
  To: Scott Feldman; +Cc: Samudrala, Sridhar, Netdev

On 05/08/15 16:58, Scott Feldman wrote:
> On Fri, May 8, 2015 at 5:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 05/07/15 00:42, Samudrala, Sridhar wrote:
>>>

> Bottom line is netlink is not great for in-kernel driver APIs.  Look
> at the fdb and bridge_set/get/dellink ndo ops in netdevice.h to see
> what kind of horse already left the barn.  It's a swayback nag blind
> in one eye and deaf in the other eye.  The problems are:
>

Scott, I dont disagree with you in regarding to the fdb/brport.
The barn has left the deaf-in-the-other-eye horse on that one (yes,
I said that on purpose;->) and it may even make sense to write a
different interface from user  space. I could add more to your list
(why are 8 bits being used to represent a single bit flag for example,
etc).
But what i am worried about is you are setting a trend. I would hate
for this to start looking like SAI when we've had shit working really
well.
You'll find the L3 stuff is more solid in terms of its netlink usage.
But even for the bridge stuff, describing an object as, hiearachy of

An "add" looking like:
   RTM_NEWNEIGH,
      {pointer to attributeid, pointer to attribute value}+

whereas the pointers are to the original TLV (which is still sitting
somewhere in memory) sounds useful.

I dont want to slow you down and i dont have code to demonstrate it -
but hoping I dont need to.

cheers,
jamal

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-06 21:54 [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops Sridhar Samudrala
  2015-05-06 23:35 ` Jamal Hadi Salim
@ 2015-05-12 22:20 ` David Miller
  2015-05-13  0:07   ` Samudrala, Sridhar
  1 sibling, 1 reply; 10+ messages in thread
From: David Miller @ 2015-05-12 22:20 UTC (permalink / raw)
  To: sridhar.samudrala; +Cc: sfeldma, netdev

From: Sridhar Samudrala <sridhar.samudrala@intel.com>
Date: Wed,  6 May 2015 14:54:13 -0700

> - introduce port fdb obj and generic switchdev_port_fdb_add/del/dump()
> - use switchdev_port_fdb_add/del/dump in rocker/team/bonding ndo ops.
> - add support for fdb obj in switchdev_port_obj_add/del/dump()
> - switch rocker to implement fdb ops via switchdev_ops
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>

What is this patch even against?  I don't see switchdev_port_obj_add()
in any tree I maintain, yet this patch's context shows references to
it which should exist already.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops.
  2015-05-12 22:20 ` David Miller
@ 2015-05-13  0:07   ` Samudrala, Sridhar
  0 siblings, 0 replies; 10+ messages in thread
From: Samudrala, Sridhar @ 2015-05-13  0:07 UTC (permalink / raw)
  To: David Miller; +Cc: sfeldma, netdev



On 5/12/2015 3:20 PM, David Miller wrote:
> From: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Date: Wed,  6 May 2015 14:54:13 -0700
>
>> - introduce port fdb obj and generic switchdev_port_fdb_add/del/dump()
>> - use switchdev_port_fdb_add/del/dump in rocker/team/bonding ndo ops.
>> - add support for fdb obj in switchdev_port_obj_add/del/dump()
>> - switch rocker to implement fdb ops via switchdev_ops
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> What is this patch even against?  I don't see switchdev_port_obj_add()
> in any tree I maintain, yet this patch's context shows references to
> it which should exist already.

Sorry, I should have mentioned that this patch is on top of v5 version 
of Scott's switchdev spring cleanup patches.
Now that v7 version is merged, i will resubmit on top of net-next.

Thanks
Sridhar

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-05-13  0:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-06 21:54 [PATCH net-next] switchdev: add support for fdb add/del/dump via switchdev_port_obj ops Sridhar Samudrala
2015-05-06 23:35 ` Jamal Hadi Salim
2015-05-07  4:42   ` Samudrala, Sridhar
2015-05-07 12:51     ` roopa
2015-05-08 12:05     ` Jamal Hadi Salim
2015-05-08 20:44       ` Samudrala, Sridhar
2015-05-08 20:58       ` Scott Feldman
2015-05-11 13:15         ` Jamal Hadi Salim
2015-05-12 22:20 ` David Miller
2015-05-13  0:07   ` Samudrala, Sridhar

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).