Re: [tipc-discussion] [net-next] tipc: eliminate checking netns if node acknowledge

[Jon M. <jon...@er...>]

Hi Hoang,
I have a hard time to understand this change. Can you please explain it a little better?
See more comments below.

BR
///jon


> -----Original Message-----
> From: Hoang Le <hoa...@de...>
> Sent: 6-Nov-19 00:03
> To: tip...@li...; Jon Maloy <jon...@er...>; ma...@do...
> Subject: [net-next] tipc: eliminate checking netns if node acknowledge
> 
> At current we do check netns local for every neighbor discovery that
> is being sent from external netns node. This is become unnecessary
> for node acknowledge.
> 
> We now improve above checking for peer node come back and discovery
> message sent from unacknowledge node.
> 
> Fixes: f73b12812a3d ("tipc: improve throughput between nodes in netns")
> Signed-off-by: Hoang Le <hoa...@de...>
> ---
>  net/tipc/node.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 4b60928049ea..742c04756d72 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -472,10 +472,8 @@ static struct tipc_node *tipc_node_create(struct net *net, u32 addr,
>  				 tipc_bc_sndlink(net),
>  				 &n->bc_entry.link)) {
>  		pr_warn("Broadcast rcv link creation failed, no memory\n");
> -		if (n->peer_net) {
> -			n->peer_net = NULL;
> -			n->peer_hash_mix = 0;
> -		}
> +		n->peer_net = NULL;
> +		n->peer_hash_mix = 0;

This looks pretty unnecessary if the node object anyway is freed on the next code line.

>  		kfree(n);
>  		n = NULL;
>  		goto exit;
> @@ -1068,6 +1066,9 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>  	if (sign_match && addr_match && link_up) {
>  		/* All is fine. Do nothing. */
>  		reset = false;
> +		/* Peer node is not a container/netns local */
> +		if (!n->peer_hash_mix)
> +			n->peer_hash_mix = hash_mixes;

I don't understand why you are doing this. 
If we already have established that the node is non-local, and it is still up and everything matches, why do we need to set this?

Regards
///jon

>  	} else if (sign_match && addr_match && !link_up) {
>  		/* Respond. The link will come up in due time */
>  		*respond = true;
> @@ -1393,11 +1394,8 @@ static void node_lost_contact(struct tipc_node *n,
> 
>  	/* Notify publications from this node */
>  	n->action_flags |= TIPC_NOTIFY_NODE_DOWN;
> -
> -	if (n->peer_net) {
> -		n->peer_net = NULL;
> -		n->peer_hash_mix = 0;
> -	}
> +	n->peer_net = NULL;
> +	n->peer_hash_mix = 0;
>  	/* Notify sockets connected to node */
>  	list_for_each_entry_safe(conn, safe, conns, list) {
>  		skb = tipc_msg_create(TIPC_CRITICAL_IMPORTANCE, TIPC_CONN_MSG,
> --
> 2.20.1