Netdev List
 help / color / mirror / Atom feed
* [net-next 11/18] tipc: introduce group unicast messaging
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

We now make it possible to send connectionless unicast messages
within a communication group. To send a message, the sender can use
either a direct port address, aka port identity, or an indirect port
name to be looked up.

This type of messages are subject to the same start synchronization
and flow control mechanism as group broadcast messages.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c  |  48 ++++++++++++++++++++++-
 net/tipc/group.h  |   3 ++
 net/tipc/msg.h    |   3 +-
 net/tipc/socket.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 153 insertions(+), 13 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 50dd7d1..2d624c0 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -186,6 +186,17 @@ struct tipc_member *tipc_group_find_member(struct tipc_group *grp,
 	return NULL;
 }
 
+static struct tipc_member *tipc_group_find_dest(struct tipc_group *grp,
+						u32 node, u32 port)
+{
+	struct tipc_member *m;
+
+	m = tipc_group_find_member(grp, node, port);
+	if (m && tipc_group_is_enabled(m))
+		return m;
+	return NULL;
+}
+
 static struct tipc_member *tipc_group_find_node(struct tipc_group *grp,
 						u32 node)
 {
@@ -318,9 +329,42 @@ void tipc_group_update_bc_members(struct tipc_group *grp, int len)
 	grp->bc_snd_nxt++;
 }
 
-bool tipc_group_bc_cong(struct tipc_group *grp, int len)
+bool tipc_group_cong(struct tipc_group *grp, u32 dnode, u32 dport,
+		     int len, struct tipc_member **mbr)
 {
+	struct sk_buff_head xmitq;
 	struct tipc_member *m;
+	int adv, state;
+
+	if (list_empty(&grp->congested))
+		return false;
+
+	m = tipc_group_find_dest(grp, dnode, dport);
+	*mbr = m;
+	if (!m)
+		return false;
+	if (m->usr_pending)
+		return true;
+	if (m->window >= len)
+		return false;
+	m->usr_pending = true;
+
+	/* If not fully advertised, do it now to prevent mutual blocking */
+	adv = m->advertised;
+	state = m->state;
+	if (state < MBR_JOINED)
+		return true;
+	if (state == MBR_JOINED && adv == ADV_IDLE)
+		return true;
+	skb_queue_head_init(&xmitq);
+	tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, &xmitq);
+	tipc_node_distr_xmit(grp->net, &xmitq);
+	return true;
+}
+
+bool tipc_group_bc_cong(struct tipc_group *grp, int len)
+{
+	struct tipc_member *m = NULL;
 
 	if (list_empty(&grp->congested))
 		return false;
@@ -329,7 +373,7 @@ bool tipc_group_bc_cong(struct tipc_group *grp, int len)
 	if (m->window >= len)
 		return false;
 
-	return true;
+	return tipc_group_cong(grp, m->node, m->port, len, &m);
 }
 
 /* tipc_group_filter_msg() - determine if we should accept arriving message
diff --git a/net/tipc/group.h b/net/tipc/group.h
index 0e2740e..8f77290 100644
--- a/net/tipc/group.h
+++ b/net/tipc/group.h
@@ -61,9 +61,12 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *wakeup,
 			  struct sk_buff_head *inputq,
 			  struct sk_buff_head *xmitq);
 void tipc_group_update_bc_members(struct tipc_group *grp, int len);
+bool tipc_group_cong(struct tipc_group *grp, u32 dnode, u32 dport,
+		     int len, struct tipc_member **m);
 bool tipc_group_bc_cong(struct tipc_group *grp, int len);
 void tipc_group_update_rcv_win(struct tipc_group *grp, int blks, u32 node,
 			       u32 port, struct sk_buff_head *xmitq);
 u16 tipc_group_bc_snd_nxt(struct tipc_group *grp);
+void tipc_group_update_member(struct tipc_member *m, int len);
 int tipc_group_size(struct tipc_group *grp);
 #endif
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 820720c..7430fe7 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -67,6 +67,7 @@ struct plist;
 #define TIPC_DIRECT_MSG		3
 #define TIPC_GRP_MEMBER_EVT	4
 #define TIPC_GRP_BCAST_MSG	5
+#define TIPC_GRP_UCAST_MSG	6
 
 /*
  * Internal message users
@@ -261,7 +262,7 @@ static inline int msg_in_group(struct tipc_msg *m)
 {
 	int mtyp = msg_type(m);
 
-	return (mtyp == TIPC_GRP_BCAST_MSG) || (mtyp == TIPC_GRP_MEMBER_EVT);
+	return mtyp >= TIPC_GRP_MEMBER_EVT && mtyp <= TIPC_GRP_UCAST_MSG;
 }
 
 static inline bool msg_is_grp_evt(struct tipc_msg *m)
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index d2f83fc9..4f7fee2 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -813,6 +813,93 @@ static int tipc_sendmcast(struct  socket *sock, struct tipc_name_seq *seq,
 }
 
 /**
+ * tipc_send_group_msg - send a message to a member in the group
+ * @net: network namespace
+ * @m: message to send
+ * @mb: group member
+ * @dnode: destination node
+ * @dport: destination port
+ * @dlen: total length of message data
+ */
+static int tipc_send_group_msg(struct net *net, struct tipc_sock *tsk,
+			       struct msghdr *m, struct tipc_member *mb,
+			       u32 dnode, u32 dport, int dlen)
+{
+	struct tipc_msg *hdr = &tsk->phdr;
+	struct sk_buff_head pkts;
+	int blks = tsk_blocks(GROUP_H_SIZE + dlen);
+	int mtu, rc;
+
+	/* Complete message header */
+	msg_set_type(hdr, TIPC_GRP_UCAST_MSG);
+	msg_set_hdr_sz(hdr, GROUP_H_SIZE);
+	msg_set_destport(hdr, dport);
+	msg_set_destnode(hdr, dnode);
+
+	/* Build message as chain of buffers */
+	skb_queue_head_init(&pkts);
+	mtu = tipc_node_get_mtu(net, dnode, tsk->portid);
+	rc = tipc_msg_build(hdr, m, 0, dlen, mtu, &pkts);
+	if (unlikely(rc != dlen))
+		return rc;
+
+	/* Send message */
+	rc = tipc_node_xmit(net, &pkts, dnode, tsk->portid);
+	if (unlikely(rc == -ELINKCONG)) {
+		tipc_dest_push(&tsk->cong_links, dnode, 0);
+		tsk->cong_link_cnt++;
+	}
+
+	/* Update send window and sequence number */
+	tipc_group_update_member(mb, blks);
+
+	return dlen;
+}
+
+/**
+ * tipc_send_group_unicast - send message to a member in the group
+ * @sock: socket structure
+ * @m: message to send
+ * @dlen: total length of message data
+ * @timeout: timeout to wait for wakeup
+ *
+ * Called from function tipc_sendmsg(), which has done all sanity checks
+ * Returns the number of bytes sent on success, or errno
+ */
+static int tipc_send_group_unicast(struct socket *sock, struct msghdr *m,
+				   int dlen, long timeout)
+{
+	DECLARE_SOCKADDR(struct sockaddr_tipc *, dest, m->msg_name);
+	struct sock *sk = sock->sk;
+	struct net *net = sock_net(sk);
+	struct tipc_sock *tsk = tipc_sk(sk);
+	struct tipc_group *grp = tsk->group;
+	struct tipc_member *mb = NULL;
+	int blks = tsk_blocks(GROUP_H_SIZE + dlen);
+	u32 node, port;
+	int rc;
+
+	node = dest->addr.id.node;
+	port = dest->addr.id.ref;
+	if (!port && !node)
+		return -EHOSTUNREACH;
+
+	/* Block or return if destination link or member is congested */
+	rc = tipc_wait_for_cond(sock, &timeout,
+				!tipc_dest_find(&tsk->cong_links, node, 0) &&
+				!tipc_group_cong(grp, node, port, blks, &mb));
+	if (unlikely(rc))
+		return rc;
+
+	if (unlikely(!mb))
+		return -EHOSTUNREACH;
+
+	rc = tipc_send_group_msg(net, tsk, m, mb, node, port, dlen);
+
+	return rc ? rc : dlen;
+}
+
+/**
  * tipc_send_group_bcast - send message to all members in communication group
  * @sk: socket structure
  * @m: message to send
@@ -1025,8 +1112,20 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	if (unlikely(dlen > TIPC_MAX_USER_MSG_SIZE))
 		return -EMSGSIZE;
 
-	if (unlikely(grp && !dest))
-		return tipc_send_group_bcast(sock, m, dlen, timeout);
+	if (likely(dest)) {
+		if (unlikely(m->msg_namelen < sizeof(*dest)))
+			return -EINVAL;
+		if (unlikely(dest->family != AF_TIPC))
+			return -EINVAL;
+	}
+
+	if (grp) {
+		if (!dest)
+			return tipc_send_group_bcast(sock, m, dlen, timeout);
+		if (dest->addrtype == TIPC_ADDR_ID)
+			return tipc_send_group_unicast(sock, m, dlen, timeout);
+		return -EINVAL;
+	}
 
 	if (unlikely(!dest)) {
 		dest = &tsk->peer;
@@ -1034,12 +1133,6 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 			return -EDESTADDRREQ;
 	}
 
-	if (unlikely(m->msg_namelen < sizeof(*dest)))
-		return -EINVAL;
-
-	if (unlikely(dest->family != AF_TIPC))
-		return -EINVAL;
-
 	if (unlikely(syn)) {
 		if (sk->sk_state == TIPC_LISTEN)
 			return -EPIPE;
@@ -1072,7 +1165,6 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 		msg_set_destport(hdr, dport);
 		if (unlikely(!dport && !dnode))
 			return -EHOSTUNREACH;
-
 	} else if (dest->addrtype == TIPC_ADDR_ID) {
 		dnode = dest->addr.id.node;
 		msg_set_type(hdr, TIPC_DIRECT_MSG);
@@ -1845,7 +1937,7 @@ static void tipc_sk_filter_rcv(struct sock *sk, struct sk_buff *skb,
 
 	if (unlikely(!msg_isdata(hdr)))
 		tipc_sk_proto_rcv(sk, &inputq, xmitq);
-	else if (unlikely(msg_type(hdr) > TIPC_GRP_BCAST_MSG))
+	else if (unlikely(msg_type(hdr) > TIPC_GRP_UCAST_MSG))
 		return kfree_skb(skb);
 
 	if (unlikely(grp))
-- 
2.1.4

^ permalink raw reply related

* [net-next 12/18] tipc: introduce group anycast messaging
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

In this commit, we make it possible to send connectionless unicast
messages to any member corresponding to the given member identity,
when there is more than one such member. The sender must use a
TIPC_ADDR_NAME address to achieve this effect.

We also perform load balancing between the destinations, i.e., we
primarily select one which has advertised sufficient send window
to not cause a block/EAGAIN delay, if any. This mechanism is
overlayed on the always present round-robin selection.

Anycast messages are subject to the same start synchronization
and flow control mechanism as group broadcast messages.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c      |  7 +++++
 net/tipc/group.h      |  3 ++
 net/tipc/name_table.c | 41 +++++++++++++++++++++++++
 net/tipc/name_table.h |  3 ++
 net/tipc/socket.c     | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 138 insertions(+)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 2d624c0..d3b8aba 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -116,6 +116,13 @@ static bool tipc_group_is_receiver(struct tipc_member *m)
 	return m && m->state >= MBR_JOINED;
 }
 
+u32 tipc_group_exclude(struct tipc_group *grp)
+{
+	if (!grp->loopback)
+		return grp->portid;
+	return 0;
+}
+
 int tipc_group_size(struct tipc_group *grp)
 {
 	return grp->member_cnt;
diff --git a/net/tipc/group.h b/net/tipc/group.h
index 8f77290..e432066 100644
--- a/net/tipc/group.h
+++ b/net/tipc/group.h
@@ -49,6 +49,7 @@ void tipc_group_add_member(struct tipc_group *grp, u32 node, u32 port);
 struct tipc_nlist *tipc_group_dests(struct tipc_group *grp);
 void tipc_group_self(struct tipc_group *grp, struct tipc_name_seq *seq,
 		     int *scope);
+u32 tipc_group_exclude(struct tipc_group *grp);
 void tipc_group_filter_msg(struct tipc_group *grp,
 			   struct sk_buff_head *inputq,
 			   struct sk_buff_head *xmitq);
@@ -68,5 +69,7 @@ void tipc_group_update_rcv_win(struct tipc_group *grp, int blks, u32 node,
 			       u32 port, struct sk_buff_head *xmitq);
 u16 tipc_group_bc_snd_nxt(struct tipc_group *grp);
 void tipc_group_update_member(struct tipc_member *m, int len);
+struct tipc_member *tipc_group_find_sender(struct tipc_group *grp,
+					   u32 node, u32 port);
 int tipc_group_size(struct tipc_group *grp);
 #endif
diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 941aed3..a87aff0 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -597,6 +597,47 @@ u32 tipc_nametbl_translate(struct net *net, u32 type, u32 instance,
 	return ref;
 }
 
+bool tipc_nametbl_lookup(struct net *net, u32 type, u32 instance, u32 domain,
+			 struct list_head *dsts, int *dstcnt, u32 exclude,
+			 bool all)
+{
+	struct publication *publ;
+	struct name_info *info;
+	struct name_seq *seq;
+	struct sub_seq *sseq;
+	u32 self = tipc_own_addr(net);
+
+	if (!tipc_in_scope(domain, self))
+		return false;
+
+	*dstcnt = 0;
+	rcu_read_lock();
+	seq = nametbl_find_seq(net, type);
+	if (unlikely(!seq))
+		goto exit;
+	spin_lock_bh(&seq->lock);
+	sseq = nameseq_find_subseq(seq, instance);
+	if (likely(sseq)) {
+		info = sseq->info;
+		list_for_each_entry(publ, &info->zone_list, zone_list) {
+			if (!tipc_in_scope(domain, publ->node))
+				continue;
+			if (publ->ref == exclude && publ->node == self)
+				continue;
+			tipc_dest_push(dsts, publ->node, publ->ref);
+			(*dstcnt)++;
+			if (all)
+				continue;
+			list_move_tail(&publ->zone_list, &info->zone_list);
+			break;
+		}
+	}
+	spin_unlock_bh(&seq->lock);
+exit:
+	rcu_read_unlock();
+	return !list_empty(dsts);
+}
+
 int tipc_nametbl_mc_translate(struct net *net, u32 type, u32 lower, u32 upper,
 			      u32 limit, struct list_head *dports)
 {
diff --git a/net/tipc/name_table.h b/net/tipc/name_table.h
index 97646b1..71926e4 100644
--- a/net/tipc/name_table.h
+++ b/net/tipc/name_table.h
@@ -107,6 +107,9 @@ void tipc_nametbl_build_group(struct net *net, struct tipc_group *grp,
 void tipc_nametbl_lookup_dst_nodes(struct net *net, u32 type, u32 lower,
 				   u32 upper, u32 domain,
 				   struct tipc_nlist *nodes);
+bool tipc_nametbl_lookup(struct net *net, u32 type, u32 instance, u32 domain,
+			 struct list_head *dsts, int *dstcnt, u32 exclude,
+			 bool all);
 struct publication *tipc_nametbl_publish(struct net *net, u32 type, u32 lower,
 					 u32 upper, u32 scope, u32 port_ref,
 					 u32 key);
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 4f7fee2..5ea93ac 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -900,6 +900,88 @@ static int tipc_send_group_unicast(struct socket *sock, struct msghdr *m,
 }
 
 /**
+ * tipc_send_group_anycast - send message to any member with given identity
+ * @sock: socket structure
+ * @m: message to send
+ * @dlen: total length of message data
+ * @timeout: timeout to wait for wakeup
+ *
+ * Called from function tipc_sendmsg(), which has done all sanity checks
+ * Returns the number of bytes sent on success, or errno
+ */
+static int tipc_send_group_anycast(struct socket *sock, struct msghdr *m,
+				   int dlen, long timeout)
+{
+	DECLARE_SOCKADDR(struct sockaddr_tipc *, dest, m->msg_name);
+	struct sock *sk = sock->sk;
+	struct net *net = sock_net(sk);
+	struct tipc_sock *tsk = tipc_sk(sk);
+	struct tipc_group *grp = tsk->group;
+	struct tipc_member *first = NULL;
+	struct tipc_member *mbr = NULL;
+	struct list_head *cong_links = &tsk->cong_links;
+	struct list_head dsts;
+	int blks = tsk_blocks(GROUP_H_SIZE + dlen);
+	u32 type, inst, domain;
+	u32 node, port, exclude;
+	int lookups = 0;
+	int dstcnt, rc;
+	bool cong;
+
+	INIT_LIST_HEAD(&dsts);
+
+	type = dest->addr.name.name.type;
+	inst = dest->addr.name.name.instance;
+	domain = addr_domain(net, dest->scope);
+	exclude = tipc_group_exclude(grp);
+
+	while (++lookups < 4) {
+		first = NULL;
+
+		/* Look for a non-congested destination member, if any */
+		while (1) {
+			if (!tipc_nametbl_lookup(net, type, inst, domain, &dsts,
+						 &dstcnt, exclude, false))
+				return -EHOSTUNREACH;
+			tipc_dest_pop(&dsts, &node, &port);
+			cong = tipc_group_cong(grp, node, port, blks, &mbr);
+			if (!cong)
+				break;
+			if (mbr == first)
+				break;
+			if (!first)
+				first = mbr;
+		}
+
+		/* Start over if destination was not in member list */
+		if (unlikely(!mbr))
+			continue;
+
+		if (likely(!cong && !tipc_dest_find(cong_links, node, 0)))
+			break;
+
+		/* Block or return if destination link or member is congested */
+		rc = tipc_wait_for_cond(sock, &timeout,
+					!tipc_dest_find(cong_links, node, 0) &&
+					!tipc_group_cong(grp, node, port,
+							 blks, &mbr));
+		if (unlikely(rc))
+			return rc;
+
+		/* Send, unless destination disappeared while waiting */
+		if (likely(mbr))
+			break;
+	}
+
+	if (unlikely(lookups >= 4))
+		return -EHOSTUNREACH;
+
+	rc = tipc_send_group_msg(net, tsk, m, mbr, node, port, dlen);
+
+	return rc ? rc : dlen;
+}
+
+/**
  * tipc_send_group_bcast - send message to all members in communication group
  * @sk: socket structure
  * @m: message to send
@@ -1122,6 +1204,8 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	if (grp) {
 		if (!dest)
 			return tipc_send_group_bcast(sock, m, dlen, timeout);
+		if (dest->addrtype == TIPC_ADDR_NAME)
+			return tipc_send_group_anycast(sock, m, dlen, timeout);
 		if (dest->addrtype == TIPC_ADDR_ID)
 			return tipc_send_group_unicast(sock, m, dlen, timeout);
 		return -EINVAL;
-- 
2.1.4

^ permalink raw reply related

* [net-next 16/18] tipc: guarantee delivery of UP event before first broadcast
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

The following scenario is possible:
- A user joins a group, and immediately sends out a broadcast message
  to its members.
- The broadcast message, following a different data path than the
  initial JOIN message sent out during the joining procedure, arrives
  to a receiver before the latter..
- The receiver drops the message, since it is not ready to accept any
  messages until the JOIN has arrived.

We avoid this by treating group protocol JOIN messages like unicast
messages.
- We let them pass through the recipient's multicast input queue, just
  like ordinary unicasts.
- We force the first following broadacst to be sent as replicated
  unicast and being acknowledged by the recipient before accepting
  any more broadcast transmissions.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/link.c   | 7 +++++--
 net/tipc/socket.c | 4 ++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 70a2149..723dd69 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1039,6 +1039,7 @@ int tipc_link_retrans(struct tipc_link *l, struct tipc_link *nacker,
 static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 			    struct sk_buff_head *inputq)
 {
+	struct sk_buff_head *mc_inputq = l->bc_rcvlink->inputq;
 	struct tipc_msg *hdr = buf_msg(skb);
 
 	switch (msg_user(hdr)) {
@@ -1047,12 +1048,14 @@ static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
 	case TIPC_HIGH_IMPORTANCE:
 	case TIPC_CRITICAL_IMPORTANCE:
 		if (unlikely(msg_in_group(hdr) || msg_mcast(hdr))) {
-			skb_queue_tail(l->bc_rcvlink->inputq, skb);
+			skb_queue_tail(mc_inputq, skb);
 			return true;
 		}
-	case GROUP_PROTOCOL:
 	case CONN_MANAGER:
 		return true;
+	case GROUP_PROTOCOL:
+		skb_queue_tail(mc_inputq, skb);
+		return true;
 	case NAME_DISTRIBUTOR:
 		l->bc_rcvlink->state = LINK_ESTABLISHED;
 		skb_queue_tail(l->namedq, skb);
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 379c5b9..358a603 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2761,6 +2761,10 @@ static int tipc_sk_join(struct tipc_sock *tsk, struct tipc_group_req *mreq)
 	rc = tipc_sk_publish(tsk, mreq->scope, &seq);
 	if (rc)
 		tipc_group_delete(net, grp);
+
+	/* Eliminate any risk that a broadcast overtakes the sent JOIN */
+	tsk->mc_method.rcast = true;
+	tsk->mc_method.mandatory = true;
 	return rc;
 }
 
-- 
2.1.4

^ permalink raw reply related

* [net-next 10/18] tipc: introduce flow control for group broadcast messages
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

We introduce an end-to-end flow control mechanism for group broadcast
messages. This ensures that no messages are ever lost because of
destination receive buffer overflow, with minimal impact on performance.
For now, the algorithm is based on the assumption that there is only one
active transmitter at any moment in time.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c  | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 net/tipc/group.h  |  11 ++--
 net/tipc/msg.h    |   5 +-
 net/tipc/socket.c |  48 +++++++++++++-----
 4 files changed, 190 insertions(+), 22 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index fe2a76f..50dd7d1 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -46,6 +46,7 @@
 
 #define ADV_UNIT (((MAX_MSG_SIZE + MAX_H_SIZE) / FLOWCTL_BLK_SZ) + 1)
 #define ADV_IDLE ADV_UNIT
+#define ADV_ACTIVE (ADV_UNIT * 12)
 
 enum mbr_state {
 	MBR_QUARANTINED,
@@ -59,16 +60,22 @@ enum mbr_state {
 struct tipc_member {
 	struct rb_node tree_node;
 	struct list_head list;
+	struct list_head congested;
 	struct sk_buff *event_msg;
+	struct tipc_group *group;
 	u32 node;
 	u32 port;
 	u32 instance;
 	enum mbr_state state;
+	u16 advertised;
+	u16 window;
 	u16 bc_rcv_nxt;
+	bool usr_pending;
 };
 
 struct tipc_group {
 	struct rb_root members;
+	struct list_head congested;
 	struct tipc_nlist dests;
 	struct net *net;
 	int subid;
@@ -86,11 +93,24 @@ struct tipc_group {
 static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 				  int mtyp, struct sk_buff_head *xmitq);
 
+static int tipc_group_rcvbuf_limit(struct tipc_group *grp)
+{
+	int mcnt = grp->member_cnt + 1;
+
+	/* Scale to bytes, considering worst-case truesize/msgsize ratio */
+	return mcnt * ADV_ACTIVE * FLOWCTL_BLK_SZ * 4;
+}
+
 u16 tipc_group_bc_snd_nxt(struct tipc_group *grp)
 {
 	return grp->bc_snd_nxt;
 }
 
+static bool tipc_group_is_enabled(struct tipc_member *m)
+{
+	return m->state != MBR_QUARANTINED && m->state != MBR_LEAVING;
+}
+
 static bool tipc_group_is_receiver(struct tipc_member *m)
 {
 	return m && m->state >= MBR_JOINED;
@@ -111,6 +131,7 @@ struct tipc_group *tipc_group_create(struct net *net, u32 portid,
 	if (!grp)
 		return NULL;
 	tipc_nlist_init(&grp->dests, tipc_own_addr(net));
+	INIT_LIST_HEAD(&grp->congested);
 	grp->members = RB_ROOT;
 	grp->net = net;
 	grp->portid = portid;
@@ -213,6 +234,8 @@ static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 	if (!m)
 		return NULL;
 	INIT_LIST_HEAD(&m->list);
+	INIT_LIST_HEAD(&m->congested);
+	m->group = grp;
 	m->node = node;
 	m->port = port;
 	grp->member_cnt++;
@@ -233,6 +256,7 @@ static void tipc_group_delete_member(struct tipc_group *grp,
 	rb_erase(&m->tree_node, &grp->members);
 	grp->member_cnt--;
 	list_del_init(&m->list);
+	list_del_init(&m->congested);
 
 	/* If last member on a node, remove node from dest list */
 	if (!tipc_group_find_node(grp, m->node))
@@ -255,11 +279,59 @@ void tipc_group_self(struct tipc_group *grp, struct tipc_name_seq *seq,
 	*scope = grp->scope;
 }
 
-void tipc_group_update_bc_members(struct tipc_group *grp)
+void tipc_group_update_member(struct tipc_member *m, int len)
+{
+	struct tipc_group *grp = m->group;
+	struct tipc_member *_m, *tmp;
+
+	if (!tipc_group_is_enabled(m))
+		return;
+
+	m->window -= len;
+
+	if (m->window >= ADV_IDLE)
+		return;
+
+	if (!list_empty(&m->congested))
+		return;
+
+	/* Sort member into congested members' list */
+	list_for_each_entry_safe(_m, tmp, &grp->congested, congested) {
+		if (m->window > _m->window)
+			continue;
+		list_add_tail(&m->congested, &_m->congested);
+		return;
+	}
+	list_add_tail(&m->congested, &grp->congested);
+}
+
+void tipc_group_update_bc_members(struct tipc_group *grp, int len)
 {
+	struct tipc_member *m;
+	struct rb_node *n;
+
+	for (n = rb_first(&grp->members); n; n = rb_next(n)) {
+		m = container_of(n, struct tipc_member, tree_node);
+		if (tipc_group_is_enabled(m))
+			tipc_group_update_member(m, len);
+	}
 	grp->bc_snd_nxt++;
 }
 
+bool tipc_group_bc_cong(struct tipc_group *grp, int len)
+{
+	struct tipc_member *m;
+
+	if (list_empty(&grp->congested))
+		return false;
+
+	m = list_first_entry(&grp->congested, struct tipc_member, congested);
+	if (m->window >= len)
+		return false;
+
+	return true;
+}
+
 /* tipc_group_filter_msg() - determine if we should accept arriving message
  */
 void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
@@ -302,11 +374,36 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 	kfree_skb(skb);
 }
 
+void tipc_group_update_rcv_win(struct tipc_group *grp, int blks, u32 node,
+			       u32 port, struct sk_buff_head *xmitq)
+{
+	struct tipc_member *m;
+
+	m = tipc_group_find_member(grp, node, port);
+	if (!m)
+		return;
+
+	m->advertised -= blks;
+
+	switch (m->state) {
+	case MBR_JOINED:
+		if (m->advertised <= (ADV_ACTIVE - ADV_UNIT))
+			tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, xmitq);
+		break;
+	case MBR_DISCOVERED:
+	case MBR_JOINING:
+	case MBR_LEAVING:
+	default:
+		break;
+	}
+}
+
 static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 				  int mtyp, struct sk_buff_head *xmitq)
 {
 	struct sk_buff *skb;
 	struct tipc_msg *hdr;
+	int adv = 0;
 
 	skb = tipc_msg_create(GROUP_PROTOCOL, mtyp, INT_H_SIZE, 0,
 			      m->node, tipc_own_addr(grp->net),
@@ -314,14 +411,24 @@ static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 	if (!skb)
 		return;
 
+	if (m->state == MBR_JOINED)
+		adv = ADV_ACTIVE - m->advertised;
+
 	hdr = buf_msg(skb);
-	if (mtyp == GRP_JOIN_MSG)
+
+	if (mtyp == GRP_JOIN_MSG) {
 		msg_set_grp_bc_syncpt(hdr, grp->bc_snd_nxt);
+		msg_set_adv_win(hdr, adv);
+		m->advertised += adv;
+	} else if (mtyp == GRP_ADV_MSG) {
+		msg_set_adv_win(hdr, adv);
+		m->advertised += adv;
+	}
 	__skb_queue_tail(xmitq, skb);
 }
 
-void tipc_group_proto_rcv(struct tipc_group *grp, struct tipc_msg *hdr,
-			  struct sk_buff_head *inputq,
+void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
+			  struct tipc_msg *hdr, struct sk_buff_head *inputq,
 			  struct sk_buff_head *xmitq)
 {
 	struct tipc_member *m;
@@ -341,14 +448,22 @@ void tipc_group_proto_rcv(struct tipc_group *grp, struct tipc_msg *hdr,
 		if (!m)
 			return;
 		m->bc_rcv_nxt = msg_grp_bc_syncpt(hdr);
+		m->window += msg_adv_win(hdr);
 
 		/* Wait until PUBLISH event is received */
 		if (m->state == MBR_DISCOVERED) {
 			m->state = MBR_JOINING;
 		} else if (m->state == MBR_PUBLISHED) {
 			m->state = MBR_JOINED;
+			*usr_wakeup = true;
+			m->usr_pending = false;
+			tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, xmitq);
 			__skb_queue_tail(inputq, m->event_msg);
 		}
+		if (m->window < ADV_IDLE)
+			tipc_group_update_member(m, 0);
+		else
+			list_del_init(&m->congested);
 		return;
 	case GRP_LEAVE_MSG:
 		if (!m)
@@ -361,14 +476,28 @@ void tipc_group_proto_rcv(struct tipc_group *grp, struct tipc_msg *hdr,
 		}
 		/* Otherwise deliver already received WITHDRAW event */
 		__skb_queue_tail(inputq, m->event_msg);
+		*usr_wakeup = m->usr_pending;
 		tipc_group_delete_member(grp, m);
+		list_del_init(&m->congested);
+		return;
+	case GRP_ADV_MSG:
+		if (!m)
+			return;
+		m->window += msg_adv_win(hdr);
+		*usr_wakeup = m->usr_pending;
+		m->usr_pending = false;
+		list_del_init(&m->congested);
 		return;
 	default:
 		pr_warn("Received unknown GROUP_PROTO message\n");
 	}
 }
 
+/* tipc_group_member_evt() - receive and handle a member up/down event
+ */
 void tipc_group_member_evt(struct tipc_group *grp,
+			   bool *usr_wakeup,
+			   int *sk_rcvbuf,
 			   struct sk_buff *skb,
 			   struct sk_buff_head *inputq,
 			   struct sk_buff_head *xmitq)
@@ -416,16 +545,25 @@ void tipc_group_member_evt(struct tipc_group *grp,
 		} else {
 			__skb_queue_tail(inputq, skb);
 			m->state = MBR_JOINED;
+			*usr_wakeup = true;
+			m->usr_pending = false;
 		}
 		m->instance = instance;
 		TIPC_SKB_CB(skb)->orig_member = m->instance;
 		tipc_group_proto_xmit(grp, m, GRP_JOIN_MSG, xmitq);
+		if (m->window < ADV_IDLE)
+			tipc_group_update_member(m, 0);
+		else
+			list_del_init(&m->congested);
 	} else if (event == TIPC_WITHDRAWN) {
 		if (!m)
 			goto drop;
 
 		TIPC_SKB_CB(skb)->orig_member = m->instance;
 
+		*usr_wakeup = m->usr_pending;
+		m->usr_pending = false;
+
 		/* Hold back event if more messages might be expected */
 		if (m->state != MBR_LEAVING && tipc_node_is_up(net, node)) {
 			m->event_msg = skb;
@@ -434,7 +572,9 @@ void tipc_group_member_evt(struct tipc_group *grp,
 			__skb_queue_tail(inputq, skb);
 			tipc_group_delete_member(grp, m);
 		}
+		list_del_init(&m->congested);
 	}
+	*sk_rcvbuf = tipc_group_rcvbuf_limit(grp);
 	return;
 drop:
 	kfree_skb(skb);
diff --git a/net/tipc/group.h b/net/tipc/group.h
index 5d3f10d..0e2740e 100644
--- a/net/tipc/group.h
+++ b/net/tipc/group.h
@@ -52,15 +52,18 @@ void tipc_group_self(struct tipc_group *grp, struct tipc_name_seq *seq,
 void tipc_group_filter_msg(struct tipc_group *grp,
 			   struct sk_buff_head *inputq,
 			   struct sk_buff_head *xmitq);
-void tipc_group_member_evt(struct tipc_group *grp,
-			   struct sk_buff *skb,
+void tipc_group_member_evt(struct tipc_group *grp, bool *wakeup,
+			   int *sk_rcvbuf, struct sk_buff *skb,
 			   struct sk_buff_head *inputq,
 			   struct sk_buff_head *xmitq);
-void tipc_group_proto_rcv(struct tipc_group *grp,
+void tipc_group_proto_rcv(struct tipc_group *grp, bool *wakeup,
 			  struct tipc_msg *hdr,
 			  struct sk_buff_head *inputq,
 			  struct sk_buff_head *xmitq);
-void tipc_group_update_bc_members(struct tipc_group *grp);
+void tipc_group_update_bc_members(struct tipc_group *grp, int len);
+bool tipc_group_bc_cong(struct tipc_group *grp, int len);
+void tipc_group_update_rcv_win(struct tipc_group *grp, int blks, u32 node,
+			       u32 port, struct sk_buff_head *xmitq);
 u16 tipc_group_bc_snd_nxt(struct tipc_group *grp);
 int tipc_group_size(struct tipc_group *grp);
 #endif
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 3384ee6..820720c 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -538,6 +538,7 @@ static inline void msg_set_nameupper(struct tipc_msg *m, u32 n)
  */
 #define GRP_JOIN_MSG         0
 #define GRP_LEAVE_MSG        1
+#define GRP_ADV_MSG          2
 
 /*
  * Word 1
@@ -790,12 +791,12 @@ static inline void msg_set_conn_ack(struct tipc_msg *m, u32 n)
 	msg_set_bits(m, 9, 16, 0xffff, n);
 }
 
-static inline u32 msg_adv_win(struct tipc_msg *m)
+static inline u16 msg_adv_win(struct tipc_msg *m)
 {
 	return msg_bits(m, 9, 0, 0xffff);
 }
 
-static inline void msg_set_adv_win(struct tipc_msg *m, u32 n)
+static inline void msg_set_adv_win(struct tipc_msg *m, u16 n)
 {
 	msg_set_bits(m, 9, 0, 0xffff, n);
 }
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index ed0907c..d2f83fc9 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -196,6 +196,11 @@ static bool tsk_conn_cong(struct tipc_sock *tsk)
 	return tsk->snt_unacked > tsk->snd_win;
 }
 
+static u16 tsk_blocks(int len)
+{
+	return ((len / FLOWCTL_BLK_SZ) + 1);
+}
+
 /* tsk_blocks(): translate a buffer size in bytes to number of
  * advertisable blocks, taking into account the ratio truesize(len)/len
  * We can trust that this ratio is always < 4 for len >= FLOWCTL_BLK_SZ
@@ -828,20 +833,22 @@ static int tipc_send_group_bcast(struct socket *sock, struct msghdr *m,
 	struct tipc_nlist *dsts = tipc_group_dests(grp);
 	struct tipc_mc_method *method = &tsk->mc_method;
 	struct sk_buff_head pkts;
+	int blks = tsk_blocks(MCAST_H_SIZE + dlen);
 	int mtu = tipc_bcast_get_mtu(net);
 	int rc = -EHOSTUNREACH;
 
 	if (!dsts->local && !dsts->remote)
 		return -EHOSTUNREACH;
 
-	/* Block or return if any destination link is congested */
-	rc = tipc_wait_for_cond(sock, &timeout,	!tsk->cong_link_cnt);
+	/* Block or return if any destination link or member is congested */
+	rc = tipc_wait_for_cond(sock, &timeout,	!tsk->cong_link_cnt &&
+				!tipc_group_bc_cong(grp, blks));
 	if (unlikely(rc))
 		return rc;
 
 	/* Complete message header */
 	msg_set_type(hdr, TIPC_GRP_BCAST_MSG);
-	msg_set_hdr_sz(hdr, MCAST_H_SIZE);
+	msg_set_hdr_sz(hdr, GROUP_H_SIZE);
 	msg_set_destport(hdr, 0);
 	msg_set_destnode(hdr, 0);
 	msg_set_nameinst(hdr, 0);
@@ -859,9 +866,8 @@ static int tipc_send_group_bcast(struct socket *sock, struct msghdr *m,
 	if (unlikely(rc))
 		return rc;
 
-	/* Update broadcast sequence number */
-	tipc_group_update_bc_members(tsk->group);
-
+	/* Update broadcast sequence number and send windows */
+	tipc_group_update_bc_members(tsk->group, blks);
 	return dlen;
 }
 
@@ -1019,7 +1025,7 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 	if (unlikely(dlen > TIPC_MAX_USER_MSG_SIZE))
 		return -EMSGSIZE;
 
-	if (unlikely(grp))
+	if (unlikely(grp && !dest))
 		return tipc_send_group_bcast(sock, m, dlen, timeout);
 
 	if (unlikely(!dest)) {
@@ -1419,6 +1425,7 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 	struct tipc_sock *tsk = tipc_sk(sk);
 	struct sk_buff *skb;
 	struct tipc_msg *hdr;
+	struct sk_buff_head xmitq;
 	bool connected = !tipc_sk_type_connectionless(sk);
 	bool grp_evt;
 	int rc, err, hlen, dlen, copy;
@@ -1435,8 +1442,8 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 	}
 	timeout = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
 
+	/* Step rcv queue to first msg with data or error; wait if necessary */
 	do {
-		/* Look at first msg in receive queue; wait if necessary */
 		rc = tipc_wait_for_rcvmsg(sock, &timeout);
 		if (unlikely(rc))
 			goto exit;
@@ -1484,12 +1491,21 @@ static int tipc_recvmsg(struct socket *sock, struct msghdr *m,
 	if (unlikely(flags & MSG_PEEK))
 		goto exit;
 
+	/* Send group flow control advertisement when applicable */
+	if (tsk->group && msg_in_group(hdr) && !grp_evt) {
+		skb_queue_head_init(&xmitq);
+		tipc_group_update_rcv_win(tsk->group, tsk_blocks(hlen + dlen),
+					  msg_orignode(hdr), msg_origport(hdr),
+					  &xmitq);
+		tipc_node_distr_xmit(sock_net(sk), &xmitq);
+	}
+
 	tsk_advance_rx_queue(sk);
 
 	if (likely(!connected))
 		goto exit;
 
-	/* Send connection flow control ack when applicable */
+	/* Send connection flow control advertisement when applicable */
 	tsk->rcv_unacked += tsk_inc(tsk, hlen + dlen);
 	if (tsk->rcv_unacked >= tsk->rcv_win / TIPC_ACK_RATE)
 		tipc_sk_send_ack(tsk);
@@ -1649,6 +1665,7 @@ static void tipc_sk_proto_rcv(struct sock *sk,
 	struct sk_buff *skb = __skb_dequeue(inputq);
 	struct tipc_msg *hdr = buf_msg(skb);
 	struct tipc_group *grp = tsk->group;
+	bool wakeup = false;
 
 	switch (msg_user(hdr)) {
 	case CONN_MANAGER:
@@ -1657,19 +1674,23 @@ static void tipc_sk_proto_rcv(struct sock *sk,
 	case SOCK_WAKEUP:
 		tipc_dest_del(&tsk->cong_links, msg_orignode(hdr), 0);
 		tsk->cong_link_cnt--;
-		sk->sk_write_space(sk);
+		wakeup = true;
 		break;
 	case GROUP_PROTOCOL:
-		tipc_group_proto_rcv(grp, hdr, inputq, xmitq);
+		tipc_group_proto_rcv(grp, &wakeup, hdr, inputq, xmitq);
 		break;
 	case TOP_SRV:
-		tipc_group_member_evt(tsk->group, skb, inputq, xmitq);
+		tipc_group_member_evt(tsk->group, &wakeup, &sk->sk_rcvbuf,
+				      skb, inputq, xmitq);
 		skb = NULL;
 		break;
 	default:
 		break;
 	}
 
+	if (wakeup)
+		sk->sk_write_space(sk);
+
 	kfree_skb(skb);
 }
 
@@ -1784,6 +1805,9 @@ static unsigned int rcvbuf_limit(struct sock *sk, struct sk_buff *skb)
 	struct tipc_sock *tsk = tipc_sk(sk);
 	struct tipc_msg *hdr = buf_msg(skb);
 
+	if (unlikely(msg_in_group(hdr)))
+		return sk->sk_rcvbuf;
+
 	if (unlikely(!msg_connected(hdr)))
 		return sk->sk_rcvbuf << msg_importance(hdr);
 
-- 
2.1.4

^ permalink raw reply related

* [net-next 17/18] tipc: guarantee delivery of last broadcast before DOWN event
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

The following scenario is possible:
- A user sends a broadcast message, and thereafter immediately leaves
  the group.
- The LEAVE message, following a different path than the broadcast,
  arrives ahead of the broadcast, and the sending member is removed
  from the receiver's list.
- The broadcast message arrives, but is dropped because the sender
  now is unknown to the receipient.

We fix this by sequence numbering membership events, just like ordinary
unicast messages. Currently, when a JOIN is sent to a peer, it contains
a synchronization point, - the sequence number of the next sent
broadcast, in order to give the receiver a start synchronization point.
We now let even LEAVE messages contain such an "end synchronization"
point, so that the recipient can delay the removal of the sending member
until it knows that all messages have been received.

The received synchronization points are added as sequence numbers to the
generated membership events, making it possible to handle them almost
the same way as regular unicasts in the receiving filter function. In
particular, a DOWN event with a too high sequence number will be kept
in the reordering queue until the missing broadcast(s) arrive and have
been delivered.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 1203929..0a405f2 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -71,6 +71,7 @@ struct tipc_member {
 	u16 advertised;
 	u16 window;
 	u16 bc_rcv_nxt;
+	u16 bc_syncpt;
 	u16 bc_acked;
 	bool usr_pending;
 };
@@ -413,7 +414,7 @@ static void tipc_group_sort_msg(struct sk_buff *skb, struct sk_buff_head *defq)
 	u16 bc_seqno = msg_grp_bc_seqno(hdr);
 	int mtyp = msg_type(hdr);
 
-	/* Bcast may be bypassed by unicast or other bcast, - sort it in */
+	/* Bcast/mcast may be bypassed by ucast or other bcast, - sort it in */
 	if (mtyp == TIPC_GRP_BCAST_MSG || mtyp == TIPC_GRP_MCAST_MSG) {
 		skb_queue_walk_safe(defq, _skb, tmp) {
 			_hdr = buf_msg(_skb);
@@ -437,7 +438,7 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 	struct sk_buff_head *defq;
 	struct tipc_member *m;
 	struct tipc_msg *hdr;
-	bool ack, deliver, update;
+	bool ack, deliver, update, leave = false;
 	u32 node, port;
 	int mtyp, blks;
 
@@ -451,13 +452,6 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 	if (!msg_in_group(hdr))
 		goto drop;
 
-	if (msg_is_grp_evt(hdr)) {
-		if (!grp->events)
-			goto drop;
-		__skb_queue_tail(inputq, skb);
-		return;
-	}
-
 	m = tipc_group_find_member(grp, node, port);
 	if (!tipc_group_is_receiver(m))
 		goto drop;
@@ -493,6 +487,12 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 			break;
 		case TIPC_GRP_UCAST_MSG:
 			break;
+		case TIPC_GRP_MEMBER_EVT:
+			if (m->state == MBR_LEAVING)
+				leave = true;
+			if (!grp->events)
+				deliver = false;
+			break;
 		default:
 			break;
 		}
@@ -507,6 +507,11 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 		if (ack)
 			tipc_group_proto_xmit(grp, m, GRP_ACK_MSG, xmitq);
 
+		if (leave) {
+			tipc_group_delete_member(grp, m);
+			__skb_queue_purge(defq);
+			break;
+		}
 		if (!update)
 			continue;
 
@@ -564,6 +569,8 @@ static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 		msg_set_grp_bc_syncpt(hdr, grp->bc_snd_nxt);
 		msg_set_adv_win(hdr, adv);
 		m->advertised += adv;
+	} else if (mtyp == GRP_LEAVE_MSG) {
+		msg_set_grp_bc_syncpt(hdr, grp->bc_snd_nxt);
 	} else if (mtyp == GRP_ADV_MSG) {
 		msg_set_adv_win(hdr, adv);
 		m->advertised += adv;
@@ -578,6 +585,7 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 			  struct sk_buff_head *xmitq)
 {
 	struct tipc_member *m;
+	struct tipc_msg *ehdr;
 	u32 node = msg_orignode(hdr);
 	u32 port = msg_origport(hdr);
 
@@ -593,7 +601,8 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 						     MBR_QUARANTINED);
 		if (!m)
 			return;
-		m->bc_rcv_nxt = msg_grp_bc_syncpt(hdr);
+		m->bc_syncpt = msg_grp_bc_syncpt(hdr);
+		m->bc_rcv_nxt = m->bc_syncpt;
 		m->window += msg_adv_win(hdr);
 
 		/* Wait until PUBLISH event is received */
@@ -604,6 +613,8 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 			*usr_wakeup = true;
 			m->usr_pending = false;
 			tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, xmitq);
+			ehdr = buf_msg(m->event_msg);
+			msg_set_grp_bc_seqno(ehdr, m->bc_syncpt);
 			__skb_queue_tail(inputq, m->event_msg);
 		}
 		if (m->window < ADV_IDLE)
@@ -614,6 +625,7 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 	case GRP_LEAVE_MSG:
 		if (!m)
 			return;
+		m->bc_syncpt = msg_grp_bc_syncpt(hdr);
 
 		/* Wait until WITHDRAW event is received */
 		if (m->state != MBR_LEAVING) {
@@ -621,9 +633,10 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 			return;
 		}
 		/* Otherwise deliver already received WITHDRAW event */
+		ehdr = buf_msg(m->event_msg);
+		msg_set_grp_bc_seqno(ehdr, m->bc_syncpt);
 		__skb_queue_tail(inputq, m->event_msg);
 		*usr_wakeup = true;
-		tipc_group_delete_member(grp, m);
 		list_del_init(&m->congested);
 		return;
 	case GRP_ADV_MSG:
@@ -666,6 +679,7 @@ void tipc_group_member_evt(struct tipc_group *grp,
 	u32 port = evt->port.ref;
 	u32 node = evt->port.node;
 	u32 self;
+	bool node_up;
 
 	if (!grp)
 		goto drop;
@@ -698,6 +712,7 @@ void tipc_group_member_evt(struct tipc_group *grp,
 			m->event_msg = skb;
 			m->state = MBR_PUBLISHED;
 		} else {
+			msg_set_grp_bc_seqno(hdr, m->bc_syncpt);
 			__skb_queue_tail(inputq, skb);
 			m->state = MBR_JOINED;
 			*usr_wakeup = true;
@@ -718,14 +733,18 @@ void tipc_group_member_evt(struct tipc_group *grp,
 
 		*usr_wakeup = true;
 		m->usr_pending = false;
+		node_up = tipc_node_is_up(net, node);
 
 		/* Hold back event if more messages might be expected */
-		if (m->state != MBR_LEAVING && tipc_node_is_up(net, node)) {
+		if (m->state != MBR_LEAVING && node_up) {
 			m->event_msg = skb;
 			m->state = MBR_LEAVING;
 		} else {
+			if (node_up)
+				msg_set_grp_bc_seqno(hdr, m->bc_syncpt);
+			else
+				msg_set_grp_bc_seqno(hdr, m->bc_rcv_nxt);
 			__skb_queue_tail(inputq, skb);
-			tipc_group_delete_member(grp, m);
 		}
 		list_del_init(&m->congested);
 	}
-- 
2.1.4

^ permalink raw reply related

* [net-next 14/18] tipc: guarantee group unicast doesn't bypass group broadcast
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

Group unicast messages don't follow the same path as broadcast messages,
and there is a high risk that unicasts sent from a socket might bypass
previously sent broadcasts from the same socket.

We fix this by letting all unicast messages carry the sequence number of
the next sent broadcast from the same node, but without updating this
number at the receiver. This way, a receiver can check and if necessary
re-order such messages before they are added to the socket receive buffer.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c  | 87 +++++++++++++++++++++++++++++++++++++++++++++----------
 net/tipc/socket.c |  2 ++
 2 files changed, 74 insertions(+), 15 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 2fa49cc7..5edceb6 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -62,6 +62,7 @@ struct tipc_member {
 	struct list_head list;
 	struct list_head congested;
 	struct sk_buff *event_msg;
+	struct sk_buff_head deferredq;
 	struct tipc_group *group;
 	u32 node;
 	u32 port;
@@ -253,6 +254,7 @@ static struct tipc_member *tipc_group_create_member(struct tipc_group *grp,
 		return NULL;
 	INIT_LIST_HEAD(&m->list);
 	INIT_LIST_HEAD(&m->congested);
+	__skb_queue_head_init(&m->deferredq);
 	m->group = grp;
 	m->node = node;
 	m->port = port;
@@ -383,29 +385,54 @@ bool tipc_group_bc_cong(struct tipc_group *grp, int len)
 	return tipc_group_cong(grp, m->node, m->port, len, &m);
 }
 
+/* tipc_group_sort_msg() - sort msg into queue by bcast sequence number
+ */
+static void tipc_group_sort_msg(struct sk_buff *skb, struct sk_buff_head *defq)
+{
+	struct sk_buff *_skb, *tmp;
+	struct tipc_msg *_hdr, *hdr = buf_msg(skb);
+	u16 bc_seqno = msg_grp_bc_seqno(hdr);
+	int mtyp = msg_type(hdr);
+
+	/* Bcast may be bypassed by unicast, - sort it in */
+	if (mtyp == TIPC_GRP_BCAST_MSG || mtyp == TIPC_GRP_MCAST_MSG) {
+		skb_queue_walk_safe(defq, _skb, tmp) {
+			_hdr = buf_msg(_skb);
+			if (!less(bc_seqno, msg_grp_bc_seqno(_hdr)))
+				continue;
+			__skb_queue_before(defq, _skb, skb);
+			return;
+		}
+		/* Bcast was not bypassed, - add to tail */
+	}
+	/* Unicasts are never bypassed, - always add to tail */
+	__skb_queue_tail(defq, skb);
+}
+
 /* tipc_group_filter_msg() - determine if we should accept arriving message
  */
 void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 			   struct sk_buff_head *xmitq)
 {
 	struct sk_buff *skb = __skb_dequeue(inputq);
+	struct sk_buff_head *defq;
 	struct tipc_member *m;
 	struct tipc_msg *hdr;
+	bool deliver, update;
 	u32 node, port;
-	int mtyp;
+	int mtyp, blks;
 
 	if (!skb)
 		return;
 
 	hdr = buf_msg(skb);
-	mtyp = msg_type(hdr);
 	node =  msg_orignode(hdr);
 	port = msg_origport(hdr);
 
 	if (!msg_in_group(hdr))
 		goto drop;
 
-	if (mtyp == TIPC_GRP_MEMBER_EVT) {
+	if (msg_is_grp_evt(hdr)) {
 		if (!grp->events)
 			goto drop;
 		__skb_queue_tail(inputq, skb);
@@ -416,22 +443,52 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 	if (!tipc_group_is_receiver(m))
 		goto drop;
 
-	m->bc_rcv_nxt = msg_grp_bc_seqno(hdr) + 1;
+	if (less(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt))
+		goto drop;
 
-	/* Drop multicast here if not for this member */
-	if (mtyp == TIPC_GRP_MCAST_MSG) {
-		if (msg_nameinst(hdr) != grp->instance) {
-			m->bc_rcv_nxt = msg_grp_bc_seqno(hdr) + 1;
-			tipc_group_update_rcv_win(grp, msg_blocks(hdr),
-						  node, port, xmitq);
-			kfree_skb(skb);
-			return;
+	TIPC_SKB_CB(skb)->orig_member = m->instance;
+	defq = &m->deferredq;
+	tipc_group_sort_msg(skb, defq);
+
+	while ((skb = skb_peek(defq))) {
+		hdr = buf_msg(skb);
+		mtyp = msg_type(hdr);
+		deliver = true;
+		update = false;
+
+		if (more(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt))
+			break;
+
+		/* Decide what to do with message */
+		switch (mtyp) {
+		case TIPC_GRP_MCAST_MSG:
+			if (msg_nameinst(hdr) != grp->instance) {
+				update = true;
+				deliver = false;
+			}
+			/* Fall thru */
+		case TIPC_GRP_BCAST_MSG:
+			m->bc_rcv_nxt++;
+			break;
+		case TIPC_GRP_UCAST_MSG:
+			break;
+		default:
+			break;
 		}
-	}
 
-	TIPC_SKB_CB(skb)->orig_member = m->instance;
-	__skb_queue_tail(inputq, skb);
+		/* Execute decisions */
+		__skb_dequeue(defq);
+		if (deliver)
+			__skb_queue_tail(inputq, skb);
+		else
+			kfree_skb(skb);
+
+		if (!update)
+			continue;
 
+		blks = msg_blocks(hdr);
+		tipc_group_update_rcv_win(grp, blks, node, port, xmitq);
+	}
 	return;
 drop:
 	kfree_skb(skb);
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index ad41f64..015cdd9 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -827,6 +827,7 @@ static int tipc_send_group_msg(struct net *net, struct tipc_sock *tsk,
 {
 	struct tipc_msg *hdr = &tsk->phdr;
 	struct sk_buff_head pkts;
+	u16 bc_snd_nxt = tipc_group_bc_snd_nxt(tsk->group);
 	int blks = tsk_blocks(GROUP_H_SIZE + dlen);
 	int mtu, rc;
 
@@ -835,6 +836,7 @@ static int tipc_send_group_msg(struct net *net, struct tipc_sock *tsk,
 	msg_set_hdr_sz(hdr, GROUP_H_SIZE);
 	msg_set_destport(hdr, dport);
 	msg_set_destnode(hdr, dnode);
+	msg_set_grp_bc_seqno(hdr, bc_snd_nxt);
 
 	/* Build message as chain of buffers */
 	skb_queue_head_init(&pkts);
-- 
2.1.4

^ permalink raw reply related

* [net-next 13/18] tipc: introduce group multicast messaging
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

The previously introduced message transport to all group members is
based on the tipc multicast service, but is logically a broadcast
service within the group, and that is what we call it.

We now add functionality for sending messages to all group members
having a certain identity. Correspondingly, we call this feature 'group
multicast'. The service is using unicast when only one destination is
found, otherwise it will use the bearer broadcast service to transfer
the messages. In the latter case, the receiving members filter arriving
messages by looking at the intended destination instance. If there is
no match, the message will be dropped, while still being considered
received and read as seen by the flow control mechanism.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c  | 14 +++++++++++++-
 net/tipc/msg.h    | 11 +++++++++--
 net/tipc/socket.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index d3b8aba..2fa49cc7 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -416,10 +416,22 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 	if (!tipc_group_is_receiver(m))
 		goto drop;
 
+	m->bc_rcv_nxt = msg_grp_bc_seqno(hdr) + 1;
+
+	/* Drop multicast here if not for this member */
+	if (mtyp == TIPC_GRP_MCAST_MSG) {
+		if (msg_nameinst(hdr) != grp->instance) {
+			m->bc_rcv_nxt = msg_grp_bc_seqno(hdr) + 1;
+			tipc_group_update_rcv_win(grp, msg_blocks(hdr),
+						  node, port, xmitq);
+			kfree_skb(skb);
+			return;
+		}
+	}
+
 	TIPC_SKB_CB(skb)->orig_member = m->instance;
 	__skb_queue_tail(inputq, skb);
 
-	m->bc_rcv_nxt = msg_grp_bc_seqno(hdr) + 1;
 	return;
 drop:
 	kfree_skb(skb);
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 7430fe7..b40a050 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -67,7 +67,8 @@ struct plist;
 #define TIPC_DIRECT_MSG		3
 #define TIPC_GRP_MEMBER_EVT	4
 #define TIPC_GRP_BCAST_MSG	5
-#define TIPC_GRP_UCAST_MSG	6
+#define TIPC_GRP_MCAST_MSG	6
+#define TIPC_GRP_UCAST_MSG	7
 
 /*
  * Internal message users
@@ -195,6 +196,11 @@ static inline u32 msg_size(struct tipc_msg *m)
 	return msg_bits(m, 0, 0, 0x1ffff);
 }
 
+static inline u32 msg_blocks(struct tipc_msg *m)
+{
+	return (msg_size(m) / 1024) + 1;
+}
+
 static inline u32 msg_data_sz(struct tipc_msg *m)
 {
 	return msg_size(m) - msg_hdr_sz(m);
@@ -279,7 +285,8 @@ static inline u32 msg_mcast(struct tipc_msg *m)
 {
 	int mtyp = msg_type(m);
 
-	return ((mtyp == TIPC_MCAST_MSG) || (mtyp == TIPC_GRP_BCAST_MSG));
+	return ((mtyp == TIPC_MCAST_MSG) || (mtyp == TIPC_GRP_BCAST_MSG) ||
+		(mtyp == TIPC_GRP_MCAST_MSG));
 }
 
 static inline u32 msg_connected(struct tipc_msg *m)
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 5ea93ac..ad41f64 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -994,6 +994,7 @@ static int tipc_send_group_anycast(struct socket *sock, struct msghdr *m,
 static int tipc_send_group_bcast(struct socket *sock, struct msghdr *m,
 				 int dlen, long timeout)
 {
+	DECLARE_SOCKADDR(struct sockaddr_tipc *, dest, m->msg_name);
 	struct sock *sk = sock->sk;
 	struct net *net = sock_net(sk);
 	struct tipc_sock *tsk = tipc_sk(sk);
@@ -1016,11 +1017,16 @@ static int tipc_send_group_bcast(struct socket *sock, struct msghdr *m,
 		return rc;
 
 	/* Complete message header */
-	msg_set_type(hdr, TIPC_GRP_BCAST_MSG);
+	if (dest) {
+		msg_set_type(hdr, TIPC_GRP_MCAST_MSG);
+		msg_set_nameinst(hdr, dest->addr.name.name.instance);
+	} else {
+		msg_set_type(hdr, TIPC_GRP_BCAST_MSG);
+		msg_set_nameinst(hdr, 0);
+	}
 	msg_set_hdr_sz(hdr, GROUP_H_SIZE);
 	msg_set_destport(hdr, 0);
 	msg_set_destnode(hdr, 0);
-	msg_set_nameinst(hdr, 0);
 	msg_set_grp_bc_seqno(hdr, tipc_group_bc_snd_nxt(grp));
 
 	/* Build message as chain of buffers */
@@ -1041,6 +1047,48 @@ static int tipc_send_group_bcast(struct socket *sock, struct msghdr *m,
 }
 
 /**
+ * tipc_send_group_mcast - send message to all members with given identity
+ * @sock: socket structure
+ * @m: message to send
+ * @dlen: total length of message data
+ * @timeout: timeout to wait for wakeup
+ *
+ * Called from function tipc_sendmsg(), which has done all sanity checks
+ * Returns the number of bytes sent on success, or errno
+ */
+static int tipc_send_group_mcast(struct socket *sock, struct msghdr *m,
+				 int dlen, long timeout)
+{
+	DECLARE_SOCKADDR(struct sockaddr_tipc *, dest, m->msg_name);
+	struct sock *sk = sock->sk;
+	struct net *net = sock_net(sk);
+	struct tipc_sock *tsk = tipc_sk(sk);
+	struct tipc_group *grp = tsk->group;
+	struct tipc_name_seq *seq = &dest->addr.nameseq;
+	struct list_head dsts;
+	u32 domain, exclude, dstcnt;
+
+	INIT_LIST_HEAD(&dsts);
+
+	if (seq->lower != seq->upper)
+		return -ENOTSUPP;
+
+	domain = addr_domain(net, dest->scope);
+	exclude = tipc_group_exclude(grp);
+	if (!tipc_nametbl_lookup(net, seq->type, seq->lower, domain,
+				 &dsts, &dstcnt, exclude, true))
+		return -EHOSTUNREACH;
+
+	if (dstcnt == 1) {
+		tipc_dest_pop(&dsts, &dest->addr.id.node, &dest->addr.id.ref);
+		return tipc_send_group_unicast(sock, m, dlen, timeout);
+	}
+
+	tipc_dest_list_purge(&dsts);
+	return tipc_send_group_bcast(sock, m, dlen, timeout);
+}
+
+/**
  * tipc_sk_mcast_rcv - Deliver multicast messages to all destination sockets
  * @arrvq: queue with arriving messages, to be cloned after destination lookup
  * @inputq: queue with cloned messages, delivered to socket after dest lookup
@@ -1208,6 +1256,8 @@ static int __tipc_sendmsg(struct socket *sock, struct msghdr *m, size_t dlen)
 			return tipc_send_group_anycast(sock, m, dlen, timeout);
 		if (dest->addrtype == TIPC_ADDR_ID)
 			return tipc_send_group_unicast(sock, m, dlen, timeout);
+		if (dest->addrtype == TIPC_ADDR_MCAST)
+			return tipc_send_group_mcast(sock, m, dlen, timeout);
 		return -EINVAL;
 	}
 
@@ -2021,8 +2071,6 @@ static void tipc_sk_filter_rcv(struct sock *sk, struct sk_buff *skb,
 
 	if (unlikely(!msg_isdata(hdr)))
 		tipc_sk_proto_rcv(sk, &inputq, xmitq);
-	else if (unlikely(msg_type(hdr) > TIPC_GRP_UCAST_MSG))
-		return kfree_skb(skb);
 
 	if (unlikely(grp))
 		tipc_group_filter_msg(grp, &inputq, xmitq);
-- 
2.1.4

^ permalink raw reply related

* [net-next 18/18] tipc: add multipoint-to-point flow control
From: Jon Maloy @ 2017-10-12 14:02 UTC (permalink / raw)
  To: davem, netdev; +Cc: parthasarathy.bhuvaragan, ying.xue, tipc-discussion
In-Reply-To: <1507816959-31787-1-git-send-email-jon.maloy@ericsson.com>

We already have point-to-multipoint flow control within a group. But
we even need the opposite; -a scheme which can handle that potentially
hundreds of sources may try to send messages to the same destination
simultaneously without causing buffer overflow at the recipient. This
commit adds such a mechanism.

The algorithm works as follows:

- When a member detects a new, joining member, it initially set its
  state to JOINED and advertises a minimum window to the new member.
  This window is chosen so that the new member can send exactly one
  maximum sized message, or several smaller ones, to the recipient
  before it must stop and wait for an additional advertisement. This
  minimum window ADV_IDLE is set to 65 1kB blocks.

- When a member receives the first data message from a JOINED member,
  it changes the state of the latter to ACTIVE, and advertises a larger
  window ADV_ACTIVE = 12 x ADV_IDLE blocks to the sender, so it can
  continue sending with minimal disturbances to the data flow.

- The active members are kept in a dedicated linked list. Each time a
  message is received from an active member, it will be moved to the
  tail of that list. This way, we keep a record of which members have
  been most (tail) and least (head) recently active.

- There is a maximum number (16) of permitted simultaneous active
  senders per receiver. When this limit is reached, the receiver will
  not advertise anything immediately to a new sender, but instead put
  it in a PENDING state, and add it to a corresponding queue. At the
  same time, it will pick the least recently active member, send it an
  advertisement RECLAIM message, and set this member to state
  RECLAIMING.

- The reclaimee member has to respond with a REMIT message, meaning that
  it goes back to a send window of ADV_IDLE, and returns its unused
  advertised blocks beyond that value to the reclaiming member.

- When the reclaiming member receives the REMIT message, it unlinks
  the reclaimee from its active list, resets its state to JOINED, and
  notes that it is now back at ADV_IDLE advertised blocks to that
  member. If there are still unread data messages sent out by
  reclaimee before the REMIT, the member goes into an intermediate
  state REMITTED, where it stays until the said messages have been
  consumed.

- The returned advertised blocks can now be re-advertised to the
  pending member, which is now set to state ACTIVE and added to
  the active member list.

- To be proactive, i.e., to minimize the risk that any member will
  end up in the pending queue, we start reclaiming resources already
  when the number of active members exceeds 3/4 of the permitted
  maximum.

Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Acked-by: Ying Xue <ying.xue@windriver.com>
---
 net/tipc/group.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 net/tipc/msg.h   |  12 ++++++
 2 files changed, 136 insertions(+), 5 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 0a405f2..4337732 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -54,6 +54,10 @@ enum mbr_state {
 	MBR_JOINING,
 	MBR_PUBLISHED,
 	MBR_JOINED,
+	MBR_PENDING,
+	MBR_ACTIVE,
+	MBR_RECLAIMING,
+	MBR_REMITTED,
 	MBR_LEAVING
 };
 
@@ -79,6 +83,9 @@ struct tipc_member {
 struct tipc_group {
 	struct rb_root members;
 	struct list_head congested;
+	struct list_head pending;
+	struct list_head active;
+	struct list_head reclaiming;
 	struct tipc_nlist dests;
 	struct net *net;
 	int subid;
@@ -88,6 +95,8 @@ struct tipc_group {
 	u32 scope;
 	u32 portid;
 	u16 member_cnt;
+	u16 active_cnt;
+	u16 max_active;
 	u16 bc_snd_nxt;
 	u16 bc_ackers;
 	bool loopback;
@@ -97,12 +106,29 @@ struct tipc_group {
 static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 				  int mtyp, struct sk_buff_head *xmitq);
 
+static void tipc_group_decr_active(struct tipc_group *grp,
+				   struct tipc_member *m)
+{
+	if (m->state == MBR_ACTIVE || m->state == MBR_RECLAIMING)
+		grp->active_cnt--;
+}
+
 static int tipc_group_rcvbuf_limit(struct tipc_group *grp)
 {
 	int mcnt = grp->member_cnt + 1;
+	int max_active, active_pool, idle_pool;
+
+	/* Limit simultaneous reception from other members */
+	max_active = min(mcnt / 8, 64);
+	max_active = max(max_active, 16);
+	grp->max_active = max_active;
+
+	/* Reserve blocks for active and idle members */
+	active_pool = max_active * ADV_ACTIVE;
+	idle_pool = (mcnt - max_active) * ADV_IDLE;
 
 	/* Scale to bytes, considering worst-case truesize/msgsize ratio */
-	return mcnt * ADV_ACTIVE * FLOWCTL_BLK_SZ * 4;
+	return (active_pool + idle_pool) * FLOWCTL_BLK_SZ * 4;
 }
 
 u16 tipc_group_bc_snd_nxt(struct tipc_group *grp)
@@ -143,6 +169,9 @@ struct tipc_group *tipc_group_create(struct net *net, u32 portid,
 		return NULL;
 	tipc_nlist_init(&grp->dests, tipc_own_addr(net));
 	INIT_LIST_HEAD(&grp->congested);
+	INIT_LIST_HEAD(&grp->active);
+	INIT_LIST_HEAD(&grp->pending);
+	INIT_LIST_HEAD(&grp->reclaiming);
 	grp->members = RB_ROOT;
 	grp->net = net;
 	grp->portid = portid;
@@ -286,6 +315,7 @@ static void tipc_group_delete_member(struct tipc_group *grp,
 
 	list_del_init(&m->list);
 	list_del_init(&m->congested);
+	tipc_group_decr_active(grp, m);
 
 	/* If last member on a node, remove node from dest list */
 	if (!tipc_group_find_node(grp, m->node))
@@ -381,6 +411,10 @@ bool tipc_group_cong(struct tipc_group *grp, u32 dnode, u32 dport,
 		return true;
 	if (state == MBR_JOINED && adv == ADV_IDLE)
 		return true;
+	if (state == MBR_ACTIVE && adv == ADV_ACTIVE)
+		return true;
+	if (state == MBR_PENDING && adv == ADV_IDLE)
+		return true;
 	skb_queue_head_init(&xmitq);
 	tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, &xmitq);
 	tipc_node_distr_xmit(grp->net, &xmitq);
@@ -526,7 +560,11 @@ void tipc_group_filter_msg(struct tipc_group *grp, struct sk_buff_head *inputq,
 void tipc_group_update_rcv_win(struct tipc_group *grp, int blks, u32 node,
 			       u32 port, struct sk_buff_head *xmitq)
 {
-	struct tipc_member *m;
+	struct tipc_member *m, *rm;
+	struct list_head *active = &grp->active;
+	int max_active = grp->max_active;
+	int reclaim_limit = max_active * 3 / 4;
+	int active_cnt = grp->active_cnt;
 
 	m = tipc_group_find_member(grp, node, port);
 	if (!m)
@@ -536,9 +574,41 @@ void tipc_group_update_rcv_win(struct tipc_group *grp, int blks, u32 node,
 
 	switch (m->state) {
 	case MBR_JOINED:
-		if (m->advertised <= (ADV_ACTIVE - ADV_UNIT))
+		/* Reclaim advertised space from least active member */
+		if (!list_empty(active) && active_cnt >= reclaim_limit) {
+			rm = list_first_entry(active, struct tipc_member, list);
+			rm->state = MBR_RECLAIMING;
+			list_move_tail(&rm->list, &grp->reclaiming);
+			tipc_group_proto_xmit(grp, rm, GRP_RECLAIM_MSG, xmitq);
+		}
+		/* If max active, become pending and wait for reclaimed space */
+		if (active_cnt >= max_active) {
+			m->state = MBR_PENDING;
+			list_add_tail(&m->list, &grp->pending);
+			break;
+		}
+		/* Otherwise become active */
+		m->state = MBR_ACTIVE;
+		list_add_tail(&m->list, &grp->active);
+		grp->active_cnt++;
+		/* Fall through */
+	case MBR_ACTIVE:
+		if (!list_is_last(&m->list, &grp->active))
+			list_move_tail(&m->list, &grp->active);
+		if (m->advertised > (ADV_ACTIVE * 3 / 4))
+			break;
+		tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, xmitq);
+		break;
+	case MBR_REMITTED:
+		if (m->advertised > ADV_IDLE)
+			break;
+		m->state = MBR_JOINED;
+		if (m->advertised < ADV_IDLE) {
+			pr_warn_ratelimited("Rcv unexpected msg after REMIT\n");
 			tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, xmitq);
+		}
 		break;
+	case MBR_RECLAIMING:
 	case MBR_DISCOVERED:
 	case MBR_JOINING:
 	case MBR_LEAVING:
@@ -560,8 +630,10 @@ static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 	if (!skb)
 		return;
 
-	if (m->state == MBR_JOINED)
+	if (m->state == MBR_ACTIVE)
 		adv = ADV_ACTIVE - m->advertised;
+	else if (m->state == MBR_JOINED || m->state == MBR_PENDING)
+		adv = ADV_IDLE - m->advertised;
 
 	hdr = buf_msg(skb);
 
@@ -576,6 +648,8 @@ static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 		m->advertised += adv;
 	} else if (mtyp == GRP_ACK_MSG) {
 		msg_set_grp_bc_acked(hdr, m->bc_rcv_nxt);
+	} else if (mtyp == GRP_REMIT_MSG) {
+		msg_set_grp_remitted(hdr, m->window);
 	}
 	__skb_queue_tail(xmitq, skb);
 }
@@ -584,10 +658,11 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 			  struct tipc_msg *hdr, struct sk_buff_head *inputq,
 			  struct sk_buff_head *xmitq)
 {
-	struct tipc_member *m;
+	struct tipc_member *m, *pm;
 	struct tipc_msg *ehdr;
 	u32 node = msg_orignode(hdr);
 	u32 port = msg_origport(hdr);
+	u16 remitted, in_flight;
 
 	if (!grp)
 		return;
@@ -629,6 +704,7 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 
 		/* Wait until WITHDRAW event is received */
 		if (m->state != MBR_LEAVING) {
+			tipc_group_decr_active(grp, m);
 			m->state = MBR_LEAVING;
 			return;
 		}
@@ -656,6 +732,48 @@ void tipc_group_proto_rcv(struct tipc_group *grp, bool *usr_wakeup,
 		*usr_wakeup = true;
 		m->usr_pending = false;
 		return;
+	case GRP_RECLAIM_MSG:
+		if (!m)
+			return;
+		*usr_wakeup = m->usr_pending;
+		m->usr_pending = false;
+		tipc_group_proto_xmit(grp, m, GRP_REMIT_MSG, xmitq);
+		m->window = ADV_IDLE;
+		return;
+	case GRP_REMIT_MSG:
+		if (!m || m->state != MBR_RECLAIMING)
+			return;
+
+		list_del_init(&m->list);
+		grp->active_cnt--;
+		remitted = msg_grp_remitted(hdr);
+
+		/* Messages preceding the REMIT still in receive queue */
+		if (m->advertised > remitted) {
+			m->state = MBR_REMITTED;
+			in_flight = m->advertised - remitted;
+		}
+		/* All messages preceding the REMIT have been read */
+		if (m->advertised <= remitted) {
+			m->state = MBR_JOINED;
+			in_flight = 0;
+		}
+		/* ..and the REMIT overtaken by more messages => re-advertise */
+		if (m->advertised < remitted)
+			tipc_group_proto_xmit(grp, m, GRP_ADV_MSG, xmitq);
+
+		m->advertised = ADV_IDLE + in_flight;
+
+		/* Set oldest pending member to active and advertise */
+		if (list_empty(&grp->pending))
+			return;
+		pm = list_first_entry(&grp->pending, struct tipc_member, list);
+		pm->state = MBR_ACTIVE;
+		list_move_tail(&pm->list, &grp->active);
+		grp->active_cnt++;
+		if (pm->advertised <= (ADV_ACTIVE * 3 / 4))
+			tipc_group_proto_xmit(grp, pm, GRP_ADV_MSG, xmitq);
+		return;
 	default:
 		pr_warn("Received unknown GROUP_PROTO message\n");
 	}
@@ -738,6 +856,7 @@ void tipc_group_member_evt(struct tipc_group *grp,
 		/* Hold back event if more messages might be expected */
 		if (m->state != MBR_LEAVING && node_up) {
 			m->event_msg = skb;
+			tipc_group_decr_active(grp, m);
 			m->state = MBR_LEAVING;
 		} else {
 			if (node_up)
diff --git a/net/tipc/msg.h b/net/tipc/msg.h
index 1c088a1..65258c3 100644
--- a/net/tipc/msg.h
+++ b/net/tipc/msg.h
@@ -548,6 +548,8 @@ static inline void msg_set_nameupper(struct tipc_msg *m, u32 n)
 #define GRP_LEAVE_MSG        1
 #define GRP_ADV_MSG          2
 #define GRP_ACK_MSG          3
+#define GRP_RECLAIM_MSG      4
+#define GRP_REMIT_MSG        5
 
 /*
  * Word 1
@@ -850,6 +852,16 @@ static inline void msg_set_grp_bc_acked(struct tipc_msg *m, u16 n)
 	msg_set_bits(m, 9, 16, 0xffff, n);
 }
 
+static inline u16 msg_grp_remitted(struct tipc_msg *m)
+{
+	return msg_bits(m, 9, 16, 0xffff);
+}
+
+static inline void msg_set_grp_remitted(struct tipc_msg *m, u16 n)
+{
+	msg_set_bits(m, 9, 16, 0xffff, n);
+}
+
 /* Word 10
  */
 static inline u16 msg_grp_evt(struct tipc_msg *m)
-- 
2.1.4

^ permalink raw reply related

* Re: [RFC 1/3] devlink: Add config parameter get/set operations
From: Jiri Pirko @ 2017-10-12 14:03 UTC (permalink / raw)
  To: Steve Lin; +Cc: netdev, jiri, davem, michael.chan, linux-pci, linville, gospo
In-Reply-To: <1507815262-33294-2-git-send-email-steven.lin1@broadcom.com>

Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>Add support for config parameter get/set commands. Initially used by
>bnxt driver, but other drivers can use the same, or new, attributes.
>The config_get() and config_set() operations operate as expected, but
>note that the driver implementation of the config_set() operation can
>indicate whether a restart is necessary for the setting to take
>effect.
>

First of all, I like this approach.

I would like to see this patch split into:
1) config-options infrastructure introduction
2) specific config options introductions - would be best to have it
   per-option. We need to make sure every option is very well described
   and explained usecases. This is needed in order vendors to share
   attributes among drivers.

More nits inlined.


>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> include/net/devlink.h        |   4 +
> include/uapi/linux/devlink.h | 108 ++++++++++++++++++++++
> net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index b9654e1..952966c 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -270,6 +270,10 @@ struct devlink_ops {
> 	int (*eswitch_inline_mode_set)(struct devlink *devlink, u8 inline_mode);
> 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
> 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode);
>+	int (*config_get)(struct devlink *devlink, enum devlink_attr attr,
>+			  u32 *value);
>+	int (*config_set)(struct devlink *devlink, enum devlink_attr attr,
>+			  u32 value, u8 *restart_reqd);
> };
> 
> static inline void *devlink_priv(struct devlink *devlink)
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 0cbca96..e959716 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -70,6 +70,9 @@ enum devlink_command {
> 	DEVLINK_CMD_DPIPE_HEADERS_GET,
> 	DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
> 
>+	DEVLINK_CMD_CONFIG_GET,
>+	DEVLINK_CMD_CONFIG_SET,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -124,6 +127,68 @@ enum devlink_eswitch_encap_mode {
> 	DEVLINK_ESWITCH_ENCAP_MODE_BASIC,
> };
> 
>+enum devlink_dcbx_mode {
>+	DEVLINK_DCBX_MODE_DISABLED,
>+	DEVLINK_DCBX_MODE_IEEE,
>+	DEVLINK_DCBX_MODE_CEE,
>+	DEVLINK_DCBX_MODE_IEEE_CEE,
>+};
>+
>+enum devlink_multifunc_mode {
>+	DEVLINK_MULTIFUNC_MODE_ALLOWED,		/* Ext switch activates MF */
>+	DEVLINK_MULTIFUNC_MODE_FORCE_SINGFUNC,
>+	DEVLINK_MULTIFUNC_MODE_NPAR10,		/* NPAR 1.0 */
>+	DEVLINK_MULTIFUNC_MODE_NPAR15,		/* NPAR 1.5 */
>+	DEVLINK_MULTIFUNC_MODE_NPAR20,		/* NPAR 2.0 */
>+};
>+
>+enum devlink_autoneg_protocol {
>+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
>+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
>+	DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
>+	DEVLINK_AUTONEG_PROTOCOL_BAM,		/* Broadcom Autoneg Mode */
>+	DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,	/* Consortium Autoneg Mode */
>+};
>+
>+enum devlink_pre_os_link_speed {
>+	DEVLINK_PRE_OS_LINK_SPEED_AUTONEG,
>+	DEVLINK_PRE_OS_LINK_SPEED_1G,
>+	DEVLINK_PRE_OS_LINK_SPEED_10G,
>+	DEVLINK_PRE_OS_LINK_SPEED_25G,
>+	DEVLINK_PRE_OS_LINK_SPEED_40G,
>+	DEVLINK_PRE_OS_LINK_SPEED_50G,
>+	DEVLINK_PRE_OS_LINK_SPEED_100G,
>+	DEVLINK_PRE_OS_LINK_SPEED_5G = 0xe,
>+	DEVLINK_PRE_OS_LINK_SPEED_100M = 0xf,
>+};
>+
>+enum devlink_mba_boot_type {
>+	DEVLINK_MBA_BOOT_TYPE_AUTO_DETECT,
>+	DEVLINK_MBA_BOOT_TYPE_BBS,		/* BIOS Boot Specification */
>+	DEVLINK_MBA_BOOT_TYPE_INTR18,		/* Hook interrupt 0x18 */
>+	DEVLINK_MBA_BOOT_TYPE_INTR19,		/* Hook interrupt 0x19 */
>+};
>+
>+enum devlink_mba_setup_hot_key {
>+	DEVLINK_MBA_SETUP_HOT_KEY_CTRL_S,
>+	DEVLINK_MBA_SETUP_HOT_KEY_CTRL_B,
>+};
>+
>+enum devlink_mba_boot_protocol {
>+	DEVLINK_MBA_BOOT_PROTOCOL_PXE,
>+	DEVLINK_MBA_BOOT_PROTOCOL_ISCSI,
>+	DEVLINK_MBA_BOOT_PROTOCOL_NONE = 0x7,
>+};
>+
>+enum devlink_mba_link_speed {
>+	DEVLINK_MBA_LINK_SPEED_AUTONEG,
>+	DEVLINK_MBA_LINK_SPEED_1G,
>+	DEVLINK_MBA_LINK_SPEED_10G,
>+	DEVLINK_MBA_LINK_SPEED_25G,
>+	DEVLINK_MBA_LINK_SPEED_40G,
>+	DEVLINK_MBA_LINK_SPEED_50G,
>+};
>+
> enum devlink_attr {
> 	/* don't change the order or add anything between, this is ABI! */
> 	DEVLINK_ATTR_UNSPEC,
>@@ -202,6 +267,49 @@ enum devlink_attr {
> 
> 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
> 
>+	/* Configuration Parameters */
>+	DEVLINK_ATTR_SRIOV_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_NUM_VF_PER_PF,		/* u32 */
>+	DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,	/* u32 */
>+	DEVLINK_ATTR_MSIX_VECTORS_PER_VF,	/* u32 */
>+	DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,	/* u32 */
>+	DEVLINK_ATTR_NPAR_BW_IN_PERCENT,	/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_RESERVATION,	/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID,	/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_LIMIT,		/* u8 */
>+	DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,	/* u8 */
>+	DEVLINK_ATTR_DCBX_MODE,			/* u8 */
>+	DEVLINK_ATTR_RDMA_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_MULTIFUNC_MODE,		/* u8 */
>+	DEVLINK_ATTR_SECURE_NIC_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,	/* u8 */
>+	DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_PME_CAPABILITY_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,	/* u8 */
>+	DEVLINK_ATTR_AUTONEG_PROTOCOL,		/* u8 */
>+	DEVLINK_ATTR_MEDIA_AUTO_DETECT,		/* u8 */
>+	DEVLINK_ATTR_PHY_SELECT,		/* u8 */
>+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,	/* u8 */
>+	DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,	/* u8 */
>+	DEVLINK_ATTR_MBA_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_MBA_BOOT_TYPE,		/* u8 */
>+	DEVLINK_ATTR_MBA_DELAY_TIME,		/* u32 */
>+	DEVLINK_ATTR_MBA_SETUP_HOT_KEY,		/* u8 */
>+	DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,	/* u8 */
>+	DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,	/* u32 */
>+	DEVLINK_ATTR_MBA_VLAN_ENABLED,		/* u8 */
>+	DEVLINK_ATTR_MBA_VLAN_TAG,		/* u16 */
>+	DEVLINK_ATTR_MBA_BOOT_PROTOCOL,		/* u8 */
>+	DEVLINK_ATTR_MBA_LINK_SPEED,		/* u8 */

Okay, I think it is about the time we should start thinking about
putting this new config attributes under nester attribute. What do you
think?


>+
>+	/* When config doesn't take effect until next reboot (config
>+	 * just changed NVM which isn't read until boot, for example),
>+	 * this attribute should be set by the driver.
>+	 */
>+	DEVLINK_ATTR_RESTART_REQUIRED,		/* u8 */
>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 7d430c1..701c84b 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -1566,6 +1566,164 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
> 	return 0;
> }
> 
>+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>+
>+static int devlink_nl_config_fill(struct sk_buff *msg,
>+				  struct devlink *devlink,
>+				  enum devlink_command cmd,
>+				  struct genl_info *info)
>+{
>+	const struct devlink_ops *ops = devlink->ops;
>+	void *hdr;
>+	int err;
>+	enum devlink_attr attr = -1;

-1 is not a valid enum value. Better to just have bool to indicate attr
was found or not.


>+	u32 value;
>+	int i;
>+	struct nla_policy policy;
>+	u8 restart_reqd;
>+
>+	if (!ops->config_get)
>+		return -EOPNOTSUPP;
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, cmd);
>+	if (!hdr) {
>+		err = -EMSGSIZE;
>+		goto nla_msg_failure;
>+	}
>+
>+	err = devlink_nl_put_handle(msg, devlink);
>+	if (err)
>+		goto nla_put_failure;
>+
>+	for (i = 0; i < DEVLINK_ATTR_MAX; i++)
>+		if (info->attrs[i])
>+			attr = i;
>+
>+	if (attr == -1) {
>+		/* Not found - invalid/unknown attribute? */
>+		err = -EINVAL;
>+		goto nla_put_failure;
>+	}
>+
>+	policy = devlink_nl_policy[attr];
>+
>+	if (cmd == DEVLINK_CMD_CONFIG_GET) {
>+		err = ops->config_get(devlink, attr, &value);
>+
>+		if (err)
>+			goto nla_put_failure;
>+
>+		switch (policy.type) {
>+		case NLA_U8:
>+			err = nla_put_u8(msg, attr, value);
>+			break;
>+		case NLA_U16:
>+			err = nla_put_u16(msg, attr, value);
>+			break;
>+		case NLA_U32:
>+			err = nla_put_u32(msg, attr, value);
>+			break;
>+		default:
>+			goto nla_put_failure;
>+		}
>+
>+		if (err)
>+			goto nla_put_failure;
>+	} else {
>+		/* Must be config_set command */
>+		u8 *val8;
>+		u16 *val16;
>+		u32 *val32;
>+
>+		switch (policy.type) {
>+		case NLA_U8:
>+			val8 = nla_data(info->attrs[attr]);
>+			value = *val8;
>+			break;
>+		case NLA_U16:
>+			val16 = nla_data(info->attrs[attr]);
>+			value = *val16;
>+			break;
>+		case NLA_U32:
>+			val32 = nla_data(info->attrs[attr]);
>+			value = *val32;
>+			break;
>+		default:
>+			goto nla_put_failure;
>+		}
>+
>+		err = ops->config_set(devlink, attr, value, &restart_reqd);
>+		if (err)
>+			goto nla_put_failure;
>+
>+		if (restart_reqd) {
>+			err = nla_put_u8(msg, DEVLINK_ATTR_RESTART_REQUIRED,
>+					 restart_reqd);
>+			if (err)
>+				goto nla_put_failure;
>+		}
>+	}
>+
>+	genlmsg_end(msg, hdr);
>+	return 0;
>+
>+nla_put_failure:
>+	genlmsg_cancel(msg, hdr);
>+nla_msg_failure:
>+	return err;
>+}
>+
>+static int devlink_nl_cmd_config_get_doit(struct sk_buff *skb,
>+					  struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct sk_buff *msg;
>+	int err;
>+
>+	if (!devlink->ops)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_GET,
>+				     info);
>+
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
>+static int devlink_nl_cmd_config_set_doit(struct sk_buff *skb,
>+					  struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct sk_buff *msg;
>+	int err;
>+
>+	if (!devlink->ops)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_SET,
>+				     info);

This is odd. You are using "fill" function for "set". It should be used
for "get". But for set, it really sounds odd. Please split the
devlink_nl_config_fill function into get and set parts.

Also, get should dump all available options. 


>+
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
> int devlink_dpipe_match_put(struct sk_buff *skb,
> 			    struct devlink_dpipe_match *match)
> {
>@@ -2291,6 +2449,41 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_ESWITCH_ENCAP_MODE] = { .type = NLA_U8 },
> 	[DEVLINK_ATTR_DPIPE_TABLE_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_SRIOV_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_NUM_VF_PER_PF] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_IN_PERCENT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_RESERVATION] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_NPAR_BW_LIMIT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_DCBX_MODE] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_RDMA_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MULTIFUNC_MODE] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_SECURE_NIC_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_IGNORE_ARI_CAPABILITY] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_AUTONEG_PROTOCOL] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MEDIA_AUTO_DETECT] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PHY_SELECT] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MBA_BOOT_TYPE] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_DELAY_TIME] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_SETUP_HOT_KEY] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_VLAN_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_MBA_VLAN_TAG] = { .type = NLA_U16 },
>+	[DEVLINK_ATTR_MBA_BOOT_PROTOCOL] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_MBA_LINK_SPEED] = { .type = NLA_U32 },
> };
> 
> static const struct genl_ops devlink_nl_ops[] = {
>@@ -2451,6 +2644,20 @@ static const struct genl_ops devlink_nl_ops[] = {
> 		.flags = GENL_ADMIN_PERM,
> 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
> 	},
>+	{
>+		.cmd = DEVLINK_CMD_CONFIG_GET,
>+		.doit = devlink_nl_cmd_config_get_doit,
>+		.policy = devlink_nl_policy,
>+		.flags = GENL_ADMIN_PERM,
>+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>+	},
>+	{
>+		.cmd = DEVLINK_CMD_CONFIG_SET,
>+		.doit = devlink_nl_cmd_config_set_doit,
>+		.policy = devlink_nl_policy,
>+		.flags = GENL_ADMIN_PERM,
>+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>+	},
> };
> 
> static struct genl_family devlink_nl_family __ro_after_init = {
>-- 
>2.7.4
>

^ permalink raw reply

* [PATCH net-next] icmp: don't fail on fragment reassembly time exceeded
From: Matteo Croce @ 2017-10-12 14:12 UTC (permalink / raw)
  To: David S . Miller , netdev

The ICMP implementation currently replies to an ICMP time exceeded message
(type 11) with an ICMP host unreachable message (type 3, code 1).

However, time exceeded messages can either represent "time to live exceeded
in transit" (code 0) or "fragment reassembly time exceeded" (code 1).

Unconditionally replying to "fragment reassembly time exceeded" with
host unreachable messages might cause unjustified connection resets
which are now easily triggered as UFO has been removed, because, in turn,
sending large buffers triggers IP fragmentation.

The issue can be easily reproduced by running a lot of UDP streams
which is likely to trigger IP fragmentation:

  # start netserver in the test namespace
  ip netns add test
  ip netns exec test netserver

  # create a VETH pair
  ip link add name veth0 type veth peer name veth0 netns test
  ip link set veth0 up
  ip -n test link set veth0 up

  for i in $(seq 20 29); do
      # assign addresses to both ends
      ip addr add dev veth0 192.168.$i.1/24
      ip -n test addr add dev veth0 192.168.$i.2/24

      # start the traffic
      netperf -L 192.168.$i.1 -H 192.168.$i.2 -t UDP_STREAM -l 0 &
  done

  # wait
  send_data: data send error: No route to host (errno 113)
  netperf: send_omni: send_data failed: No route to host

We need to differentiate instead: if fragment reassembly time exceeded
is reported, we need to silently drop the packet,
if time to live exceeded is reported, maintain the current behaviour.
In both cases increment the related error count "icmpInTimeExcds".

While at it, fix a typo in a comment, and convert the if statement
into a switch to mate it more readable.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 net/ipv4/icmp.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 681e33998e03..3c1570d3e22f 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -782,7 +782,7 @@ static bool icmp_tag_validation(int proto)
 }
 
 /*
- *	Handle ICMP_DEST_UNREACH, ICMP_TIME_EXCEED, ICMP_QUENCH, and
+ *	Handle ICMP_DEST_UNREACH, ICMP_TIME_EXCEEDED, ICMP_QUENCH, and
  *	ICMP_PARAMETERPROB.
  */
 
@@ -810,7 +810,8 @@ static bool icmp_unreach(struct sk_buff *skb)
 	if (iph->ihl < 5) /* Mangled header, drop. */
 		goto out_err;
 
-	if (icmph->type == ICMP_DEST_UNREACH) {
+	switch (icmph->type) {
+	case ICMP_DEST_UNREACH:
 		switch (icmph->code & 15) {
 		case ICMP_NET_UNREACH:
 		case ICMP_HOST_UNREACH:
@@ -846,8 +847,16 @@ static bool icmp_unreach(struct sk_buff *skb)
 		}
 		if (icmph->code > NR_ICMP_UNREACH)
 			goto out;
-	} else if (icmph->type == ICMP_PARAMETERPROB)
+		break;
+	case ICMP_PARAMETERPROB:
 		info = ntohl(icmph->un.gateway) >> 24;
+		break;
+	case ICMP_TIME_EXCEEDED:
+		__ICMP_INC_STATS(net, ICMP_MIB_INTIMEEXCDS);
+		if (icmph->code == ICMP_EXC_FRAGTIME)
+			goto out;
+		break;
+	}
 
 	/*
 	 *	Throw it at our lower layers
-- 
2.13.6

^ permalink raw reply related

* Re: BUG:af_packet fails to TX TSO frames
From: Anton Ivanov @ 2017-10-12 14:12 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Network Development
In-Reply-To: <CAF=yD-+q_h4SE0E9RjE59i2FDYZQ_wV+L6RQjE9B_fszen-y0A@mail.gmail.com>



On 10/12/17 14:39, Willem de Bruijn wrote:
>> If I produce a real vnet frame out of a live kernel frame using
>> virtio_net_hdr_from_skb() and try to send it it fails on the check in
>> af_packet, while succeeding for tap. If I remove the af_packet check the
>> frame is accepted by the hardware too.
>>
>> If I produce it a synthetic frame + vnet header using the test program - it
>> works. Go figure.
> Besides looking at the raw frame bytes, also compare the setup
> of virtio_net_header, as well as the tcp checksum field. The stack
> expects the pseudo header to have already been calculated.

I am feeding it a skb which is coming up in the tx routine of a User 
Mode Linux device which is marked as NETIF_F_HW_CSUM and SG - that 
results in a skb with csum-ed headers, body set up for CSUM_PARTIAL and 
multiple fragments (always at least 1 more frag besides the TCP head).

That has everything in order as expected by virtio_net_hdr_from_skb and 
this is what I use to generate the vnet header. It works correctly for 
csum and GRO with af_packet and it works correctly for everything using 
a tap device. It fails only on GSO + af_packet TX.

What I am doing is the same thing virtio_net does - it just takes the 
output of virtio_net_hdr_from_skb and does nothing more. There should be 
no need to do anything more :(

It should just work.

Unless there is a gremlin somewhere in the machinery and that gremlin 
needs some light to be flushed out.
>
>> I am going to continue digging into it.
>>
>> At the very least I now have a positive test case which uses the same
>> semantics as my code so I have something to compare to.
> Glad to hear that the test is helpful. I wrote it because I
> have run into these exact same issues in the past.

It is. I have changes ready for it so it also supports vector IO, need 
to finish fighting with it.

A.

>

-- 
Anton R. Ivanov

Cambridge Greys Limited, England and Wales company No 10273661
http://www.cambridgegreys.com/

^ permalink raw reply

* Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions
From: Timur Tabi @ 2017-10-12 14:13 UTC (permalink / raw)
  To: David Laight, David S. Miller, netdev@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD00923AE@AcuExch.aculab.com>

On 10/12/17 4:30 AM, David Laight wrote:
> Isn't the memory allocated by a single kzalloc() call?

dma_alloc_coherenent, actually.

> IIRC that guarantees it doesn't cross a power or 2 boundary less than
> the size.

I'm pretty sure that kzalloc does not make that guarantee, and I don't 
think dma_alloc_coherent does either.

> So if you allocate any size between 4k and 8k it won't cross an odd
> 4k boundary (etc).
> 
> So these checks are entirely pointless.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply

* RFC(v2): Audit Kernel Container IDs
From: Richard Guy Briggs @ 2017-10-12 14:14 UTC (permalink / raw)
  To: cgroups, Linux Containers, Linux API, Linux Audit, Linux FS Devel,
	Linux Kernel, Linux Network Development
  Cc: Simo Sorce, Carlos O'Donell, Aristeu Rozanski, David Howells,
	Eric W. Biederman, Eric Paris, jlayton, Andy Lutomirski, mszeredi,
	Paul Moore, Serge E. Hallyn, Steve Grubb, trondmy, Al Viro

Containers are a userspace concept.  The kernel knows nothing of them.

The Linux audit system needs a way to be able to track the container
provenance of events and actions.  Audit needs the kernel's help to do
this.

Since the concept of a container is entirely a userspace concept, a
registration from the userspace container orchestration system initiates
this.  This will define a point in time and a set of resources
associated with a particular container with an audit container ID.

The registration is a pseudo filesystem (proc, since PID tree already
exists) write of a u8[16] UUID representing the container ID to a file
representing a process that will become the first process in a new
container.  This write might place restrictions on mount namespaces
required to define a container, or at least careful checking of
namespaces in the kernel to verify permissions of the orchestrator so it
can't change its own container ID.  A bind mount of nsfs may be
necessary in the container orchestrator's mntNS.
Note: Use a 128-bit scalar rather than a string to make compares faster
and simpler.

Require a new CAP_CONTAINER_ADMIN to be able to carry out the
registration.  At that time, record the target container's user-supplied
container identifier along with the target container's first process
(which may become the target container's "init" process) process ID
(referenced from the initial PID namespace), all namespace IDs (in the
form of a nsfs device number and inode number tuple) in a new auxilliary
record AUDIT_CONTAINER with a qualifying op=$action field.

Issue a new auxilliary record AUDIT_CONTAINER_INFO for each valid
container ID present on an auditable action or event.

Forked and cloned processes inherit their parent's container ID,
referenced in the process' task_struct.

Mimic setns(2) and return an error if the process has already initiated
threading or forked since this registration should happen before the
process execution is started by the orchestrator and hence should not
yet have any threads or children.  If this is deemed overly restrictive,
switch all threads and children to the new containerID.

Trust the orchestrator to judiciously use and restrict CAP_CONTAINER_ADMIN.

Log the creation of every namespace, inheriting/adding its spawning
process' containerID(s), if applicable.  Include the spawning and
spawned namespace IDs (device and inode number tuples).
[AUDIT_NS_CREATE, AUDIT_NS_DESTROY] [clone(2), unshare(2), setns(2)]
Note: At this point it appears only network namespaces may need to track
container IDs apart from processes since incoming packets may cause an
auditable event before being associated with a process.

Log the destruction of every namespace when it is no longer used by any
process, include the namespace IDs (device and inode number tuples).
[AUDIT_NS_DESTROY] [process exit, unshare(2), setns(2)]

Issue a new auxilliary record AUDIT_NS_CHANGE listing (opt: op=$action)
the parent and child namespace IDs for any changes to a process'
namespaces. [setns(2)]
Note: It may be possible to combine AUDIT_NS_* record formats and
distinguish them with an op=$action field depending on the fields
required for each message type.

When a container ceases to exist because the last process in that
container has exited and hence the last namespace has been destroyed and
its refcount dropping to zero, log the fact.
(This latter is likely needed for certification accountability.)  A
container object may need a list of processes and/or namespaces.

A namespace cannot directly migrate from one container to another but
could be assigned to a newly spawned container.  A namespace can be
moved from one container to another indirectly by having that namespace
used in a second process in another container and then ending all the
processes in the first container.

(v2)
- switch from u64 to u128 UUID
- switch from "signal" and "trigger" to "register"
- restrict registration to single process or force all threads and children into same container

- RGB

^ permalink raw reply

* Re: [RFC 1/3] devlink: Add config parameter get/set operations
From: Jiri Pirko @ 2017-10-12 14:15 UTC (permalink / raw)
  To: Steve Lin; +Cc: netdev, jiri, davem, michael.chan, linux-pci, linville, gospo
In-Reply-To: <1507815262-33294-2-git-send-email-steven.lin1@broadcom.com>

Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>+
>+	/* When config doesn't take effect until next reboot (config
>+	 * just changed NVM which isn't read until boot, for example),
>+	 * this attribute should be set by the driver.
>+	 */
>+	DEVLINK_ATTR_RESTART_REQUIRED,		/* u8 */
>+

I think it would be nice to return this information as a part of retply
to set message - extack

Also, we need to expose to the user the original value (currently being
used) and the new one (to be used after driver re-instatiation)

^ permalink raw reply

* Re: [PATCH net-next v2 1/1] bridge: return error code when deleting Vlan
From: David Ahern @ 2017-10-12 14:19 UTC (permalink / raw)
  To: Roman Mashak, davem; +Cc: stephen, netdev
In-Reply-To: <1507816314-2896-1-git-send-email-mrv@mojatatu.com>

On 10/12/17 7:51 AM, Roman Mashak wrote:
> v2:
>  Return err immediately if nbp_vlan_delete() fails (pointed by David Ahern)
> 
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
> ---
>  net/bridge/br_netlink.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index f0e8268..1efdd48 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -527,11 +527,13 @@ static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
>  
>  	case RTM_DELLINK:
>  		if (p) {
> -			nbp_vlan_delete(p, vinfo->vid);
> +			err = nbp_vlan_delete(p, vinfo->vid);
> +			if (err)
> +				break;

I'm not sure a break is the right thing to do. Seems like you leave it
in a half configured state.

>  			if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
> -				br_vlan_delete(p->br, vinfo->vid);
> +				err = br_vlan_delete(p->br, vinfo->vid);
>  		} else {
> -			br_vlan_delete(br, vinfo->vid);
> +			err = br_vlan_delete(br, vinfo->vid);
>  		}
>  		break;
>  	}
> 

Why do you want to return the error code here? Walking the code paths
seems like ENOENT or err from switchdev_port_obj_del are the 2 error
possibilities.

^ permalink raw reply

* Re: [RFC 0/3] Adding config get/set to devlink
From: Roopa Prabhu @ 2017-10-12 14:35 UTC (permalink / raw)
  To: Steve Lin
  Cc: netdev@vger.kernel.org, Jiri Pirko, davem@davemloft.net,
	michael.chan, linux-pci, John W. Linville, gospo
In-Reply-To: <1507815262-33294-1-git-send-email-steven.lin1@broadcom.com>

On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
> Adds a devlink command for getting & setting device configuration
> parameters, and enumerates a bunch of those parameters as devlink
> attributes.  Also introduces an attribute that can be set by a
> driver to indicate that the config change doesn't take effect
> until the next restart (as in the case of the bnxt driver changes
> in this patchset, for which all the configuration changes affect NVM
> only, and aren't loaded until the next restart.)
>
> bnxt driver patches make use of these new devlink cmds/attributes.
>
> Steve Lin (3):
>   devlink: Add config parameter get/set operations
>   bnxt: Move generic devlink code to new file
>   bnxt: Add devlink support for config get/set
>

Is the goal here to move all ethtool operations to devlink (I saw some
attrs related to speed etc). ?.
We do need to move ethtool attrs to netlink and devlink is a good
place (and of-course leave the current ethtool api around for backward
compatibility).

^ permalink raw reply

* Re: [RFC 1/3] devlink: Add config parameter get/set operations
From: Steve Lin @ 2017-10-12 14:37 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: netdev, jiri, David S . Miller, Michael Chan, linux-pci,
	John Linville, Andy Gospodarek
In-Reply-To: <20171012140317.GC14672@nanopsycho>

Jiri,

Thanks for your feedback below and in your other response.  I will
make some changes to address those issues and resubmit.

Thanks again!
Steve

On Thu, Oct 12, 2017 at 10:03 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 03:34:20PM CEST, steven.lin1@broadcom.com wrote:
>>Add support for config parameter get/set commands. Initially used by
>>bnxt driver, but other drivers can use the same, or new, attributes.
>>The config_get() and config_set() operations operate as expected, but
>>note that the driver implementation of the config_set() operation can
>>indicate whether a restart is necessary for the setting to take
>>effect.
>>
>
> First of all, I like this approach.
>
> I would like to see this patch split into:
> 1) config-options infrastructure introduction
> 2) specific config options introductions - would be best to have it
>    per-option. We need to make sure every option is very well described
>    and explained usecases. This is needed in order vendors to share
>    attributes among drivers.
>
> More nits inlined.
>
>
>>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>>---
>> include/net/devlink.h        |   4 +
>> include/uapi/linux/devlink.h | 108 ++++++++++++++++++++++
>> net/core/devlink.c           | 207 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 319 insertions(+)
>>
>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>index b9654e1..952966c 100644
>>--- a/include/net/devlink.h
>>+++ b/include/net/devlink.h
>>@@ -270,6 +270,10 @@ struct devlink_ops {
>>       int (*eswitch_inline_mode_set)(struct devlink *devlink, u8 inline_mode);
>>       int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
>>       int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode);
>>+      int (*config_get)(struct devlink *devlink, enum devlink_attr attr,
>>+                        u32 *value);
>>+      int (*config_set)(struct devlink *devlink, enum devlink_attr attr,
>>+                        u32 value, u8 *restart_reqd);
>> };
>>
>> static inline void *devlink_priv(struct devlink *devlink)
>>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>index 0cbca96..e959716 100644
>>--- a/include/uapi/linux/devlink.h
>>+++ b/include/uapi/linux/devlink.h
>>@@ -70,6 +70,9 @@ enum devlink_command {
>>       DEVLINK_CMD_DPIPE_HEADERS_GET,
>>       DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
>>
>>+      DEVLINK_CMD_CONFIG_GET,
>>+      DEVLINK_CMD_CONFIG_SET,
>>+
>>       /* add new commands above here */
>>       __DEVLINK_CMD_MAX,
>>       DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>>@@ -124,6 +127,68 @@ enum devlink_eswitch_encap_mode {
>>       DEVLINK_ESWITCH_ENCAP_MODE_BASIC,
>> };
>>
>>+enum devlink_dcbx_mode {
>>+      DEVLINK_DCBX_MODE_DISABLED,
>>+      DEVLINK_DCBX_MODE_IEEE,
>>+      DEVLINK_DCBX_MODE_CEE,
>>+      DEVLINK_DCBX_MODE_IEEE_CEE,
>>+};
>>+
>>+enum devlink_multifunc_mode {
>>+      DEVLINK_MULTIFUNC_MODE_ALLOWED,         /* Ext switch activates MF */
>>+      DEVLINK_MULTIFUNC_MODE_FORCE_SINGFUNC,
>>+      DEVLINK_MULTIFUNC_MODE_NPAR10,          /* NPAR 1.0 */
>>+      DEVLINK_MULTIFUNC_MODE_NPAR15,          /* NPAR 1.5 */
>>+      DEVLINK_MULTIFUNC_MODE_NPAR20,          /* NPAR 2.0 */
>>+};
>>+
>>+enum devlink_autoneg_protocol {
>>+      DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_BAM,
>>+      DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY_CONSORTIUM,
>>+      DEVLINK_AUTONEG_PROTOCOL_IEEE8023BY,
>>+      DEVLINK_AUTONEG_PROTOCOL_BAM,           /* Broadcom Autoneg Mode */
>>+      DEVLINK_AUTONEG_PROTOCOL_CONSORTIUM,    /* Consortium Autoneg Mode */
>>+};
>>+
>>+enum devlink_pre_os_link_speed {
>>+      DEVLINK_PRE_OS_LINK_SPEED_AUTONEG,
>>+      DEVLINK_PRE_OS_LINK_SPEED_1G,
>>+      DEVLINK_PRE_OS_LINK_SPEED_10G,
>>+      DEVLINK_PRE_OS_LINK_SPEED_25G,
>>+      DEVLINK_PRE_OS_LINK_SPEED_40G,
>>+      DEVLINK_PRE_OS_LINK_SPEED_50G,
>>+      DEVLINK_PRE_OS_LINK_SPEED_100G,
>>+      DEVLINK_PRE_OS_LINK_SPEED_5G = 0xe,
>>+      DEVLINK_PRE_OS_LINK_SPEED_100M = 0xf,
>>+};
>>+
>>+enum devlink_mba_boot_type {
>>+      DEVLINK_MBA_BOOT_TYPE_AUTO_DETECT,
>>+      DEVLINK_MBA_BOOT_TYPE_BBS,              /* BIOS Boot Specification */
>>+      DEVLINK_MBA_BOOT_TYPE_INTR18,           /* Hook interrupt 0x18 */
>>+      DEVLINK_MBA_BOOT_TYPE_INTR19,           /* Hook interrupt 0x19 */
>>+};
>>+
>>+enum devlink_mba_setup_hot_key {
>>+      DEVLINK_MBA_SETUP_HOT_KEY_CTRL_S,
>>+      DEVLINK_MBA_SETUP_HOT_KEY_CTRL_B,
>>+};
>>+
>>+enum devlink_mba_boot_protocol {
>>+      DEVLINK_MBA_BOOT_PROTOCOL_PXE,
>>+      DEVLINK_MBA_BOOT_PROTOCOL_ISCSI,
>>+      DEVLINK_MBA_BOOT_PROTOCOL_NONE = 0x7,
>>+};
>>+
>>+enum devlink_mba_link_speed {
>>+      DEVLINK_MBA_LINK_SPEED_AUTONEG,
>>+      DEVLINK_MBA_LINK_SPEED_1G,
>>+      DEVLINK_MBA_LINK_SPEED_10G,
>>+      DEVLINK_MBA_LINK_SPEED_25G,
>>+      DEVLINK_MBA_LINK_SPEED_40G,
>>+      DEVLINK_MBA_LINK_SPEED_50G,
>>+};
>>+
>> enum devlink_attr {
>>       /* don't change the order or add anything between, this is ABI! */
>>       DEVLINK_ATTR_UNSPEC,
>>@@ -202,6 +267,49 @@ enum devlink_attr {
>>
>>       DEVLINK_ATTR_ESWITCH_ENCAP_MODE,        /* u8 */
>>
>>+      /* Configuration Parameters */
>>+      DEVLINK_ATTR_SRIOV_ENABLED,             /* u8 */
>>+      DEVLINK_ATTR_NUM_VF_PER_PF,             /* u32 */
>>+      DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT,      /* u32 */
>>+      DEVLINK_ATTR_MSIX_VECTORS_PER_VF,       /* u32 */
>>+      DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT,      /* u32 */
>>+      DEVLINK_ATTR_NPAR_BW_IN_PERCENT,        /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_RESERVATION,       /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID, /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_LIMIT,             /* u8 */
>>+      DEVLINK_ATTR_NPAR_BW_LIMIT_VALID,       /* u8 */
>>+      DEVLINK_ATTR_DCBX_MODE,                 /* u8 */
>>+      DEVLINK_ATTR_RDMA_ENABLED,              /* u8 */
>>+      DEVLINK_ATTR_MULTIFUNC_MODE,            /* u8 */
>>+      DEVLINK_ATTR_SECURE_NIC_ENABLED,        /* u8 */
>>+      DEVLINK_ATTR_IGNORE_ARI_CAPABILITY,     /* u8 */
>>+      DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED,       /* u8 */
>>+      DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED,       /* u8 */
>>+      DEVLINK_ATTR_PME_CAPABILITY_ENABLED,    /* u8 */
>>+      DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED,  /* u8 */
>>+      DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED,      /* u8 */
>>+      DEVLINK_ATTR_AUTONEG_PROTOCOL,          /* u8 */
>>+      DEVLINK_ATTR_MEDIA_AUTO_DETECT,         /* u8 */
>>+      DEVLINK_ATTR_PHY_SELECT,                /* u8 */
>>+      DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0,      /* u8 */
>>+      DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3,      /* u8 */
>>+      DEVLINK_ATTR_MBA_ENABLED,               /* u8 */
>>+      DEVLINK_ATTR_MBA_BOOT_TYPE,             /* u8 */
>>+      DEVLINK_ATTR_MBA_DELAY_TIME,            /* u32 */
>>+      DEVLINK_ATTR_MBA_SETUP_HOT_KEY,         /* u8 */
>>+      DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT,     /* u8 */
>>+      DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT,      /* u32 */
>>+      DEVLINK_ATTR_MBA_VLAN_ENABLED,          /* u8 */
>>+      DEVLINK_ATTR_MBA_VLAN_TAG,              /* u16 */
>>+      DEVLINK_ATTR_MBA_BOOT_PROTOCOL,         /* u8 */
>>+      DEVLINK_ATTR_MBA_LINK_SPEED,            /* u8 */
>
> Okay, I think it is about the time we should start thinking about
> putting this new config attributes under nester attribute. What do you
> think?
>
>
>>+
>>+      /* When config doesn't take effect until next reboot (config
>>+       * just changed NVM which isn't read until boot, for example),
>>+       * this attribute should be set by the driver.
>>+       */
>>+      DEVLINK_ATTR_RESTART_REQUIRED,          /* u8 */
>>+
>>       /* add new attributes above here, update the policy in devlink.c */
>>
>>       __DEVLINK_ATTR_MAX,
>>diff --git a/net/core/devlink.c b/net/core/devlink.c
>>index 7d430c1..701c84b 100644
>>--- a/net/core/devlink.c
>>+++ b/net/core/devlink.c
>>@@ -1566,6 +1566,164 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
>>       return 0;
>> }
>>
>>+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>>+
>>+static int devlink_nl_config_fill(struct sk_buff *msg,
>>+                                struct devlink *devlink,
>>+                                enum devlink_command cmd,
>>+                                struct genl_info *info)
>>+{
>>+      const struct devlink_ops *ops = devlink->ops;
>>+      void *hdr;
>>+      int err;
>>+      enum devlink_attr attr = -1;
>
> -1 is not a valid enum value. Better to just have bool to indicate attr
> was found or not.
>
>
>>+      u32 value;
>>+      int i;
>>+      struct nla_policy policy;
>>+      u8 restart_reqd;
>>+
>>+      if (!ops->config_get)
>>+              return -EOPNOTSUPP;
>>+
>>+      hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>>+                        &devlink_nl_family, 0, cmd);
>>+      if (!hdr) {
>>+              err = -EMSGSIZE;
>>+              goto nla_msg_failure;
>>+      }
>>+
>>+      err = devlink_nl_put_handle(msg, devlink);
>>+      if (err)
>>+              goto nla_put_failure;
>>+
>>+      for (i = 0; i < DEVLINK_ATTR_MAX; i++)
>>+              if (info->attrs[i])
>>+                      attr = i;
>>+
>>+      if (attr == -1) {
>>+              /* Not found - invalid/unknown attribute? */
>>+              err = -EINVAL;
>>+              goto nla_put_failure;
>>+      }
>>+
>>+      policy = devlink_nl_policy[attr];
>>+
>>+      if (cmd == DEVLINK_CMD_CONFIG_GET) {
>>+              err = ops->config_get(devlink, attr, &value);
>>+
>>+              if (err)
>>+                      goto nla_put_failure;
>>+
>>+              switch (policy.type) {
>>+              case NLA_U8:
>>+                      err = nla_put_u8(msg, attr, value);
>>+                      break;
>>+              case NLA_U16:
>>+                      err = nla_put_u16(msg, attr, value);
>>+                      break;
>>+              case NLA_U32:
>>+                      err = nla_put_u32(msg, attr, value);
>>+                      break;
>>+              default:
>>+                      goto nla_put_failure;
>>+              }
>>+
>>+              if (err)
>>+                      goto nla_put_failure;
>>+      } else {
>>+              /* Must be config_set command */
>>+              u8 *val8;
>>+              u16 *val16;
>>+              u32 *val32;
>>+
>>+              switch (policy.type) {
>>+              case NLA_U8:
>>+                      val8 = nla_data(info->attrs[attr]);
>>+                      value = *val8;
>>+                      break;
>>+              case NLA_U16:
>>+                      val16 = nla_data(info->attrs[attr]);
>>+                      value = *val16;
>>+                      break;
>>+              case NLA_U32:
>>+                      val32 = nla_data(info->attrs[attr]);
>>+                      value = *val32;
>>+                      break;
>>+              default:
>>+                      goto nla_put_failure;
>>+              }
>>+
>>+              err = ops->config_set(devlink, attr, value, &restart_reqd);
>>+              if (err)
>>+                      goto nla_put_failure;
>>+
>>+              if (restart_reqd) {
>>+                      err = nla_put_u8(msg, DEVLINK_ATTR_RESTART_REQUIRED,
>>+                                       restart_reqd);
>>+                      if (err)
>>+                              goto nla_put_failure;
>>+              }
>>+      }
>>+
>>+      genlmsg_end(msg, hdr);
>>+      return 0;
>>+
>>+nla_put_failure:
>>+      genlmsg_cancel(msg, hdr);
>>+nla_msg_failure:
>>+      return err;
>>+}
>>+
>>+static int devlink_nl_cmd_config_get_doit(struct sk_buff *skb,
>>+                                        struct genl_info *info)
>>+{
>>+      struct devlink *devlink = info->user_ptr[0];
>>+      struct sk_buff *msg;
>>+      int err;
>>+
>>+      if (!devlink->ops)
>>+              return -EOPNOTSUPP;
>>+
>>+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>+      if (!msg)
>>+              return -ENOMEM;
>>+
>>+      err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_GET,
>>+                                   info);
>>+
>>+      if (err) {
>>+              nlmsg_free(msg);
>>+              return err;
>>+      }
>>+
>>+      return genlmsg_reply(msg, info);
>>+}
>>+
>>+static int devlink_nl_cmd_config_set_doit(struct sk_buff *skb,
>>+                                        struct genl_info *info)
>>+{
>>+      struct devlink *devlink = info->user_ptr[0];
>>+      struct sk_buff *msg;
>>+      int err;
>>+
>>+      if (!devlink->ops)
>>+              return -EOPNOTSUPP;
>>+
>>+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>+      if (!msg)
>>+              return -ENOMEM;
>>+
>>+      err = devlink_nl_config_fill(msg, devlink, DEVLINK_CMD_CONFIG_SET,
>>+                                   info);
>
> This is odd. You are using "fill" function for "set". It should be used
> for "get". But for set, it really sounds odd. Please split the
> devlink_nl_config_fill function into get and set parts.
>
> Also, get should dump all available options.
>
>
>>+
>>+      if (err) {
>>+              nlmsg_free(msg);
>>+              return err;
>>+      }
>>+
>>+      return genlmsg_reply(msg, info);
>>+}
>>+
>> int devlink_dpipe_match_put(struct sk_buff *skb,
>>                           struct devlink_dpipe_match *match)
>> {
>>@@ -2291,6 +2449,41 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
>>       [DEVLINK_ATTR_ESWITCH_ENCAP_MODE] = { .type = NLA_U8 },
>>       [DEVLINK_ATTR_DPIPE_TABLE_NAME] = { .type = NLA_NUL_STRING },
>>       [DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_SRIOV_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_NUM_VF_PER_PF] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MAX_NUM_PF_MSIX_VECT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MSIX_VECTORS_PER_VF] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_NUM_PARTITIONS_PER_PORT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_IN_PERCENT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_RESERVATION] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_RESERVATION_VALID] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_NPAR_BW_LIMIT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_NPAR_BW_LIMIT_VALID] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_DCBX_MODE] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_RDMA_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MULTIFUNC_MODE] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_SECURE_NIC_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_IGNORE_ARI_CAPABILITY] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_LLDP_NEAREST_BRIDGE_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_LLDP_NEAREST_NONTPMR_BRIDGE_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_PME_CAPABILITY_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MAGIC_PACKET_WOL_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_EEE_PWR_SAVE_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_AUTONEG_PROTOCOL] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MEDIA_AUTO_DETECT] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_PHY_SELECT] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_PRE_OS_LINK_SPEED_D0] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_PRE_OS_LINK_SPEED_D3] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MBA_BOOT_TYPE] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_DELAY_TIME] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_SETUP_HOT_KEY] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_HIDE_SETUP_PROMPT] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MBA_BOOT_RETRY_COUNT] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_VLAN_ENABLED] = { .type = NLA_U8 },
>>+      [DEVLINK_ATTR_MBA_VLAN_TAG] = { .type = NLA_U16 },
>>+      [DEVLINK_ATTR_MBA_BOOT_PROTOCOL] = { .type = NLA_U32 },
>>+      [DEVLINK_ATTR_MBA_LINK_SPEED] = { .type = NLA_U32 },
>> };
>>
>> static const struct genl_ops devlink_nl_ops[] = {
>>@@ -2451,6 +2644,20 @@ static const struct genl_ops devlink_nl_ops[] = {
>>               .flags = GENL_ADMIN_PERM,
>>               .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>       },
>>+      {
>>+              .cmd = DEVLINK_CMD_CONFIG_GET,
>>+              .doit = devlink_nl_cmd_config_get_doit,
>>+              .policy = devlink_nl_policy,
>>+              .flags = GENL_ADMIN_PERM,
>>+              .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>+      },
>>+      {
>>+              .cmd = DEVLINK_CMD_CONFIG_SET,
>>+              .doit = devlink_nl_cmd_config_set_doit,
>>+              .policy = devlink_nl_policy,
>>+              .flags = GENL_ADMIN_PERM,
>>+              .internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>>+      },
>> };
>>
>> static struct genl_family devlink_nl_family __ro_after_init = {
>>--
>>2.7.4
>>

^ permalink raw reply

* Re: [RFC 0/3] Adding config get/set to devlink
From: Jiri Pirko @ 2017-10-12 14:40 UTC (permalink / raw)
  To: Roopa Prabhu
  Cc: Steve Lin, netdev@vger.kernel.org, Jiri Pirko,
	davem@davemloft.net, michael.chan, linux-pci, John W. Linville,
	gospo
In-Reply-To: <CAJieiUjuKtmYDZ5-7UjYkWFJuMGu2gajhziLQKAZLTz6g63f2A@mail.gmail.com>

Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>> Adds a devlink command for getting & setting device configuration
>> parameters, and enumerates a bunch of those parameters as devlink
>> attributes.  Also introduces an attribute that can be set by a
>> driver to indicate that the config change doesn't take effect
>> until the next restart (as in the case of the bnxt driver changes
>> in this patchset, for which all the configuration changes affect NVM
>> only, and aren't loaded until the next restart.)
>>
>> bnxt driver patches make use of these new devlink cmds/attributes.
>>
>> Steve Lin (3):
>>   devlink: Add config parameter get/set operations
>>   bnxt: Move generic devlink code to new file
>>   bnxt: Add devlink support for config get/set
>>
>
>Is the goal here to move all ethtool operations to devlink (I saw some
>attrs related to speed etc). ?.
>We do need to move ethtool attrs to netlink and devlink is a good
>place (and of-course leave the current ethtool api around for backward
>compatibility).

We need to make sure we are not moving things to devlink which don't
belong there. All options that use "netdev" as a handle should go into
rtnetlink instead.

^ permalink raw reply

* Re: [RFC 0/3] Adding config get/set to devlink
From: Steve Lin @ 2017-10-12 14:45 UTC (permalink / raw)
  To: Roopa Prabhu
  Cc: netdev@vger.kernel.org, Jiri Pirko, davem@davemloft.net,
	Michael Chan, linux-pci, John W. Linville, Andy Gospodarek
In-Reply-To: <CAJieiUjuKtmYDZ5-7UjYkWFJuMGu2gajhziLQKAZLTz6g63f2A@mail.gmail.com>

Hi Roopa,

The attributes added in this patchset are not really the same type as
ethtool - these are more device configuration type attributes.  The
speeds you saw, for example, affect the pre-OS [i.e. PXE boot time]
configuration for a port, and aren't run-time speed changes on a given
netdev like ethtool configures.  As Jiri mentioned, I will add some
comments to better describe each of the attributes.

So I don't think there's much duplication here with ethtool.

That said, there also shouldn't be anything in the patchset that would
preclude some future migration of ethtool settings to using devlink or
rtnetlink API.

Steve

On Thu, Oct 12, 2017 at 10:35 AM, Roopa Prabhu
<roopa@cumulusnetworks.com> wrote:
> On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>> Adds a devlink command for getting & setting device configuration
>> parameters, and enumerates a bunch of those parameters as devlink
>> attributes.  Also introduces an attribute that can be set by a
>> driver to indicate that the config change doesn't take effect
>> until the next restart (as in the case of the bnxt driver changes
>> in this patchset, for which all the configuration changes affect NVM
>> only, and aren't loaded until the next restart.)
>>
>> bnxt driver patches make use of these new devlink cmds/attributes.
>>
>> Steve Lin (3):
>>   devlink: Add config parameter get/set operations
>>   bnxt: Move generic devlink code to new file
>>   bnxt: Add devlink support for config get/set
>>
>
> Is the goal here to move all ethtool operations to devlink (I saw some
> attrs related to speed etc). ?.
> We do need to move ethtool attrs to netlink and devlink is a good
> place (and of-course leave the current ethtool api around for backward
> compatibility).

^ permalink raw reply

* Re: [RFC 0/3] Adding config get/set to devlink
From: Roopa Prabhu @ 2017-10-12 14:46 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Steve Lin, netdev@vger.kernel.org, Jiri Pirko,
	davem@davemloft.net, michael.chan, linux-pci, John W. Linville,
	gospo
In-Reply-To: <20171012144032.GG14672@nanopsycho>

On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>> Adds a devlink command for getting & setting device configuration
>>> parameters, and enumerates a bunch of those parameters as devlink
>>> attributes.  Also introduces an attribute that can be set by a
>>> driver to indicate that the config change doesn't take effect
>>> until the next restart (as in the case of the bnxt driver changes
>>> in this patchset, for which all the configuration changes affect NVM
>>> only, and aren't loaded until the next restart.)
>>>
>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>
>>> Steve Lin (3):
>>>   devlink: Add config parameter get/set operations
>>>   bnxt: Move generic devlink code to new file
>>>   bnxt: Add devlink support for config get/set
>>>
>>
>>Is the goal here to move all ethtool operations to devlink (I saw some
>>attrs related to speed etc). ?.
>>We do need to move ethtool attrs to netlink and devlink is a good
>>place (and of-course leave the current ethtool api around for backward
>>compatibility).
>
> We need to make sure we are not moving things to devlink which don't
> belong there. All options that use "netdev" as a handle should go into
> rtnetlink instead.
>

Any reason you want to keep that restriction ?.
FWIS, devlink is a driver api just like ethtool is.
and ethtool needs to move to netlink soon...and It would be better to
not put the rtnl_lock burden on ethtool driver operations. Instead of
adding yet another driver api, extending devlink seems like a great
fit to me.

^ permalink raw reply

* Re: [RFC 0/3] Adding config get/set to devlink
From: Roopa Prabhu @ 2017-10-12 14:51 UTC (permalink / raw)
  To: Steve Lin
  Cc: netdev@vger.kernel.org, Jiri Pirko, davem@davemloft.net,
	Michael Chan, linux-pci, John W. Linville, Andy Gospodarek
In-Reply-To: <CA+Jmh7GOo4K8gpyhQ8kceFtpNuASp89WYi0NR8AaoXncTCAksQ@mail.gmail.com>

On Thu, Oct 12, 2017 at 7:45 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
> Hi Roopa,
>
> The attributes added in this patchset are not really the same type as
> ethtool - these are more device configuration type attributes.  The
> speeds you saw, for example, affect the pre-OS [i.e. PXE boot time]
> configuration for a port, and aren't run-time speed changes on a given
> netdev like ethtool configures.  As Jiri mentioned, I will add some
> comments to better describe each of the attributes.
>
> So I don't think there's much duplication here with ethtool.
>
> That said, there also shouldn't be anything in the patchset that would
> preclude some future migration of ethtool settings to using devlink or
> rtnetlink API.
>

ok, ack, thanks for the clarification. Just trying to find a best
netlink place for future ethtool migration to netlink.

^ permalink raw reply

* Re: [RFC] Support for UNARP (RFC 1868)
From: Eric Dumazet @ 2017-10-12 14:57 UTC (permalink / raw)
  To: Girish Moodalbail; +Cc: netdev, davem, kuznet
In-Reply-To: <1507782977-2443-1-git-send-email-girish.moodalbail@oracle.com>

On Wed, 2017-10-11 at 21:36 -0700, Girish Moodalbail wrote:
> Add support for UNARP, as detailed in the IETF RFC 1868 (ARP Extension -
> UNARP). The central idea here is for a node to announce that it is
> leaving the network and that all the nodes on the L2 broadcast domain to
> update their ARP tables accordingly (i.e., mark the neighbor entry state
> to FAILED). Even though the ARP timers on nodes would eventually  mark
> such entries as FAILED it will be more robust if those entries gets
> marked FAILED sooner with the help from the host that is going away.
> 
> Besides providing a solution for an usecase, as captured in RFC, of an
> IP address moving across a proxy server, this feature is even more
> important for certain use cases in the Cloud. Imagine a tenant who is
> bringing up and down VM instances for some workload of theirs. If these
> instances are part of a small subnet, then the new VM instances may be
> assigned the same IP address (since the subnet pool is small) but with a
> different MAC address. So, if there is a client which has a stale
> mapping of the IP address to the old MAC address, then that client will
> fail to communicate with the new VM instance for some time.
> 
> Another usecase that comes to mind is that of the Live VM
> Migration. Imagine a client that is communicating with a VM. Now, let us
> migrate this VM to a destination machine. The IP address to MAC address
> mapping for a VM doesn't change after the Live Migration. However, there
> will be a small amount of time (till the VM sends gratuitous ARP from
> the destination machine) during which packets from a client will be
> forwarded to the source machine. This occurs because:
> 
>  - the ARP entry in the client is not invalidated yet and it continues
>    to use the same MAC address and
> 
>  - the MAC address table of all of the intermediate switches between the
>    client and the source machine are not updated yet for the MAC address
>    move.
> 
> This issue of forwarding the packets to wrong target could be avoided by
> sending UNARP packets from the source machine. This would invalidate the
> ARP entry on the client and forces it to resolve the IP address again by
> broadcasting an ARP request to the network. The VM on the destination
> machine would then respond back with an ARP response. The ARP response
> back from the VM should also clean up the MAC address table of the
> intermediate switches.
> 
> The following changes implements the UNARP receive processing in the
> kernel. Once the changes are in the kernel, arping(8) program can be
> updated to send UNARP packets.
> 
> Any Thoughts/Comments?
> 
> Signed-off-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> ---

Hi Girish

Your description (or patch title) is misleading. You apparently
implement the receive side of the RFC.

And the RFC had Proxy ARP in mind.

What about security implications ? Will TCP flows be terminated, instead
of being smoothly migrated (TCP_REPAIR)

What about IPv6 ? Or maybe more abruptly, do we still need to add
features to IPv4 in 2017,  22 years after this RFC came ? ;)

Thanks.

^ permalink raw reply

* Re: [PATCH] netconsole: make config_item_type const
From: Bhumika Goyal @ 2017-10-12 15:04 UTC (permalink / raw)
  To: Julia Lawall, David Miller, akpm, nab, Linux Kernel Mailing List,
	netdev
  Cc: Bhumika Goyal
In-Reply-To: <1507811352-9962-1-git-send-email-bhumirks@gmail.com>

On Thu, Oct 12, 2017 at 2:29 PM, Bhumika Goyal <bhumirks@gmail.com> wrote:
> This is a followup patch for: https://lkml.org/lkml/2017/10/11/375 and
> https://patchwork.kernel.org/patch/9999649/
>
> Make these structures const as they are either passed to the functions
> having the argument as const or stored as a reference in the "ci_type"
> const field of a config_item structure.
>
> Done using Coccienlle.
>

Actually, this patch is dependent on the patches in the links
https://lkml.org/lkml/2017/10/11/375 and
https://patchwork.kernel.org/patch/9999649/. Therefore, this patch
won't be correct unless the patches in these links gets applied.

> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> ---
>  drivers/net/netconsole.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 0e27920..be9aa36 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -616,7 +616,7 @@ static void netconsole_target_release(struct config_item *item)
>         .release                = netconsole_target_release,
>  };
>
> -static struct config_item_type netconsole_target_type = {
> +static const struct config_item_type netconsole_target_type = {
>         .ct_attrs               = netconsole_target_attrs,
>         .ct_item_ops            = &netconsole_target_item_ops,
>         .ct_owner               = THIS_MODULE,
> @@ -682,7 +682,7 @@ static void drop_netconsole_target(struct config_group *group,
>         .drop_item      = drop_netconsole_target,
>  };
>
> -static struct config_item_type netconsole_subsys_type = {
> +static const struct config_item_type netconsole_subsys_type = {
>         .ct_group_ops   = &netconsole_subsys_group_ops,
>         .ct_owner       = THIS_MODULE,
>  };
> --
> 1.9.1
>

^ permalink raw reply

* Re: [RFC 0/3] Adding config get/set to devlink
From: Jiri Pirko @ 2017-10-12 15:04 UTC (permalink / raw)
  To: Roopa Prabhu
  Cc: Steve Lin, netdev@vger.kernel.org, Jiri Pirko,
	davem@davemloft.net, michael.chan, linux-pci, John W. Linville,
	gospo
In-Reply-To: <CAJieiUiFmBr72yYP9VNtm0VBTNjJcA6Lj5nHJNBjDBC4moxE2A@mail.gmail.com>

Thu, Oct 12, 2017 at 04:46:24PM CEST, roopa@cumulusnetworks.com wrote:
>On Thu, Oct 12, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Thu, Oct 12, 2017 at 04:35:10PM CEST, roopa@cumulusnetworks.com wrote:
>>>On Thu, Oct 12, 2017 at 6:34 AM, Steve Lin <steven.lin1@broadcom.com> wrote:
>>>> Adds a devlink command for getting & setting device configuration
>>>> parameters, and enumerates a bunch of those parameters as devlink
>>>> attributes.  Also introduces an attribute that can be set by a
>>>> driver to indicate that the config change doesn't take effect
>>>> until the next restart (as in the case of the bnxt driver changes
>>>> in this patchset, for which all the configuration changes affect NVM
>>>> only, and aren't loaded until the next restart.)
>>>>
>>>> bnxt driver patches make use of these new devlink cmds/attributes.
>>>>
>>>> Steve Lin (3):
>>>>   devlink: Add config parameter get/set operations
>>>>   bnxt: Move generic devlink code to new file
>>>>   bnxt: Add devlink support for config get/set
>>>>
>>>
>>>Is the goal here to move all ethtool operations to devlink (I saw some
>>>attrs related to speed etc). ?.
>>>We do need to move ethtool attrs to netlink and devlink is a good
>>>place (and of-course leave the current ethtool api around for backward
>>>compatibility).
>>
>> We need to make sure we are not moving things to devlink which don't
>> belong there. All options that use "netdev" as a handle should go into
>> rtnetlink instead.
>>
>
>Any reason you want to keep that restriction ?.
>FWIS, devlink is a driver api just like ethtool is.
>and ethtool needs to move to netlink soon...and It would be better to
>not put the rtnl_lock burden on ethtool driver operations. Instead of
>adding yet another driver api, extending devlink seems like a great
>fit to me.

Hmm, the original purpose of devlink was to obtain iface for things that
could not use "netdev" as a handle. I try to stick with it as we already
have iface for things that could use "netdev" as a handle - rtnetlink.

Not sure we want to go this way and add "netdev"-handle things into
devlink. Thoughts?

^ permalink raw reply

* [PATCH net-next 0/2] net: stmmac: Improvements for multi-queuing and for AVB
From: Jose Abreu @ 2017-10-12 15:14 UTC (permalink / raw)
  To: netdev
  Cc: Jose Abreu, David S. Miller, Joao Pinto, Giuseppe Cavallaro,
	Alexandre Torgue

Hi,

Two improvements for stmmac: First one corrects the available fifo size per queue, second one corrects enabling of AVB queues. More info in commit log.

Best regards,
Jose Miguel Abreu

Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>

Jose Abreu (2):
  net: stmmac: Use correct values in TQS/RQS fields
  net: stmmac: Disable flow ctrl for RX AVB queues and really enable TX
    AVB queues

 drivers/net/ethernet/stmicro/stmmac/common.h      |  5 +--
 drivers/net/ethernet/stmicro/stmmac/dwmac4.h      |  2 ++
 drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c  | 27 ++++++++++------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 39 +++++++++++++++++++----
 4 files changed, 56 insertions(+), 17 deletions(-)

-- 
1.9.1

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox