From 7d33939f475d403e79124e3143d7951dcfe8629f Mon Sep 17 00:00:00 2001 From: Jon Paul Maloy Date: Thu, 13 Feb 2014 17:29:16 -0500 Subject: tipc: delay delete of link when failover is needed When a bearer is disabled, all its attached links are deleted. Ideally, we should do link failover to redundant links on other bearers, if there are any, in such cases. This would be consistent with current behavior when a link is reset, but not deleted. However, due to the complexity involved, and the (wrongly) perceived low demand for this feature, it was never implemented until now. We mark the doomed link for deletion with a new flag, but wait until the failover process is finished before we actually delete it. With the improved link tunnelling/failover code introduced earlier in this commit series, it is now easy to identify a spot in the code where the failover is finished and it is safe to delete the marked link. Moreover, the test for the flag and the deletion can be done synchronously, and outside the most time critical data path. Signed-off-by: Jon Maloy Reviewed-by: Ying Xue Signed-off-by: David S. Miller --- net/tipc/node.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index efe4d41bf11b..833324b73fe1 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -249,7 +249,13 @@ void tipc_node_attach_link(struct tipc_node *n_ptr, struct tipc_link *l_ptr) void tipc_node_detach_link(struct tipc_node *n_ptr, struct tipc_link *l_ptr) { - n_ptr->links[l_ptr->b_ptr->identity] = NULL; + int i; + + for (i = 0; i < MAX_BEARERS; i++) { + if (l_ptr == n_ptr->links[i]) + break; + } + n_ptr->links[i] = NULL; atomic_dec(&tipc_num_links); n_ptr->link_cnt--; } -- cgit v1.2.3 From 074bb43e9e594bec647ec45cc5bbc8c1ac2306aa Mon Sep 17 00:00:00 2001 From: Jon Paul Maloy Date: Fri, 14 Feb 2014 16:40:43 -0500 Subject: tipc: fix a loop style problem In commit 7d33939f475d403e79124e3143d7951dcfe8629f ("tipc: delay delete of link when failover is needed") we introduced a loop for finding and removing a link pointer in an array. The removal is done after we have left the loop, giving the impression that one may remove the wrong pointer if no matching element is found. This is not really a bug, since we know that there will always be a matching element, but it looks wrong, and causes a smatch warning. We fix this loop with this commit. Signed-off-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index 833324b73fe1..8596880877c0 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -252,12 +252,12 @@ void tipc_node_detach_link(struct tipc_node *n_ptr, struct tipc_link *l_ptr) int i; for (i = 0; i < MAX_BEARERS; i++) { - if (l_ptr == n_ptr->links[i]) - break; + if (l_ptr != n_ptr->links[i]) + continue; + n_ptr->links[i] = NULL; + atomic_dec(&tipc_num_links); + n_ptr->link_cnt--; } - n_ptr->links[i] = NULL; - atomic_dec(&tipc_num_links); - n_ptr->link_cnt--; } static void node_established_contact(struct tipc_node *n_ptr) -- cgit v1.2.3 From 247f0f3c3176c55b46cb9a20011d3d6757634815 Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Tue, 18 Feb 2014 16:06:46 +0800 Subject: tipc: align tipc function names with common naming practice in the network Rename the following functions, which are shorter and more in line with common naming practice in the network subsystem. tipc_bclink_send_msg->tipc_bclink_xmit tipc_bclink_recv_pkt->tipc_bclink_rcv tipc_disc_recv_msg->tipc_disc_rcv tipc_link_send_proto_msg->tipc_link_proto_xmit link_recv_proto_msg->tipc_link_proto_rcv link_send_sections_long->tipc_link_iovec_long_xmit tipc_link_send_sections_fast->tipc_link_iovec_xmit_fast tipc_link_send_sync->tipc_link_sync_xmit tipc_link_recv_sync->tipc_link_sync_rcv tipc_link_send_buf->__tipc_link_xmit tipc_link_send->tipc_link_xmit tipc_link_send_names->tipc_link_names_xmit tipc_named_recv->tipc_named_rcv tipc_link_recv_bundle->tipc_link_bundle_rcv tipc_link_dup_send_queue->tipc_link_dup_queue_xmit link_send_long_buf->tipc_link_frag_xmit tipc_multicast->tipc_port_mcast_xmit tipc_port_recv_mcast->tipc_port_mcast_rcv tipc_port_reject_sections->tipc_port_iovec_reject tipc_port_recv_proto_msg->tipc_port_proto_rcv tipc_connect->tipc_port_connect __tipc_connect->__tipc_port_connect __tipc_disconnect->__tipc_port_disconnect tipc_disconnect->tipc_port_disconnect tipc_shutdown->tipc_port_shutdown tipc_port_recv_msg->tipc_port_rcv tipc_port_recv_sections->tipc_port_iovec_rcv release->tipc_release accept->tipc_accept bind->tipc_bind get_name->tipc_getname poll->tipc_poll send_msg->tipc_sendmsg send_packet->tipc_send_packet send_stream->tipc_send_stream recv_msg->tipc_recvmsg recv_stream->tipc_recv_stream connect->tipc_connect listen->tipc_listen shutdown->tipc_shutdown setsockopt->tipc_setsockopt getsockopt->tipc_getsockopt Above changes have no impact on current users of the functions. Signed-off-by: Ying Xue Reviewed-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index 8596880877c0..0b0f6c7da965 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -162,7 +162,7 @@ void tipc_node_link_up(struct tipc_node *n_ptr, struct tipc_link *l_ptr) pr_info("New link <%s> becomes standby\n", l_ptr->name); return; } - tipc_link_dup_send_queue(active[0], l_ptr); + tipc_link_dup_queue_xmit(active[0], l_ptr); if (l_ptr->priority == active[0]->priority) { active[0] = l_ptr; return; -- cgit v1.2.3 From 76d7882420d94075c806c074de241602a06e47e6 Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Thu, 27 Mar 2014 12:54:30 +0800 Subject: tipc: remove unnecessary checking for node object tipc_node_create routine doesn't need to check whether a node object specified with a node address exists or not because its caller(ie, tipc_disc_recv_msg routine) has checked this before calling it. Signed-off-by: Ying Xue Reviewed-by: Erik Hugne Reviewed-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 6 ------ 1 file changed, 6 deletions(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index 0b0f6c7da965..7c9b6673e2ab 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -95,12 +95,6 @@ struct tipc_node *tipc_node_create(u32 addr) spin_lock_bh(&node_create_lock); - n_ptr = tipc_node_find(addr); - if (n_ptr) { - spin_unlock_bh(&node_create_lock); - return n_ptr; - } - n_ptr = kzalloc(sizeof(*n_ptr), GFP_ATOMIC); if (!n_ptr) { spin_unlock_bh(&node_create_lock); -- cgit v1.2.3 From 46651c59c483f14fd35cf7df2104feac0e54e258 Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Thu, 27 Mar 2014 12:54:36 +0800 Subject: tipc: rename node create lock to protect node list and hlist When a node is created, tipc_net_lock read lock is first held and then node_create_lock is grabbed in order to prevent the same node from being created and inserted into both node list and hlist twice. But when we query node from the two node lists, we only hold tipc_net_lock read lock without grabbing node_create_lock. Obviously this locking policy is unable to guarantee that the two node lists are always synchronized especially when the operation of changing and accessing them occurs in different contexts like currently doing. Therefore, rename node_create_lock to node_list_lock to protect the two node lists, that is, whenever node is inserted into them or node is queried from them, the node_list_lock should be always held. As a result, tipc_net_lock read lock becomes redundant and then can be removed from the node query functions. Signed-off-by: Ying Xue Reviewed-by: Erik Hugne Reviewed-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 59 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 30 insertions(+), 29 deletions(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index 7c9b6673e2ab..ec8360736239 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -2,7 +2,7 @@ * net/tipc/node.c: TIPC node management routines * * Copyright (c) 2000-2006, 2012 Ericsson AB - * Copyright (c) 2005-2006, 2010-2011, Wind River Systems + * Copyright (c) 2005-2006, 2010-2014, Wind River Systems * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -44,11 +44,10 @@ static void node_lost_contact(struct tipc_node *n_ptr); static void node_established_contact(struct tipc_node *n_ptr); -static DEFINE_SPINLOCK(node_create_lock); - static struct hlist_head node_htable[NODE_HTABLE_SIZE]; LIST_HEAD(tipc_node_list); static u32 tipc_num_nodes; +static DEFINE_SPINLOCK(node_list_lock); static atomic_t tipc_num_links = ATOMIC_INIT(0); @@ -73,31 +72,26 @@ struct tipc_node *tipc_node_find(u32 addr) if (unlikely(!in_own_cluster_exact(addr))) return NULL; + spin_lock_bh(&node_list_lock); hlist_for_each_entry(node, &node_htable[tipc_hashfn(addr)], hash) { - if (node->addr == addr) + if (node->addr == addr) { + spin_unlock_bh(&node_list_lock); return node; + } } + spin_unlock_bh(&node_list_lock); return NULL; } -/** - * tipc_node_create - create neighboring node - * - * Currently, this routine is called by neighbor discovery code, which holds - * net_lock for reading only. We must take node_create_lock to ensure a node - * isn't created twice if two different bearers discover the node at the same - * time. (It would be preferable to switch to holding net_lock in write mode, - * but this is a non-trivial change.) - */ struct tipc_node *tipc_node_create(u32 addr) { struct tipc_node *n_ptr, *temp_node; - spin_lock_bh(&node_create_lock); + spin_lock_bh(&node_list_lock); n_ptr = kzalloc(sizeof(*n_ptr), GFP_ATOMIC); if (!n_ptr) { - spin_unlock_bh(&node_create_lock); + spin_unlock_bh(&node_list_lock); pr_warn("Node creation failed, no memory\n"); return NULL; } @@ -120,11 +114,11 @@ struct tipc_node *tipc_node_create(u32 addr) tipc_num_nodes++; - spin_unlock_bh(&node_create_lock); + spin_unlock_bh(&node_list_lock); return n_ptr; } -void tipc_node_delete(struct tipc_node *n_ptr) +static void tipc_node_delete(struct tipc_node *n_ptr) { list_del(&n_ptr->list); hlist_del(&n_ptr->hash); @@ -133,6 +127,16 @@ void tipc_node_delete(struct tipc_node *n_ptr) tipc_num_nodes--; } +void tipc_node_stop(void) +{ + struct tipc_node *node, *t_node; + + spin_lock_bh(&node_list_lock); + list_for_each_entry_safe(node, t_node, &tipc_node_list, list) + tipc_node_delete(node); + spin_unlock_bh(&node_list_lock); +} + /** * tipc_node_link_up - handle addition of link * @@ -335,22 +339,22 @@ struct sk_buff *tipc_node_get_nodes(const void *req_tlv_area, int req_tlv_space) return tipc_cfg_reply_error_string(TIPC_CFG_INVALID_VALUE " (network address)"); - read_lock_bh(&tipc_net_lock); + spin_lock_bh(&node_list_lock); if (!tipc_num_nodes) { - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return tipc_cfg_reply_none(); } /* For now, get space for all other nodes */ payload_size = TLV_SPACE(sizeof(node_info)) * tipc_num_nodes; if (payload_size > 32768u) { - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return tipc_cfg_reply_error_string(TIPC_CFG_NOT_SUPPORTED " (too many nodes)"); } buf = tipc_cfg_reply_alloc(payload_size); if (!buf) { - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return NULL; } @@ -363,8 +367,7 @@ struct sk_buff *tipc_node_get_nodes(const void *req_tlv_area, int req_tlv_space) tipc_cfg_append_tlv(buf, TIPC_TLV_NODE_INFO, &node_info, sizeof(node_info)); } - - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return buf; } @@ -387,19 +390,18 @@ struct sk_buff *tipc_node_get_links(const void *req_tlv_area, int req_tlv_space) if (!tipc_own_addr) return tipc_cfg_reply_none(); - read_lock_bh(&tipc_net_lock); - + spin_lock_bh(&node_list_lock); /* Get space for all unicast links + broadcast link */ payload_size = TLV_SPACE(sizeof(link_info)) * (atomic_read(&tipc_num_links) + 1); if (payload_size > 32768u) { - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return tipc_cfg_reply_error_string(TIPC_CFG_NOT_SUPPORTED " (too many links)"); } buf = tipc_cfg_reply_alloc(payload_size); if (!buf) { - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return NULL; } @@ -427,7 +429,6 @@ struct sk_buff *tipc_node_get_links(const void *req_tlv_area, int req_tlv_space) } tipc_node_unlock(n_ptr); } - - read_unlock_bh(&tipc_net_lock); + spin_unlock_bh(&node_list_lock); return buf; } -- cgit v1.2.3 From 6c7a762e70637a256229f9dc9ca793908e8bd01b Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Thu, 27 Mar 2014 12:54:37 +0800 Subject: tipc: tipc: convert node list and node hlist to RCU lists Convert tipc_node_list list and node_htable hash list to RCU lists. On read side, the two lists are protected with RCU read lock, and on update side, node_list_lock is applied to them. Signed-off-by: Ying Xue Reviewed-by: Erik Hugne Reviewed-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index ec8360736239..4f517ff783d9 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -72,14 +72,14 @@ struct tipc_node *tipc_node_find(u32 addr) if (unlikely(!in_own_cluster_exact(addr))) return NULL; - spin_lock_bh(&node_list_lock); - hlist_for_each_entry(node, &node_htable[tipc_hashfn(addr)], hash) { + rcu_read_lock(); + hlist_for_each_entry_rcu(node, &node_htable[tipc_hashfn(addr)], hash) { if (node->addr == addr) { - spin_unlock_bh(&node_list_lock); + rcu_read_unlock(); return node; } } - spin_unlock_bh(&node_list_lock); + rcu_read_unlock(); return NULL; } @@ -102,13 +102,13 @@ struct tipc_node *tipc_node_create(u32 addr) INIT_LIST_HEAD(&n_ptr->list); INIT_LIST_HEAD(&n_ptr->nsub); - hlist_add_head(&n_ptr->hash, &node_htable[tipc_hashfn(addr)]); + hlist_add_head_rcu(&n_ptr->hash, &node_htable[tipc_hashfn(addr)]); - list_for_each_entry(temp_node, &tipc_node_list, list) { + list_for_each_entry_rcu(temp_node, &tipc_node_list, list) { if (n_ptr->addr < temp_node->addr) break; } - list_add_tail(&n_ptr->list, &temp_node->list); + list_add_tail_rcu(&n_ptr->list, &temp_node->list); n_ptr->block_setup = WAIT_PEER_DOWN; n_ptr->signature = INVALID_NODE_SIG; @@ -120,9 +120,9 @@ struct tipc_node *tipc_node_create(u32 addr) static void tipc_node_delete(struct tipc_node *n_ptr) { - list_del(&n_ptr->list); - hlist_del(&n_ptr->hash); - kfree(n_ptr); + list_del_rcu(&n_ptr->list); + hlist_del_rcu(&n_ptr->hash); + kfree_rcu(n_ptr, rcu); tipc_num_nodes--; } @@ -359,7 +359,8 @@ struct sk_buff *tipc_node_get_nodes(const void *req_tlv_area, int req_tlv_space) } /* Add TLVs for all nodes in scope */ - list_for_each_entry(n_ptr, &tipc_node_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(n_ptr, &tipc_node_list, list) { if (!tipc_in_scope(domain, n_ptr->addr)) continue; node_info.addr = htonl(n_ptr->addr); @@ -367,6 +368,7 @@ struct sk_buff *tipc_node_get_nodes(const void *req_tlv_area, int req_tlv_space) tipc_cfg_append_tlv(buf, TIPC_TLV_NODE_INFO, &node_info, sizeof(node_info)); } + rcu_read_unlock(); spin_unlock_bh(&node_list_lock); return buf; } @@ -412,7 +414,8 @@ struct sk_buff *tipc_node_get_links(const void *req_tlv_area, int req_tlv_space) tipc_cfg_append_tlv(buf, TIPC_TLV_LINK_INFO, &link_info, sizeof(link_info)); /* Add TLVs for any other links in scope */ - list_for_each_entry(n_ptr, &tipc_node_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(n_ptr, &tipc_node_list, list) { u32 i; if (!tipc_in_scope(domain, n_ptr->addr)) @@ -429,6 +432,7 @@ struct sk_buff *tipc_node_get_links(const void *req_tlv_area, int req_tlv_space) } tipc_node_unlock(n_ptr); } + rcu_read_unlock(); spin_unlock_bh(&node_list_lock); return buf; } -- cgit v1.2.3 From 2220646a53aa588798653232e26172ec36ab06cd Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Thu, 27 Mar 2014 12:54:38 +0800 Subject: tipc: use node_list_lock to protect tipc_num_nodes variable As tipc_node_list is protected by rcu read lock on read side, it's unnecessary to hold node_list_lock to protect tipc_node_list in tipc_node_get_links(). Instead, node_list_lock should just protects tipc_num_nodes in the function. Signed-off-by: Ying Xue Reviewed-by: Erik Hugne Reviewed-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index 4f517ff783d9..85405a6e3076 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -352,11 +352,11 @@ struct sk_buff *tipc_node_get_nodes(const void *req_tlv_area, int req_tlv_space) return tipc_cfg_reply_error_string(TIPC_CFG_NOT_SUPPORTED " (too many nodes)"); } + spin_unlock_bh(&node_list_lock); + buf = tipc_cfg_reply_alloc(payload_size); - if (!buf) { - spin_unlock_bh(&node_list_lock); + if (!buf) return NULL; - } /* Add TLVs for all nodes in scope */ rcu_read_lock(); @@ -369,7 +369,6 @@ struct sk_buff *tipc_node_get_nodes(const void *req_tlv_area, int req_tlv_space) &node_info, sizeof(node_info)); } rcu_read_unlock(); - spin_unlock_bh(&node_list_lock); return buf; } -- cgit v1.2.3 From dde2026608fbf24e1687a2b62c4752022f429252 Mon Sep 17 00:00:00 2001 From: Ying Xue Date: Thu, 27 Mar 2014 12:54:39 +0800 Subject: tipc: use node list lock to protect tipc_num_links variable Without properly implicit or explicit read memory barrier, it's unsafe to read an atomic variable with atomic_read() from another thread which is different with the thread of changing the atomic variable with atomic_inc() or atomic_dec(). So a stale tipc_num_links may be got with atomic_read() in tipc_node_get_links(). If the tipc_num_links variable type is converted from atomic to unsigned integer and node list lock is used to protect it, the issue would be avoided. Signed-off-by: Ying Xue Reviewed-by: Erik Hugne Reviewed-by: Jon Maloy Signed-off-by: David S. Miller --- net/tipc/node.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) (limited to 'net/tipc/node.c') diff --git a/net/tipc/node.c b/net/tipc/node.c index 85405a6e3076..1d3a4999a70f 100644 --- a/net/tipc/node.c +++ b/net/tipc/node.c @@ -47,10 +47,9 @@ static void node_established_contact(struct tipc_node *n_ptr); static struct hlist_head node_htable[NODE_HTABLE_SIZE]; LIST_HEAD(tipc_node_list); static u32 tipc_num_nodes; +static u32 tipc_num_links; static DEFINE_SPINLOCK(node_list_lock); -static atomic_t tipc_num_links = ATOMIC_INIT(0); - /* * A trivial power-of-two bitmask technique is used for speed, since this * operation is done for every incoming TIPC packet. The number of hash table @@ -241,7 +240,9 @@ int tipc_node_is_up(struct tipc_node *n_ptr) void tipc_node_attach_link(struct tipc_node *n_ptr, struct tipc_link *l_ptr) { n_ptr->links[l_ptr->b_ptr->identity] = l_ptr; - atomic_inc(&tipc_num_links); + spin_lock_bh(&node_list_lock); + tipc_num_links++; + spin_unlock_bh(&node_list_lock); n_ptr->link_cnt++; } @@ -253,7 +254,9 @@ void tipc_node_detach_link(struct tipc_node *n_ptr, struct tipc_link *l_ptr) if (l_ptr != n_ptr->links[i]) continue; n_ptr->links[i] = NULL; - atomic_dec(&tipc_num_links); + spin_lock_bh(&node_list_lock); + tipc_num_links--; + spin_unlock_bh(&node_list_lock); n_ptr->link_cnt--; } } @@ -393,18 +396,17 @@ struct sk_buff *tipc_node_get_links(const void *req_tlv_area, int req_tlv_space) spin_lock_bh(&node_list_lock); /* Get space for all unicast links + broadcast link */ - payload_size = TLV_SPACE(sizeof(link_info)) * - (atomic_read(&tipc_num_links) + 1); + payload_size = TLV_SPACE((sizeof(link_info)) * (tipc_num_links + 1)); if (payload_size > 32768u) { spin_unlock_bh(&node_list_lock); return tipc_cfg_reply_error_string(TIPC_CFG_NOT_SUPPORTED " (too many links)"); } + spin_unlock_bh(&node_list_lock); + buf = tipc_cfg_reply_alloc(payload_size); - if (!buf) { - spin_unlock_bh(&node_list_lock); + if (!buf) return NULL; - } /* Add TLV for broadcast link */ link_info.dest = htonl(tipc_cluster_mask(tipc_own_addr)); @@ -432,6 +434,5 @@ struct sk_buff *tipc_node_get_links(const void *req_tlv_area, int req_tlv_space) tipc_node_unlock(n_ptr); } rcu_read_unlock(); - spin_unlock_bh(&node_list_lock); return buf; } -- cgit v1.2.3