On 6/3/20 1:16 AM, Hoang Huu Le wrote:
Thanks. While we are at it, I suggest we rename the function tipc_bcast_get_broadcast_mode() to something shorter, e.g., tipc_bcast_get_mode(). Having both 'bcast' and 'broadcast' in this name is redundant, and simplifying it makes the code look nicer.-----Original Message----- From: Jon Maloy <jma...@redhat.com> Sent: Tuesday, June 2, 2020 8:37 PM To: Hoang Huu Le <hoang.h...@dektech.com.au>; tipc-discussion@lists.sourceforge.net Cc: Tung Quang Nguyen <tung.q.ngu...@dektech.com.au>; Tuong Tong Lien <tuong.t.l...@dektech.com.au>; ma...@donjonn.com; x...@redhat.com; ying....@windriver.com; parthasarathy.bhuvara...@gmail.com Subject: Re: [ ] tipc: update a binding service via broadcast On 6/1/20 7:45 PM, Hoang Huu Le wrote:Hi Jon, See my comment inline. Hoang -----Original Message----- From: Jon Maloy <jma...@redhat.com> Sent: Monday, June 1, 2020 7:40 PM To: Hoang Huu Le <hoang.h...@dektech.com.au>; tipc-discussion@lists.sourceforge.net Cc: Tung Quang Nguyen <tung.q.ngu...@dektech.com.au>; Tuong Tong Lien <tuong.t.l...@dektech.com.au>; ma...@donjonn.com; x...@redhat.com; ying....@windriver.com; parthasarathy.bhuvara...@gmail.com Subject: Re: [ ] tipc: update a binding service via broadcast Hi Hoang. See below. On 6/1/20 5:33 AM, Hoang Huu Le wrote:Hi Jon, There is a concern in function tipc_node_broadcast. See my comment inline. Regards, Hoang -----Original Message----- From: jma...@redhat.com <jma...@redhat.com> Sent: Friday, May 29, 2020 10:34 PM To: tipc-discussion@lists.sourceforge.net Cc: Tung Quang Nguyen <tung.q.ngu...@dektech.com.au>; Hoang Huu Le <hoang.h...@dektech.com.au>; Tuong Tong Lien <tuong.t.l...@dektech.com.au>; jma...@redhat.com; ma...@donjonn.com; x...@redhat.com; ying....@windriver.com; parthasarathy.bhuvara...@gmail.com Subject: [ ] tipc: update a binding service via broadcast From: Hoang Huu Le <hoang.h...@dektech.com.au> Currently, updating binding table (add service binding to name table/withdraw a service binding) is being sent over replicast. However, if we are scaling up clusters to > 100 nodes/containers this method is less affection because of looping through nodes in a cluster one by one.[...]+ /* Use broadcast if all nodes support it */+ if (!rc_dests) { + __skb_queue_head_init(&xmitq); + __skb_queue_tail(&xmitq, skb); + tipc_bcast_xmit(net, &xmitq, &dummy); + return; + } + [Hoang] We could not use 'rc_dests' to send as broadcast mode because of it is not fully broadcast supporting in the cluster. As stated, if there is a node in the cluster not supporting broadcast mode, we need to use replicast instead. That's why we introduced the feature "Smooth switch between replicast/broadcast in bcast.c" and a new command line to configure the broadcast link. If we assume base on 'rc_dests' (i.e capability flags TIPC_NAMED_BCAST), probably 'forced replicast' configuration on broadcast link becomes useless. Then, destination nodes will be missed the publication item.You misunderstand this function. rc_dests is a *counter*, not a flag. It counts the number of peer nodes that don't support TIPC_NAMED_BCAST, and if this is non-zero, we use replicast. So this is safe. [Hoang] Yes, I know about this counter. What I'm considering about the L2 interface (e.g VXLAN) pseudo support broadcast (we discussed the topic a year ago), then, the sending side must be switching to replicast (method->force_bcast = true). But above likely forcing to use broadcast careless setting from broadcast/replicast from L2.Yes you are right. I only checked this for UDP, where it seems to be ok, but forgot about VXLAN. Still, we can avoid building up 'dest' lists by making a small change to tipc_node_bcast(): /* Use broadcast if bearer and all nodes support it */ if (!rc_dests && tipc_bcast_get_broadcast_mode(net) != BCLINK_MODE_RCAST) { u16 dummy; __skb_queue_head_init(&xmitq); __skb_queue_tail(&xmitq, skb); tipc_bcast_xmit(net, &xmitq, &dummy); return; } [Hoang] It sounds fine with this way. I really believe sequence numbering and these three new bits solve our problem in a simple and robust way. Strictly, even the 'non_legacy' bit could be omitted, since the receiver already has this info from the capability map, but I found it simpler to do it that way. Also, I realized that tipc_named_dequeue() function can be simplified further, since there can be no duplicate messages. [Hoang] I will do further testing your solution and improve this one.
Regards ///jon
BR ///jon///jon+ /* Otherwise use legacy replicast method */ rcu_read_lock(); list_for_each_entry_rcu(n, tipc_nodes(net), list) { dst = n->addr; @@ -1749,7 +1762,6 @@ void tipc_node_broadcast(struct net *net, struct sk_buff *skb) tipc_node_xmit_skb(net, txskb, dst, 0); } rcu_read_unlock(); - kfree_skb(skb); }@@ -1844,7 +1856,9 @@ static void tipc_node_bc_rcv(struct net *net, struct sk_buff *skb, int bearer_id /* Handle NAME_DISTRIBUTOR messages sent from 1.7 nodes */if (!skb_queue_empty(&n->bc_entry.namedq)) - tipc_named_rcv(net, &n->bc_entry.namedq); + tipc_named_rcv(net, &n->bc_entry.namedq, + &n->bc_entry.named_rcv_nxt, + &n->bc_entry.named_open);/* If reassembly or retransmission failure => reset all links to peer */if (rc & TIPC_LINK_DOWN_EVT) @@ -2109,7 +2123,9 @@ void tipc_rcv(struct net *net, struct sk_buff *skb, struct tipc_bearer *b) tipc_node_link_down(n, bearer_id, false);if (unlikely(!skb_queue_empty(&n->bc_entry.namedq)))- tipc_named_rcv(net, &n->bc_entry.namedq); + tipc_named_rcv(net, &n->bc_entry.namedq, + &n->bc_entry.named_rcv_nxt, + &n->bc_entry.named_open);if (unlikely(!skb_queue_empty(&n->bc_entry.inputq1)))tipc_node_mcast_rcv(n); diff --git a/net/tipc/node.h b/net/tipc/node.h index a6803b449a2c..9f6f13f1604f 100644 --- a/net/tipc/node.h +++ b/net/tipc/node.h @@ -55,7 +55,8 @@ enum { TIPC_MCAST_RBCTL = (1 << 7), TIPC_GAP_ACK_BLOCK = (1 << 8), TIPC_TUNNEL_ENHANCED = (1 << 9), - TIPC_NAGLE = (1 << 10) + TIPC_NAGLE = (1 << 10), + TIPC_NAMED_BCAST = (1 << 11) };#define TIPC_NODE_CAPABILITIES (TIPC_SYN_BIT | \@@ -68,7 +69,8 @@ enum { TIPC_MCAST_RBCTL | \ TIPC_GAP_ACK_BLOCK | \ TIPC_TUNNEL_ENHANCED | \ - TIPC_NAGLE) + TIPC_NAGLE | \ + TIPC_NAMED_BCAST)#define INVALID_BEARER_ID -1 @@ -101,7 +103,7 @@ int tipc_node_xmit_skb(struct net *net, struct sk_buff *skb, u32 dest,u32 selector); void tipc_node_subscribe(struct net *net, struct list_head *subscr, u32 addr); void tipc_node_unsubscribe(struct net *net, struct list_head *subscr, u32 addr); -void tipc_node_broadcast(struct net *net, struct sk_buff *skb); +void tipc_node_broadcast(struct net *net, struct sk_buff *skb, int rc_dests); int tipc_node_add_conn(struct net *net, u32 dnode, u32 port, u32 peer_port); void tipc_node_remove_conn(struct net *net, u32 dnode, u32 port); int tipc_node_get_mtu(struct net *net, u32 addr, u32 sel, bool connected);
_______________________________________________ tipc-discussion mailing list tipc-discussion@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tipc-discussion
