Hi Jon, See my comment inline.
Hoang -----Original Message----- From: Jon Maloy <[email protected]> Sent: Monday, June 1, 2020 7:40 PM To: Hoang Huu Le <[email protected]>; [email protected] Cc: Tung Quang Nguyen <[email protected]>; Tuong Tong Lien <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected] 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: [email protected] <[email protected]> > Sent: Friday, May 29, 2020 10:34 PM > To: [email protected] > Cc: Tung Quang Nguyen <[email protected]>; Hoang Huu Le > <[email protected]>; Tuong Tong Lien <[email protected]>; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected] > Subject: [ ] tipc: update a binding service via broadcast > > From: Hoang Huu Le <[email protected]> > > 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. ///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 [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
